| Message ID | 20220916135713.143890-3-m.felsch@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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; > } >
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 >
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; }
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(-)