Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
From:   Ji Sheng Teoh <jisheng.teoh@starfivetech.com>
To:     <andi.shyti@kernel.org>
CC:     <jisheng.teoh@starfivetech.com>, <leyfoon.tan@starfivetech.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-i2c@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <michal.simek@amd.com>
Subject: Re: [PATCH v2] i2c: cadence: Add system suspend and resume PM support
Date:   Sun, 10 Dec 2023 13:20:17 +0800
Message-ID: <20231210052018.2063-1-jisheng.teoh@starfivetech.com>
In-Reply-To: <20231209205744.ehmthjvn7nuslvhd@zenone.zhora.eu>
References: <20231209205744.ehmthjvn7nuslvhd@zenone.zhora.eu>
MIME-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Sat, 9 Dec 2023 21:57:44 +0100
Andi Shyti <andi.shyti@kernel.org> 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@starfivetech.com>
> > ---
> > 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
> >