* Re: [PATCH] i2c: i801: use i2c_mark_adapter_suspended/resumed
2023-09-20 7:29 [PATCH] i2c: i801: use i2c_mark_adapter_suspended/resumed Heiner Kallweit
@ 2023-09-21 13:01 ` Jean Delvare
2023-09-21 21:14 ` Andi Shyti
2023-09-22 9:43 ` Wolfram Sang
2 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2023-09-21 13:01 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Wolfram Sang, Andi Shyti, linux-i2c
Hi Heiner, Wolrfam,
On Wed, 20 Sep 2023 09:29:28 +0200, Heiner Kallweit wrote:
> When entering the suspend callback, at first we should ensure that
> transfers are finished and I2C core can't start further transfers.
> Use i2c_mark_adapter_suspended() for this purpose, and complement it
> with a call to i2c_mark_adapter_resumed() in the resume path.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> Rebased version of a previously discussed patch, now w/o touching
> the remove and shutdown path.
> ---
> drivers/i2c/busses/i2c-i801.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 6d02a8b88..26f132277 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1818,6 +1818,7 @@ static int i801_suspend(struct device *dev)
> {
> struct i801_priv *priv = dev_get_drvdata(dev);
>
> + i2c_mark_adapter_suspended(&priv->adapter);
> i801_restore_regs(priv);
>
> return 0;
> @@ -1829,6 +1830,7 @@ static int i801_resume(struct device *dev)
>
> i801_setup_hstcfg(priv);
> i801_enable_host_notify(&priv->adapter);
> + i2c_mark_adapter_resumed(&priv->adapter);
>
> return 0;
> }
As I said before, I wish this was handled by the driver core instead of
device drivers individually, but until that is implemented, I'm fine
with this patch, only because other drivers are already doing the same,
so it can't be that bad.
Reviewed-by: Jean Delvare <jdelvare@suse.de>
To be honest, I'm not even sure why this is necessary. I thought that
the driver core was already suspending the device tree from the leafs to
the root, and resuming from the root to the leafs, so a child would
never be active when its parent isn't. So I just can't see how an I2C
transfer can be initiated by an I2C device driver (child) to a
suspended I2C adapter (parent). But I'm probably missing something in
the driver model. Power management has become so complex over the
years...
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] i2c: i801: use i2c_mark_adapter_suspended/resumed
2023-09-20 7:29 [PATCH] i2c: i801: use i2c_mark_adapter_suspended/resumed Heiner Kallweit
2023-09-21 13:01 ` Jean Delvare
@ 2023-09-21 21:14 ` Andi Shyti
2023-09-22 9:43 ` Wolfram Sang
2 siblings, 0 replies; 4+ messages in thread
From: Andi Shyti @ 2023-09-21 21:14 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Jean Delvare, linux-i2c@vger.kernel.org
Hi Heiner,
On Wed, Sep 20, 2023 at 09:29:28AM +0200, Heiner Kallweit wrote:
> When entering the suspend callback, at first we should ensure that
> transfers are finished and I2C core can't start further transfers.
> Use i2c_mark_adapter_suspended() for this purpose, and complement it
> with a call to i2c_mark_adapter_resumed() in the resume path.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> Rebased version of a previously discussed patch, now w/o touching
> the remove and shutdown path.
> ---
> drivers/i2c/busses/i2c-i801.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 6d02a8b88..26f132277 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1818,6 +1818,7 @@ static int i801_suspend(struct device *dev)
> {
> struct i801_priv *priv = dev_get_drvdata(dev);
>
> + i2c_mark_adapter_suspended(&priv->adapter);
> i801_restore_regs(priv);
>
> return 0;
> @@ -1829,6 +1830,7 @@ static int i801_resume(struct device *dev)
>
> i801_setup_hstcfg(priv);
> i801_enable_host_notify(&priv->adapter);
> + i2c_mark_adapter_resumed(&priv->adapter);
I think I already reviewed this patch previously and I had same
concerns as Jean, anyway,
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Andi
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] i2c: i801: use i2c_mark_adapter_suspended/resumed
2023-09-20 7:29 [PATCH] i2c: i801: use i2c_mark_adapter_suspended/resumed Heiner Kallweit
2023-09-21 13:01 ` Jean Delvare
2023-09-21 21:14 ` Andi Shyti
@ 2023-09-22 9:43 ` Wolfram Sang
2 siblings, 0 replies; 4+ messages in thread
From: Wolfram Sang @ 2023-09-22 9:43 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Jean Delvare, Andi Shyti, linux-i2c@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 438 bytes --]
On Wed, Sep 20, 2023 at 09:29:28AM +0200, Heiner Kallweit wrote:
> When entering the suspend callback, at first we should ensure that
> transfers are finished and I2C core can't start further transfers.
> Use i2c_mark_adapter_suspended() for this purpose, and complement it
> with a call to i2c_mark_adapter_resumed() in the resume path.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Applied to for-next, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread