Am Donnerstag, dem 06.04.2023 um 04:35 +0300 schrieb Laurent Pinchart:
> Hi Martin,
>
> Thank you for the patch.
>
> On Wed, Apr 05, 2023 at 11:29:04AM +0200, Martin Kepplinger wrote:
> > The hi846 driver changed the "streaming" state inside of
> > "start/stop_streaming".
> > The problem is that inside of the (system) suspend callback, it
> > calls
> > "stop_streaming" unconditionally. So streaming would always be
> > stopped
> > when suspending.
> >
> > That makes sense with runtime pm for example, after s_stream(...,
> > 0) but
> > does not preserve the "streaming" state during system suspend when
> > currently streaming.
>
> The driver shouldn't need to stop streaming at system suspend time.
> It
> should have had its .s_stream(0) operation called and shouldn't be
> streaming anymore. If that's not the case, there's an issue somewhere
> else, which should be fixed. The code that stops streaming at system
> suspend and restarts it at system resume should then be dropped from
> this driver.
>
> > Fix this by simply setting the streaming state outside of
> > "start/stop_streaming"
> > which is s_stream().
> >
> > While at it, improve things a bit by not assigning "1", but the
> > "enable"
> > value we later compare against, and fix one error handling path in
> > resume().
> >
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@xxxxxxx>
> > ---
> > drivers/media/i2c/hi846.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> > index 0b0eda2e223cd..1ca6e9407d618 100644
> > --- a/drivers/media/i2c/hi846.c
> > +++ b/drivers/media/i2c/hi846.c
> > @@ -1780,8 +1780,6 @@ static int hi846_start_streaming(struct hi846
> > *hi846)
> > return ret;
> > }
> >
> > - hi846->streaming = 1;
> > -
> > dev_dbg(&client->dev, "%s: started streaming
> > successfully\n", __func__);
> >
> > return ret;
> > @@ -1793,8 +1791,6 @@ static void hi846_stop_streaming(struct hi846
> > *hi846)
> >
> > if (hi846_write_reg(hi846, HI846_REG_MODE_SELECT,
> > HI846_MODE_STANDBY))
> > dev_err(&client->dev, "failed to stop stream");
> > -
> > - hi846->streaming = 0;
> > }
> >
> > static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
> > @@ -1816,10 +1812,12 @@ static int hi846_set_stream(struct
> > v4l2_subdev *sd, int enable)
> > }
> >
> > ret = hi846_start_streaming(hi846);
> > + hi846->streaming = enable;
> > }
> >
> > if (!enable || ret) {
> > hi846_stop_streaming(hi846);
> > + hi846->streaming = 0;
> > pm_runtime_put(&client->dev);
> > }
> >
> > @@ -1898,6 +1896,8 @@ static int __maybe_unused hi846_resume(struct
> > device *dev)
> > if (ret) {
> > dev_err(dev, "%s: start streaming failed:
> > %d\n",
> > __func__, ret);
> > + hi846_stop_streaming(hi846);
> > + hi846->streaming = 0;
> > goto error;
> > }
> > }
>
hi Laurent,
ok I see. My first test without any streaming-state handling in
suspend/resume doesn't succeed. But now I know that's the goal and I'll
put this on my list to do.
Since this driver *already* tracks "streaming", would you be ok with
this fix in the meantime?
thanks,
martin
