| Message ID | 20210128120945.5062-15-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | V4L2 Async notifier API cleanup | expand |
On 1/28/21 9:09 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. > > Document functions that drivers should use, and their purpose. > > 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 | 41 ++++++++++++++++--- > include/media/v4l2-async.h | 15 +++++-- > 2 files changed, 47 insertions(+), 9 deletions(-) > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > index 0e82c77cf3e2..a6b82b9c8210 100644 > --- a/Documentation/driver-api/media/v4l2-subdev.rst > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > @@ -201,11 +201,42 @@ 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 and the needs of the > +driver. > + > +:c:func:`v4l2_async_register_subdev_sensor_common` is a helper function for > +sensor drivers registering their own async sub-device, but it also supports > +registering lens and flash devices. The function registers an async notifier for > +the sub-device which is unregistered with the async sub-device. > + > +:c:func:`v4l2_async_notifier_add_fwnode_remote_subdev`, > +:c:func:`v4l2_async_notifier_add_fwnode_subdev` and > +:c:func:`v4l2_async_notifier_add_i2c_subdev` are for bridge and ISP drivers for > +registering their async sub-devices. If I understand correctly, these functions are for bridge and ISP drivers to tell the framework they are waiting asynchronously for another sub-device. I wonder if this could be re-phrased a bit to convey that. Shouldn't __v4l2_async_notifier_add_subdev() and v4l2_async_notifier_parse_fwnode_endpoints() also be mentioned here? Or maybe just don't mention __v4l2_async_notifier_add_subdev() here to discourage its usage. I see that v4l2_async_notifier_parse_fwnode_endpoints() is only used by sun6i_csi.c, I wonder if sun6i is a special case of if we could use one of those 3 functions instead and discourage the usage of v4l2_async_notifier_parse_fwnode_endpoints() as well. > + > +These functions allocate an async sub-device descriptor which is of type struct > +:c:type:`v4l2_async_subdev` embedded in a driver-specific 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 fwnode_handle *ep; > + > + ... > + > + my_asd = v4l2_async_notifier_add_fwnode_remote_subdev(¬ifier, ep, > + struct my_async_subdev); > + fwnode_handle_put(ep); > + > + if (IS_ERR(asd)) > + return PTR_ERR(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 b94f0a0a8042..6dac6cb6290f 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_remote_subdev, > + * @v4l2_async_notifier_add_fwnode_subdev, > + * @v4l2_async_notifier_add_i2c_subdev, > + * @v4l2_async_notifier_add_subdev or v4l2_async_notifier_add_subdev() was renamed on patch 12/14. Maybe just don't mention it here to discourage its usage? > + * @v4l2_async_notifier_parse_fwnode_endpoints. > */ > void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier); > > @@ -262,9 +267,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_remote_subdev, > + * @v4l2_async_notifier_add_fwnode_subdev, > + * @v4l2_async_notifier_add_i2c_subdev, > + * @v4l2_async_notifier_add_subdev or Same here. Thanks Helen > + * @v4l2_async_notifier_parse_fwnode_endpoints. > * > * There is no harm from calling v4l2_async_notifier_cleanup in other > * cases as long as its memory has been zeroed after it has been >
Hi Helen, Thanks for reviewing the series! It's a tough one, with plenty of tiny details that are so easy to miss. On Mon, 2021-02-01 at 17:17 -0300, Helen Koike wrote: > > > On 1/28/21 9:09 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. > > > > Document functions that drivers should use, and their purpose. > > > > 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 | 41 ++++++++++++++++--- > > include/media/v4l2-async.h | 15 +++++-- > > 2 files changed, 47 insertions(+), 9 deletions(-) > > > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > > index 0e82c77cf3e2..a6b82b9c8210 100644 > > --- a/Documentation/driver-api/media/v4l2-subdev.rst > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > > @@ -201,11 +201,42 @@ 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 and the needs of the > > +driver. > > + > > +:c:func:`v4l2_async_register_subdev_sensor_common` is a helper function for > > +sensor drivers registering their own async sub-device, but it also supports > > +registering lens and flash devices. The function registers an async notifier for > > +the sub-device which is unregistered with the async sub-device. > > + > > +:c:func:`v4l2_async_notifier_add_fwnode_remote_subdev`, > > +:c:func:`v4l2_async_notifier_add_fwnode_subdev` and > > +:c:func:`v4l2_async_notifier_add_i2c_subdev` are for bridge and ISP drivers for > > +registering their async sub-devices. > > If I understand correctly, these functions are for bridge and ISP drivers to tell > the framework they are waiting asynchronously for another sub-device. > I wonder if this could be re-phrased a bit to convey that. > That's a good point. Any suggestions for how to phrase this better? > Shouldn't __v4l2_async_notifier_add_subdev() and v4l2_async_notifier_parse_fwnode_endpoints() > also be mentioned here? > Or maybe just don't mention __v4l2_async_notifier_add_subdev() here to discourage its usage. > The plan is to make __v4l2_async_notifier_add_subdev private or at least not exported. We don't really want it to be part of the API. > I see that v4l2_async_notifier_parse_fwnode_endpoints() is only used by sun6i_csi.c, > I wonder if sun6i is a special case of if we could use one of those 3 functions instead > and discourage the usage of v4l2_async_notifier_parse_fwnode_endpoints() as well. > The plan here is to convert sun6i_csi.c and then just drop v4l2_async_notifier_parse_fwnode_endpoints(). > > + > > +These functions allocate an async sub-device descriptor which is of type struct > > +:c:type:`v4l2_async_subdev` embedded in a driver-specific 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 fwnode_handle *ep; > > + > > + ... > > + > > + my_asd = v4l2_async_notifier_add_fwnode_remote_subdev(¬ifier, ep, > > + struct my_async_subdev); > > + fwnode_handle_put(ep); > > + > > + if (IS_ERR(asd)) > > + return PTR_ERR(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 b94f0a0a8042..6dac6cb6290f 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_remote_subdev, > > + * @v4l2_async_notifier_add_fwnode_subdev, > > + * @v4l2_async_notifier_add_i2c_subdev, > > + * @v4l2_async_notifier_add_subdev or > > v4l2_async_notifier_add_subdev() was renamed on patch 12/14. > > Maybe just don't mention it here to discourage its usage? > Indeed, I don't think we should mention __v4l2_async_notifier_add_subdev anymore. Thanks a lot! Ezequiel
Hi Helen, On Mon, Feb 01, 2021 at 05:17:15PM -0300, Helen Koike wrote: > > > On 1/28/21 9:09 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. > > > > Document functions that drivers should use, and their purpose. > > > > 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 | 41 ++++++++++++++++--- > > include/media/v4l2-async.h | 15 +++++-- > > 2 files changed, 47 insertions(+), 9 deletions(-) > > > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > > index 0e82c77cf3e2..a6b82b9c8210 100644 > > --- a/Documentation/driver-api/media/v4l2-subdev.rst > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > > @@ -201,11 +201,42 @@ 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 and the needs of the > > +driver. > > + > > +:c:func:`v4l2_async_register_subdev_sensor_common` is a helper function for > > +sensor drivers registering their own async sub-device, but it also supports > > +registering lens and flash devices. The function registers an async notifier for > > +the sub-device which is unregistered with the async sub-device. > > + > > +:c:func:`v4l2_async_notifier_add_fwnode_remote_subdev`, > > +:c:func:`v4l2_async_notifier_add_fwnode_subdev` and > > +:c:func:`v4l2_async_notifier_add_i2c_subdev` are for bridge and ISP drivers for > > +registering their async sub-devices. > > If I understand correctly, these functions are for bridge and ISP drivers to tell > the framework they are waiting asynchronously for another sub-device. > I wonder if this could be re-phrased a bit to convey that. This applies to all async sub-devices, also those registered indirectly by the sensor driver. I'll see if I could improve it for v5. > Shouldn't __v4l2_async_notifier_add_subdev() and v4l2_async_notifier_parse_fwnode_endpoints() > also be mentioned here? > Or maybe just don't mention __v4l2_async_notifier_add_subdev() here to discourage its usage. > > I see that v4l2_async_notifier_parse_fwnode_endpoints() is only used by sun6i_csi.c, > I wonder if sun6i is a special case of if we could use one of those 3 functions instead > and discourage the usage of v4l2_async_notifier_parse_fwnode_endpoints() as well. I left out these two because the former is not intended to be used by drivers and the latter is deprecated. Once the sun6i driver is converted, the function can be removed. > > > + > > +These functions allocate an async sub-device descriptor which is of type struct > > +:c:type:`v4l2_async_subdev` embedded in a driver-specific 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 fwnode_handle *ep; > > + > > + ... > > + > > + my_asd = v4l2_async_notifier_add_fwnode_remote_subdev(¬ifier, ep, > > + struct my_async_subdev); > > + fwnode_handle_put(ep); > > + > > + if (IS_ERR(asd)) > > + return PTR_ERR(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 b94f0a0a8042..6dac6cb6290f 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_remote_subdev, > > + * @v4l2_async_notifier_add_fwnode_subdev, > > + * @v4l2_async_notifier_add_i2c_subdev, > > + * @v4l2_async_notifier_add_subdev or > > v4l2_async_notifier_add_subdev() was renamed on patch 12/14. > > Maybe just don't mention it here to discourage its usage? I wanted to keep it here since it is possible to use it, and using it requires initialising and cleaning up. The documentation also applies to the framework. The ReST documentation is more driver developer oriented. > > > + * @v4l2_async_notifier_parse_fwnode_endpoints. > > */ > > void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier); > > > > @@ -262,9 +267,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_remote_subdev, > > + * @v4l2_async_notifier_add_fwnode_subdev, > > + * @v4l2_async_notifier_add_i2c_subdev, > > + * @v4l2_async_notifier_add_subdev or > > Same here. > > > Thanks > Helen > > > + * @v4l2_async_notifier_parse_fwnode_endpoints. > > * > > * There is no harm from calling v4l2_async_notifier_cleanup in other > > * cases as long as its memory has been zeroed after it has been > > -- Kind regards, Sakari Ailus
diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst index 0e82c77cf3e2..a6b82b9c8210 100644 --- a/Documentation/driver-api/media/v4l2-subdev.rst +++ b/Documentation/driver-api/media/v4l2-subdev.rst @@ -201,11 +201,42 @@ 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 and the needs of the +driver. + +:c:func:`v4l2_async_register_subdev_sensor_common` is a helper function for +sensor drivers registering their own async sub-device, but it also supports +registering lens and flash devices. The function registers an async notifier for +the sub-device which is unregistered with the async sub-device. + +:c:func:`v4l2_async_notifier_add_fwnode_remote_subdev`, +:c:func:`v4l2_async_notifier_add_fwnode_subdev` and +:c:func:`v4l2_async_notifier_add_i2c_subdev` are for bridge and ISP drivers for +registering their async sub-devices. + +These functions allocate an async sub-device descriptor which is of type struct +:c:type:`v4l2_async_subdev` embedded in a driver-specific 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 fwnode_handle *ep; + + ... + + my_asd = v4l2_async_notifier_add_fwnode_remote_subdev(¬ifier, ep, + struct my_async_subdev); + fwnode_handle_put(ep); + + if (IS_ERR(asd)) + return PTR_ERR(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 b94f0a0a8042..6dac6cb6290f 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_remote_subdev, + * @v4l2_async_notifier_add_fwnode_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); @@ -262,9 +267,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_remote_subdev, + * @v4l2_async_notifier_add_fwnode_subdev, + * @v4l2_async_notifier_add_i2c_subdev, + * @v4l2_async_notifier_add_subdev or + * @v4l2_async_notifier_parse_fwnode_endpoints. * * There is no harm from calling v4l2_async_notifier_cleanup in other * cases as long as its memory has been zeroed after it has been