Re: [PATCH] media: i2c: ov5647: handle V4L2_CID_LINK_FREQ in s_ctrl

[Sakari Ailus]

Hi Jacopo,

On Mon, Mar 23, 2026 at 07:44:23PM +0100, Jacopo Mondi wrote:
> Hi Dave
> 
> On Mon, Mar 23, 2026 at 03:37:37PM +0000, Dave Stevenson wrote:
> > Hi Suraj
> >
> > On Sun, 22 Mar 2026 at 13:59, Suraj Sonawane
> > <surajsonawane0215@xxxxxxxxx> wrote:
> > >
> > > Handle V4L2_CID_LINK_FREQ in ov5647_s_ctrl().
> > >
> > > Currently this control is defined but not handled in s_ctrl(),
> > > so V4L2 falls back to estimating link frequency from pixel rate
> > > and prints warning like:
> > >
> > >   v4l2_get_link_freq: Link frequency estimated using pixel rate:
> > >   result might be inaccurate
> > >   v4l2_get_link_freq: Consider implementing support for V4L2_CID_LINK_FREQ
> > >   in the transmitter driver
> > >
> > > Handle it as no-op since link frequency is fixed per mode and
> > > not meant to be changed at runtime.
> >
> > I'm confused by this description compared to the patch.
> >
> > v4l2_get_link_freq searches for the V4L2_CID_LINK_FREQ control, and if
> > found then it calls g_ctrl (not s_ctrl).
> > If it can't find the control then it searches for V4L2_CID_PIXEL_RATE
> > and will log the error message quoted.
> >
> > The control is registered by the ov5647 driver, therefore it should
> > never go into that second clause, so how have you got that error
> > message logged?
> >
> > AFAIK no part of that code path will result in a call to ov5647_s_ctrl
> > that you're patching.
> > I've just run with the ov5647 driver on a Pi5 (which uses
> > v4l2_get_link_freq) running 7.0.0-rc5, and I can't get an error
> > logged. v4l2_get_link_freq finds V4L2_CID_LINK_FREQ and uses the value
> > it reports.
> >
> >
> > You are right that ov5647_s_ctrl doesn't handle V4L2_CID_LINK_FREQ,
> > which could mean that an error is returned from the __v4l2_ctrl_s_ctrl
> > calls from within the driver to change the link frequency and lead to
> > the dev_info in the driver being logged iff the sensor was powered up
> > at the time (otherwise pm_runtime_get_if_in_use will fail). Generally
> > the sensor won't be powered on as the pad format is set before
> > enable_streams, and it shouldn't be possible to change it whilst
> > streaming.
> >
> > However, as I understand it, the current preferred way to handle this
> > case of read only controls where the value is changed by the driver is
> > to pass NULL as the ops for the ctrl when registering. That is already
> > the case looking at c6e115144b50 ("media: i2c: ov5647: Add
> > V4L2_CID_LINK_FREQUENCY control"). So how are you managing to get
> 
> I was about to mention the same. Setting the ctrl ops to NULL is
> preferred for controls not handled by the driver.

Yes, indeed that's the other option. It makes sense in this case.

> 
> I'm surprised I didn't see any merged patch that address the many
> faulty i2c drivers we have which manually handle the read-only
> controls in their s_ctrl implementation!
> 
> We should WARN if a Read-Only control is registered with a valid
> ops pointer maybe

There may still be side effects from the value change the driver should
handle. I guess it could take place through other means, too, but I think
I'd keep it as-is.

-- 
Regards,

Sakari Ailus