Hi, Jai, > Hi Changhuang, > > On Jun 28, 2024 at 08:45:06 +0000, Changhuang Liang wrote: > > Hi Tomi, > > > > [...] > > > > +static int csi2rx_suspend(struct device *dev) { > > > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > > > + > > > > + mutex_lock(&csi2rx->lock); > > > > + if (csi2rx->count) > > > > + csi2rx_stop(csi2rx); > > > > + mutex_unlock(&csi2rx->lock); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int csi2rx_resume(struct device *dev) { > > > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > > > + > > > > + mutex_lock(&csi2rx->lock); > > > > + if (csi2rx->count) > > > > + csi2rx_start(csi2rx); > > > > + mutex_unlock(&csi2rx->lock); > > > > + return 0; > > > > +} > > > > + > > > > > > I don't think this looks correct. Afaik the runtime suspend/resume > > > is not called on system suspend/resume. You could change the > > > SET_RUNTIME_PM_OPS to use the same callbacks for runtime and > system > > > suspend, but I think that's a bad idea. Runtime suspend is not > > > supposed to turn off the streaming. The driver is supposed to turn > > > off the streaming, then call runtime_put, which would result in runtime > suspend callback getting called. > > > > > > > I implemented system suspend/resume based on this patch, I feel good > about it. > > > > https://lore.kernel.org/all/20240326031237.25331-1-changhuang.liang@st > > arfivetech.com/ > > Thanks for carrying this patch in your series. > > I think Tomi's point still holds - only the system suspend hook should try to > stop/start the CSI2RX device. > > Runtime PM hooks are usually only called when there are no users, so no > active streams. > OK, I will modify this in my series. Regards, Changhuang