diff mbox series

[3/4] media: mt9m111: fix device power usage

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

Commit Message

Marco Felsch Aug. 18, 2022, 2:47 p.m. UTC
Currently the driver turn off the power after probe and toggle it during
.stream by using the .s_power callback. This is problematic since other
callbacks like .set_fmt accessing the hardware as well which will fail.
So in the end the default format is the only supported format.

Remove the hardware register access from the callbacks and instead sync
the state once right before the stream gets enabled to fix this.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/mt9m111.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi Aug. 19, 2022, 7:26 a.m. UTC | #1
Hi Marco

On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> Currently the driver turn off the power after probe and toggle it during
> .stream by using the .s_power callback. This is problematic since other
> callbacks like .set_fmt accessing the hardware as well which will fail.

Ouch!

> So in the end the default format is the only supported format.
>
> Remove the hardware register access from the callbacks and instead sync
> the state once right before the stream gets enabled to fix this.

Where does it happen in this patch ?

>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/media/i2c/mt9m111.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 53c4dac4e4bd..cd74c408e110 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -481,8 +481,6 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
>  	width = min(mt9m111->width, rect.width);
>  	height = min(mt9m111->height, rect.height);
>
> -

Why in mainline I don't see these empty lines ?

> -	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
>  	mt9m111->rect = rect;
>  	mt9m111->width = width;
>  	mt9m111->height = height;
> @@ -611,7 +609,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
>  	if (mt9m111->pclk_sample == 0)
>  		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
>
> -
>  	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
>  			 data_outfmt2, mask_outfmt2);
>  	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> @@ -678,9 +675,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
>  		return 0;
>  	}
>
> -
> -	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> -	mt9m111_set_pixfmt(mt9m111, mf->code);

Are we looking at two different versions of the driver ??
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9m111.c#L684

>  	mt9m111->width	= mf->width;
>  	mt9m111->height	= mf->height;
>  	mt9m111->fmt	= fmt;
> @@ -743,6 +737,8 @@ mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
>  	return mode;
>  }
>
> +static int mt9m111_s_power(struct v4l2_subdev *sd, int on);
> +
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  static int mt9m111_g_register(struct v4l2_subdev *sd,
>  			      struct v4l2_dbg_register *reg)
> @@ -753,10 +749,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd,
>  	if (reg->reg > 0x2ff)
>  		return -EINVAL;
>
> +	mt9m111_s_power(sd, 1);
> +
>  	val = mt9m111_reg_read(client, reg->reg);
>  	reg->size = 2;
>  	reg->val = (u64)val;
>
> +	mt9m111_s_power(sd, 0);
> +
>  	if (reg->val > 0xffff)
>  		return -EIO;
>
> @@ -771,9 +771,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
>  	if (reg->reg > 0x2ff)
>  		return -EINVAL;
>
> +	mt9m111_s_power(sd, 1);
> +
>  	if (mt9m111_reg_write(client, reg->reg, reg->val) < 0)
>  		return -EIO;
>
> +	mt9m111_s_power(sd, 0);
> +
>  	return 0;
>  }
>  #endif
> @@ -896,6 +900,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
>  					       struct mt9m111, hdl);
>  	int ret;
>
> +	if (!mt9m111->is_streaming)
> +		return 0;
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_VFLIP:
>  		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> @@ -927,7 +934,6 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = -EINVAL;
>  	}
>
> -
>  	return ret;
>  }
>
> --
> 2.30.2
>
Marco Felsch Aug. 19, 2022, 7:32 a.m. UTC | #2
Hi Jacopo,

On 22-08-19, Jacopo Mondi wrote:
> Hi Marco
> 
> On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > Currently the driver turn off the power after probe and toggle it during
> > .stream by using the .s_power callback. This is problematic since other
> > callbacks like .set_fmt accessing the hardware as well which will fail.
> 
> Ouch!
> 
> > So in the end the default format is the only supported format.
> >
> > Remove the hardware register access from the callbacks and instead sync
> > the state once right before the stream gets enabled to fix this.
> 
> Where does it happen in this patch ?

during mt9m111_s_power which gets called by s_power() or after this
small series during s_stream().

> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/media/i2c/mt9m111.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 53c4dac4e4bd..cd74c408e110 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -481,8 +481,6 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> >  	width = min(mt9m111->width, rect.width);
> >  	height = min(mt9m111->height, rect.height);
> >
> > -
> 
> Why in mainline I don't see these empty lines ?

Hm.. because I introduced this during my "media: mt9m111: fix subdev API
usage" patch.. Sorry.

> > -	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> >  	mt9m111->rect = rect;
> >  	mt9m111->width = width;
> >  	mt9m111->height = height;
> > @@ -611,7 +609,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> >  	if (mt9m111->pclk_sample == 0)
> >  		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> >
> > -
> >  	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> >  			 data_outfmt2, mask_outfmt2);
> >  	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> > @@ -678,9 +675,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> >  		return 0;
> >  	}
> >
> > -
> > -	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> > -	mt9m111_set_pixfmt(mt9m111, mf->code);
> 
> Are we looking at two different versions of the driver ??
> https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9m111.c#L684

Same here.

> >  	mt9m111->width	= mf->width;
> >  	mt9m111->height	= mf->height;
> >  	mt9m111->fmt	= fmt;
> > @@ -743,6 +737,8 @@ mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
> >  	return mode;
> >  }
> >
> > +static int mt9m111_s_power(struct v4l2_subdev *sd, int on);
> > +
> >  #ifdef CONFIG_VIDEO_ADV_DEBUG
> >  static int mt9m111_g_register(struct v4l2_subdev *sd,
> >  			      struct v4l2_dbg_register *reg)
> > @@ -753,10 +749,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd,
> >  	if (reg->reg > 0x2ff)
> >  		return -EINVAL;
> >
> > +	mt9m111_s_power(sd, 1);
> > +
> >  	val = mt9m111_reg_read(client, reg->reg);
> >  	reg->size = 2;
> >  	reg->val = (u64)val;
> >
> > +	mt9m111_s_power(sd, 0);
> > +
> >  	if (reg->val > 0xffff)
> >  		return -EIO;
> >
> > @@ -771,9 +771,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
> >  	if (reg->reg > 0x2ff)
> >  		return -EINVAL;
> >
> > +	mt9m111_s_power(sd, 1);
> > +
> >  	if (mt9m111_reg_write(client, reg->reg, reg->val) < 0)
> >  		return -EIO;
> >
> > +	mt9m111_s_power(sd, 0);
> > +
> >  	return 0;
> >  }
> >  #endif
> > @@ -896,6 +900,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> >  					       struct mt9m111, hdl);
> >  	int ret;
> >
> > +	if (!mt9m111->is_streaming)
> > +		return 0;
> > +
> >  	switch (ctrl->id) {
> >  	case V4L2_CID_VFLIP:
> >  		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> > @@ -927,7 +934,6 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> >  		ret = -EINVAL;
> >  	}
> >
> > -
> >  	return ret;
> >  }
> >
> > --
> > 2.30.2
> >
>
Sakari Ailus Aug. 22, 2022, 6:31 a.m. UTC | #3
Hi Marco,

On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> Currently the driver turn off the power after probe and toggle it during
> .stream by using the .s_power callback. This is problematic since other
> callbacks like .set_fmt accessing the hardware as well which will fail.
> So in the end the default format is the only supported format.

It'd be much better to add runtime PM support to the driver instead.
Marco Felsch Aug. 22, 2022, 7:54 a.m. UTC | #4
Hi Sakari,

On 22-08-22, Sakari Ailus wrote:
> Hi Marco,
> 
> On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > Currently the driver turn off the power after probe and toggle it during
> > .stream by using the .s_power callback. This is problematic since other
> > callbacks like .set_fmt accessing the hardware as well which will fail.
> > So in the end the default format is the only supported format.
> 
> It'd be much better to add runtime PM support to the driver instead.

I got your point, but didn't have the time for it right now, I will drop
the patch from my v2.

Regards,
  Marco

> 
> -- 
> Sakari Ailus
>
Sakari Ailus Aug. 22, 2022, 9:18 a.m. UTC | #5
On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> Hi Sakari,
> 
> On 22-08-22, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > Currently the driver turn off the power after probe and toggle it during
> > > .stream by using the .s_power callback. This is problematic since other
> > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > So in the end the default format is the only supported format.
> > 
> > It'd be much better to add runtime PM support to the driver instead.
> 
> I got your point, but didn't have the time for it right now, I will drop
> the patch from my v2.

The API is different but generally involves doing more or less the same
what this and the 4th patch do together.
Marco Felsch Aug. 23, 2022, 2:44 p.m. UTC | #6
Hi Sakari,

On 22-08-22, Sakari Ailus wrote:
> On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> > Hi Sakari,
> > 
> > On 22-08-22, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > > Currently the driver turn off the power after probe and toggle it during
> > > > .stream by using the .s_power callback. This is problematic since other
> > > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > > So in the end the default format is the only supported format.
> > > 
> > > It'd be much better to add runtime PM support to the driver instead.
> > 
> > I got your point, but didn't have the time for it right now, I will drop
> > the patch from my v2.
> 
> The API is different but generally involves doing more or less the same
> what this and the 4th patch do together.

I know :) as soon as I got feedback on my TC35 series [1] I give it a
try and change it to dev-pm.

[1] https://lore.kernel.org/linux-media/20220818143307.967150-1-m.felsch@pengutronix.de/T/#t

Regards,
  Marco


> 
> -- 
> Sakari Ailus
>
Sakari Ailus Jan. 16, 2023, 10:14 p.m. UTC | #7
Hi Marco,

On Tue, Aug 23, 2022 at 04:44:50PM +0200, Marco Felsch wrote:
> Hi Sakari,
> 
> On 22-08-22, Sakari Ailus wrote:
> > On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> > > Hi Sakari,
> > > 
> > > On 22-08-22, Sakari Ailus wrote:
> > > > Hi Marco,
> > > > 
> > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > > > Currently the driver turn off the power after probe and toggle it during
> > > > > .stream by using the .s_power callback. This is problematic since other
> > > > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > > > So in the end the default format is the only supported format.
> > > > 
> > > > It'd be much better to add runtime PM support to the driver instead.
> > > 
> > > I got your point, but didn't have the time for it right now, I will drop
> > > the patch from my v2.
> > 
> > The API is different but generally involves doing more or less the same
> > what this and the 4th patch do together.
> 
> I know :) as soon as I got feedback on my TC35 series [1] I give it a
> try and change it to dev-pm.

What's the status of this set?

These are nice improvements but I was expecting v2.

Thanks.
Marco Felsch Jan. 17, 2023, 11:29 a.m. UTC | #8
Hi Sakari,

On 23-01-17, Sakari Ailus wrote:
> Hi Marco,
> 
> On Tue, Aug 23, 2022 at 04:44:50PM +0200, Marco Felsch wrote:
> > Hi Sakari,
> > 
> > On 22-08-22, Sakari Ailus wrote:
> > > On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> > > > Hi Sakari,
> > > > 
> > > > On 22-08-22, Sakari Ailus wrote:
> > > > > Hi Marco,
> > > > > 
> > > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > > > > Currently the driver turn off the power after probe and toggle it during
> > > > > > .stream by using the .s_power callback. This is problematic since other
> > > > > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > > > > So in the end the default format is the only supported format.
> > > > > 
> > > > > It'd be much better to add runtime PM support to the driver instead.
> > > > 
> > > > I got your point, but didn't have the time for it right now, I will drop
> > > > the patch from my v2.
> > > 
> > > The API is different but generally involves doing more or less the same
> > > what this and the 4th patch do together.
> > 
> > I know :) as soon as I got feedback on my TC35 series [1] I give it a
> > try and change it to dev-pm.
> 
> What's the status of this set?
> 
> These are nice improvements but I was expecting v2.

Unfortunately this was just a testing vehicle I had for the TC35 bridge
chip. As soon as I have access to it again I will send a new version.

Regards,
  Marco
Sakari Ailus Jan. 17, 2023, 11:32 a.m. UTC | #9
On Tue, Jan 17, 2023 at 12:29:59PM +0100, Marco Felsch wrote:
> Hi Sakari,
> 
> On 23-01-17, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Tue, Aug 23, 2022 at 04:44:50PM +0200, Marco Felsch wrote:
> > > Hi Sakari,
> > > 
> > > On 22-08-22, Sakari Ailus wrote:
> > > > On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > On 22-08-22, Sakari Ailus wrote:
> > > > > > Hi Marco,
> > > > > > 
> > > > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > > > > > Currently the driver turn off the power after probe and toggle it during
> > > > > > > .stream by using the .s_power callback. This is problematic since other
> > > > > > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > > > > > So in the end the default format is the only supported format.
> > > > > > 
> > > > > > It'd be much better to add runtime PM support to the driver instead.
> > > > > 
> > > > > I got your point, but didn't have the time for it right now, I will drop
> > > > > the patch from my v2.
> > > > 
> > > > The API is different but generally involves doing more or less the same
> > > > what this and the 4th patch do together.
> > > 
> > > I know :) as soon as I got feedback on my TC35 series [1] I give it a
> > > try and change it to dev-pm.
> > 
> > What's the status of this set?
> > 
> > These are nice improvements but I was expecting v2.
> 
> Unfortunately this was just a testing vehicle I had for the TC35 bridge
> chip. As soon as I have access to it again I will send a new version.

Ack, thanks!
diff mbox series

Patch

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 53c4dac4e4bd..cd74c408e110 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -481,8 +481,6 @@  static int mt9m111_set_selection(struct v4l2_subdev *sd,
 	width = min(mt9m111->width, rect.width);
 	height = min(mt9m111->height, rect.height);
 
-
-	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
 	mt9m111->rect = rect;
 	mt9m111->width = width;
 	mt9m111->height = height;
@@ -611,7 +609,6 @@  static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
 	if (mt9m111->pclk_sample == 0)
 		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
 
-
 	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
 			 data_outfmt2, mask_outfmt2);
 	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
@@ -678,9 +675,6 @@  static int mt9m111_set_fmt(struct v4l2_subdev *sd,
 		return 0;
 	}
 
-
-	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
-	mt9m111_set_pixfmt(mt9m111, mf->code);
 	mt9m111->width	= mf->width;
 	mt9m111->height	= mf->height;
 	mt9m111->fmt	= fmt;
@@ -743,6 +737,8 @@  mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
 	return mode;
 }
 
+static int mt9m111_s_power(struct v4l2_subdev *sd, int on);
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int mt9m111_g_register(struct v4l2_subdev *sd,
 			      struct v4l2_dbg_register *reg)
@@ -753,10 +749,14 @@  static int mt9m111_g_register(struct v4l2_subdev *sd,
 	if (reg->reg > 0x2ff)
 		return -EINVAL;
 
+	mt9m111_s_power(sd, 1);
+
 	val = mt9m111_reg_read(client, reg->reg);
 	reg->size = 2;
 	reg->val = (u64)val;
 
+	mt9m111_s_power(sd, 0);
+
 	if (reg->val > 0xffff)
 		return -EIO;
 
@@ -771,9 +771,13 @@  static int mt9m111_s_register(struct v4l2_subdev *sd,
 	if (reg->reg > 0x2ff)
 		return -EINVAL;
 
+	mt9m111_s_power(sd, 1);
+
 	if (mt9m111_reg_write(client, reg->reg, reg->val) < 0)
 		return -EIO;
 
+	mt9m111_s_power(sd, 0);
+
 	return 0;
 }
 #endif
@@ -896,6 +900,9 @@  static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
 					       struct mt9m111, hdl);
 	int ret;
 
+	if (!mt9m111->is_streaming)
+		return 0;
+
 	switch (ctrl->id) {
 	case V4L2_CID_VFLIP:
 		ret = mt9m111_set_flip(mt9m111, ctrl->val,
@@ -927,7 +934,6 @@  static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = -EINVAL;
 	}
 
-
 	return ret;
 }