diff mbox series

[10/16] media: i2c: ov9282: Action CID_VBLANK when set.

Message ID 20221005152809.3785786-11-dave.stevenson@raspberrypi.com (mailing list archive)
State Changes Requested
Delegated to: Sakari Ailus
Headers
Series Updates to ov9282 sensor driver |

Commit Message

Dave Stevenson Oct. 5, 2022, 3:28 p.m. UTC 
  Programming the sensor with TIMING_VTS (aka LPFR) was done
when triggered by a change in exposure or gain, but not
when V4L2_CID_VBLANK was changed. Dynamic frame rate
changes could therefore not be achieved.

Separate out programming TIMING_VTS so that it is triggered
by set_ctrl(V4L2_CID_VBLANK)

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

Comments

Jacopo Mondi Oct. 6, 2022, 9:29 a.m. UTC | #1 
Hi Dave

On Wed, Oct 05, 2022 at 04:28:03PM +0100, Dave Stevenson wrote:
> Programming the sensor with TIMING_VTS (aka LPFR) was done
> when triggered by a change in exposure or gain, but not
> when V4L2_CID_VBLANK was changed. Dynamic frame rate
> changes could therefore not be achieved.
>
> Separate out programming TIMING_VTS so that it is triggered
> by set_ctrl(V4L2_CID_VBLANK)
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 183283d191b1..5ddef6e2b3ac 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -419,22 +419,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
>   */
>  static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
>  {
> -	u32 lpfr;
>  	int ret;
>
> -	lpfr = ov9282->vblank + ov9282->cur_mode->height;
> -
> -	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u, lpfr %u",
> -		exposure, gain, lpfr);
> +	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
> +		exposure, gain);
>
>  	ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
>  	if (ret)
>  		return ret;
>
> -	ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
> -	if (ret)
> -		goto error_release_group_hold;
> -
>  	ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4);
>  	if (ret)
>  		goto error_release_group_hold;
> @@ -465,6 +458,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>  		container_of(ctrl->handler, struct ov9282, ctrl_handler);
>  	u32 analog_gain;
>  	u32 exposure;
> +	u32 lpfr;

Only a nit about the fact lpfr is a u32 while you're writing 2 bytes.
I guess it's safe as we likely don't risk any overflow

>  	int ret;
>
>  	switch (ctrl->id) {
> @@ -482,10 +476,14 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>  					       OV9282_EXPOSURE_OFFSET,
>  					       1, OV9282_EXPOSURE_DEFAULT);
>  		break;
> +	}
> +
> +	/* Set controls only if sensor is in power on state */
> +	if (!pm_runtime_get_if_in_use(ov9282->dev))
> +		return 0;
> +
> +	switch (ctrl->id) {
>  	case V4L2_CID_EXPOSURE:
> -		/* Set controls only if sensor is in power on state */
> -		if (!pm_runtime_get_if_in_use(ov9282->dev))
> -			return 0;
>
>  		exposure = ctrl->val;
>  		analog_gain = ov9282->again_ctrl->val;
> @@ -495,14 +493,19 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>
>  		ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
>
> -		pm_runtime_put(ov9282->dev);
>
Double empty line
With this fixed:

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j
> +		break;
> +	case V4L2_CID_VBLANK:
> +		lpfr = ov9282->vblank + ov9282->cur_mode->height;
> +		ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
>  		break;
>  	default:
>  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
>  		ret = -EINVAL;
>  	}
>
> +	pm_runtime_put(ov9282->dev);
> +
>  	return ret;
>  }
>
> --
> 2.34.1
>
Dave Stevenson Oct. 6, 2022, 1:21 p.m. UTC | #2 
Hi Jacopo

On Thu, 6 Oct 2022 at 10:29, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:28:03PM +0100, Dave Stevenson wrote:
> > Programming the sensor with TIMING_VTS (aka LPFR) was done
> > when triggered by a change in exposure or gain, but not
> > when V4L2_CID_VBLANK was changed. Dynamic frame rate
> > changes could therefore not be achieved.
> >
> > Separate out programming TIMING_VTS so that it is triggered
> > by set_ctrl(V4L2_CID_VBLANK)
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 29 ++++++++++++++++-------------
> >  1 file changed, 16 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 183283d191b1..5ddef6e2b3ac 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -419,22 +419,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
> >   */
> >  static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> >  {
> > -     u32 lpfr;
> >       int ret;
> >
> > -     lpfr = ov9282->vblank + ov9282->cur_mode->height;
> > -
> > -     dev_dbg(ov9282->dev, "Set exp %u, analog gain %u, lpfr %u",
> > -             exposure, gain, lpfr);
> > +     dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
> > +             exposure, gain);
> >
> >       ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
> >       if (ret)
> >               return ret;
> >
> > -     ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
> > -     if (ret)
> > -             goto error_release_group_hold;
> > -
> >       ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4);
> >       if (ret)
> >               goto error_release_group_hold;
> > @@ -465,6 +458,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >               container_of(ctrl->handler, struct ov9282, ctrl_handler);
> >       u32 analog_gain;
> >       u32 exposure;
> > +     u32 lpfr;
>
> Only a nit about the fact lpfr is a u32 while you're writing 2 bytes.
> I guess it's safe as we likely don't risk any overflow

I was moving u32 lpfr from ov9282_update_exp_gain to here. All the
handling of lpfr (aka TIMING_VTS) is done as u32 in this driver.

The max range of V4L2_CID_VBLANK is set from the ov9282_mode
definition (not strictly necessary as it should be a fixed max value
handled by the register), so as long as the mode is defined correctly
then there should be no overflow.

  Dave

> >       int ret;
> >
> >       switch (ctrl->id) {
> > @@ -482,10 +476,14 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >                                              OV9282_EXPOSURE_OFFSET,
> >                                              1, OV9282_EXPOSURE_DEFAULT);
> >               break;
> > +     }
> > +
> > +     /* Set controls only if sensor is in power on state */
> > +     if (!pm_runtime_get_if_in_use(ov9282->dev))
> > +             return 0;
> > +
> > +     switch (ctrl->id) {
> >       case V4L2_CID_EXPOSURE:
> > -             /* Set controls only if sensor is in power on state */
> > -             if (!pm_runtime_get_if_in_use(ov9282->dev))
> > -                     return 0;
> >
> >               exposure = ctrl->val;
> >               analog_gain = ov9282->again_ctrl->val;
> > @@ -495,14 +493,19 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >
> >               ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
> >
> > -             pm_runtime_put(ov9282->dev);
> >
> Double empty line
> With this fixed:
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>    j
> > +             break;
> > +     case V4L2_CID_VBLANK:
> > +             lpfr = ov9282->vblank + ov9282->cur_mode->height;
> > +             ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
> >               break;
> >       default:
> >               dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >               ret = -EINVAL;
> >       }
> >
> > +     pm_runtime_put(ov9282->dev);
> > +
> >       return ret;
> >  }
> >
> > --
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 183283d191b1..5ddef6e2b3ac 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -419,22 +419,15 @@  static int ov9282_update_controls(struct ov9282 *ov9282,
  */
 static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 {
-	u32 lpfr;
 	int ret;
 
-	lpfr = ov9282->vblank + ov9282->cur_mode->height;
-
-	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u, lpfr %u",
-		exposure, gain, lpfr);
+	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
+		exposure, gain);
 
 	ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
 	if (ret)
 		return ret;
 
-	ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
-	if (ret)
-		goto error_release_group_hold;
-
 	ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4);
 	if (ret)
 		goto error_release_group_hold;
@@ -465,6 +458,7 @@  static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		container_of(ctrl->handler, struct ov9282, ctrl_handler);
 	u32 analog_gain;
 	u32 exposure;
+	u32 lpfr;
 	int ret;
 
 	switch (ctrl->id) {
@@ -482,10 +476,14 @@  static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 					       OV9282_EXPOSURE_OFFSET,
 					       1, OV9282_EXPOSURE_DEFAULT);
 		break;
+	}
+
+	/* Set controls only if sensor is in power on state */
+	if (!pm_runtime_get_if_in_use(ov9282->dev))
+		return 0;
+
+	switch (ctrl->id) {
 	case V4L2_CID_EXPOSURE:
-		/* Set controls only if sensor is in power on state */
-		if (!pm_runtime_get_if_in_use(ov9282->dev))
-			return 0;
 
 		exposure = ctrl->val;
 		analog_gain = ov9282->again_ctrl->val;
@@ -495,14 +493,19 @@  static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
 
-		pm_runtime_put(ov9282->dev);
 
+		break;
+	case V4L2_CID_VBLANK:
+		lpfr = ov9282->vblank + ov9282->cur_mode->height;
+		ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
 		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
 	}
 
+	pm_runtime_put(ov9282->dev);
+
 	return ret;
 }