RE: [PATCH v2 RESEND] i2c: cadence: Add system suspend and resume PM support

[JiSheng Teoh]

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
> >