Re: [PATCH v4 4/9] media: Documentation: dw100: Add user documentation for the DW100 driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Apr 25, 2022 at 08:18:21AM +0200, Hans Verkuil wrote:
> On 28/03/2022 16:13, Xavier Roumegue wrote:
> > Add user documentation for the DW100 driver.
> > 
> > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> > ---
> >  .../userspace-api/media/drivers/dw100.rst     | 23 +++++++++++++++++++
> >  .../userspace-api/media/drivers/index.rst     |  1 +
> >  2 files changed, 24 insertions(+)
> >  create mode 100644 Documentation/userspace-api/media/drivers/dw100.rst
> > 
> > diff --git a/Documentation/userspace-api/media/drivers/dw100.rst b/Documentation/userspace-api/media/drivers/dw100.rst
> > new file mode 100644
> > index 000000000000..4cd55c75628e
> > --- /dev/null
> > +++ b/Documentation/userspace-api/media/drivers/dw100.rst
> > @@ -0,0 +1,23 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +DW100 dewarp driver
> > +===========================

Looks like the underline can be shorten.

> > +
> > +The Vivante DW100 Dewarp Processor IP core found on i.MX8MP SoC applies a
> > +programmable geometrical transformation on input image to correct distorsion
> 
> distorsion -> distortion
> 
> > +introduced by lenses.
> > +
> > +The transformation function is exposed by the hardware as a grid map with 16x16
> > +pixel macroblocks indexed using X, Y vertex coordinates. Each x, y coordinate
> > +register uses 16 bits to record the coordinate address in UQ12.4 fixed point
> > +format.
> > +
> > +The dewarping map can be set from application through a dedicated v4l2 control.

s/v4l2/V4L2/

I'd mention the control ID.

> > +If not set or invalid, the driver computes an identity map prior to start the
> > +processing engine. The map is evaluated as invalid if the array size does not
> > +match the expected size inherited from the destination image resolution.

I'd swap the two sentences and clarify this a bit:

----
The dewarping map is set from applications using the
V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP control. The control contains
an array of u32 values storing (x, y) destination coordinates for each
vertex of the grid. The x coordinate is stored in the 16 LSBs and the y
coordinate in the 16 MSBs. The number of elements in the array must
match the image size:

    elems = (DIV_ROUND_UP(width, 16) + 1)
          * (DIV_ROUND_UP(height, 16) + 1);

If the control doesn't contain the correct number of elements, the
driver uses an identity map.
----

This assumes I got the size calculation right :-)

Bonus points if you can format the code block correctly for sphinx.

I'm also wondering if the driver shouldn't return an error when the map
is incorrect, defaulting silently to an identity map can be confusing.

> > +
> > +More details on the DW100 hardware operations can be found in
> > +*chapter 13.15 DeWarp* of IMX8MP_ reference manuel.
> 
> manuel -> manual
> 
> > +
> > +.. _IMX8MP: https://www.nxp.com/webapp/Download?colCode=IMX8MPRM
> > diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst
> > index 12e3c512d718..8826777321b0 100644
> > --- a/Documentation/userspace-api/media/drivers/index.rst
> > +++ b/Documentation/userspace-api/media/drivers/index.rst
> > @@ -33,6 +33,7 @@ For more details see the file COPYING in the source distribution of Linux.
> >  
> >  	ccs
> >  	cx2341x-uapi
> > +	dw100
> >          hantro

While at it you could replace the spaces with a tab here (and mention
that in the commit message, with a "While at it, ..." line).

> >  	imx-uapi
> >  	max2175

-- 
Regards,

Laurent Pinchart