| Message ID | 20230330115853.1628216-7-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Separate links and async sub-devices | expand |
Hi Sakari On Thu, Mar 30, 2023 at 02:58:41PM +0300, Sakari Ailus wrote: > Pass only information required for sub-device matching to functions > checking whether the async sub-device already exists. Do the same for > debug message printing. This makes further changes to other aspects of > async sub-devices easier. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 93 ++++++++++++++-------------- > 1 file changed, 46 insertions(+), 47 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index fc9ae22e2b47..224ebf50f2d0 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -62,14 +62,14 @@ static void v4l2_async_nf_call_destroy(struct v4l2_async_notifier *n, > } > > static bool match_i2c(struct v4l2_async_notifier *notifier, > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > + struct v4l2_subdev *sd, struct v4l2_async_match *match) > { > #if IS_ENABLED(CONFIG_I2C) > struct i2c_client *client = i2c_verify_client(sd->dev); > > return client && > - asd->match.i2c.adapter_id == client->adapter->nr && > - asd->match.i2c.address == client->addr; > + match->i2c.adapter_id == client->adapter->nr && > + match->i2c.address == client->addr; > #else > return false; > #endif > @@ -84,26 +84,26 @@ static struct device *notifier_dev(struct v4l2_async_notifier *notifier) > static bool > match_fwnode_one(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > - struct v4l2_async_subdev *asd) > + struct v4l2_async_match *match) > { > struct fwnode_handle *asd_dev_fwnode; > bool ret; > > dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", > - sd_fwnode, asd->match.fwnode); > + sd_fwnode, match->fwnode); > > - if (sd_fwnode == asd->match.fwnode) { > + if (sd_fwnode == match->fwnode) { > dev_dbg(sd->dev, "async: direct match found\n"); > return true; > } > > - if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { > + if (!fwnode_graph_is_endpoint(match->fwnode)) { > dev_dbg(sd->dev, > "async: async subdev fwnode not endpoint, no match\n"); > return false; > } > > - asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > + asd_dev_fwnode = fwnode_graph_get_port_parent(match->fwnode); > > ret = sd_fwnode == asd_dev_fwnode; > > @@ -116,12 +116,12 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > } > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > + struct v4l2_subdev *sd, struct v4l2_async_match *match) > { > dev_dbg(sd->dev, "async: matching for notifier %pfw, sd %pfw\n", > dev_fwnode(notifier_dev(notifier)), sd->fwnode); > > - if (match_fwnode_one(notifier, sd, sd->fwnode, asd)) > + if (match_fwnode_one(notifier, sd, sd->fwnode, match)) > return true; > > /* Also check the secondary fwnode. */ > @@ -130,7 +130,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier, > > dev_dbg(sd->dev, "async: trying secondary fwnode match\n"); > > - return match_fwnode_one(notifier, sd, sd->fwnode->secondary, asd); > + return match_fwnode_one(notifier, sd, sd->fwnode->secondary, match); > } > > static LIST_HEAD(subdev_list); > @@ -142,7 +142,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd) > { > bool (*match)(struct v4l2_async_notifier *notifier, > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd); > + struct v4l2_subdev *sd, struct v4l2_async_match *match); > struct v4l2_async_subdev *asd; > > list_for_each_entry(asd, ¬ifier->waiting, list) { > @@ -161,7 +161,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > } > > /* match cannot be NULL here */ > - if (match(notifier, sd, asd)) > + if (match(notifier, sd, &asd->match)) > return asd; > } > > @@ -169,20 +169,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > } > > /* Compare two async sub-device descriptors for equivalence */ > -static bool asd_equal(struct v4l2_async_subdev *asd_x, > - struct v4l2_async_subdev *asd_y) > +static bool asd_equal(struct v4l2_async_match *match1, > + struct v4l2_async_match *match2) > { > - if (asd_x->match.type != asd_y->match.type) > + if (match1->type != match2->type) > return false; > > - switch (asd_x->match.type) { > + switch (match1->type) { > case V4L2_ASYNC_MATCH_I2C: > - return asd_x->match.i2c.adapter_id == > - asd_y->match.i2c.adapter_id && > - asd_x->match.i2c.address == > - asd_y->match.i2c.address; > + return match1->i2c.adapter_id == match2->i2c.adapter_id && > + match1->i2c.address == match2->i2c.address; > case V4L2_ASYNC_MATCH_FWNODE: > - return asd_x->match.fwnode == asd_y->match.fwnode; > + return match1->fwnode == match2->fwnode; > default: > break; > } > @@ -434,20 +432,20 @@ v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, > /* See if an async sub-device can be found in a notifier's lists. */ > static bool > __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd) > + struct v4l2_async_match *match) > { > - struct v4l2_async_subdev *asd_y; > + struct v4l2_async_subdev *asd; > struct v4l2_subdev *sd; > > - list_for_each_entry(asd_y, ¬ifier->waiting, list) > - if (asd_equal(asd, asd_y)) > + list_for_each_entry(asd, ¬ifier->waiting, list) > + if (asd_equal(&asd->match, match)) > return true; > > list_for_each_entry(sd, ¬ifier->done, async_list) { > if (WARN_ON(!sd->asd)) > continue; > > - if (asd_equal(asd, sd->asd)) > + if (asd_equal(&sd->asd->match, match)) > return true; > } > > @@ -460,49 +458,50 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > */ > static bool > v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd, bool skip_self) > + struct v4l2_async_match *match, bool skip_self) > { > - struct v4l2_async_subdev *asd_y; > + struct v4l2_async_subdev *asd; > > lockdep_assert_held(&list_lock); > > /* Check that an asd is not being added more than once. */ > - list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) { > - if (asd == asd_y) > + list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > + if (&asd->match == match) > break; > - if (asd_equal(asd, asd_y)) > + if (asd_equal(&asd->match, match)) > return true; > } > > /* Check that an asd does not exist in other notifiers. */ > list_for_each_entry(notifier, ¬ifier_list, list) > - if (__v4l2_async_nf_has_async_subdev(notifier, asd)) > + if (__v4l2_async_nf_has_async_subdev(notifier, match)) > return true; > > return false; > } > > static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd, > + struct v4l2_async_match *match, I would have kept the asd here, but I presume having match here makes things easier in the next patches > bool skip_self) > { > struct device *dev = > notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL; > > - if (!asd) > + if (!match) > return -EINVAL; Match cannot be null, as it's a member of struct v4l2_async_subdev > > - switch (asd->match.type) { > + switch (match->type) { > case V4L2_ASYNC_MATCH_I2C: > case V4L2_ASYNC_MATCH_FWNODE: > - if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { > + if (v4l2_async_nf_has_async_subdev(notifier, match, > + skip_self)) { > dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n"); > return -EEXIST; > } > break; > default: > - dev_err(dev, "Invalid match type %u on %p\n", > - asd->match.type, asd); > + dev_err(dev, "Invalid match type %u on %p\n", match->type, > + match); > return -EINVAL; > } > > @@ -526,7 +525,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > mutex_lock(&list_lock); > > list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > - ret = v4l2_async_nf_asd_valid(notifier, asd, true); > + ret = v4l2_async_nf_asd_valid(notifier, &asd->match, true); > if (ret) > goto err_unlock; > > @@ -659,7 +658,7 @@ int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier, > > mutex_lock(&list_lock); > > - ret = v4l2_async_nf_asd_valid(notifier, asd, false); > + ret = v4l2_async_nf_asd_valid(notifier, &asd->match, false); > if (ret) > goto unlock; > > @@ -846,15 +845,15 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > static void print_waiting_subdev(struct seq_file *s, > - struct v4l2_async_subdev *asd) > + struct v4l2_async_match *match) > { > - switch (asd->match.type) { > + switch (match->type) { > case V4L2_ASYNC_MATCH_I2C: > - seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id, > - asd->match.i2c.address); > + seq_printf(s, " [i2c] dev=%d-%04x\n", match->i2c.adapter_id, > + match->i2c.address); > break; > case V4L2_ASYNC_MATCH_FWNODE: { > - struct fwnode_handle *devnode, *fwnode = asd->match.fwnode; > + struct fwnode_handle *devnode, *fwnode = match->fwnode; > > devnode = fwnode_graph_is_endpoint(fwnode) ? > fwnode_graph_get_port_parent(fwnode) : > @@ -891,7 +890,7 @@ static int pending_subdevs_show(struct seq_file *s, void *data) > list_for_each_entry(notif, ¬ifier_list, list) { > seq_printf(s, "%s:\n", v4l2_async_nf_name(notif)); > list_for_each_entry(asd, ¬if->waiting, list) > - print_waiting_subdev(s, asd); > + print_waiting_subdev(s, &asd->match); > } > > mutex_unlock(&list_lock); > -- > 2.30.2 >
Hi Sakari, Thank you for the patch. On Thu, Mar 30, 2023 at 02:58:41PM +0300, Sakari Ailus wrote: > Pass only information required for sub-device matching to functions > checking whether the async sub-device already exists. Do the same for > debug message printing. This makes further changes to other aspects of > async sub-devices easier. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 93 ++++++++++++++-------------- > 1 file changed, 46 insertions(+), 47 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index fc9ae22e2b47..224ebf50f2d0 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -62,14 +62,14 @@ static void v4l2_async_nf_call_destroy(struct v4l2_async_notifier *n, > } > > static bool match_i2c(struct v4l2_async_notifier *notifier, > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > + struct v4l2_subdev *sd, struct v4l2_async_match *match) > { > #if IS_ENABLED(CONFIG_I2C) > struct i2c_client *client = i2c_verify_client(sd->dev); > > return client && > - asd->match.i2c.adapter_id == client->adapter->nr && > - asd->match.i2c.address == client->addr; > + match->i2c.adapter_id == client->adapter->nr && > + match->i2c.address == client->addr; > #else > return false; > #endif > @@ -84,26 +84,26 @@ static struct device *notifier_dev(struct v4l2_async_notifier *notifier) > static bool > match_fwnode_one(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > - struct v4l2_async_subdev *asd) > + struct v4l2_async_match *match) > { > struct fwnode_handle *asd_dev_fwnode; match_dev_node, to remove references to asd ? > bool ret; > > dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", > - sd_fwnode, asd->match.fwnode); > + sd_fwnode, match->fwnode); > > - if (sd_fwnode == asd->match.fwnode) { > + if (sd_fwnode == match->fwnode) { > dev_dbg(sd->dev, "async: direct match found\n"); > return true; > } > > - if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { > + if (!fwnode_graph_is_endpoint(match->fwnode)) { > dev_dbg(sd->dev, > "async: async subdev fwnode not endpoint, no match\n"); The reference to "subdev" could be dropped here too. > return false; > } > > - asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > + asd_dev_fwnode = fwnode_graph_get_port_parent(match->fwnode); > > ret = sd_fwnode == asd_dev_fwnode; > > @@ -116,12 +116,12 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > } > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > + struct v4l2_subdev *sd, struct v4l2_async_match *match) > { > dev_dbg(sd->dev, "async: matching for notifier %pfw, sd %pfw\n", > dev_fwnode(notifier_dev(notifier)), sd->fwnode); > > - if (match_fwnode_one(notifier, sd, sd->fwnode, asd)) > + if (match_fwnode_one(notifier, sd, sd->fwnode, match)) > return true; > > /* Also check the secondary fwnode. */ > @@ -130,7 +130,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier, > > dev_dbg(sd->dev, "async: trying secondary fwnode match\n"); > > - return match_fwnode_one(notifier, sd, sd->fwnode->secondary, asd); > + return match_fwnode_one(notifier, sd, sd->fwnode->secondary, match); > } > > static LIST_HEAD(subdev_list); > @@ -142,7 +142,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd) > { > bool (*match)(struct v4l2_async_notifier *notifier, > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd); > + struct v4l2_subdev *sd, struct v4l2_async_match *match); > struct v4l2_async_subdev *asd; > > list_for_each_entry(asd, ¬ifier->waiting, list) { > @@ -161,7 +161,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > } > > /* match cannot be NULL here */ > - if (match(notifier, sd, asd)) > + if (match(notifier, sd, &asd->match)) > return asd; > } > > @@ -169,20 +169,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > } > > /* Compare two async sub-device descriptors for equivalence */ > -static bool asd_equal(struct v4l2_async_subdev *asd_x, > - struct v4l2_async_subdev *asd_y) > +static bool asd_equal(struct v4l2_async_match *match1, > + struct v4l2_async_match *match2) The function doesn't deal with asds anymore, let's rename it. v4l2_async_match_equal() could be a good name. Please also update the comment above the function. > { > - if (asd_x->match.type != asd_y->match.type) > + if (match1->type != match2->type) > return false; > > - switch (asd_x->match.type) { > + switch (match1->type) { > case V4L2_ASYNC_MATCH_I2C: > - return asd_x->match.i2c.adapter_id == > - asd_y->match.i2c.adapter_id && > - asd_x->match.i2c.address == > - asd_y->match.i2c.address; > + return match1->i2c.adapter_id == match2->i2c.adapter_id && > + match1->i2c.address == match2->i2c.address; > case V4L2_ASYNC_MATCH_FWNODE: > - return asd_x->match.fwnode == asd_y->match.fwnode; > + return match1->fwnode == match2->fwnode; > default: > break; > } > @@ -434,20 +432,20 @@ v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, > /* See if an async sub-device can be found in a notifier's lists. */ > static bool > __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd) > + struct v4l2_async_match *match) This function should be renamed to drop "subdev" as well. __v4l2_async_nf_has_match() is a candidate, __v4l2_async_nf_has_match_entry() is also an option to avoid implying that the function tests for a match between a subdev and a match entry. > { > - struct v4l2_async_subdev *asd_y; > + struct v4l2_async_subdev *asd; > struct v4l2_subdev *sd; > > - list_for_each_entry(asd_y, ¬ifier->waiting, list) > - if (asd_equal(asd, asd_y)) > + list_for_each_entry(asd, ¬ifier->waiting, list) > + if (asd_equal(&asd->match, match)) > return true; > > list_for_each_entry(sd, ¬ifier->done, async_list) { > if (WARN_ON(!sd->asd)) > continue; > > - if (asd_equal(asd, sd->asd)) > + if (asd_equal(&sd->asd->match, match)) > return true; > } > > @@ -460,49 +458,50 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > */ > static bool > v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd, bool skip_self) > + struct v4l2_async_match *match, bool skip_self) Same here. > { > - struct v4l2_async_subdev *asd_y; > + struct v4l2_async_subdev *asd; > > lockdep_assert_held(&list_lock); > > /* Check that an asd is not being added more than once. */ > - list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) { > - if (asd == asd_y) > + list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > + if (&asd->match == match) > break; > - if (asd_equal(asd, asd_y)) > + if (asd_equal(&asd->match, match)) > return true; > } > > /* Check that an asd does not exist in other notifiers. */ > list_for_each_entry(notifier, ¬ifier_list, list) > - if (__v4l2_async_nf_has_async_subdev(notifier, asd)) > + if (__v4l2_async_nf_has_async_subdev(notifier, match)) > return true; > > return false; > } > > static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd, > + struct v4l2_async_match *match, > bool skip_self) And here. > { > struct device *dev = > notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL; > > - if (!asd) > + if (!match) > return -EINVAL; > > - switch (asd->match.type) { > + switch (match->type) { > case V4L2_ASYNC_MATCH_I2C: > case V4L2_ASYNC_MATCH_FWNODE: > - if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { > + if (v4l2_async_nf_has_async_subdev(notifier, match, > + skip_self)) { > dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n"); "match descriptor" ? > return -EEXIST; > } > break; > default: > - dev_err(dev, "Invalid match type %u on %p\n", > - asd->match.type, asd); > + dev_err(dev, "Invalid match type %u on %p\n", match->type, > + match); > return -EINVAL; > } > > @@ -526,7 +525,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > mutex_lock(&list_lock); > > list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > - ret = v4l2_async_nf_asd_valid(notifier, asd, true); > + ret = v4l2_async_nf_asd_valid(notifier, &asd->match, true); > if (ret) > goto err_unlock; > > @@ -659,7 +658,7 @@ int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier, > > mutex_lock(&list_lock); > > - ret = v4l2_async_nf_asd_valid(notifier, asd, false); > + ret = v4l2_async_nf_asd_valid(notifier, &asd->match, false); > if (ret) > goto unlock; > > @@ -846,15 +845,15 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > static void print_waiting_subdev(struct seq_file *s, Same here. v4l2_async_print_match() or v4l2_async_print_match_entry() could be good names. In general, it would be useful to have a glossary of terms used in v4l2-async, and make sure they're used consistently. > - struct v4l2_async_subdev *asd) > + struct v4l2_async_match *match) > { > - switch (asd->match.type) { > + switch (match->type) { > case V4L2_ASYNC_MATCH_I2C: > - seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id, > - asd->match.i2c.address); > + seq_printf(s, " [i2c] dev=%d-%04x\n", match->i2c.adapter_id, > + match->i2c.address); > break; > case V4L2_ASYNC_MATCH_FWNODE: { > - struct fwnode_handle *devnode, *fwnode = asd->match.fwnode; > + struct fwnode_handle *devnode, *fwnode = match->fwnode; > > devnode = fwnode_graph_is_endpoint(fwnode) ? > fwnode_graph_get_port_parent(fwnode) : > @@ -891,7 +890,7 @@ static int pending_subdevs_show(struct seq_file *s, void *data) > list_for_each_entry(notif, ¬ifier_list, list) { > seq_printf(s, "%s:\n", v4l2_async_nf_name(notif)); > list_for_each_entry(asd, ¬if->waiting, list) > - print_waiting_subdev(s, asd); > + print_waiting_subdev(s, &asd->match); > } > > mutex_unlock(&list_lock);
Hi Laurent, On Tue, Apr 25, 2023 at 04:24:26AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. Thank you for the review. > > On Thu, Mar 30, 2023 at 02:58:41PM +0300, Sakari Ailus wrote: > > Pass only information required for sub-device matching to functions > > checking whether the async sub-device already exists. Do the same for > > debug message printing. This makes further changes to other aspects of > > async sub-devices easier. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 93 ++++++++++++++-------------- > > 1 file changed, 46 insertions(+), 47 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index fc9ae22e2b47..224ebf50f2d0 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -62,14 +62,14 @@ static void v4l2_async_nf_call_destroy(struct v4l2_async_notifier *n, > > } > > > > static bool match_i2c(struct v4l2_async_notifier *notifier, > > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > + struct v4l2_subdev *sd, struct v4l2_async_match *match) > > { > > #if IS_ENABLED(CONFIG_I2C) > > struct i2c_client *client = i2c_verify_client(sd->dev); > > > > return client && > > - asd->match.i2c.adapter_id == client->adapter->nr && > > - asd->match.i2c.address == client->addr; > > + match->i2c.adapter_id == client->adapter->nr && > > + match->i2c.address == client->addr; > > #else > > return false; > > #endif > > @@ -84,26 +84,26 @@ static struct device *notifier_dev(struct v4l2_async_notifier *notifier) > > static bool > > match_fwnode_one(struct v4l2_async_notifier *notifier, > > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > > - struct v4l2_async_subdev *asd) > > + struct v4l2_async_match *match) > > { > > struct fwnode_handle *asd_dev_fwnode; > > match_dev_node, to remove references to asd ? Sounds good. > > > bool ret; > > > > dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", > > - sd_fwnode, asd->match.fwnode); > > + sd_fwnode, match->fwnode); > > > > - if (sd_fwnode == asd->match.fwnode) { > > + if (sd_fwnode == match->fwnode) { > > dev_dbg(sd->dev, "async: direct match found\n"); > > return true; > > } > > > > - if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { > > + if (!fwnode_graph_is_endpoint(match->fwnode)) { > > dev_dbg(sd->dev, > > "async: async subdev fwnode not endpoint, no match\n"); > > The reference to "subdev" could be dropped here too. This will get changed in an earlier patch to "v4l2-async: direct match failed". > > > return false; > > } > > > > - asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > + asd_dev_fwnode = fwnode_graph_get_port_parent(match->fwnode); > > > > ret = sd_fwnode == asd_dev_fwnode; > > > > @@ -116,12 +116,12 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > > } > > > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > + struct v4l2_subdev *sd, struct v4l2_async_match *match) > > { > > dev_dbg(sd->dev, "async: matching for notifier %pfw, sd %pfw\n", > > dev_fwnode(notifier_dev(notifier)), sd->fwnode); > > > > - if (match_fwnode_one(notifier, sd, sd->fwnode, asd)) > > + if (match_fwnode_one(notifier, sd, sd->fwnode, match)) > > return true; > > > > /* Also check the secondary fwnode. */ > > @@ -130,7 +130,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier, > > > > dev_dbg(sd->dev, "async: trying secondary fwnode match\n"); > > > > - return match_fwnode_one(notifier, sd, sd->fwnode->secondary, asd); > > + return match_fwnode_one(notifier, sd, sd->fwnode->secondary, match); > > } > > > > static LIST_HEAD(subdev_list); > > @@ -142,7 +142,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > > struct v4l2_subdev *sd) > > { > > bool (*match)(struct v4l2_async_notifier *notifier, > > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd); > > + struct v4l2_subdev *sd, struct v4l2_async_match *match); > > struct v4l2_async_subdev *asd; > > > > list_for_each_entry(asd, ¬ifier->waiting, list) { > > @@ -161,7 +161,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > > } > > > > /* match cannot be NULL here */ > > - if (match(notifier, sd, asd)) > > + if (match(notifier, sd, &asd->match)) > > return asd; > > } > > > > @@ -169,20 +169,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > > } > > > > /* Compare two async sub-device descriptors for equivalence */ > > -static bool asd_equal(struct v4l2_async_subdev *asd_x, > > - struct v4l2_async_subdev *asd_y) > > +static bool asd_equal(struct v4l2_async_match *match1, > > + struct v4l2_async_match *match2) > > The function doesn't deal with asds anymore, let's rename it. > v4l2_async_match_equal() could be a good name. Please also update the > comment above the function. Seems fine. > > > { > > - if (asd_x->match.type != asd_y->match.type) > > + if (match1->type != match2->type) > > return false; > > > > - switch (asd_x->match.type) { > > + switch (match1->type) { > > case V4L2_ASYNC_MATCH_I2C: > > - return asd_x->match.i2c.adapter_id == > > - asd_y->match.i2c.adapter_id && > > - asd_x->match.i2c.address == > > - asd_y->match.i2c.address; > > + return match1->i2c.adapter_id == match2->i2c.adapter_id && > > + match1->i2c.address == match2->i2c.address; > > case V4L2_ASYNC_MATCH_FWNODE: > > - return asd_x->match.fwnode == asd_y->match.fwnode; > > + return match1->fwnode == match2->fwnode; > > default: > > break; > > } > > @@ -434,20 +432,20 @@ v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, > > /* See if an async sub-device can be found in a notifier's lists. */ > > static bool > > __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > - struct v4l2_async_subdev *asd) > > + struct v4l2_async_match *match) > > This function should be renamed to drop "subdev" as well. > __v4l2_async_nf_has_match() is a candidate, > __v4l2_async_nf_has_match_entry() is also an option to avoid implying > that the function tests for a match between a subdev and a match entry. Seems fine to me. > > > { > > - struct v4l2_async_subdev *asd_y; > > + struct v4l2_async_subdev *asd; > > struct v4l2_subdev *sd; > > > > - list_for_each_entry(asd_y, ¬ifier->waiting, list) > > - if (asd_equal(asd, asd_y)) > > + list_for_each_entry(asd, ¬ifier->waiting, list) > > + if (asd_equal(&asd->match, match)) > > return true; > > > > list_for_each_entry(sd, ¬ifier->done, async_list) { > > if (WARN_ON(!sd->asd)) > > continue; > > > > - if (asd_equal(asd, sd->asd)) > > + if (asd_equal(&sd->asd->match, match)) > > return true; > > } > > > > @@ -460,49 +458,50 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > */ > > static bool > > v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > - struct v4l2_async_subdev *asd, bool skip_self) > > + struct v4l2_async_match *match, bool skip_self) > > Same here. > > > { > > - struct v4l2_async_subdev *asd_y; > > + struct v4l2_async_subdev *asd; > > > > lockdep_assert_held(&list_lock); > > > > /* Check that an asd is not being added more than once. */ > > - list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) { > > - if (asd == asd_y) > > + list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > > + if (&asd->match == match) > > break; > > - if (asd_equal(asd, asd_y)) > > + if (asd_equal(&asd->match, match)) > > return true; > > } > > > > /* Check that an asd does not exist in other notifiers. */ > > list_for_each_entry(notifier, ¬ifier_list, list) > > - if (__v4l2_async_nf_has_async_subdev(notifier, asd)) > > + if (__v4l2_async_nf_has_async_subdev(notifier, match)) > > return true; > > > > return false; > > } > > > > static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > > - struct v4l2_async_subdev *asd, > > + struct v4l2_async_match *match, > > bool skip_self) > > And here. > > > { > > struct device *dev = > > notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL; > > > > - if (!asd) > > + if (!match) > > return -EINVAL; > > > > - switch (asd->match.type) { > > + switch (match->type) { > > case V4L2_ASYNC_MATCH_I2C: > > case V4L2_ASYNC_MATCH_FWNODE: > > - if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { > > + if (v4l2_async_nf_has_async_subdev(notifier, match, > > + skip_self)) { > > dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n"); > > "match descriptor" ? Yes! > > > return -EEXIST; > > } > > break; > > default: > > - dev_err(dev, "Invalid match type %u on %p\n", > > - asd->match.type, asd); > > + dev_err(dev, "Invalid match type %u on %p\n", match->type, > > + match); > > return -EINVAL; > > } > > > > @@ -526,7 +525,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > > mutex_lock(&list_lock); > > > > list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > > - ret = v4l2_async_nf_asd_valid(notifier, asd, true); > > + ret = v4l2_async_nf_asd_valid(notifier, &asd->match, true); > > if (ret) > > goto err_unlock; > > > > @@ -659,7 +658,7 @@ int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier, > > > > mutex_lock(&list_lock); > > > > - ret = v4l2_async_nf_asd_valid(notifier, asd, false); > > + ret = v4l2_async_nf_asd_valid(notifier, &asd->match, false); > > if (ret) > > goto unlock; > > > > @@ -846,15 +845,15 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > > > static void print_waiting_subdev(struct seq_file *s, > > Same here. v4l2_async_print_match() or v4l2_async_print_match_entry() > could be good names. v4l2_async_print_waiting_matches()? > > In general, it would be useful to have a glossary of terms used in > v4l2-async, and make sure they're used consistently. I think this should be already outside this set. I'm all for consolidated terms though. I'll go through the set in this respect as well. > > > - struct v4l2_async_subdev *asd) > > + struct v4l2_async_match *match) > > { > > - switch (asd->match.type) { > > + switch (match->type) { > > case V4L2_ASYNC_MATCH_I2C: > > - seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id, > > - asd->match.i2c.address); > > + seq_printf(s, " [i2c] dev=%d-%04x\n", match->i2c.adapter_id, > > + match->i2c.address); > > break; > > case V4L2_ASYNC_MATCH_FWNODE: { > > - struct fwnode_handle *devnode, *fwnode = asd->match.fwnode; > > + struct fwnode_handle *devnode, *fwnode = match->fwnode; > > > > devnode = fwnode_graph_is_endpoint(fwnode) ? > > fwnode_graph_get_port_parent(fwnode) : > > @@ -891,7 +890,7 @@ static int pending_subdevs_show(struct seq_file *s, void *data) > > list_for_each_entry(notif, ¬ifier_list, list) { > > seq_printf(s, "%s:\n", v4l2_async_nf_name(notif)); > > list_for_each_entry(asd, ¬if->waiting, list) > > - print_waiting_subdev(s, asd); > > + print_waiting_subdev(s, &asd->match); > > } > > > > mutex_unlock(&list_lock); >
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index fc9ae22e2b47..224ebf50f2d0 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -62,14 +62,14 @@ static void v4l2_async_nf_call_destroy(struct v4l2_async_notifier *n, } static bool match_i2c(struct v4l2_async_notifier *notifier, - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) + struct v4l2_subdev *sd, struct v4l2_async_match *match) { #if IS_ENABLED(CONFIG_I2C) struct i2c_client *client = i2c_verify_client(sd->dev); return client && - asd->match.i2c.adapter_id == client->adapter->nr && - asd->match.i2c.address == client->addr; + match->i2c.adapter_id == client->adapter->nr && + match->i2c.address == client->addr; #else return false; #endif @@ -84,26 +84,26 @@ static struct device *notifier_dev(struct v4l2_async_notifier *notifier) static bool match_fwnode_one(struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, - struct v4l2_async_subdev *asd) + struct v4l2_async_match *match) { struct fwnode_handle *asd_dev_fwnode; bool ret; dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", - sd_fwnode, asd->match.fwnode); + sd_fwnode, match->fwnode); - if (sd_fwnode == asd->match.fwnode) { + if (sd_fwnode == match->fwnode) { dev_dbg(sd->dev, "async: direct match found\n"); return true; } - if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { + if (!fwnode_graph_is_endpoint(match->fwnode)) { dev_dbg(sd->dev, "async: async subdev fwnode not endpoint, no match\n"); return false; } - asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); + asd_dev_fwnode = fwnode_graph_get_port_parent(match->fwnode); ret = sd_fwnode == asd_dev_fwnode; @@ -116,12 +116,12 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, } static bool match_fwnode(struct v4l2_async_notifier *notifier, - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) + struct v4l2_subdev *sd, struct v4l2_async_match *match) { dev_dbg(sd->dev, "async: matching for notifier %pfw, sd %pfw\n", dev_fwnode(notifier_dev(notifier)), sd->fwnode); - if (match_fwnode_one(notifier, sd, sd->fwnode, asd)) + if (match_fwnode_one(notifier, sd, sd->fwnode, match)) return true; /* Also check the secondary fwnode. */ @@ -130,7 +130,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier, dev_dbg(sd->dev, "async: trying secondary fwnode match\n"); - return match_fwnode_one(notifier, sd, sd->fwnode->secondary, asd); + return match_fwnode_one(notifier, sd, sd->fwnode->secondary, match); } static LIST_HEAD(subdev_list); @@ -142,7 +142,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd) { bool (*match)(struct v4l2_async_notifier *notifier, - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd); + struct v4l2_subdev *sd, struct v4l2_async_match *match); struct v4l2_async_subdev *asd; list_for_each_entry(asd, ¬ifier->waiting, list) { @@ -161,7 +161,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, } /* match cannot be NULL here */ - if (match(notifier, sd, asd)) + if (match(notifier, sd, &asd->match)) return asd; } @@ -169,20 +169,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, } /* Compare two async sub-device descriptors for equivalence */ -static bool asd_equal(struct v4l2_async_subdev *asd_x, - struct v4l2_async_subdev *asd_y) +static bool asd_equal(struct v4l2_async_match *match1, + struct v4l2_async_match *match2) { - if (asd_x->match.type != asd_y->match.type) + if (match1->type != match2->type) return false; - switch (asd_x->match.type) { + switch (match1->type) { case V4L2_ASYNC_MATCH_I2C: - return asd_x->match.i2c.adapter_id == - asd_y->match.i2c.adapter_id && - asd_x->match.i2c.address == - asd_y->match.i2c.address; + return match1->i2c.adapter_id == match2->i2c.adapter_id && + match1->i2c.address == match2->i2c.address; case V4L2_ASYNC_MATCH_FWNODE: - return asd_x->match.fwnode == asd_y->match.fwnode; + return match1->fwnode == match2->fwnode; default: break; } @@ -434,20 +432,20 @@ v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, /* See if an async sub-device can be found in a notifier's lists. */ static bool __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, - struct v4l2_async_subdev *asd) + struct v4l2_async_match *match) { - struct v4l2_async_subdev *asd_y; + struct v4l2_async_subdev *asd; struct v4l2_subdev *sd; - list_for_each_entry(asd_y, ¬ifier->waiting, list) - if (asd_equal(asd, asd_y)) + list_for_each_entry(asd, ¬ifier->waiting, list) + if (asd_equal(&asd->match, match)) return true; list_for_each_entry(sd, ¬ifier->done, async_list) { if (WARN_ON(!sd->asd)) continue; - if (asd_equal(asd, sd->asd)) + if (asd_equal(&sd->asd->match, match)) return true; } @@ -460,49 +458,50 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, */ static bool v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, - struct v4l2_async_subdev *asd, bool skip_self) + struct v4l2_async_match *match, bool skip_self) { - struct v4l2_async_subdev *asd_y; + struct v4l2_async_subdev *asd; lockdep_assert_held(&list_lock); /* Check that an asd is not being added more than once. */ - list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) { - if (asd == asd_y) + list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { + if (&asd->match == match) break; - if (asd_equal(asd, asd_y)) + if (asd_equal(&asd->match, match)) return true; } /* Check that an asd does not exist in other notifiers. */ list_for_each_entry(notifier, ¬ifier_list, list) - if (__v4l2_async_nf_has_async_subdev(notifier, asd)) + if (__v4l2_async_nf_has_async_subdev(notifier, match)) return true; return false; } static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, - struct v4l2_async_subdev *asd, + struct v4l2_async_match *match, bool skip_self) { struct device *dev = notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL; - if (!asd) + if (!match) return -EINVAL; - switch (asd->match.type) { + switch (match->type) { case V4L2_ASYNC_MATCH_I2C: case V4L2_ASYNC_MATCH_FWNODE: - if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { + if (v4l2_async_nf_has_async_subdev(notifier, match, + skip_self)) { dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n"); return -EEXIST; } break; default: - dev_err(dev, "Invalid match type %u on %p\n", - asd->match.type, asd); + dev_err(dev, "Invalid match type %u on %p\n", match->type, + match); return -EINVAL; } @@ -526,7 +525,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) mutex_lock(&list_lock); list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { - ret = v4l2_async_nf_asd_valid(notifier, asd, true); + ret = v4l2_async_nf_asd_valid(notifier, &asd->match, true); if (ret) goto err_unlock; @@ -659,7 +658,7 @@ int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier, mutex_lock(&list_lock); - ret = v4l2_async_nf_asd_valid(notifier, asd, false); + ret = v4l2_async_nf_asd_valid(notifier, &asd->match, false); if (ret) goto unlock; @@ -846,15 +845,15 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) EXPORT_SYMBOL(v4l2_async_unregister_subdev); static void print_waiting_subdev(struct seq_file *s, - struct v4l2_async_subdev *asd) + struct v4l2_async_match *match) { - switch (asd->match.type) { + switch (match->type) { case V4L2_ASYNC_MATCH_I2C: - seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id, - asd->match.i2c.address); + seq_printf(s, " [i2c] dev=%d-%04x\n", match->i2c.adapter_id, + match->i2c.address); break; case V4L2_ASYNC_MATCH_FWNODE: { - struct fwnode_handle *devnode, *fwnode = asd->match.fwnode; + struct fwnode_handle *devnode, *fwnode = match->fwnode; devnode = fwnode_graph_is_endpoint(fwnode) ? fwnode_graph_get_port_parent(fwnode) : @@ -891,7 +890,7 @@ static int pending_subdevs_show(struct seq_file *s, void *data) list_for_each_entry(notif, ¬ifier_list, list) { seq_printf(s, "%s:\n", v4l2_async_nf_name(notif)); list_for_each_entry(asd, ¬if->waiting, list) - print_waiting_subdev(s, asd); + print_waiting_subdev(s, &asd->match); } mutex_unlock(&list_lock);
Pass only information required for sub-device matching to functions checking whether the async sub-device already exists. Do the same for debug message printing. This makes further changes to other aspects of async sub-devices easier. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-async.c | 93 ++++++++++++++-------------- 1 file changed, 46 insertions(+), 47 deletions(-)