Re: [PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

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

On Mon, Dec 16, 2024 at 08:07:26AM +0000, Sakari Ailus wrote:
> On Sun, Dec 15, 2024 at 07:02:40PM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 13, 2024 at 07:15:55AM +0000, Sakari Ailus wrote:
> > > On Thu, Dec 12, 2024 at 05:53:53PM +0100, Jacopo Mondi wrote:
> > > > On Tue, Dec 10, 2024 at 09:59:04AM +0200, Sakari Ailus wrote:
> > > > > Add the get_mbus_config() as the means for conveying the link frequency
> > > > > towards the receiver drivers.
> > > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  Documentation/driver-api/media/tx-rx.rst | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > > > > index dd09484df1d3..1430c330fd52 100644
> > > > > --- a/Documentation/driver-api/media/tx-rx.rst
> > > > > +++ b/Documentation/driver-api/media/tx-rx.rst
> > > > > @@ -49,6 +49,10 @@ Link frequency
> > > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > >
> > > > > +On devices where the link frequency isn't configurable, the link_freq field of
> > > > > +struct v4l2_mbus_config is recommended over controls for conveying the link
> > > > > +frequency to the downstream driver in the pipeline.
> > > > 
> > > > When read in its entirety, the paragraphs
> > > > 
> > > >  Link frequency
> > > >  ^^^^^^^^^^^^^^
> > > > 
> > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > 
> > > >  On devices where the link frequency isn't configurable, the link_freq field of
> > > >  struct v4l2_mbus_config is recommended over controls for conveying the link
> > > >  frequency.
> > > > 
> > > > Sounds simpler without the last 7 words.
> > > > 
> > > > A nit and just tastes maybe
> > > 
> > > I used:
> > > 
> > > On devices where the link frequency isn't configurable, using the ``link_freq``
> > > field of struct v4l2_mbus_config is recommended over controls.
> > 
> > Is it a recommendation for the transmitter driver or the receiver driver
> > ? If it's a recommendation for the transmitter driver, does that mean it
> > should not implement V4L2_CID_LINK_FREQ in that case ? How about the
> > need to expose the link frequency to userspace when it's fixed ?
> 
> For the transmitter. Receivers do not implement get_mbus_config op. I can
> add a note on this.

They don't implement it, but they call it. I wasn't sure if this
documentation was from the point of view of the caller or callee. As you
mentioned separately that it's located in the transmitter section, that
makes it clearer, but we can also improve the test:

Drivers that have a fixed link frequency should report it through the
``.get_mbus_config()`` subdev operation, in the``link_freq`` field of
struct v4l2_mbus_config, instead of through controls.

(I'll let you fix the markup for the mention of the .get_mbus_config()
operation.)

> The only reason this has been exposed to the user space is because it's
> been a control that has been modifiable. Also HDMI to CSI-2 bridges work
> this way.
> 
> The LINK_FREQ control is also an integer menu so the same control couldn't
> be used as-is. These were the reasons why the earlier review concluded with
> impelmenting this via get_mbus_config().
> 
> > I think the documentation needs further clarification to clearly explain
> > what transmitters should do and what receivers should do.
> 
> I can add this.
> 
> > > > 
> > > > I like where this is going!
> > > > 
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

-- 
Regards,

Laurent Pinchart