On Sat, 9 Dec 2023 21:57:44 +0100
Andi Shyti <andi.shyti@xxxxxxxxxx> wrote:
> Hi Ji Sheng,
>
> On Sat, Dec 09, 2023 at 09:15:16PM +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>
> > ---
> > Changes since v1:
> > - Add missing err assignment in cdns_i2c_resume().
>
> thanks for the quick version update. However, while it's nice to
> see such prompt proactivity, we also need to allow more time for
> others to review your change.
>
> Next time, please give it a bit more time before sending out
> version 2. :-)
>
Thanks, will take note of that.
> > ---
> > 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);
> > +
> > + 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)
> > +{
>
> I am not really understanding what you are trying to do here:
>
> > + struct cdns_i2c *xi2c = dev_get_drvdata(dev);
> > + int err;
> > +
> > + err = cdns_i2c_runtime_resume(dev);
>
> First you try to resume...
>
> > + if (err)
> > + return err;
> > +
> > + if (pm_runtime_status_suspended(dev)) {
>
> ... then you check if you are suspended ...
This serves as a check and balance to ensure that when the system
resumes with device in runtime suspend state, we disable the clock
enabled in earlier cdns_i2c_runtime_resume() to ensure a balanced clock
reference count for subsequent runtime resume transition.
Similar implementation can be found in this commit:
https://github.com/torvalds/linux/commit/44c99904cf61f945d02ac9976ab10dd5ccaea393
>
> > + err = cdns_i2c_runtime_suspend(dev);
>
> ... and suspend again? Shouldn't this be _resume()?
>
> Thanks,
[O> Andi
>
> > + if (err)
> > + return err;
> > + }
> > +
> > + 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.25.1
> >
