Hi Martin, On Tue, Apr 11, 2023 at 11:39:32AM +0200, Martin Kepplinger wrote: > Am Donnerstag, dem 06.04.2023 um 04:35 +0300 schrieb Laurent Pinchart: > > 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? I'd rather get a patch that drops streaming handling :-) It's fine carrying a local patch in the Librem5 kernel to work around the problem until a proper fix is available, but do we have a need to get the workaround in mainline too ? When it comes to a proper fix, it may be as simple as manually calling device_link_add() in consumer (e.g. the CSI-2 receiver) drivers to create links to suppliers(e.g. the camera sensor). The PM core should then guarantee that the consumer gets suspended before the producer. Would you be able to test that ? -- Regards, Laurent Pinchart