Re: [PATCH] v4l: doc: clarify v4l2_mbus_fmt height definition

[Kieran Bingham]

Hi Hans, Niklas, Sakari,

Thank you for the very prompt reviews!

I fired the patch - disappeared to teach code club, and came back to the answers
:-D - very streamlined!

On 08/01/18 15:48, Hans Verkuil wrote:
> On 01/08/2018 04:32 PM, Niklas SÃderlund wrote:
>> Hi,
>>
>> Thanks for your patch.
>>
>> On 2018-01-08 17:13:53 +0200, Sakari Ailus wrote:
>>> Hi Kieran,
>>>
>>> On Mon, Jan 08, 2018 at 02:45:49PM +0000, Kieran Bingham wrote:
>>>> The v4l2_mbus_fmt width and height corresponds directly with the
>>>> v4l2_pix_format definitions, yet the differences in documentation make
>>>> it ambiguous what to do in the event of field heights.
>>>>
>>>> Clarify this by referencing the v4l2_pix_format which is explicit on the
>>>> matter, and by matching the terminology of 'image height' rather than
>>>> the misleading 'frame height'.
>>
>> Nice that this relationship is documented as it have contributed to some 
>> confusion on my side in the past!
>>
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>>>> ---
>>>>  Documentation/media/uapi/v4l/subdev-formats.rst | 6 ++++--
>>>>  include/uapi/linux/v4l2-mediabus.h              | 4 ++--
>>>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst b/Documentation/media/uapi/v4l/subdev-formats.rst
>>>> index b1eea44550e1..a2a00202b430 100644
>>>> --- a/Documentation/media/uapi/v4l/subdev-formats.rst
>>>> +++ b/Documentation/media/uapi/v4l/subdev-formats.rst
>>>> @@ -16,10 +16,12 @@ Media Bus Formats
>>>>  
>>>>      * - __u32
>>>>        - ``width``
>>>> -      - Image width, in pixels.
>>>> +      - Image width in pixels. See struct
>>>> +	:c:type:`v4l2_pix_format`.
>>>>      * - __u32
>>>>        - ``height``
>>>> -      - Image height, in pixels.
>>>> +      - Image height in pixels. See struct
>>>> +	:c:type:`v4l2_pix_format`.
>>>>      * - __u32
>>>>        - ``code``
>>>>        - Format code, from enum
>>>> diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
>>>> index 6e20de63ec59..6b34108d0338 100644
>>>> --- a/include/uapi/linux/v4l2-mediabus.h
>>>> +++ b/include/uapi/linux/v4l2-mediabus.h
>>>> @@ -18,8 +18,8 @@
>>>>  
>>>>  /**
>>>>   * struct v4l2_mbus_framefmt - frame format on the media bus
>>>> - * @width:	frame width
>>>> - * @height:	frame height
>>>> + * @width:	image width
>>>> + * @height:	image height (see struct v4l2_pix_format)
>>>
>>> Hmm. This is the media bus format and it has no direct relation to
>>> v4l2_pix_format. So no, I can't see what would be the point in making such
>>> a reference.
>>
>> Well we have functions like v4l2_fill_pix_format() that do
>>
>>     pix_fmt->width = mbus_fmt->width;
>>     pix_fmt->height = mbus_fmt->height;
>>
>> So I think there at least is an implicit relation between the two 
>> structs. The issue I think Kieran is trying to address is in the case of 
>> TOP, BOTTOM and ALTERNATE field formats. From the v4l2_pix_format 
>> documentation on the height field:


Yes, it was this relationship which made me feel it was appropriate to just
reference in the same way that the subdevice version did.

>>    "Image height in pixels. If field is one of V4L2_FIELD_TOP, 
>>    V4L2_FIELD_BOTTOM or V4L2_FIELD_ALTERNATE then height refers to the 
>>    number of lines in the field, otherwise it refers to the number of 
>>    lines in the frame (which is twice the field height for interlaced 
>>    formats)."
> 
> Right, and I'd just copy this text to subdev-formats.rst rather than referring
> to it.

Ok - Copied for V2.

Thanks

Kieran