From: Dave Stevenson <dave.stevenson@raspberrypi.com>
The link frequency can vary between modes, so add it as a
control.
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
#define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
+#define FREQ_INDEX_FULL 0
+#define FREQ_INDEX_VGA 1
+static const s64 ov5647_link_freqs[] = {
+ [FREQ_INDEX_FULL] = 218500000,
+ [FREQ_INDEX_VGA] = 208333000,
+};
+
struct regval_list {
u16 addr;
u8 data;
@@ -106,6 +113,7 @@ struct ov5647_mode {
struct v4l2_mbus_framefmt format;
struct v4l2_rect crop;
u64 pixel_rate;
+ unsigned int link_freq_index;
int hts;
int vts;
const struct regval_list *reg_list;
@@ -128,6 +136,7 @@ struct ov5647 {
struct v4l2_ctrl *exposure;
struct v4l2_ctrl *hflip;
struct v4l2_ctrl *vflip;
+ struct v4l2_ctrl *link_freq;
};
static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
@@ -376,6 +385,7 @@ static const struct ov5647_mode ov5647_modes[] = {
.height = 1944
},
.pixel_rate = 87500000,
+ .link_freq_index = FREQ_INDEX_FULL,
.hts = 2844,
.vts = 0x7b0,
.reg_list = ov5647_2592x1944_10bpp,
@@ -397,6 +407,7 @@ static const struct ov5647_mode ov5647_modes[] = {
.height = 1080,
},
.pixel_rate = 87500000,
+ .link_freq_index = FREQ_INDEX_FULL,
.hts = 2416,
.vts = 0x450,
.reg_list = ov5647_1080p30_10bpp,
@@ -418,6 +429,7 @@ static const struct ov5647_mode ov5647_modes[] = {
.height = 1944,
},
.pixel_rate = 87500000,
+ .link_freq_index = FREQ_INDEX_FULL,
.hts = 1896,
.vts = 0x59b,
.reg_list = ov5647_2x2binned_10bpp,
@@ -439,6 +451,7 @@ static const struct ov5647_mode ov5647_modes[] = {
.height = 1920,
},
.pixel_rate = 55000000,
+ .link_freq_index = FREQ_INDEX_VGA,
.hts = 1852,
.vts = 0x1f8,
.reg_list = ov5647_640x480_10bpp,
@@ -925,6 +938,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
sensor->exposure->minimum,
exposure_max, sensor->exposure->step,
exposure_def);
+
+ __v4l2_ctrl_s_ctrl(sensor->link_freq, mode->link_freq_index);
}
*fmt = mode->format;
/* The code we pass back must reflect the current h/vflips. */
@@ -1230,7 +1245,7 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
int hblank, exposure_max, exposure_def;
struct v4l2_fwnode_device_properties props;
- v4l2_ctrl_handler_init(&sensor->ctrls, 9);
+ v4l2_ctrl_handler_init(&sensor->ctrls, 10);
v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
@@ -1290,6 +1305,14 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
if (sensor->vflip)
sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+ sensor->link_freq =
+ v4l2_ctrl_new_int_menu(&sensor->ctrls, &ov5647_ctrl_ops,
+ V4L2_CID_LINK_FREQ,
+ ARRAY_SIZE(ov5647_link_freqs) - 1, 0,
+ ov5647_link_freqs);
+ if (sensor->link_freq)
+ sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
v4l2_fwnode_device_parse(dev, &props);
v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
--
2.51.0Hi Jai
On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> The link frequency can vary between modes, so add it as a
> control.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
> drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
>
> #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
>
> +#define FREQ_INDEX_FULL 0
> +#define FREQ_INDEX_VGA 1
> +static const s64 ov5647_link_freqs[] = {
> + [FREQ_INDEX_FULL] = 218500000,
The full mode pixel rate is set to 87500000, which considering CSI-2
DDR mode and the 2 lanes in use give me a link freq of 21875000.
Do you know where 218500000 comes from ? (it might be perfectly legit,
I'm not questioning that).
> + [FREQ_INDEX_VGA] = 208333000,
> +};
> +
> struct regval_list {
> u16 addr;
> u8 data;
> @@ -106,6 +113,7 @@ struct ov5647_mode {
> struct v4l2_mbus_framefmt format;
> struct v4l2_rect crop;
> u64 pixel_rate;
> + unsigned int link_freq_index;
> int hts;
> int vts;
> const struct regval_list *reg_list;
> @@ -128,6 +136,7 @@ struct ov5647 {
> struct v4l2_ctrl *exposure;
> struct v4l2_ctrl *hflip;
> struct v4l2_ctrl *vflip;
> + struct v4l2_ctrl *link_freq;
> };
>
> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> @@ -376,6 +385,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> .height = 1944
> },
> .pixel_rate = 87500000,
> + .link_freq_index = FREQ_INDEX_FULL,
> .hts = 2844,
> .vts = 0x7b0,
> .reg_list = ov5647_2592x1944_10bpp,
> @@ -397,6 +407,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> .height = 1080,
> },
> .pixel_rate = 87500000,
> + .link_freq_index = FREQ_INDEX_FULL,
> .hts = 2416,
> .vts = 0x450,
> .reg_list = ov5647_1080p30_10bpp,
> @@ -418,6 +429,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> .height = 1944,
> },
> .pixel_rate = 87500000,
> + .link_freq_index = FREQ_INDEX_FULL,
> .hts = 1896,
> .vts = 0x59b,
> .reg_list = ov5647_2x2binned_10bpp,
> @@ -439,6 +451,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> .height = 1920,
> },
> .pixel_rate = 55000000,
> + .link_freq_index = FREQ_INDEX_VGA,
> .hts = 1852,
> .vts = 0x1f8,
> .reg_list = ov5647_640x480_10bpp,
> @@ -925,6 +938,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> sensor->exposure->minimum,
> exposure_max, sensor->exposure->step,
> exposure_def);
> +
> + __v4l2_ctrl_s_ctrl(sensor->link_freq, mode->link_freq_index);
Doesn't this cause an error in s_ctrl where the control is not handled
?
> }
> *fmt = mode->format;
> /* The code we pass back must reflect the current h/vflips. */
> @@ -1230,7 +1245,7 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> int hblank, exposure_max, exposure_def;
> struct v4l2_fwnode_device_properties props;
>
> - v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> + v4l2_ctrl_handler_init(&sensor->ctrls, 10);
>
> v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> @@ -1290,6 +1305,14 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> if (sensor->vflip)
> sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>
> + sensor->link_freq =
> + v4l2_ctrl_new_int_menu(&sensor->ctrls, &ov5647_ctrl_ops,
As suggested for PIXEL_RATE, if you make the control read-only you
should set the control ops to NULL.
> + V4L2_CID_LINK_FREQ,
> + ARRAY_SIZE(ov5647_link_freqs) - 1, 0,
> + ov5647_link_freqs);
> + if (sensor->link_freq)
> + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
You know, I thought link_freq was set as READ_ONLY by the framework,
but it's actuallt PIXEL_RATE (you can remove setting the flags
in the driver if you send a patch to remove the control ops when
registering PIXEL_RATE).
Thanks
j
> +
> v4l2_fwnode_device_parse(dev, &props);
>
> v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
>
> --
> 2.51.0
>
Hi Jacopo,
Quoting Jacopo Mondi (2025-11-02 16:59:02)
> Hi Jai
>
> On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > The link frequency can vary between modes, so add it as a
> > control.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> > drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
> > 1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
> >
> > #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> >
> > +#define FREQ_INDEX_FULL 0
> > +#define FREQ_INDEX_VGA 1
> > +static const s64 ov5647_link_freqs[] = {
> > + [FREQ_INDEX_FULL] = 218500000,
>
> The full mode pixel rate is set to 87500000, which considering CSI-2
> DDR mode and the 2 lanes in use give me a link freq of 21875000.
Indeed, I get the same value, will update.
>
> Do you know where 218500000 comes from ? (it might be perfectly legit,
> I'm not questioning that).
>
> > + [FREQ_INDEX_VGA] = 208333000,
This value should be 137500000 if we do the same calculation using the
pixel rate for the VGA mode. But for the VGA mode, the sensor does 2x2
binning + 2x2 subsampling, which is quite a bit different than other modes.
https://docs.kernel.org/driver-api/media/tx-rx.html#pixel-rate mentions
that the pixel rate value calculated from the bus link frequency does not
necessarily have to match the PIXEL_RATE control value (which is for the
sensor's internal readout of pixels including blanking).
Ultimately, these values are coming from the BSP where the CFE driver is
using the link frequency control to configure the DPHY-RX rate, so I think
it would be wiser to not reduce the VGA link frequency value, which may
cause issues with DPHY-RX latching. We can always fix it later if needed.
> > +};
> > +
> > struct regval_list {
> > u16 addr;
> > u8 data;
> > @@ -106,6 +113,7 @@ struct ov5647_mode {
> > struct v4l2_mbus_framefmt format;
> > struct v4l2_rect crop;
> > u64 pixel_rate;
> > + unsigned int link_freq_index;
> > int hts;
> > int vts;
> > const struct regval_list *reg_list;
> > @@ -128,6 +136,7 @@ struct ov5647 {
> > struct v4l2_ctrl *exposure;
> > struct v4l2_ctrl *hflip;
> > struct v4l2_ctrl *vflip;
> > + struct v4l2_ctrl *link_freq;
> > };
> >
> > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > @@ -376,6 +385,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > .height = 1944
> > },
> > .pixel_rate = 87500000,
> > + .link_freq_index = FREQ_INDEX_FULL,
> > .hts = 2844,
> > .vts = 0x7b0,
> > .reg_list = ov5647_2592x1944_10bpp,
> > @@ -397,6 +407,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > .height = 1080,
> > },
> > .pixel_rate = 87500000,
> > + .link_freq_index = FREQ_INDEX_FULL,
> > .hts = 2416,
> > .vts = 0x450,
> > .reg_list = ov5647_1080p30_10bpp,
> > @@ -418,6 +429,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > .height = 1944,
> > },
> > .pixel_rate = 87500000,
> > + .link_freq_index = FREQ_INDEX_FULL,
> > .hts = 1896,
> > .vts = 0x59b,
> > .reg_list = ov5647_2x2binned_10bpp,
> > @@ -439,6 +451,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > .height = 1920,
> > },
> > .pixel_rate = 55000000,
> > + .link_freq_index = FREQ_INDEX_VGA,
> > .hts = 1852,
> > .vts = 0x1f8,
> > .reg_list = ov5647_640x480_10bpp,
> > @@ -925,6 +938,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> > sensor->exposure->minimum,
> > exposure_max, sensor->exposure->step,
> > exposure_def);
> > +
> > + __v4l2_ctrl_s_ctrl(sensor->link_freq, mode->link_freq_index);
>
> Doesn't this cause an error in s_ctrl where the control is not handled
> ?
The framework returns -EACCESS for read-only controls in validate_ctrls()
>
> > }
> > *fmt = mode->format;
> > /* The code we pass back must reflect the current h/vflips. */
> > @@ -1230,7 +1245,7 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > int hblank, exposure_max, exposure_def;
> > struct v4l2_fwnode_device_properties props;
> >
> > - v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> > + v4l2_ctrl_handler_init(&sensor->ctrls, 10);
> >
> > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> > @@ -1290,6 +1305,14 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > if (sensor->vflip)
> > sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> >
> > + sensor->link_freq =
> > + v4l2_ctrl_new_int_menu(&sensor->ctrls, &ov5647_ctrl_ops,
>
> As suggested for PIXEL_RATE, if you make the control read-only you
> should set the control ops to NULL.
Will do in v2.
> > + V4L2_CID_LINK_FREQ,
> > + ARRAY_SIZE(ov5647_link_freqs) - 1, 0,
> > + ov5647_link_freqs);
> > + if (sensor->link_freq)
> > + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> You know, I thought link_freq was set as READ_ONLY by the framework,
> but it's actuallt PIXEL_RATE (you can remove setting the flags
> in the driver if you send a patch to remove the control ops when
> registering PIXEL_RATE).
Will do.
>
> Thanks
> j
>
> > +
> > v4l2_fwnode_device_parse(dev, &props);
> >
> > v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> >
> > --
> > 2.51.0
> >
Thanks,
Jai
Hi Jacopo & Jai
On Tue, 18 Nov 2025 at 11:28, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Quoting Jacopo Mondi (2025-11-02 16:59:02)
> > Hi Jai
> >
> > On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > >
> > > The link frequency can vary between modes, so add it as a
> > > control.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > ---
> > > drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
> > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> > > --- a/drivers/media/i2c/ov5647.c
> > > +++ b/drivers/media/i2c/ov5647.c
> > > @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
> > >
> > > #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> > >
> > > +#define FREQ_INDEX_FULL 0
> > > +#define FREQ_INDEX_VGA 1
> > > +static const s64 ov5647_link_freqs[] = {
> > > + [FREQ_INDEX_FULL] = 218500000,
> >
> > The full mode pixel rate is set to 87500000, which considering CSI-2
> > DDR mode and the 2 lanes in use give me a link freq of 21875000.
>
> Indeed, I get the same value, will update.
Agreed. I obviously lost a digit.
> >
> > Do you know where 218500000 comes from ? (it might be perfectly legit,
> > I'm not questioning that).
> >
>
> > > + [FREQ_INDEX_VGA] = 208333000,
>
> This value should be 137500000 if we do the same calculation using the
> pixel rate for the VGA mode. But for the VGA mode, the sensor does 2x2
> binning + 2x2 subsampling, which is quite a bit different than other modes.
>
> https://docs.kernel.org/driver-api/media/tx-rx.html#pixel-rate mentions
> that the pixel rate value calculated from the bus link frequency does not
> necessarily have to match the PIXEL_RATE control value (which is for the
> sensor's internal readout of pixels including blanking).
Indeed you should never assume that pixel rate and link frequency are
directly linked. So many sensors have separate PLLs for the pixel
array vs the MIPI block.
Having said that, OV5647 appears to use the same PLL for pixel clock
and MIPI, although it does have a separate PLLADCLK which is
presumably for the ADC.
> Ultimately, these values are coming from the BSP where the CFE driver is
> using the link frequency control to configure the DPHY-RX rate, so I think
> it would be wiser to not reduce the VGA link frequency value, which may
> cause issues with DPHY-RX latching. We can always fix it later if needed.
It's been a long time since I looked at these settings, but I do have
a spreadsheet from Omnivision that gives clock frequencies based on
register values.
In my experience the link frequency isn't critical to be exactly right
as it typically only sets up timeout ranges in the PHY. Even if the
value is significantly out it will generally work just fine.
VGA and full modes differ in register 0x3036 (SC_CMMN_PLL_MULTIPLIER)
which alters all the timings.
Running the numbers again, I get the VGA link frequency to be
145.8333MHz, but also the pixel rate to be 58.333MPix/s vs 55 in the
driver. I don't recall the VGA mode being 6% out on frame rate and
exposure setup, so I can't quite square that with reality. I'll try to
find 10 minutes to confirm, unless either of you happen to have one
set up and could validate the frame times.
Dave
> > > +};
> > > +
> > > struct regval_list {
> > > u16 addr;
> > > u8 data;
> > > @@ -106,6 +113,7 @@ struct ov5647_mode {
> > > struct v4l2_mbus_framefmt format;
> > > struct v4l2_rect crop;
> > > u64 pixel_rate;
> > > + unsigned int link_freq_index;
> > > int hts;
> > > int vts;
> > > const struct regval_list *reg_list;
> > > @@ -128,6 +136,7 @@ struct ov5647 {
> > > struct v4l2_ctrl *exposure;
> > > struct v4l2_ctrl *hflip;
> > > struct v4l2_ctrl *vflip;
> > > + struct v4l2_ctrl *link_freq;
> > > };
> > >
> > > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > > @@ -376,6 +385,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > > .height = 1944
> > > },
> > > .pixel_rate = 87500000,
> > > + .link_freq_index = FREQ_INDEX_FULL,
> > > .hts = 2844,
> > > .vts = 0x7b0,
> > > .reg_list = ov5647_2592x1944_10bpp,
> > > @@ -397,6 +407,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > > .height = 1080,
> > > },
> > > .pixel_rate = 87500000,
> > > + .link_freq_index = FREQ_INDEX_FULL,
> > > .hts = 2416,
> > > .vts = 0x450,
> > > .reg_list = ov5647_1080p30_10bpp,
> > > @@ -418,6 +429,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > > .height = 1944,
> > > },
> > > .pixel_rate = 87500000,
> > > + .link_freq_index = FREQ_INDEX_FULL,
> > > .hts = 1896,
> > > .vts = 0x59b,
> > > .reg_list = ov5647_2x2binned_10bpp,
> > > @@ -439,6 +451,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > > .height = 1920,
> > > },
> > > .pixel_rate = 55000000,
> > > + .link_freq_index = FREQ_INDEX_VGA,
> > > .hts = 1852,
> > > .vts = 0x1f8,
> > > .reg_list = ov5647_640x480_10bpp,
> > > @@ -925,6 +938,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> > > sensor->exposure->minimum,
> > > exposure_max, sensor->exposure->step,
> > > exposure_def);
> > > +
> > > + __v4l2_ctrl_s_ctrl(sensor->link_freq, mode->link_freq_index);
> >
> > Doesn't this cause an error in s_ctrl where the control is not handled
> > ?
>
> The framework returns -EACCESS for read-only controls in validate_ctrls()
>
> >
> > > }
> > > *fmt = mode->format;
> > > /* The code we pass back must reflect the current h/vflips. */
> > > @@ -1230,7 +1245,7 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > > int hblank, exposure_max, exposure_def;
> > > struct v4l2_fwnode_device_properties props;
> > >
> > > - v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> > > + v4l2_ctrl_handler_init(&sensor->ctrls, 10);
> > >
> > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > > V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> > > @@ -1290,6 +1305,14 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > > if (sensor->vflip)
> > > sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > >
> > > + sensor->link_freq =
> > > + v4l2_ctrl_new_int_menu(&sensor->ctrls, &ov5647_ctrl_ops,
> >
> > As suggested for PIXEL_RATE, if you make the control read-only you
> > should set the control ops to NULL.
>
> Will do in v2.
>
> > > + V4L2_CID_LINK_FREQ,
> > > + ARRAY_SIZE(ov5647_link_freqs) - 1, 0,
> > > + ov5647_link_freqs);
> > > + if (sensor->link_freq)
> > > + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > You know, I thought link_freq was set as READ_ONLY by the framework,
> > but it's actuallt PIXEL_RATE (you can remove setting the flags
> > in the driver if you send a patch to remove the control ops when
> > registering PIXEL_RATE).
>
> Will do.
>
> >
> > Thanks
> > j
> >
> > > +
> > > v4l2_fwnode_device_parse(dev, &props);
> > >
> > > v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> > >
> > > --
> > > 2.51.0
> > >
>
> Thanks,
> Jai
Hi Dave,
Quoting Dave Stevenson (2025-11-18 18:17:25)
> Hi Jacopo & Jai
>
> On Tue, 18 Nov 2025 at 11:28, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > Quoting Jacopo Mondi (2025-11-02 16:59:02)
> > > Hi Jai
> > >
> > > On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > >
> > > > The link frequency can vary between modes, so add it as a
> > > > control.
> > > >
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > > ---
> > > > drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
> > > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> > > > --- a/drivers/media/i2c/ov5647.c
> > > > +++ b/drivers/media/i2c/ov5647.c
> > > > @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
> > > >
> > > > #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> > > >
> > > > +#define FREQ_INDEX_FULL 0
> > > > +#define FREQ_INDEX_VGA 1
> > > > +static const s64 ov5647_link_freqs[] = {
> > > > + [FREQ_INDEX_FULL] = 218500000,
> > >
> > > The full mode pixel rate is set to 87500000, which considering CSI-2
> > > DDR mode and the 2 lanes in use give me a link freq of 21875000.
> >
> > Indeed, I get the same value, will update.
>
> Agreed. I obviously lost a digit.
>
> > >
> > > Do you know where 218500000 comes from ? (it might be perfectly legit,
> > > I'm not questioning that).
> > >
> >
> > > > + [FREQ_INDEX_VGA] = 208333000,
> >
> > This value should be 137500000 if we do the same calculation using the
> > pixel rate for the VGA mode. But for the VGA mode, the sensor does 2x2
> > binning + 2x2 subsampling, which is quite a bit different than other modes.
> >
> > https://docs.kernel.org/driver-api/media/tx-rx.html#pixel-rate mentions
> > that the pixel rate value calculated from the bus link frequency does not
> > necessarily have to match the PIXEL_RATE control value (which is for the
> > sensor's internal readout of pixels including blanking).
>
> Indeed you should never assume that pixel rate and link frequency are
> directly linked. So many sensors have separate PLLs for the pixel
> array vs the MIPI block.
>
> Having said that, OV5647 appears to use the same PLL for pixel clock
> and MIPI, although it does have a separate PLLADCLK which is
> presumably for the ADC.
Ah that's really helpful information, thanks.
>
> > Ultimately, these values are coming from the BSP where the CFE driver is
> > using the link frequency control to configure the DPHY-RX rate, so I think
> > it would be wiser to not reduce the VGA link frequency value, which may
> > cause issues with DPHY-RX latching. We can always fix it later if needed.
>
> It's been a long time since I looked at these settings, but I do have
> a spreadsheet from Omnivision that gives clock frequencies based on
> register values.
> In my experience the link frequency isn't critical to be exactly right
> as it typically only sets up timeout ranges in the PHY. Even if the
> value is significantly out it will generally work just fine.
>
> VGA and full modes differ in register 0x3036 (SC_CMMN_PLL_MULTIPLIER)
> which alters all the timings.
>
> Running the numbers again, I get the VGA link frequency to be
> 145.8333MHz, but also the pixel rate to be 58.333MPix/s vs 55 in the
> driver. I don't recall the VGA mode being 6% out on frame rate and
> exposure setup, so I can't quite square that with reality. I'll try to
> find 10 minutes to confirm, unless either of you happen to have one
> set up and could validate the frame times.
>
Running 640x480 capture with default hblank/vblank I get frames at
62.49fps.
58333000/(1852*504) = 62.4946
So 58.333MPix/s seems to be the correct value instead of 55MPix/s.
I had already sent a v2, so I'll wait till next -rc1 is tagged for any
more comments, and will update these values in v3.
Thanks,
Jai
> Dave
>
[snip]
© 2016 - 2026 Red Hat, Inc.