[PATCH 1/3] media: docs: v4l2-subdev: fix typo

[PATCH 1/3] media: docs: v4l2-subdev: fix typo

[Luca Ceresoli]

Fix "will to" -> "will do".

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 Documentation/driver-api/media/v4l2-subdev.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
index bc7e1fc40a9d..9a7dddd97f5a 100644
--- a/Documentation/driver-api/media/v4l2-subdev.rst
+++ b/Documentation/driver-api/media/v4l2-subdev.rst
@@ -191,7 +191,7 @@ but it is better and easier to use this macro:
 
 	err = v4l2_subdev_call(sd, core, g_std, &norm);
 
-The macro will to the right ``NULL`` pointer checks and returns ``-ENODEV``
+The macro will do the right ``NULL`` pointer checks and returns ``-ENODEV``
 if :c:type:`sd <v4l2_subdev>` is ``NULL``, ``-ENOIOCTLCMD`` if either
 :c:type:`sd <v4l2_subdev>`->core or :c:type:`sd <v4l2_subdev>`->core->g_std is ``NULL``, or the actual result of the
 :c:type:`sd <v4l2_subdev>`->ops->core->g_std ops.
-- 
2.28.0

[PATCH 2/3] media: docs: v4l2-subdev: move "Subdev registration" to a subsection

[Luca Ceresoli]

The subdev registration topic is pretty lengthy, and takes more than
half of the "V4L2 sub-devices" section. Help readers in finding their
way through the document by dedicating a subsection to
"Subdev registration".

Each of the two registration methods takes many paragraphs, but since
adding a subsubsection would be overkill, just emphasize them in bold.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 Documentation/driver-api/media/v4l2-subdev.rst | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
index 9a7dddd97f5a..fb66163deb38 100644
--- a/Documentation/driver-api/media/v4l2-subdev.rst
+++ b/Documentation/driver-api/media/v4l2-subdev.rst
@@ -138,6 +138,9 @@ ensures that width, height and the media bus pixel code are equal on both source
 and sink of the link. Subdev drivers are also free to use this function to
 perform the checks mentioned above in addition to their own checks.
 
+Subdev registration
+~~~~~~~~~~~~~~~~~~~
+
 There are currently two ways to register subdevices with the V4L2 core. The
 first (traditional) possibility is to have subdevices registered by bridge
 drivers. This can be done when the bridge driver has the complete information
@@ -157,7 +160,7 @@ below.
 Using one or the other registration method only affects the probing process, the
 run-time bridge-subdevice interaction is in both cases the same.
 
-In the synchronous case a device (bridge) driver needs to register the
+In the **synchronous** case a device (bridge) driver needs to register the
 :c:type:`v4l2_subdev` with the v4l2_device:
 
 	:c:func:`v4l2_device_register_subdev <v4l2_device_register_subdev>`
@@ -238,9 +241,9 @@ contain several subdevs that use an I2C bus, but also a subdev that is
 controlled through GPIO pins. This distinction is only relevant when setting
 up the device, but once the subdev is registered it is completely transparent.
 
-In the asynchronous case subdevice probing can be invoked independently of the
-bridge driver availability. The subdevice driver then has to verify whether all
-the requirements for a successful probing are satisfied. This can include a
+In the **asynchronous** case subdevice probing can be invoked independently of
+the bridge driver availability. The subdevice driver then has to verify whether
+all the requirements for a successful probing are satisfied. This can include a
 check for a master clock availability. If any of the conditions aren't satisfied
 the driver might decide to return ``-EPROBE_DEFER`` to request further reprobing
 attempts. Once all conditions are met the subdevice shall be registered using
-- 
2.28.0

Re: [PATCH 2/3] media: docs: v4l2-subdev: move "Subdev registration" to a subsection

[Jacopo Mondi]

Hi Luca,

On Fri, Sep 04, 2020 at 11:51:40PM +0200, Luca Ceresoli wrote:
> The subdev registration topic is pretty lengthy, and takes more than
> half of the "V4L2 sub-devices" section. Help readers in finding their
> way through the document by dedicating a subsection to
> "Subdev registration".
>
> Each of the two registration methods takes many paragraphs, but since
> adding a subsubsection would be overkill, just emphasize them in bold.

I see bold being used mostly for C symbols names, I don't know if
that's a kind of convention or not.

I would not mind a subsection to be honest...
I think the patch is good and I'll provide a tag, maybe others can
provide opinions too!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> ---
>  Documentation/driver-api/media/v4l2-subdev.rst | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> index 9a7dddd97f5a..fb66163deb38 100644
> --- a/Documentation/driver-api/media/v4l2-subdev.rst
> +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> @@ -138,6 +138,9 @@ ensures that width, height and the media bus pixel code are equal on both source
>  and sink of the link. Subdev drivers are also free to use this function to
>  perform the checks mentioned above in addition to their own checks.
>
> +Subdev registration
> +~~~~~~~~~~~~~~~~~~~
> +
>  There are currently two ways to register subdevices with the V4L2 core. The
>  first (traditional) possibility is to have subdevices registered by bridge
>  drivers. This can be done when the bridge driver has the complete information
> @@ -157,7 +160,7 @@ below.
>  Using one or the other registration method only affects the probing process, the
>  run-time bridge-subdevice interaction is in both cases the same.
>
> -In the synchronous case a device (bridge) driver needs to register the
> +In the **synchronous** case a device (bridge) driver needs to register the
>  :c:type:`v4l2_subdev` with the v4l2_device:
>
>  	:c:func:`v4l2_device_register_subdev <v4l2_device_register_subdev>`
> @@ -238,9 +241,9 @@ contain several subdevs that use an I2C bus, but also a subdev that is
>  controlled through GPIO pins. This distinction is only relevant when setting
>  up the device, but once the subdev is registered it is completely transparent.
>
> -In the asynchronous case subdevice probing can be invoked independently of the
> -bridge driver availability. The subdevice driver then has to verify whether all
> -the requirements for a successful probing are satisfied. This can include a
> +In the **asynchronous** case subdevice probing can be invoked independently of
> +the bridge driver availability. The subdevice driver then has to verify whether
> +all the requirements for a successful probing are satisfied. This can include a
>  check for a master clock availability. If any of the conditions aren't satisfied
>  the driver might decide to return ``-EPROBE_DEFER`` to request further reprobing
>  attempts. Once all conditions are met the subdevice shall be registered using
> --
> 2.28.0
>

Re: [PATCH 2/3] media: docs: v4l2-subdev: move "Subdev registration" to a subsection

[Luca Ceresoli]

Hi Jacopo,

On 15/09/20 15:45, Jacopo Mondi wrote:
> Hi Luca,
> 
> On Fri, Sep 04, 2020 at 11:51:40PM +0200, Luca Ceresoli wrote:
>> The subdev registration topic is pretty lengthy, and takes more than
>> half of the "V4L2 sub-devices" section. Help readers in finding their
>> way through the document by dedicating a subsection to
>> "Subdev registration".
>>
>> Each of the two registration methods takes many paragraphs, but since
>> adding a subsubsection would be overkill, just emphasize them in bold.
> 
> I see bold being used mostly for C symbols names, I don't know if
> that's a kind of convention or not.
> 
> I would not mind a subsection to be honest...
> I think the patch is good and I'll provide a tag, maybe others can
> provide opinions too!

I'd say it's a matter of taste, so I'll keep the patch as is unless
there are more votes for changing it.

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks,
-- 
Luca

[PATCH 3/3] media: docs: v4l2-subdev: move generic paragraph to the introduction

[Luca Ceresoli]

This paragraph provides generic information to explain what v4l2_subdev is
useful for. Placing it in the middle of paragraphs describing the details
of subdev registration does not make much sense. Move it near the beginning
of the section when the v4l2_subdev idea has just been introduced and
before going into its details.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 Documentation/driver-api/media/v4l2-subdev.rst | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
index fb66163deb38..1c1e3f9da142 100644
--- a/Documentation/driver-api/media/v4l2-subdev.rst
+++ b/Documentation/driver-api/media/v4l2-subdev.rst
@@ -12,6 +12,12 @@ Usually these are I2C devices, but not necessarily. In order to provide the
 driver with a consistent interface to these sub-devices the
 :c:type:`v4l2_subdev` struct (v4l2-subdev.h) was created.
 
+The advantage of using :c:type:`v4l2_subdev` is that it is a generic struct and
+does not contain any knowledge about the underlying hardware. So a driver might
+contain several subdevs that use an I2C bus, but also a subdev that is
+controlled through GPIO pins. This distinction is only relevant when setting
+up the device, but once the subdev is registered it is completely transparent.
+
 Each sub-device driver must have a :c:type:`v4l2_subdev` struct. This struct
 can be stand-alone for simple sub-devices or it might be embedded in a larger
 struct if more state information needs to be stored. Usually there is a
@@ -235,12 +241,6 @@ it can call ``v4l2_subdev_notify(sd, notification, arg)``. This macro checks
 whether there is a ``notify()`` callback defined and returns ``-ENODEV`` if not.
 Otherwise the result of the ``notify()`` call is returned.
 
-The advantage of using :c:type:`v4l2_subdev` is that it is a generic struct and
-does not contain any knowledge about the underlying hardware. So a driver might
-contain several subdevs that use an I2C bus, but also a subdev that is
-controlled through GPIO pins. This distinction is only relevant when setting
-up the device, but once the subdev is registered it is completely transparent.
-
 In the **asynchronous** case subdevice probing can be invoked independently of
 the bridge driver availability. The subdevice driver then has to verify whether
 all the requirements for a successful probing are satisfied. This can include a
-- 
2.28.0

Re: [PATCH 3/3] media: docs: v4l2-subdev: move generic paragraph to the introduction

[Jacopo Mondi]

Hi Luca,

On Fri, Sep 04, 2020 at 11:51:41PM +0200, Luca Ceresoli wrote:
> This paragraph provides generic information to explain what v4l2_subdev is
> useful for. Placing it in the middle of paragraphs describing the details
> of subdev registration does not make much sense. Move it near the beginning
> of the section when the v4l2_subdev idea has just been introduced and
> before going into its details.
>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> ---
>  Documentation/driver-api/media/v4l2-subdev.rst | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> index fb66163deb38..1c1e3f9da142 100644
> --- a/Documentation/driver-api/media/v4l2-subdev.rst
> +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> @@ -12,6 +12,12 @@ Usually these are I2C devices, but not necessarily. In order to provide the
>  driver with a consistent interface to these sub-devices the
>  :c:type:`v4l2_subdev` struct (v4l2-subdev.h) was created.
>
> +The advantage of using :c:type:`v4l2_subdev` is that it is a generic struct and
> +does not contain any knowledge about the underlying hardware. So a driver might
> +contain several subdevs that use an I2C bus, but also a subdev that is
> +controlled through GPIO pins. This distinction is only relevant when setting
> +up the device, but once the subdev is registered it is completely transparent.
> +
>  Each sub-device driver must have a :c:type:`v4l2_subdev` struct. This struct
>  can be stand-alone for simple sub-devices or it might be embedded in a larger
>  struct if more state information needs to be stored. Usually there is a
> @@ -235,12 +241,6 @@ it can call ``v4l2_subdev_notify(sd, notification, arg)``. This macro checks
>  whether there is a ``notify()`` callback defined and returns ``-ENODEV`` if not.
>  Otherwise the result of the ``notify()`` call is returned.
>
> -The advantage of using :c:type:`v4l2_subdev` is that it is a generic struct and
> -does not contain any knowledge about the underlying hardware. So a driver might
> -contain several subdevs that use an I2C bus, but also a subdev that is
> -controlled through GPIO pins. This distinction is only relevant when setting
> -up the device, but once the subdev is registered it is completely transparent.
> -

Have you considered moving the whole part that describes how to call
operations, which comes after the synchronous registration case to a
dedicated sub-section ? The above paragraph makes sense in the context
of describing why v4l2_subdev is advantageous as it abstract the
underlying details under a unified call interface.

This could become

V4L2 sub-devices
----------------

Intro
~~~~~

Registration
~~~~~~~~~~~~

**synchronous**
**asynchronous**

Operations call (or a better name :)
~~~~~~~~~~~~~~~

What do you think ?

Thanks
  j

>  In the **asynchronous** case subdevice probing can be invoked independently of
>  the bridge driver availability. The subdevice driver then has to verify whether
>  all the requirements for a successful probing are satisfied. This can include a
> --
> 2.28.0
>

Re: [PATCH 3/3] media: docs: v4l2-subdev: move generic paragraph to the introduction

[Luca Ceresoli]

Hi Jacopo,

thanks for reviewing.

On 15/09/20 15:34, Jacopo Mondi wrote:
> Hi Luca,
> 
> On Fri, Sep 04, 2020 at 11:51:41PM +0200, Luca Ceresoli wrote:
>> This paragraph provides generic information to explain what v4l2_subdev is
>> useful for. Placing it in the middle of paragraphs describing the details
>> of subdev registration does not make much sense. Move it near the beginning
>> of the section when the v4l2_subdev idea has just been introduced and
>> before going into its details.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>> ---
>>  Documentation/driver-api/media/v4l2-subdev.rst | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
>> index fb66163deb38..1c1e3f9da142 100644
>> --- a/Documentation/driver-api/media/v4l2-subdev.rst
>> +++ b/Documentation/driver-api/media/v4l2-subdev.rst
>> @@ -12,6 +12,12 @@ Usually these are I2C devices, but not necessarily. In order to provide the
>>  driver with a consistent interface to these sub-devices the
>>  :c:type:`v4l2_subdev` struct (v4l2-subdev.h) was created.
>>
>> +The advantage of using :c:type:`v4l2_subdev` is that it is a generic struct and
>> +does not contain any knowledge about the underlying hardware. So a driver might
>> +contain several subdevs that use an I2C bus, but also a subdev that is
>> +controlled through GPIO pins. This distinction is only relevant when setting
>> +up the device, but once the subdev is registered it is completely transparent.
>> +
>>  Each sub-device driver must have a :c:type:`v4l2_subdev` struct. This struct
>>  can be stand-alone for simple sub-devices or it might be embedded in a larger
>>  struct if more state information needs to be stored. Usually there is a
>> @@ -235,12 +241,6 @@ it can call ``v4l2_subdev_notify(sd, notification, arg)``. This macro checks
>>  whether there is a ``notify()`` callback defined and returns ``-ENODEV`` if not.
>>  Otherwise the result of the ``notify()`` call is returned.
>>
>> -The advantage of using :c:type:`v4l2_subdev` is that it is a generic struct and
>> -does not contain any knowledge about the underlying hardware. So a driver might
>> -contain several subdevs that use an I2C bus, but also a subdev that is
>> -controlled through GPIO pins. This distinction is only relevant when setting
>> -up the device, but once the subdev is registered it is completely transparent.
>> -
> 
> Have you considered moving the whole part that describes how to call
> operations, which comes after the synchronous registration case to a
> dedicated sub-section ? The above paragraph makes sense in the context
> of describing why v4l2_subdev is advantageous as it abstract the
> underlying details under a unified call interface.
> 
> This could become
> 
> V4L2 sub-devices
> ----------------
> 
> Intro
> ~~~~~
> 
> Registration
> ~~~~~~~~~~~~
> 
> **synchronous**
> **asynchronous**
> 
> Operations call (or a better name :)
> ~~~~~~~~~~~~~~~
> 
> What do you think ?

I think you're right. Incoming v2 with the new layout as you suggested.

-- 
Luca

Re: [PATCH 1/3] media: docs: v4l2-subdev: fix typo

[Jacopo Mondi]

Hi Luca,

On Fri, Sep 04, 2020 at 11:51:39PM +0200, Luca Ceresoli wrote:
> Fix "will to" -> "will do".
>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>


Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j


> ---
>  Documentation/driver-api/media/v4l2-subdev.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> index bc7e1fc40a9d..9a7dddd97f5a 100644
> --- a/Documentation/driver-api/media/v4l2-subdev.rst
> +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> @@ -191,7 +191,7 @@ but it is better and easier to use this macro:
>
>  	err = v4l2_subdev_call(sd, core, g_std, &norm);
>
> -The macro will to the right ``NULL`` pointer checks and returns ``-ENODEV``
> +The macro will do the right ``NULL`` pointer checks and returns ``-ENODEV``
>  if :c:type:`sd <v4l2_subdev>` is ``NULL``, ``-ENOIOCTLCMD`` if either
>  :c:type:`sd <v4l2_subdev>`->core or :c:type:`sd <v4l2_subdev>`->core->g_std is ``NULL``, or the actual result of the
>  :c:type:`sd <v4l2_subdev>`->ops->core->g_std ops.
> --
> 2.28.0
>