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
