Received-SPF: pass (google.com: domain of linux-kernel+bounces-36025-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 23 Jan 2024 21:32:10 +0100
From: Andi Shyti <andi.shyti@kernel.org>
To: JiSheng Teoh <jisheng.teoh@starfivetech.com>
Cc: Michal Simek <michal.simek@amd.com>, 
	Leyfoon Tan <leyfoon.tan@starfivetech.com>, 
	"linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 RESEND] i2c: cadence: Add system suspend and resume PM
 support
Message-ID: <3w4vaxoderuhwkqec6rwem2wrjlvql2ohyh77zqpwege7ercpl@5ac4p5mw7nhp>
References: <20240119013326.3405484-1-jisheng.teoh@starfivetech.com>
 <ko44i4n5synf3uugp4wmjoe6eikyw3bzjtmarduwvskmk4d3dr@uewx27aa6ake>
 <ZQ0PR01MB1160390D00404D24A31A1522EB75A@ZQ0PR01MB1160.CHNPR01.prod.partner.outlook.cn>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZQ0PR01MB1160390D00404D24A31A1522EB75A@ZQ0PR01MB1160.CHNPR01.prod.partner.outlook.cn>

Hi Ji Sheng,

..

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

If this is your issue, what if we do not enable the clock during
resume? and we just mark the device as resumed?

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

I'd prefer checking the error value, even though we are sure on
the expected return. It's more future proof.

Andi

> > > +	}
> > > +
> > > +	i2c_mark_adapter_resumed(&xi2c->adap);
> > > +
> > > +	return 0;
> > > +}