Message-ID: <20220428130448.bfht2nf23n6lidsk@uno.localdomain>
Date:   Thu, 28 Apr 2022 15:04:48 +0200
From:   Jacopo Mondi <jacopo@...ndi.org>
To:     Dorota Czaplejewicz <dorota.czaplejewicz@...i.sm>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...i.sm
Subject: Re: [PATCHv2 1/2] doc/media api: Try to make enum usage clearer

Hi Dorota,


On Thu, Apr 28, 2022 at 10:52:11AM +0200, Dorota Czaplejewicz wrote:
> Added: mbus codes must not repeat
> Added: no holes in the enumeration
> Added: enumerations per what?
> Added: who fills in what in calls
> Changed: "zero" -> "0"
> Changed: "given" -> "specified"
>
A more discoursive commit message would be appreciated. Just a few lines
before the crude list of changes:

Something like

"Update the documentation of ... in order to clarify etc etc"

> Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@...i.sm>
> ---
> Hello,
>
> this is the second attempt at updating the media documentation.
>
> Differences from previous: "selected" is now "specified", "array" is now "enumeration", and "caller" is now "application".

Please stay in 80 cols even for parts that won't end up in the commit
message, it's hard to read this if you have multiple terminal windows
open.

>
> No differences: I haven't used the frame intervals calls and haven't gathered practical knowledge about where docs may be insufficient, so I didn't touch its documentation.

I think Hans required to change the documentation of that ioctl to
match the style of the changes you have made here, not because
something is missing there.

>
> Regards,
> Dorota
>
>  .../v4l/vidioc-subdev-enum-mbus-code.rst      | 39 +++++++++++++------
>  1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst
> index 417f1a19bcc4..87572de0fd26 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst
> @@ -31,15 +31,29 @@ Arguments
>  Description
>  ===========
>
> -To enumerate media bus formats available at a given sub-device pad
> -applications initialize the ``pad``, ``which`` and ``index`` fields of
> -struct
> -:c:type:`v4l2_subdev_mbus_code_enum` and
> -call the :ref:`VIDIOC_SUBDEV_ENUM_MBUS_CODE` ioctl with a pointer to this
> -structure. Drivers fill the rest of the structure or return an ``EINVAL``
> -error code if either the ``pad`` or ``index`` are invalid. All media bus
> -formats are enumerable by beginning at index zero and incrementing by
> -one until ``EINVAL`` is returned.
> +This call is used by the application to access the enumeration of bus formats
> +for the selected pad.

This is a good introductory phrase.

> +
> +The enumerations are defined by the driver, and indexed using the ``index`` field
> +of struct :c:type:`v4l2_subdev_mbus_code_enum`.
> +Each value of ``pad`` corresponds to a separate enumeration.

Isn't this a repetition of the above "enumeration of bus formats for
the selected pad" ? Also, the fact different mbus codes are available
at different pads is an intrinsic characteristics of the device
capabilities and of what a pad represents. Put it in this way it seems
it's an API requirement.

> +Each enumeration starts with the ``index`` of 0, and
> +the lowest invalid index marks the end of enumeration.
> +
> +Therefore, to enumerate media bus formats available at a given sub-device pad,
> +initialize the ``pad``, and ``which`` fields to desired values,
> +and set ``index`` to 0.
> +Then call the :ref:`VIDIOC_SUBDEV_ENUM_MBUS_CODE` ioctl
> +with a pointer to this structure.

Could these two paragraphs be just:

To enumerate all the media bus codes available at a give sub-device pad,
an application set the ``index`` field to 0 and then call the
:ref:`VIDIOC_SUBDEV_ENUM_MBUS_CODE` ioctl with a pointer to this structure,
incrementing ``index`` by one until ``EINVAL`` is returned.

> +
> +A successful call will return with the ``code`` field filled in
> +with a mbus format value.

Generally, I see "mbus code", not "mbus format" as far as
I'm aware..

> +Repeat with increasing ``index`` until ``EINVAL`` is received.
> +``EINVAL`` means that either ``pad`` is invalid,
> +or that there are no more codes available at this pad.

Is it necessary to add this last paragraph. Isn't it specified in the
error code description below ?

EINVAL
    The struct
    :c:type:`v4l2_subdev_mbus_code_enum`
    ``pad`` references a non-existing pad, or the ``index`` field is out
    of bounds.


> +
> +The driver must not return the same value of ``code`` for different indices
> +at the same pad.

This might be a good thing to specify, a little obvious maybe but it
doesn't hurt.

>
>  Available media bus formats may depend on the current 'try' formats at
>  other pads of the sub-device, as well as on the current active links.
> @@ -57,14 +71,15 @@ information about the try formats.
>
>      * - __u32
>        - ``pad``
> -      - Pad number as reported by the media controller API.
> +      - Pad number as reported by the media controller API. Filled in by the application.
>      * - __u32
>        - ``index``
> -      - Number of the format in the enumeration, set by the application.
> +      - Index of the mbus code in the enumeration belonging to the given pad.
> +    Filled in by the application.

These last changes are good, provided this phrase still renders correctly
now that you added a line break.

>      * - __u32
>        - ``code``
>        - The media bus format code, as defined in
> -	:ref:`v4l2-mbus-format`.
> +	:ref:`v4l2-mbus-format`. Filled in by the driver.
>      * - __u32
>        - ``which``
>        - Media bus format codes to be enumerated, from enum

In general there are a few good additions, but to be honest I would
keep the changes small for sake of consistency with the existing
documentation of other enumeration-related ioctls

Thanks
   j

> --
> 2.35.1
>



Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists