diff mbox series

[v2,3/3] media: mt9m111: don't turn on the output while powering it

Message ID 20220916135713.143890-3-m.felsch@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series [v2,1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support | expand

Commit Message

Marco Felsch Sept. 16, 2022, 1:57 p.m. UTC 
Currently the .s_power() turn on/off the device and enables/disables the
sensor output. This is wrong since it should only handle the power not
not the sensor output behaviour. Enabling the sensor output should be
part of the .s_stream() callback.

Fix this by adding mt9m111_set_output() which gets called by the
.s_stream() callback and remove the output register bits from
mt9m111_resume/suspend.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:

v2:
- new patch, replaces: "media: mt9m111: remove .s_power callback"
---
 drivers/media/i2c/mt9m111.c | 38 ++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

Comments

Laurent Pinchart Sept. 19, 2022, 1 p.m. UTC | #1 
Hi Marco,

Thank you for the patch.

On Fri, Sep 16, 2022 at 03:57:13PM +0200, Marco Felsch wrote:
> Currently the .s_power() turn on/off the device and enables/disables the
> sensor output. This is wrong since it should only handle the power not
> not the sensor output behaviour. Enabling the sensor output should be
> part of the .s_stream() callback.
> 
> Fix this by adding mt9m111_set_output() which gets called by the
> .s_stream() callback and remove the output register bits from
> mt9m111_resume/suspend.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Changelog:
> 
> v2:
> - new patch, replaces: "media: mt9m111: remove .s_power callback"
> ---
>  drivers/media/i2c/mt9m111.c | 38 ++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 8de93ab99cbc..2cc0b0da7636 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -426,10 +426,25 @@ static int mt9m111_setup_geometry(struct mt9m111 *mt9m111, struct v4l2_rect *rec
>  	return ret;
>  }
>  
> -static int mt9m111_enable(struct mt9m111 *mt9m111)
> +static int mt9m111_set_output(struct mt9m111 *mt9m111, int on)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> -	return reg_write(RESET, MT9M111_RESET_CHIP_ENABLE);
> +	int ret;
> +
> +	if (on) {
> +		ret = reg_clear(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> +		if (ret)
> +			return ret;
> +
> +		return reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
> +	}
> +
> +	/* disable */
> +	ret = reg_set(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> +	if (ret)
> +		return ret;
> +
> +	return reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);

Unless the hardware specifically requires this sequence, I'd use the
inverse of the enable sequence here.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  }
>  
>  static int mt9m111_reset(struct mt9m111 *mt9m111)
> @@ -927,10 +942,7 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
>  	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
>  	if (!ret)
>  		ret = reg_set(RESET, MT9M111_RESET_RESET_SOC |
> -			      MT9M111_RESET_OUTPUT_DISABLE |
>  			      MT9M111_RESET_ANALOG_STANDBY);
> -	if (!ret)
> -		ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
>  
>  	return ret;
>  }
> @@ -951,9 +963,9 @@ static void mt9m111_restore_state(struct mt9m111 *mt9m111)
>  
>  static int mt9m111_resume(struct mt9m111 *mt9m111)
>  {
> -	int ret = mt9m111_enable(mt9m111);
> -	if (!ret)
> -		ret = mt9m111_reset(mt9m111);
> +	int ret;
> +
> +	ret = mt9m111_reset(mt9m111);
>  	if (!ret)
>  		mt9m111_restore_state(mt9m111);
>  
> @@ -965,9 +977,7 @@ static int mt9m111_init(struct mt9m111 *mt9m111)
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>  	int ret;
>  
> -	ret = mt9m111_enable(mt9m111);
> -	if (!ret)
> -		ret = mt9m111_reset(mt9m111);
> +	ret = mt9m111_reset(mt9m111);
>  	if (!ret)
>  		ret = mt9m111_set_context(mt9m111, mt9m111->ctx);
>  	if (ret)
> @@ -1116,8 +1126,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
>  static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> +	int ret;
> +
> +	ret = mt9m111_set_output(mt9m111, enable);
> +	if (ret)
> +		return ret;
>  
>  	mt9m111->is_streaming = !!enable;
> +
>  	return 0;
>  }
>
Marco Felsch Sept. 19, 2022, 1:17 p.m. UTC | #2 
Hi Laurent,

On 22-09-19, Laurent Pinchart wrote:
> Hi Marco,
> 
> Thank you for the patch.
> 
> On Fri, Sep 16, 2022 at 03:57:13PM +0200, Marco Felsch wrote:
> > Currently the .s_power() turn on/off the device and enables/disables the
> > sensor output. This is wrong since it should only handle the power not
> > not the sensor output behaviour. Enabling the sensor output should be
> > part of the .s_stream() callback.
> > 
> > Fix this by adding mt9m111_set_output() which gets called by the
> > .s_stream() callback and remove the output register bits from
> > mt9m111_resume/suspend.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Changelog:
> > 
> > v2:
> > - new patch, replaces: "media: mt9m111: remove .s_power callback"
> > ---
> >  drivers/media/i2c/mt9m111.c | 38 ++++++++++++++++++++++++++-----------
> >  1 file changed, 27 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 8de93ab99cbc..2cc0b0da7636 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -426,10 +426,25 @@ static int mt9m111_setup_geometry(struct mt9m111 *mt9m111, struct v4l2_rect *rec
> >  	return ret;
> >  }
> >  
> > -static int mt9m111_enable(struct mt9m111 *mt9m111)
> > +static int mt9m111_set_output(struct mt9m111 *mt9m111, int on)
> >  {
> >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> > -	return reg_write(RESET, MT9M111_RESET_CHIP_ENABLE);
> > +	int ret;
> > +
> > +	if (on) {
> > +		ret = reg_clear(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
> > +	}
> > +
> > +	/* disable */
> > +	ret = reg_set(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
> 
> Unless the hardware specifically requires this sequence, I'd use the
> inverse of the enable sequence here.

Output-disable: -> put output pins into tri-state.
chip-enable:  -> reset sensor readout path.

Enable:
  -> set output pins accordingly
     -> put sensor out of reset --> start sensor readout

Disable:
  -> set output pins to tri-state
     -> put sensor into reset --> stop sensor readout

To avoid possible artifacts I disabled the pins first before resetting
the sensor core (stopping the readout).

Regards,
  Marco

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  }
> >  
> >  static int mt9m111_reset(struct mt9m111 *mt9m111)
> > @@ -927,10 +942,7 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
> >  	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
> >  	if (!ret)
> >  		ret = reg_set(RESET, MT9M111_RESET_RESET_SOC |
> > -			      MT9M111_RESET_OUTPUT_DISABLE |
> >  			      MT9M111_RESET_ANALOG_STANDBY);
> > -	if (!ret)
> > -		ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
> >  
> >  	return ret;
> >  }
> > @@ -951,9 +963,9 @@ static void mt9m111_restore_state(struct mt9m111 *mt9m111)
> >  
> >  static int mt9m111_resume(struct mt9m111 *mt9m111)
> >  {
> > -	int ret = mt9m111_enable(mt9m111);
> > -	if (!ret)
> > -		ret = mt9m111_reset(mt9m111);
> > +	int ret;
> > +
> > +	ret = mt9m111_reset(mt9m111);
> >  	if (!ret)
> >  		mt9m111_restore_state(mt9m111);
> >  
> > @@ -965,9 +977,7 @@ static int mt9m111_init(struct mt9m111 *mt9m111)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> >  	int ret;
> >  
> > -	ret = mt9m111_enable(mt9m111);
> > -	if (!ret)
> > -		ret = mt9m111_reset(mt9m111);
> > +	ret = mt9m111_reset(mt9m111);
> >  	if (!ret)
> >  		ret = mt9m111_set_context(mt9m111, mt9m111->ctx);
> >  	if (ret)
> > @@ -1116,8 +1126,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
> >  static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> > +	int ret;
> > +
> > +	ret = mt9m111_set_output(mt9m111, enable);
> > +	if (ret)
> > +		return ret;
> >  
> >  	mt9m111->is_streaming = !!enable;
> > +
> >  	return 0;
> >  }
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>
Jacopo Mondi Sept. 20, 2022, 10:39 a.m. UTC | #3 
Hi Marco

On Fri, Sep 16, 2022 at 03:57:13PM +0200, Marco Felsch wrote:
> Currently the .s_power() turn on/off the device and enables/disables the
> sensor output. This is wrong since it should only handle the power not
> not the sensor output behaviour. Enabling the sensor output should be
> part of the .s_stream() callback.
>
> Fix this by adding mt9m111_set_output() which gets called by the
> .s_stream() callback and remove the output register bits from
> mt9m111_resume/suspend.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

I can't say the driver is nice to read (it has been around since a
long time and comes from the soc_camera times) but this makes it nicer
and fixes an issue

With Laurent's comment on the disabling order clarified:

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

Thanks
   j

> ---
> Changelog:
>
> v2:
> - new patch, replaces: "media: mt9m111: remove .s_power callback"
> ---
>  drivers/media/i2c/mt9m111.c | 38 ++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 8de93ab99cbc..2cc0b0da7636 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -426,10 +426,25 @@ static int mt9m111_setup_geometry(struct mt9m111 *mt9m111, struct v4l2_rect *rec
>  	return ret;
>  }
>
> -static int mt9m111_enable(struct mt9m111 *mt9m111)
> +static int mt9m111_set_output(struct mt9m111 *mt9m111, int on)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> -	return reg_write(RESET, MT9M111_RESET_CHIP_ENABLE);
> +	int ret;
> +
> +	if (on) {
> +		ret = reg_clear(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> +		if (ret)
> +			return ret;
> +
> +		return reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
> +	}
> +
> +	/* disable */
> +	ret = reg_set(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> +	if (ret)
> +		return ret;
> +
> +	return reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
>  }
>
>  static int mt9m111_reset(struct mt9m111 *mt9m111)
> @@ -927,10 +942,7 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
>  	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
>  	if (!ret)
>  		ret = reg_set(RESET, MT9M111_RESET_RESET_SOC |
> -			      MT9M111_RESET_OUTPUT_DISABLE |
>  			      MT9M111_RESET_ANALOG_STANDBY);
> -	if (!ret)
> -		ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
>
>  	return ret;
>  }
> @@ -951,9 +963,9 @@ static void mt9m111_restore_state(struct mt9m111 *mt9m111)
>
>  static int mt9m111_resume(struct mt9m111 *mt9m111)
>  {
> -	int ret = mt9m111_enable(mt9m111);
> -	if (!ret)
> -		ret = mt9m111_reset(mt9m111);
> +	int ret;
> +
> +	ret = mt9m111_reset(mt9m111);
>  	if (!ret)
>  		mt9m111_restore_state(mt9m111);
>
> @@ -965,9 +977,7 @@ static int mt9m111_init(struct mt9m111 *mt9m111)
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>  	int ret;
>
> -	ret = mt9m111_enable(mt9m111);
> -	if (!ret)
> -		ret = mt9m111_reset(mt9m111);
> +	ret = mt9m111_reset(mt9m111);
>  	if (!ret)
>  		ret = mt9m111_set_context(mt9m111, mt9m111->ctx);
>  	if (ret)
> @@ -1116,8 +1126,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
>  static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> +	int ret;
> +
> +	ret = mt9m111_set_output(mt9m111, enable);
> +	if (ret)
> +		return ret;
>
>  	mt9m111->is_streaming = !!enable;
> +
>  	return 0;
>  }
>
> --
> 2.30.2
>
diff mbox series

Patch

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 8de93ab99cbc..2cc0b0da7636 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -426,10 +426,25 @@  static int mt9m111_setup_geometry(struct mt9m111 *mt9m111, struct v4l2_rect *rec
 	return ret;
 }
 
-static int mt9m111_enable(struct mt9m111 *mt9m111)
+static int mt9m111_set_output(struct mt9m111 *mt9m111, int on)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
-	return reg_write(RESET, MT9M111_RESET_CHIP_ENABLE);
+	int ret;
+
+	if (on) {
+		ret = reg_clear(RESET, MT9M111_RESET_OUTPUT_DISABLE);
+		if (ret)
+			return ret;
+
+		return reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
+	}
+
+	/* disable */
+	ret = reg_set(RESET, MT9M111_RESET_OUTPUT_DISABLE);
+	if (ret)
+		return ret;
+
+	return reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
 }
 
 static int mt9m111_reset(struct mt9m111 *mt9m111)
@@ -927,10 +942,7 @@  static int mt9m111_suspend(struct mt9m111 *mt9m111)
 	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
 	if (!ret)
 		ret = reg_set(RESET, MT9M111_RESET_RESET_SOC |
-			      MT9M111_RESET_OUTPUT_DISABLE |
 			      MT9M111_RESET_ANALOG_STANDBY);
-	if (!ret)
-		ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
 
 	return ret;
 }
@@ -951,9 +963,9 @@  static void mt9m111_restore_state(struct mt9m111 *mt9m111)
 
 static int mt9m111_resume(struct mt9m111 *mt9m111)
 {
-	int ret = mt9m111_enable(mt9m111);
-	if (!ret)
-		ret = mt9m111_reset(mt9m111);
+	int ret;
+
+	ret = mt9m111_reset(mt9m111);
 	if (!ret)
 		mt9m111_restore_state(mt9m111);
 
@@ -965,9 +977,7 @@  static int mt9m111_init(struct mt9m111 *mt9m111)
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
 	int ret;
 
-	ret = mt9m111_enable(mt9m111);
-	if (!ret)
-		ret = mt9m111_reset(mt9m111);
+	ret = mt9m111_reset(mt9m111);
 	if (!ret)
 		ret = mt9m111_set_context(mt9m111, mt9m111->ctx);
 	if (ret)
@@ -1116,8 +1126,14 @@  static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
 static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
+	int ret;
+
+	ret = mt9m111_set_output(mt9m111, enable);
+	if (ret)
+		return ret;
 
 	mt9m111->is_streaming = !!enable;
+
 	return 0;
 }