回复: 回复: [PATCH v2 01/13] media: cadence: csi2rx: Support runtime PM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux