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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

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@gmail.com> 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.

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

> ov5647_s_ctrl called for control V4L2_CID_LINK_FREQ at all when it has
> no s_ctrl op?
>
> Have I totally missed something here?
>
>   Dave
>
> > Avoid these warnings when control is queried.
> >
> > Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
> > ---
> >  drivers/media/i2c/ov5647.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 6a46ef723..a5a9cff5a 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -999,6 +999,9 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> >                 ret = cci_write(sensor->regmap, OV5647_REG_HTS,
> >                                 sensor->mode->format.width + ctrl->val, &ret);
> >                 break;
> > +       case V4L2_CID_LINK_FREQ:
> > +               ret = 0;
> > +               break;
> >         case V4L2_CID_TEST_PATTERN:
> >                 ret = cci_write(sensor->regmap, OV5647_REG_ISPCTRL3D,
> >                                 ov5647_test_pattern_val[ctrl->val], NULL);
> > --
> > 2.34.1
> >