Re: [PATCH v1 2/2] media: hi846: preserve the streaming state during system suspend

[Martin Kepplinger]

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