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

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

[Suraj Sonawane]

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.

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

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

[Sakari Ailus]

Hi Suraj,

Thanks for the patch.

On Sun, Mar 22, 2026 at 07:23:48PM +0530, Suraj Sonawane 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.
> 
> 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);

Can you instead turn the first if () in the function into a switch and
handle this there, too, returning zero on the LINK_FREQ control?

-- 
Regards,

Sakari Ailus

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

[Dave Stevenson]

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
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
>

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

[Jacopo Mondi]

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
> >

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

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