Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Mon, Jan 18, 2016 at 6:47 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
>> When probing platform drivers let's check if corresponding devices have
>> "wakeup-source" property defined (either in device tree, ACPI, or static
>> platform properties) and automatically enable such devices as wakeup
>> sources for the system. This will help us standardize on the name for this
>> property and reduce amount of boilerplate code in the drivers.
>
> ACPI has other ways of telling the OS that the device is wakeup-capable,
> but I guess the property in question can be used too (as long as it is
> consistent with the other methods).

I was thinking that down the road ACPI can wire its internal wakeup
knowledge into generic device property. Unfortunately at the moment
most drivers go like this:

- if it is device tree platform: good, we'll take the data from device
tree property and set up driver behavior accordingly;
- if it is legacy board setup: OK, take for driver-specific platform data;
- ACPI... hmm... dunno... enable wakeup and if it is not correct the
system will just not wake up, not the best but not terribly wrong
either.

>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/base/platform.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index 1dd6d3b..d14071a 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -514,9 +514,14 @@ static int platform_drv_probe(struct device *_dev)
>>
>>       ret = dev_pm_domain_attach(_dev, true);
>>       if (ret != -EPROBE_DEFER && drv->probe) {
>> +             bool wakeup = device_property_read_bool(_dev, "wakeup-source");
>> +
>> +             device_init_wakeup(_dev, wakeup);
>
> But I'm wondering if this should be device_set_wakeup_capable(dev, true) rather?
>
> device_init_wakeup() additionally sets the default in sysfs to "do wakeup"
> which in principle may unblock spurious wakeups on some systems.

Why would we not want enable wakeup for devices where system
(platform) tells us that they are wakeup sources? This is how most of
the drivers I am involved in behave. All of them use
device_init_wakeup() and then we adjust the behavior depending on
runtime state, i.e. do not wake up from touchpad when lid is closed
(which is useful if lid is flexible and may interact with touchpad if
slammed down hard enough).

>
>>               ret = drv->probe(dev);
>> -             if (ret)
>> +             if (ret) {
>> +                     device_init_wakeup(_dev, false);
>>                       dev_pm_domain_detach(_dev, true);
>> +             }
>>       }
>>
>>       if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
>> @@ -540,6 +545,8 @@ static int platform_drv_remove(struct device *_dev)
>>
>>       if (drv->remove)
>>               ret = drv->remove(dev);
>> +
>> +     device_init_wakeup(_dev, false);
>>       dev_pm_domain_detach(_dev, true);
>>
>>       return ret;
>>
>

Thanks.

-- 
Dmitry