Re: [PATCH v6 02/17] media: v4l2: async: Allow searching for asd of any type

[Steve Longerbeam]

Hi Mauro,


On 09/24/2018 10:06 AM, Mauro Carvalho Chehab wrote:
> Em Mon,  9 Jul 2018 15:39:02 -0700
> Steve Longerbeam <slongerbeam@xxxxxxxxx> escreveu:
>
> > Generalize v4l2_async_notifier_fwnode_has_async_subdev() to allow
> > searching for any type of async subdev, not just fwnodes. Rename to
> > v4l2_async_notifier_has_async_subdev() and pass it an asd pointer.
> >
> > Signed-off-by: Steve Longerbeam <steve_longerbeam@xxxxxxxxxx>
> > ---
> > Changes since v5:
> > - none
> > Changes since v4:
> > - none
> > Changes since v3:
> > - removed TODO to support asd compare with CUSTOM match type in
> >    asd_equal().
> > Changes since v2:
> > - code optimization in asd_equal(), and remove unneeded braces,
> >    suggested by Sakari Ailus.
> > Changes since v1:
> > - none
> > ---
> >   drivers/media/v4l2-core/v4l2-async.c | 73 +++++++++++++++++++++---------------
> >   1 file changed, 43 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 2b08d03..0e7e529 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -124,6 +124,31 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
> >   	return NULL;
> >   }
> >   
> > +/* Compare two asd's for equivalence */
> Please, on comments, instead of "asd" prefer to use what this 3 random
> letters mean, e. g.:
> 	asd -> asynchronous subdevice

Ok, I will change the comment to read:

/* Compare two async subdevice descriptors for equivalence */

> > +static bool asd_equal(struct v4l2_async_subdev *asd_x,
> > +		      struct v4l2_async_subdev *asd_y)
> > +{
> > +	if (asd_x->match_type != asd_y->match_type)
> > +		return false;
> > +
> > +	switch (asd_x->match_type) {
> > +	case V4L2_ASYNC_MATCH_DEVNAME:
> > +		return strcmp(asd_x->match.device_name,
> > +			      asd_y->match.device_name) == 0;
> > +	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;
> > +	case V4L2_ASYNC_MATCH_FWNODE:
> > +		return asd_x->match.fwnode == asd_y->match.fwnode;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >   /* Find the sub-device notifier registered by a sub-device driver. */
> >   static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> >   	struct v4l2_subdev *sd)
> > @@ -308,29 +333,22 @@ static void v4l2_async_notifier_unbind_all_subdevs(
> >   	notifier->parent = NULL;
> >   }
> >   
> > -/* See if an fwnode can be found in a notifier's lists. */
> > -static bool __v4l2_async_notifier_fwnode_has_async_subdev(
> > -	struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode)
> > +/* See if an async sub-device can be found in a notifier's lists. */
> > +static bool __v4l2_async_notifier_has_async_subdev(
> > +	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd)
> This is a minor issue, but checkpatch complains (with reason)
> (with --strict) about the above:
>
> 	CHECK: Lines should not end with a '('
> 	#63: FILE: drivers/media/v4l2-core/v4l2-async.c:337:
> 	+static bool __v4l2_async_notifier_has_async_subdev(
>
> Better to declare it, instead, as:
>
> static bool
> __v4l2_async_notifier_has_async_subdev(struct v4l2_async_notifier *notifier,
> 				       struct v4l2_async_subdev *asd)
>
> Similar warnings appear on other places:
> CHECK: Lines should not end with a '('
> #102: FILE: drivers/media/v4l2-core/v4l2-async.c:362:
> +static bool v4l2_async_notifier_has_async_subdev(
>
> CHECK: Lines should not end with a '('
> #141: FILE: drivers/media/v4l2-core/v4l2-async.c:410:
> +			if (v4l2_async_notifier_has_async_subdev(

Will fix.

> Btw, Checkpatch also complains that the author's email is different
> than the SOB's one:
>
> WARNING: Missing Signed-off-by: line by nominal patch author 'Steve Longerbeam <slongerbeam@xxxxxxxxx>'
>
> (the some comes with Signed-off-by: Steve Longerbeam <steve_longerbeam@xxxxxxxxxx>)
>
> I suspect that other patches on this series will suffer from the same issue.

Will fix when submitting v7.

> <snip>
>
> > @@ -392,12 +406,11 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> >   		case V4L2_ASYNC_MATCH_CUSTOM:
> >   		case V4L2_ASYNC_MATCH_DEVNAME:
> >   		case V4L2_ASYNC_MATCH_I2C:
> > -			break;
> >   		case V4L2_ASYNC_MATCH_FWNODE:
> > -			if (v4l2_async_notifier_fwnode_has_async_subdev(
> > -				    notifier, asd->match.fwnode, i)) {
> > +			if (v4l2_async_notifier_has_async_subdev(
> > +				    notifier, asd, i)) {
> >   				dev_err(dev,
> > -					"fwnode has already been registered or in notifier's subdev list\n");
> > +					"asd has already been registered or in notifier's subdev list\n");
> Please, never use "asd" on messages printed to the user. While someone
> may understand it while reading the source code, for a poor use,
> "asd" is just a random sequence of 3 characters.

I will change the message to read:

"subdev descriptor already listed in this or other notifiers".


Steve