Re: [RFC] i2c: core: Do not enable wakeup by default

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Feb 07, 2023 at 09:25:40AM +0200, Mika Westerberg wrote:
> After commit b38f2d5d9615 ("i2c: acpi: Use ACPI wake capability bit to
> set wake_irq") the I2C core has been setting I2C_CLIENT_WAKE for ACPI
> devices if they announce to be wake capable in their device description.
> However, on certain systems where audio codec has been connected through
> I2C this causes system suspend to wake up immediately because power to
> the codec is turned off which pulls the interrupt line "low" triggering
> wake up.
> 
> Possible reason why the interrupt is marked as wake capable is that some
> codecs apparently support "Wake on Voice" or similar functionality.
> 
> In any case, I don't think we should be enabling wakeup by default on
> all I2C devices that are wake capable. According to device_init_wakeup()
> documentation most devices should leave it disabled, with exceptions on
> devices such as keyboards, power buttons etc. Userspace can enable
> wakeup as needed by writing to device "power/wakeup" attribute.

I agree on the reasoning.

Should we have a Fixes tag?

Otherwise
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

In any case it would be nice if the initial contributors may have a chance
to test this and see how their setup is supposed to work.

> Reported-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Hi,
> 
> Sending this as RFC because I'm not too familiar with the usage of
> I2C_CLIENT_WAKE and whether this is something that is expected behaviour
> in users of I2C devices. On ACPI side I think this is the correct thing
> to do at least.
> 
>  drivers/i2c/i2c-core-base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 087e480b624c..7046549bdae7 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -527,7 +527,7 @@ static int i2c_device_probe(struct device *dev)
>  			goto put_sync_adapter;
>  		}
>  
> -		device_init_wakeup(&client->dev, true);
> +		device_init_wakeup(&client->dev, false);
>  
>  		if (wakeirq > 0 && wakeirq != client->irq)
>  			status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
> -- 
> 2.39.1
> 

-- 
With Best Regards,
Andy Shevchenko