Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Sat, 9 Dec 2023 21:57:44 +0100
From:   Andi Shyti <andi.shyti@kernel.org>
To:     Ji Sheng Teoh <jisheng.teoh@starfivetech.com>
Cc:     Michal Simek <michal.simek@amd.com>,
        Ley Foon Tan <leyfoon.tan@starfivetech.com>,
        linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] i2c: cadence: Add system suspend and resume PM support
Message-ID: <20231209205744.ehmthjvn7nuslvhd@zenone.zhora.eu>
References: <20231209131516.1916550-1-jisheng.teoh@starfivetech.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231209131516.1916550-1-jisheng.teoh@starfivetech.com>
Precedence: bulk

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. :-)

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

> +		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;
> +}
> +
>  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
>