From: Dave Stevenson <dave.stevenson@raspberrypi.com>
The driver did expose V4L2_CID_HBLANK, but as a READ_ONLY control.
The sensor only uses the HTS register to control the line length,
so convert this control to read/write, with the appropriate ranges.
Adopt the old fixed values as the minimum values permitted in each
mode to avoid issues of it not streaming.
This should allow exposure times up to ~3 seconds (up from ~1sec).
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
drivers/media/i2c/ov5647.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 3aad3dc9b5cd0c24c07a37e2567e3c61c52e4fc2..59c21b91d09d79f073a54871221f197a0bcf3aa2 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -53,6 +53,8 @@
#define OV5647_REG_AEC_AGC 0x3503
#define OV5647_REG_GAIN_HI 0x350a
#define OV5647_REG_GAIN_LO 0x350b
+#define OV5647_REG_HTS_HI 0x380c
+#define OV5647_REG_HTS_LO 0x380d
#define OV5647_REG_VTS_HI 0x380e
#define OV5647_REG_VTS_LO 0x380f
#define OV5647_REG_VFLIP 0x3820
@@ -79,6 +81,8 @@
#define OV5647_VBLANK_MIN 24
#define OV5647_VTS_MAX 32767
+#define OV5647_HTS_MAX 0x1fff
+
#define OV5647_EXPOSURE_MIN 4
#define OV5647_EXPOSURE_STEP 1
#define OV5647_EXPOSURE_DEFAULT 1000
@@ -187,8 +191,6 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
{0x3a19, 0xf8},
{0x3c01, 0x80},
{0x3b07, 0x0c},
- {0x380c, 0x0b},
- {0x380d, 0x1c},
{0x3814, 0x11},
{0x3815, 0x11},
{0x3708, 0x64},
@@ -276,8 +278,6 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
{0x3a19, 0xf8},
{0x3c01, 0x80},
{0x3b07, 0x0c},
- {0x380c, 0x09},
- {0x380d, 0x70},
{0x3814, 0x11},
{0x3815, 0x11},
{0x3708, 0x64},
@@ -375,8 +375,6 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
{0x3809, 0x10},
{0x380a, 0x03},
{0x380b, 0xcc},
- {0x380c, 0x07},
- {0x380d, 0x68},
{0x3811, 0x0c},
{0x3813, 0x06},
{0x3814, 0x31},
@@ -450,8 +448,6 @@ static struct regval_list ov5647_640x480_10bpp[] = {
{0x3a19, 0xf8},
{0x3c01, 0x80},
{0x3b07, 0x0c},
- {0x380c, 0x07},
- {0x380d, 0x3c},
{0x3814, 0x35},
{0x3815, 0x35},
{0x3708, 0x64},
@@ -1061,7 +1057,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
mode->pixel_rate, 1, mode->pixel_rate);
hblank = mode->hts - mode->format.width;
- __v4l2_ctrl_modify_range(sensor->hblank, hblank, hblank, 1,
+ __v4l2_ctrl_modify_range(sensor->hblank, hblank,
+ OV5647_HTS_MAX - mode->format.width, 1,
hblank);
vblank = mode->vts - mode->format.height;
@@ -1325,6 +1322,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
sensor->mode->format.height + ctrl->val);
break;
+ case V4L2_CID_HBLANK:
+ ret = ov5647_write16(sd, OV5647_REG_HTS_HI,
+ sensor->mode->format.width + ctrl->val);
+ break;
case V4L2_CID_TEST_PATTERN:
ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
ov5647_test_pattern_val[ctrl->val]);
@@ -1332,7 +1333,6 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
/* Read-only, but we adjust it based on mode. */
case V4L2_CID_PIXEL_RATE:
- case V4L2_CID_HBLANK:
/* Read-only, but we adjust it based on mode. */
break;
@@ -1409,10 +1409,11 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
sensor->mode->pixel_rate, 1,
sensor->mode->pixel_rate);
- /* By default, HBLANK is read only, but it does change per mode. */
hblank = sensor->mode->hts - sensor->mode->format.width;
sensor->hblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
- V4L2_CID_HBLANK, hblank, hblank, 1,
+ V4L2_CID_HBLANK, hblank,
+ OV5647_HTS_MAX -
+ sensor->mode->format.width, 1,
hblank);
sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
@@ -1446,7 +1447,6 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
goto handler_free;
sensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
- sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
sensor->sd.ctrl_handler = &sensor->ctrls;
return 0;
--
2.51.0Hi Jai
On Tue, Oct 28, 2025 at 12:57:20PM +0530, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> The driver did expose V4L2_CID_HBLANK, but as a READ_ONLY control.
>
> The sensor only uses the HTS register to control the line length,
> so convert this control to read/write, with the appropriate ranges.
> Adopt the old fixed values as the minimum values permitted in each
> mode to avoid issues of it not streaming.
>
> This should allow exposure times up to ~3 seconds (up from ~1sec).
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
> drivers/media/i2c/ov5647.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 3aad3dc9b5cd0c24c07a37e2567e3c61c52e4fc2..59c21b91d09d79f073a54871221f197a0bcf3aa2 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -53,6 +53,8 @@
> #define OV5647_REG_AEC_AGC 0x3503
> #define OV5647_REG_GAIN_HI 0x350a
> #define OV5647_REG_GAIN_LO 0x350b
> +#define OV5647_REG_HTS_HI 0x380c
> +#define OV5647_REG_HTS_LO 0x380d
> #define OV5647_REG_VTS_HI 0x380e
> #define OV5647_REG_VTS_LO 0x380f
> #define OV5647_REG_VFLIP 0x3820
> @@ -79,6 +81,8 @@
> #define OV5647_VBLANK_MIN 24
> #define OV5647_VTS_MAX 32767
>
> +#define OV5647_HTS_MAX 0x1fff
> +
> #define OV5647_EXPOSURE_MIN 4
> #define OV5647_EXPOSURE_STEP 1
> #define OV5647_EXPOSURE_DEFAULT 1000
> @@ -187,8 +191,6 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
> {0x3a19, 0xf8},
> {0x3c01, 0x80},
> {0x3b07, 0x0c},
> - {0x380c, 0x0b},
> - {0x380d, 0x1c},
> {0x3814, 0x11},
> {0x3815, 0x11},
> {0x3708, 0x64},
> @@ -276,8 +278,6 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
> {0x3a19, 0xf8},
> {0x3c01, 0x80},
> {0x3b07, 0x0c},
> - {0x380c, 0x09},
> - {0x380d, 0x70},
> {0x3814, 0x11},
> {0x3815, 0x11},
> {0x3708, 0x64},
> @@ -375,8 +375,6 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
> {0x3809, 0x10},
> {0x380a, 0x03},
> {0x380b, 0xcc},
> - {0x380c, 0x07},
> - {0x380d, 0x68},
> {0x3811, 0x0c},
> {0x3813, 0x06},
> {0x3814, 0x31},
> @@ -450,8 +448,6 @@ static struct regval_list ov5647_640x480_10bpp[] = {
> {0x3a19, 0xf8},
> {0x3c01, 0x80},
> {0x3b07, 0x0c},
> - {0x380c, 0x07},
> - {0x380d, 0x3c},
> {0x3814, 0x35},
> {0x3815, 0x35},
> {0x3708, 0x64},
> @@ -1061,7 +1057,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> mode->pixel_rate, 1, mode->pixel_rate);
>
> hblank = mode->hts - mode->format.width;
> - __v4l2_ctrl_modify_range(sensor->hblank, hblank, hblank, 1,
> + __v4l2_ctrl_modify_range(sensor->hblank, hblank,
> + OV5647_HTS_MAX - mode->format.width, 1,
Is '1' really the min ? Who knows the datasheet doesn't report that :(
> hblank);
>
> vblank = mode->vts - mode->format.height;
> @@ -1325,6 +1322,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
> sensor->mode->format.height + ctrl->val);
> break;
> + case V4L2_CID_HBLANK:
> + ret = ov5647_write16(sd, OV5647_REG_HTS_HI,
> + sensor->mode->format.width + ctrl->val);
Why are we writing HTS_HI only ? The max control value is set to
0x1fff, this spans two registers..
> + break;
> case V4L2_CID_TEST_PATTERN:
> ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
> ov5647_test_pattern_val[ctrl->val]);
> @@ -1332,7 +1333,6 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>
> /* Read-only, but we adjust it based on mode. */
> case V4L2_CID_PIXEL_RATE:
> - case V4L2_CID_HBLANK:
> /* Read-only, but we adjust it based on mode. */
We really like this comment, at the point of repeating it twice...
Speaking of which... if you set the ctrl_handler to NULL when
registering a ro control
sensor->pixel_rate = v4l2_ctrl_new_std(&sensor->ctrls, NULL,
V4L2_CID_PIXEL_RATE,
sensor->mode->pixel_rate,
sensor->mode->pixel_rate, 1,
sensor->mode->pixel_rate);
you can remove the above 4 lines.
(the background is that for RO controls the control_handler has to be
set to NULL, to avoid having to handle them in the s_ctrl handler)
Maybe add a patch to this series ?
> break;
>
> @@ -1409,10 +1409,11 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> sensor->mode->pixel_rate, 1,
> sensor->mode->pixel_rate);
>
> - /* By default, HBLANK is read only, but it does change per mode. */
> hblank = sensor->mode->hts - sensor->mode->format.width;
> sensor->hblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> - V4L2_CID_HBLANK, hblank, hblank, 1,
> + V4L2_CID_HBLANK, hblank,
> + OV5647_HTS_MAX -
> + sensor->mode->format.width, 1,
> hblank);
>
> sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> @@ -1446,7 +1447,6 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> goto handler_free;
>
> sensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> - sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> sensor->sd.ctrl_handler = &sensor->ctrls;
>
> return 0;
>
> --
> 2.51.0
>
Hi Jacopo,
Thanks for the review.
Quoting Jacopo Mondi (2025-11-02 16:37:55)
> Hi Jai
>
> On Tue, Oct 28, 2025 at 12:57:20PM +0530, Jai Luthra wrote:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > The driver did expose V4L2_CID_HBLANK, but as a READ_ONLY control.
> >
> > The sensor only uses the HTS register to control the line length,
> > so convert this control to read/write, with the appropriate ranges.
> > Adopt the old fixed values as the minimum values permitted in each
> > mode to avoid issues of it not streaming.
> >
> > This should allow exposure times up to ~3 seconds (up from ~1sec).
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> > drivers/media/i2c/ov5647.c | 26 +++++++++++++-------------
> > 1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 3aad3dc9b5cd0c24c07a37e2567e3c61c52e4fc2..59c21b91d09d79f073a54871221f197a0bcf3aa2 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -53,6 +53,8 @@
> > #define OV5647_REG_AEC_AGC 0x3503
> > #define OV5647_REG_GAIN_HI 0x350a
> > #define OV5647_REG_GAIN_LO 0x350b
> > +#define OV5647_REG_HTS_HI 0x380c
> > +#define OV5647_REG_HTS_LO 0x380d
> > #define OV5647_REG_VTS_HI 0x380e
> > #define OV5647_REG_VTS_LO 0x380f
> > #define OV5647_REG_VFLIP 0x3820
> > @@ -79,6 +81,8 @@
> > #define OV5647_VBLANK_MIN 24
> > #define OV5647_VTS_MAX 32767
> >
> > +#define OV5647_HTS_MAX 0x1fff
> > +
> > #define OV5647_EXPOSURE_MIN 4
> > #define OV5647_EXPOSURE_STEP 1
> > #define OV5647_EXPOSURE_DEFAULT 1000
> > @@ -187,8 +191,6 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
> > {0x3a19, 0xf8},
> > {0x3c01, 0x80},
> > {0x3b07, 0x0c},
> > - {0x380c, 0x0b},
> > - {0x380d, 0x1c},
> > {0x3814, 0x11},
> > {0x3815, 0x11},
> > {0x3708, 0x64},
> > @@ -276,8 +278,6 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
> > {0x3a19, 0xf8},
> > {0x3c01, 0x80},
> > {0x3b07, 0x0c},
> > - {0x380c, 0x09},
> > - {0x380d, 0x70},
> > {0x3814, 0x11},
> > {0x3815, 0x11},
> > {0x3708, 0x64},
> > @@ -375,8 +375,6 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
> > {0x3809, 0x10},
> > {0x380a, 0x03},
> > {0x380b, 0xcc},
> > - {0x380c, 0x07},
> > - {0x380d, 0x68},
> > {0x3811, 0x0c},
> > {0x3813, 0x06},
> > {0x3814, 0x31},
> > @@ -450,8 +448,6 @@ static struct regval_list ov5647_640x480_10bpp[] = {
> > {0x3a19, 0xf8},
> > {0x3c01, 0x80},
> > {0x3b07, 0x0c},
> > - {0x380c, 0x07},
> > - {0x380d, 0x3c},
> > {0x3814, 0x35},
> > {0x3815, 0x35},
> > {0x3708, 0x64},
> > @@ -1061,7 +1057,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> > mode->pixel_rate, 1, mode->pixel_rate);
> >
> > hblank = mode->hts - mode->format.width;
> > - __v4l2_ctrl_modify_range(sensor->hblank, hblank, hblank, 1,
> > + __v4l2_ctrl_modify_range(sensor->hblank, hblank,
> > + OV5647_HTS_MAX - mode->format.width, 1,
>
> Is '1' really the min ? Who knows the datasheet doesn't report that :(
>
I think 1 is set as the step value here, keeping the minimum equal to
default HTS - width (to be safe, as I presume the HTS values for a mode
come from the sensor manufacturer)
> > hblank);
> >
> > vblank = mode->vts - mode->format.height;
> > @@ -1325,6 +1322,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> > ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
> > sensor->mode->format.height + ctrl->val);
> > break;
> > + case V4L2_CID_HBLANK:
> > + ret = ov5647_write16(sd, OV5647_REG_HTS_HI,
> > + sensor->mode->format.width + ctrl->val);
>
> Why are we writing HTS_HI only ? The max control value is set to
> 0x1fff, this spans two registers..
>
IIUC ov5647_write16 sends two bytes of data over I2C, where it auto
increments the address when writing the second byte.
> > + break;
> > case V4L2_CID_TEST_PATTERN:
> > ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
> > ov5647_test_pattern_val[ctrl->val]);
> > @@ -1332,7 +1333,6 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> >
> > /* Read-only, but we adjust it based on mode. */
> > case V4L2_CID_PIXEL_RATE:
> > - case V4L2_CID_HBLANK:
> > /* Read-only, but we adjust it based on mode. */
>
> We really like this comment, at the point of repeating it twice...
>
Oops, you're right, I'll drop this in the cleanup commit for PIXEL_RATE
control.
> Speaking of which... if you set the ctrl_handler to NULL when
> registering a ro control
>
> sensor->pixel_rate = v4l2_ctrl_new_std(&sensor->ctrls, NULL,
> V4L2_CID_PIXEL_RATE,
> sensor->mode->pixel_rate,
> sensor->mode->pixel_rate, 1,
> sensor->mode->pixel_rate);
>
> you can remove the above 4 lines.
>
> (the background is that for RO controls the control_handler has to be
> set to NULL, to avoid having to handle them in the s_ctrl handler)
>
> Maybe add a patch to this series ?
Nice, will do!
>
> > break;
> >
> > @@ -1409,10 +1409,11 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > sensor->mode->pixel_rate, 1,
> > sensor->mode->pixel_rate);
> >
> > - /* By default, HBLANK is read only, but it does change per mode. */
> > hblank = sensor->mode->hts - sensor->mode->format.width;
> > sensor->hblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > - V4L2_CID_HBLANK, hblank, hblank, 1,
> > + V4L2_CID_HBLANK, hblank,
> > + OV5647_HTS_MAX -
> > + sensor->mode->format.width, 1,
> > hblank);
> >
> > sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > @@ -1446,7 +1447,6 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > goto handler_free;
> >
> > sensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > - sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > sensor->sd.ctrl_handler = &sensor->ctrls;
> >
> > return 0;
> >
> > --
> > 2.51.0
> >
Thanks,
Jai
© 2016 - 2026 Red Hat, Inc.