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

[JiSheng Teoh]

Hi Andi,

> On Sun, 10 Dec 2023 12:54:12 +0100
> Andi Shyti <andi.shyti@xxxxxxxxxx> wrote:
> 
> > Hi Ji Sheng,
> >
> > [...]
> >
> > > > > +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/44c99904cf61f945d02ac9976ab
> > > 10dd5ccaea393
> > >
> >
> > OK, this is done purely for clock balancing, but then, I still don't
> > understand the case. I expect the clock counter to be unbalanced when
> > you suspend (because is moving towards '0').
> >
> > While, if you check if the clock is unbalanced when resuming, it means
> > that the clock had a negative counter (which is impossible because the
> > clock counter is unsigned).
> >
> > If there is any unbalancing at this stage, then I recommend you to
> > check what has happened previously.
> >
> > ... Or is there anything I am missing?
> >
> > Thanks,
> > Andi
> 
> You are right, the clock counter will move towards 0 during system suspend.
> Conversely during system resume, the clock counter is incremented to 1 early on in cdns_i2c_runtime_resume(). So the clock counter
> is not negative to start with.
> At this point of time, clock counter is 1. If the device is in runtime suspend, we decrement the clock counter back to 0, so the
> subsequent runtime resume could increment it back to 1. In a sense, balancing the clock counter.
> Please help correct me if I've got it wrong.
> 
Just to check on the status of this patch. Let me know if the above statement makes sense.
> >
> > > > > +		err = cdns_i2c_runtime_suspend(dev);
> > > >
> > > > ... and suspend again? Shouldn't this be _resume()?
> > > >
> > > > Thanks,
> > > > Andi
> > > >
> > > > > +		if (err)
> > > > > +			return err;
> > > > > +	}
> > > > > +
> > > > > +	i2c_mark_adapter_resumed(&xi2c->adap);
> > > > > +
> > > > > +	return 0;
> > > > > +}
>