Re: [PATCH v12 15/26] v4l: async: Allow binding notifiers to sub-devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Hans,

On Wed, Sep 13, 2017 at 09:17:08AM +0200, Hans Verkuil wrote:
> On 09/12/2017 03:41 PM, Sakari Ailus wrote:
> > Registering a notifier has required the knowledge of struct v4l2_device
> > for the reason that sub-devices generally are registered to the
> > v4l2_device (as well as the media device, also available through
> > v4l2_device).
> > 
> > This information is not available for sub-device drivers at probe time.
> > 
> > What this patch does is that it allows registering notifiers without
> > having v4l2_device around. Instead the sub-device pointer is stored in the
> > notifier. Once the sub-device of the driver that registered the notifier
> > is registered, the notifier will gain the knowledge of the v4l2_device,
> > and the binding of async sub-devices from the sub-device driver's notifier
> > may proceed.
> > 
> > The root notifier's complete callback is only called when all sub-device
> > notifiers are completed.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> 
> Just two small comments (see below).
> 
> After changing those (the first is up to you) you can add my:
> 
> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>

Thanks; please see my comments below.

...

> > +/* Return true if all sub-device notifiers are complete, false otherwise. */
> > +static bool v4l2_async_subdev_notifiers_complete(
> > +	struct v4l2_async_notifier *notifier)
> > +{
> > +	struct v4l2_subdev *sd;
> > +
> > +	if (!list_empty(&notifier->waiting))
> > +		return false;
> > +
> > +	list_for_each_entry(sd, &notifier->done, async_list) {
> > +		struct v4l2_async_notifier *subdev_notifier =
> > +			v4l2_async_find_subdev_notifier(sd);
> > +
> > +		if (!subdev_notifier)
> > +			continue;
> > +
> > +		if (!v4l2_async_subdev_notifiers_complete(subdev_notifier))
> 
> I think it is better to change this to:
> 
> 		if (subdev_notifier &&
> 		    !v4l2_async_subdev_notifiers_complete(subdev_notifier))
> 
> and drop this if..continue above. That's a bit overkill in this simple case.
> 
> It's up to you, though.

Yes, makes sense.

...

> > +/* Try completing a notifier. */
> > +static int v4l2_async_notifier_try_complete(
> > +	struct v4l2_async_notifier *notifier)
> > +{
> > +	do {
> > +		int ret;
> > +
> > +		/* Any local async sub-devices left? */
> > +		if (!list_empty(&notifier->waiting))
> > +			return 0;
> > +
> > +		/*
> > +		 * Any sub-device notifiers waiting for async subdevs
> > +		 * to be bound?
> > +		 */
> > +		if (!v4l2_async_subdev_notifiers_complete(notifier))
> > +			return 0;
> > +
> > +		/* Proceed completing the notifier */
> > +		ret = v4l2_async_notifier_call_complete(notifier);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/*
> > +		 * Obtain notifier's parent. If there is one, repeat
> > +		 * the process, otherwise we're done here.
> > +		 */
> > +	} while ((notifier = notifier->parent));
> 
> I'd change this to:
> 
> 		notifier = notifier->parent;
> 	} while (notifier);
> 
> Assignment in a condition is frowned upon, and there is no need to do that
> here.

Wouldn't that equally apply to any statement with side effects? In other
words, what you're suggesting for patch 19? :-)

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux