Re: [PATCH v3 12/14] media: Clarify v4l2-async subdevice addition API

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Helen,

Thanks for the review.

On Mon, Jan 25, 2021 at 02:22:37PM -0300, Helen Koike wrote:
> 
> 
> On 1/25/21 10:22 AM, Sakari Ailus wrote:
> > From: Ezequiel Garcia <ezequiel@collabora.com>
> > 
> > Now that most users of v4l2_async_notifier_add_subdev have
> > been converted, let's fix the documentation so it's more clear
> > how the v4l2-async API should be used.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  .../driver-api/media/v4l2-subdev.rst          | 38 ++++++++++++++++---
> >  include/media/v4l2-async.h                    | 15 ++++++--
> >  2 files changed, 44 insertions(+), 9 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> > index 0e82c77cf3e2..490dd212345d 100644
> > --- a/Documentation/driver-api/media/v4l2-subdev.rst
> > +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> > @@ -201,11 +201,39 @@ Before registering the notifier, bridge drivers must do two things:
> >  first, the notifier must be initialized using the
> >  :c:func:`v4l2_async_notifier_init`. Second, bridge drivers can then
> >  begin to form a list of subdevice descriptors that the bridge device
> > -needs for its operation. Subdevice descriptors are added to the notifier
> > -using the :c:func:`v4l2_async_notifier_add_subdev` call. This function
> > -takes two arguments: a pointer to struct :c:type:`v4l2_async_notifier`,
> > -and a pointer to the subdevice descripter, which is of type struct
> > -:c:type:`v4l2_async_subdev`.
> > +needs for its operation. Several functions are available, to add subdevice
> > +descriptors to a notifier, depending on the type of device:
> > +:c:func:`v4l2_async_notifier_add_fwnode_subdev`,
> > +:c:func:`v4l2_async_notifier_add_fwnode_remote_subdev` or
> > +:c:func:`v4l2_async_notifier_add_i2c_subdev`.
> > +
> > +These functions allocate a subdevice descriptor, which is of
> > +type struct :c:type:`v4l2_async_subdev`, and take a size argument
> > +which can be used to embed the descriptor in a driver-specific
> > +async subdevice struct. The &struct :c:type:`v4l2_async_subdev`
> > +shall be the first member of this struct:
> > +
> > +.. code-block:: c
> > +
> > +	struct my_async_subdev {
> > +		struct v4l2_async_subdev asd;
> > +		...
> > +	};
> > +
> > +	struct my_async_subdev *my_asd;
> > +	struct v4l2_async_subdev *asd;
> > +	struct fwnode_handle *ep;
> > +
> > +	...
> > +
> > +	asd = v4l2_async_notifier_add_fwnode_subdev(
> > +			&notifier, ep, sizeof(*my_asd));
> > +	fwnode_handle_put(ep);
> > +
> > +	if (IS_ERR(asd))
> > +		return PTR_ERR(asd);
> > +
> > +	my_asd = container_of(asd, struct my_async_subdev, asd);
> >  
> >  The V4L2 core will then use these descriptors to match asynchronously
> >  registered subdevices to them. If a match is detected the ``.bound()``
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index f2cac0931372..25c9ebd3f963 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -128,7 +128,12 @@ void v4l2_async_debug_init(struct dentry *debugfs_dir);
> >   * @notifier: pointer to &struct v4l2_async_notifier
> >   *
> >   * This function initializes the notifier @asd_list. It must be called
> > - * before the first call to @v4l2_async_notifier_add_subdev.
> > + * before adding a subdevice to a notifier, using one of:
> > + * @v4l2_async_notifier_add_fwnode_subdev,
> > + * @v4l2_async_register_subdev_sensor_common,
> 
> This function doesn't receive a notifier, it is used by the sensor at probe's
> time to inform the framework this sensor is present (if I understand correctly).
> So it could be called before the isp (who is registering a notifier) is probed.
> 
> There is no need to call v4l2_async_notifier_init() before
> v4l2_async_register_subdev_sensor_common() in my understanding.

Yeah... I guess I shouldn't just write this as I recall from years ago. X-)

Anyway, I added also v4l2_async_notifier_add_fwnode_remote_subdev.

> 
> > + * @v4l2_async_notifier_add_i2c_subdev,
> > + * @v4l2_async_notifier_add_subdev or
> > + * @v4l2_async_notifier_parse_fwnode_endpoints.
> >   */
> >  void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);
> >  
> > @@ -248,9 +253,11 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
> >   * sub-devices allocated for the purposes of the notifier but not the notifier
> >   * itself. The user is responsible for calling this function to clean up the
> >   * notifier after calling
> > - * @v4l2_async_notifier_add_subdev,
> > - * @v4l2_async_notifier_parse_fwnode_endpoints or
> > - * @v4l2_async_register_subdev_sensor_common.
> > + * @v4l2_async_notifier_add_fwnode_subdev,
> > + * @v4l2_async_register_subdev_sensor_common,
> 
> The user of v4l2_async_register_*() is not responsible
> for cleaning up the notifier with v4l2_async_notifier_unregister(),
> it calls v4l2_async_unregister_*() instead
> (or am I missing something?)

Agreed. Added the same v4l2_async_notifier_add_fwnode_remote_subdev here,
too.

-- 
Kind regards,

Sakari Ailus