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@xxxxxxxxxxxxxxx>
> > ---
> > 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);
>
--
Regards,
Sakari Ailus
