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@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
>
> > }
> >
> > 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
>
