Re: [RFC PATCH v4 09/11] media: uapi: Add V4L2_CID_USER_IMX_ASRC_RATIO_MOD control

[Shengjiu Wang]

On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>
> On 20/09/2023 11:32, Shengjiu Wang wrote:
> > The input clock and output clock may not be the accurate
> > rate as the sample rate, there is some drift, so the convert
> > ratio of i.MX ASRC module need to be changed according to
> > actual clock rate.
> >
> > Add V4L2_CID_USER_IMX_ASRC_RATIO_MOD control for user to
> > adjust the ratio.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> > ---
> >  Documentation/userspace-api/media/v4l/control.rst | 5 +++++
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c         | 1 +
> >  include/uapi/linux/v4l2-controls.h                | 1 +
> >  3 files changed, 7 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/control.rst b/Documentation/userspace-api/media/v4l/control.rst
> > index 4463fce694b0..2bc175900a34 100644
> > --- a/Documentation/userspace-api/media/v4l/control.rst
> > +++ b/Documentation/userspace-api/media/v4l/control.rst
> > @@ -318,6 +318,11 @@ Control IDs
> >      depending on particular custom controls should check the driver name
> >      and version, see :ref:`querycap`.
> >
> > +.. _v4l2-audio-imx:
> > +
> > +``V4L2_CID_USER_IMX_ASRC_RATIO_MOD``
> > +    sets the rasampler ratio modifier of i.MX asrc module.
>
> rasampler -> resampler (I think?)
>
> This doesn't document at all what the type of the control is or how to interpret it.
>
> > +
> >  Applications can enumerate the available controls with the
> >  :ref:`VIDIOC_QUERYCTRL` and
> >  :ref:`VIDIOC_QUERYMENU <VIDIOC_QUERYCTRL>` ioctls, get and set a
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index 8696eb1cdd61..16f66f66198c 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > @@ -1242,6 +1242,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >       case V4L2_CID_COLORIMETRY_CLASS:        return "Colorimetry Controls";
> >       case V4L2_CID_COLORIMETRY_HDR10_CLL_INFO:               return "HDR10 Content Light Info";
> >       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:      return "HDR10 Mastering Display";
> > +     case V4L2_CID_USER_IMX_ASRC_RATIO_MOD:                  return "ASRC RATIO MOD";
>
> Let's stay consistent with the other control names:
>
> "ASRC Ratio Modifier"
>
> But if this is a driver specific control, then this doesn't belong here.
>
> Driver specific controls are defined in the driver itself, including this
> description.
>
> Same for the control documentation: if it is driver specific, then that
> typically is documented either in a driver-specific public header, or
> possibly in driver-specific documentation (Documentation/admin-guide/media/).
>
> But is this imx specific? Wouldn't other similar devices need this?

It is imx specific.

Does this mean that I need to create a header file in include/uapi/linux
folder to put this definition?  I just hesitate if this is necessary.

There is folder Documentation/userspace-api/media/drivers/ for drivers
Should this document in this folder, not in the
Documentation/admin-guide/media/?

Best regards
Wang shengjiu
>
> >       default:
> >               return NULL;
> >       }
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index c3604a0a3e30..b1c319906d12 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -162,6 +162,7 @@ enum v4l2_colorfx {
> >  /* The base for the imx driver controls.
> >   * We reserve 16 controls for this driver. */
> >  #define V4L2_CID_USER_IMX_BASE                       (V4L2_CID_USER_BASE + 0x10b0)
> > +#define V4L2_CID_USER_IMX_ASRC_RATIO_MOD     (V4L2_CID_USER_IMX_BASE + 0)
> >
> >  /*
> >   * The base for the atmel isc driver controls.
>
> Regards,
>
>         Hans