Date:   Thu, 28 Apr 2022 15:57:23 +0200
From:   Dorota Czaplejewicz <dorota.czaplejewicz@...i.sm>
To:     Jacopo Mondi <jacopo@...ndi.org>
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

Hello,

On Thu, 28 Apr 2022 15:04:48 +0200
Jacopo Mondi <jacopo@...ndi.org> wrote:

> 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.

I can try to comply with this, but I generally don't do it
because hard breaks are difficult to read
on a phone or if I have multiple messages open.
My line widths vary between 40 and 80 characters,
and hard breaks can only cover one width, at the cost of all others.
> 
> >
> > 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.
> 
What I found difficult with this documentation is that it described an algorithm,
and did not describe the basic assumption about the shape of the data.
Here, I tried to give a quick overview of the data structure in the first paragraph.
The second paragraph is just a description of details,
not needed to get a high level idea of the API.

> > +
> > +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..
I was generally confused about the "mbus" thing the whole time I used the API.
That's why I came up with the idea to describe what it's useful for (format),
instead of using an abstract "code" which could mean anything.
"mbus code" is not even searchable online much, so it's difficult to make the connection.
> 
> > +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.
> 
> 
"Out of bounds" does not say what the bounds are.
What I wrote is in the context of the incrementing algorithm,
which implies that hitting it means hitting the bound ("no more codes").
> > +
> > +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.
> 
Given that I ran head first into a bug involving repeats (prompting this patch),
I testify that nothing about it is obvious :)
> >
> >  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
> >  
> 
> 
Thanks for reviewing.

--Dorota

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists