Hi Andi, > > Hi Ji Sheng, > > I'm not fully conviced here. > > On Fri, Jan 19, 2024 at 09:33:26AM +0800, Ji Sheng Teoh wrote: > > Enable device system suspend and resume PM support, and mark the > > device state as suspended during system suspend to reject any data transfer. > > > > Signed-off-by: Ji Sheng Teoh <jisheng.teoh@xxxxxxxxxxxxxxxx> > > --- > > Initial v2 was archived, previous discussion can be found in [1]. > > [1]: > > https://lore.kernel.org/all/20231209131516.1916550-1-jisheng.teoh@star > > fivetech.com/ > > > > Changes since v1: > > - Add missing err assignment in cdns_i2c_resume(). > > --- > > drivers/i2c/busses/i2c-cadence.c | 33 > > ++++++++++++++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > > > diff --git a/drivers/i2c/busses/i2c-cadence.c > > b/drivers/i2c/busses/i2c-cadence.c > > index de3f58b60dce..4bb7d6756947 100644 > > --- a/drivers/i2c/busses/i2c-cadence.c > > +++ b/drivers/i2c/busses/i2c-cadence.c > > @@ -1176,6 +1176,18 @@ static int __maybe_unused cdns_i2c_runtime_suspend(struct device *dev) > > return 0; > > } > > > > +static int __maybe_unused cdns_i2c_suspend(struct device *dev) { > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > + > > + i2c_mark_adapter_suspended(&xi2c->adap); > > this should go before the return '0' after checking that cdns_i2c_runtime_suspend(), even though we know it always returns '0', we > still can use likely or unlikely. > Ok, that works for me. Will move it right before return '0', and append 'unlikely' to the runtime suspend check. eg: +if (unlikely(!pm_runtime_status_suspended(dev))) + cdns_i2c_runtime_suspend(dev); + +i2c_mark_adapter_suspended(&xi2c->adap); + +return 0; > > + if (!pm_runtime_status_suspended(dev)) > > + return cdns_i2c_runtime_suspend(dev); > > + > > + return 0; > > +} > > + > > /** > > * cdns_i2c_init - Controller initialisation > > * @id: Device private data structure > > @@ -1219,7 +1231,28 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) > > return 0; > > } > > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) { > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > + int err; > > + > > + err = cdns_i2c_runtime_resume(dev); > > + if (err) > > + return err; > > + > > + if (pm_runtime_status_suspended(dev)) { > > + err = cdns_i2c_runtime_suspend(dev); > > + if (err) > > + return err; > > We call the cdns_i2c_resume() functions to come up from a suspended state. But, if we fail to resume, we call the suspend and return > '0' (because this always returns '0'). > > In other words, if we take this path, we call resume, but we still end up suspended and return success. > > Andi > My understanding is that during system level resume 'cdns_i2c_resume()', the i2c device itself can still be held in runtime suspend regardless of the change in system level PM. Looking back at this, we invoke cdns_i2c_runtime_resume() to enable clock and init the i2c device, the runtime PM state is still unchanged and kept suspended. pm_runtime_status_suspended() will be evaluated as true, and runtime suspend 'cdns_i2c_runtime_suspend()' is invoked to disable the clock. This balances the clock count enabled earlier. The runtime PM state is only resumed during cdns_i2c_master_xfer() through pm_runtime_resume_and_get(), and subsequently kept suspended through pm_runtime_put_autosuspend(). Since the cdns_i2c_runtime_suspend() always return '0', I will simplify them as follow: +if (pm_runtime_status_suspended(dev)) + cdns_i2c_runtime_suspend(dev); > > + } > > + > > + i2c_mark_adapter_resumed(&xi2c->adap); > > + > > + return 0; > > +} > > + > > static const struct dev_pm_ops cdns_i2c_dev_pm_ops = { > > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cdns_i2c_suspend, cdns_i2c_resume) > > SET_RUNTIME_PM_OPS(cdns_i2c_runtime_suspend, > > cdns_i2c_runtime_resume, NULL) > > }; > > -- > > 2.43.0 > >
