* [PATCH] extcon: usb-gpio: Don't miss event during suspend/resume
@ 2016-04-06 14:01 Roger Quadros
2016-04-07 23:39 ` Chanwoo Choi
2016-04-08 7:34 ` [PATCH v2] " Roger Quadros
0 siblings, 2 replies; 11+ messages in thread
From: Roger Quadros @ 2016-04-06 14:01 UTC (permalink / raw)
To: myungjoo.ham, cw00.choi; +Cc: linux-kernel, Roger Quadros
Pin state might have changed during suspend/resume while
our interrupts were disabled. Scan for change during resume.
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
drivers/extcon/extcon-usb-gpio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index 2b2fecf..20175ec 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -192,6 +192,7 @@ static int usb_extcon_resume(struct device *dev)
}
enable_irq(info->id_irq);
+ usb_extcon_detect_cable(&info->wq_detcable.work);
return ret;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-06 14:01 [PATCH] extcon: usb-gpio: Don't miss event during suspend/resume Roger Quadros
@ 2016-04-07 23:39 ` Chanwoo Choi
2016-04-08 7:34 ` [PATCH v2] " Roger Quadros
1 sibling, 0 replies; 11+ messages in thread
From: Chanwoo Choi @ 2016-04-07 23:39 UTC (permalink / raw)
To: Roger Quadros, myungjoo.ham; +Cc: linux-kernel
On 2016년 04월 06일 23:01, Roger Quadros wrote:
> Pin state might have changed during suspend/resume while
> our interrupts were disabled. Scan for change during resume.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> drivers/extcon/extcon-usb-gpio.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index 2b2fecf..20175ec 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -192,6 +192,7 @@ static int usb_extcon_resume(struct device *dev)
> }
>
> enable_irq(info->id_irq);
> + usb_extcon_detect_cable(&info->wq_detcable.work);
If interrupt is using as wakeup source/irq,
after wake-up from suspend state, the interrupt handler will be handled.
But, if interrupt is not used for wakeup source/irq,
As your patch, we need to check the state on resume().
So, I think you need more condition to check the interrupt is whether wakeup source or not.
Thanks,
Chanwoo CHoi
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-06 14:01 [PATCH] extcon: usb-gpio: Don't miss event during suspend/resume Roger Quadros
2016-04-07 23:39 ` Chanwoo Choi
@ 2016-04-08 7:34 ` Roger Quadros
2016-04-11 0:31 ` Chanwoo Choi
2016-04-11 14:04 ` [PATCH v3] " Roger Quadros
1 sibling, 2 replies; 11+ messages in thread
From: Roger Quadros @ 2016-04-08 7:34 UTC (permalink / raw)
To: myungjoo.ham, cw00.choi; +Cc: linux-kernel, Grygorii Strashko, rogerq
Pin state might have changed during suspend/resume while
our interrupts were disabled and if device doesn't support wakeup.
Scan for change during resume for such case.
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v2:
- only check for state change during resume if device wakeup is not supported
drivers/extcon/extcon-usb-gpio.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index bc61d11..118f8ab 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -185,6 +185,8 @@ static int usb_extcon_resume(struct device *dev)
int ret = 0;
enable_irq(info->id_irq);
+ if (!device_may_wakeup(dev))
+ usb_extcon_detect_cable(&info->wq_detcable.work);
return ret;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-08 7:34 ` [PATCH v2] " Roger Quadros
@ 2016-04-11 0:31 ` Chanwoo Choi
2016-04-11 8:37 ` Grygorii Strashko
2016-04-11 14:04 ` [PATCH v3] " Roger Quadros
1 sibling, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2016-04-11 0:31 UTC (permalink / raw)
To: Roger Quadros, myungjoo.ham; +Cc: linux-kernel, Grygorii Strashko
Hi Roger,
On 2016년 04월 08일 16:34, Roger Quadros wrote:
> Pin state might have changed during suspend/resume while
> our interrupts were disabled and if device doesn't support wakeup.
>
> Scan for change during resume for such case.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> v2:
> - only check for state change during resume if device wakeup is not supported
>
> drivers/extcon/extcon-usb-gpio.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index bc61d11..118f8ab 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -185,6 +185,8 @@ static int usb_extcon_resume(struct device *dev)
> int ret = 0;
>
> enable_irq(info->id_irq);
> + if (!device_may_wakeup(dev))
> + usb_extcon_detect_cable(&info->wq_detcable.work);
The device_may_wakeup() check the following two states:
- dev->power.can_wakeup - device_init_wakeup() function set the this field.
- dev->power.wakeup - device_wakeup_enable() function set the this field.
The probe function of extcon-usb-gpio.c always call the 'device_init_wakeup(dev,true).
But, anywhere in extcon-usb-gpio.c don't handle the device_wakeup_enable() for dev->power.wakeup.
In the extcon-usb-gpio.c, device_may_wakeup(dev) return always 'false'.
If you use the only device_may_wakeup(),
device_may_wakeup() is not able to check whether interrupt is wakeup source or not.
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-11 0:31 ` Chanwoo Choi
@ 2016-04-11 8:37 ` Grygorii Strashko
2016-04-11 11:12 ` Chanwoo Choi
0 siblings, 1 reply; 11+ messages in thread
From: Grygorii Strashko @ 2016-04-11 8:37 UTC (permalink / raw)
To: Chanwoo Choi, Roger Quadros, myungjoo.ham; +Cc: linux-kernel
On 04/11/2016 03:31 AM, Chanwoo Choi wrote:
> Hi Roger,
>
> On 2016년 04월 08일 16:34, Roger Quadros wrote:
>> Pin state might have changed during suspend/resume while
>> our interrupts were disabled and if device doesn't support wakeup.
>>
>> Scan for change during resume for such case.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> v2:
>> - only check for state change during resume if device wakeup is not supported
>>
>> drivers/extcon/extcon-usb-gpio.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>> index bc61d11..118f8ab 100644
>> --- a/drivers/extcon/extcon-usb-gpio.c
>> +++ b/drivers/extcon/extcon-usb-gpio.c
>> @@ -185,6 +185,8 @@ static int usb_extcon_resume(struct device *dev)
>> int ret = 0;
>>
>> enable_irq(info->id_irq);
>> + if (!device_may_wakeup(dev))
>> + usb_extcon_detect_cable(&info->wq_detcable.work);
>
> The device_may_wakeup() check the following two states:
> - dev->power.can_wakeup - device_init_wakeup() function set the this field.
> - dev->power.wakeup - device_wakeup_enable() function set the this field.
>
> The probe function of extcon-usb-gpio.c always call the 'device_init_wakeup(dev,true).
> But, anywhere in extcon-usb-gpio.c don't handle the device_wakeup_enable() for dev->power.wakeup.
device_init_wakeup()
|-> device_wakeup_enable()
>
> In the extcon-usb-gpio.c, device_may_wakeup(dev) return always 'false'.
> If you use the only device_may_wakeup(),
> device_may_wakeup() is not able to check whether interrupt is wakeup source or not.
>
This check is correct and it also will take into account wake up settings changes
which can be made through sysfs: /sys/.../devX/power/wakeup
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-11 8:37 ` Grygorii Strashko
@ 2016-04-11 11:12 ` Chanwoo Choi
2016-04-11 11:39 ` Roger Quadros
0 siblings, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2016-04-11 11:12 UTC (permalink / raw)
To: Grygorii Strashko, Roger Quadros, myungjoo.ham; +Cc: linux-kernel
On 2016년 04월 11일 17:37, Grygorii Strashko wrote:
> On 04/11/2016 03:31 AM, Chanwoo Choi wrote:
>> Hi Roger,
>>
>> On 2016년 04월 08일 16:34, Roger Quadros wrote:
>>> Pin state might have changed during suspend/resume while
>>> our interrupts were disabled and if device doesn't support wakeup.
>>>
>>> Scan for change during resume for such case.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>> v2:
>>> - only check for state change during resume if device wakeup is not supported
>>>
>>> drivers/extcon/extcon-usb-gpio.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>>> index bc61d11..118f8ab 100644
>>> --- a/drivers/extcon/extcon-usb-gpio.c
>>> +++ b/drivers/extcon/extcon-usb-gpio.c
>>> @@ -185,6 +185,8 @@ static int usb_extcon_resume(struct device *dev)
>>> int ret = 0;
>>>
>>> enable_irq(info->id_irq);
>>> + if (!device_may_wakeup(dev))
>>> + usb_extcon_detect_cable(&info->wq_detcable.work);
>>
>> The device_may_wakeup() check the following two states:
>> - dev->power.can_wakeup - device_init_wakeup() function set the this field.
>> - dev->power.wakeup - device_wakeup_enable() function set the this field.
>>
>> The probe function of extcon-usb-gpio.c always call the 'device_init_wakeup(dev,true).
>> But, anywhere in extcon-usb-gpio.c don't handle the device_wakeup_enable() for dev->power.wakeup.
>
>
> device_init_wakeup()
> |-> device_wakeup_enable()
>
>>
>> In the extcon-usb-gpio.c, device_may_wakeup(dev) return always 'false'.
>> If you use the only device_may_wakeup(),
>> device_may_wakeup() is not able to check whether interrupt is wakeup source or not.
>>
>
> This check is correct and it also will take into account wake up settings changes
> which can be made through sysfs: /sys/.../devX/power/wakeup
>
To Grygorii,
You're right. I was mistaken. Again, I analyzed the sequence about wakeup.
Thanks for your reply.
1. Register device as wakeup_source.
device_init_wakeup(dev, true) on probe()
device_wakeup_enable(dev)
device_source_register(const char *name)
struct wakeup_source *ws;
ws = wakeup_source_create(name)
if (ws)
wakeup_source_add(ws);
...
list_add_rcu(&ws->entry, &wakeup_sources);
...
return ws;
2. Register the interrupt as wake_irq
dev_pm_set_wake_irq(struct device *dev, int irq) on probe()
struct wake_irq *wirq;
wirq->dev = dev;
wirq->irq = irq;
dev_pm_attach_wake_irq(dev, irq, wirq);
device_wakeup_attach_irq(*dev, *wakeirq)
struct wakeup_source *ws;
ws = dev->power.wakeup;
ws->wakeirq = wakeirq;
3. Enable irq wake if device is already registed to wakeup_sources.
dpm_suspend_noirq()
device_wakeup_arm_wake_irqs()
list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
if (ws->wakeirq)
dev_pm_arm_wake_irq(sw->wakeirq);
if (device_may_wakeup(wirq->dev))
enable_irq_wake(wirq->irq);
To Roger,
How about using the queue_delayed_work() instead of direct call function?
Because the spent time of wakeup from suspend state should be fast.
So, I think that you better to use the queue_delayed_work().
diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index 118f8ab3be73..f6cbdfe31519 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -186,7 +186,9 @@ static int usb_extcon_resume(struct device *dev)
enable_irq(info->id_irq);
if (!device_may_wakeup(dev))
- usb_extcon_detect_cable(&info->wq_detcable.work);
+ queue_delayed_work(system_power_efficient_wq,
+ &info->wq_detcable,
+ info->debounce_jiffies);
return ret;
}
Thanks,
Chanwoo Choi
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-11 11:12 ` Chanwoo Choi
@ 2016-04-11 11:39 ` Roger Quadros
2016-04-11 13:17 ` Chanwoo Choi
0 siblings, 1 reply; 11+ messages in thread
From: Roger Quadros @ 2016-04-11 11:39 UTC (permalink / raw)
To: Chanwoo Choi, Grygorii Strashko, myungjoo.ham; +Cc: linux-kernel
Chanwoo,
On 11/04/16 14:12, Chanwoo Choi wrote:
> On 2016년 04월 11일 17:37, Grygorii Strashko wrote:
>> On 04/11/2016 03:31 AM, Chanwoo Choi wrote:
>>> Hi Roger,
>>>
>>> On 2016년 04월 08일 16:34, Roger Quadros wrote:
>>>> Pin state might have changed during suspend/resume while
>>>> our interrupts were disabled and if device doesn't support wakeup.
>>>>
>>>> Scan for change during resume for such case.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>> v2:
>>>> - only check for state change during resume if device wakeup is not supported
>>>>
>>>> drivers/extcon/extcon-usb-gpio.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>>>> index bc61d11..118f8ab 100644
>>>> --- a/drivers/extcon/extcon-usb-gpio.c
>>>> +++ b/drivers/extcon/extcon-usb-gpio.c
>>>> @@ -185,6 +185,8 @@ static int usb_extcon_resume(struct device *dev)
>>>> int ret = 0;
>>>>
>>>> enable_irq(info->id_irq);
>>>> + if (!device_may_wakeup(dev))
>>>> + usb_extcon_detect_cable(&info->wq_detcable.work);
>>>
>>> The device_may_wakeup() check the following two states:
>>> - dev->power.can_wakeup - device_init_wakeup() function set the this field.
>>> - dev->power.wakeup - device_wakeup_enable() function set the this field.
>>>
>>> The probe function of extcon-usb-gpio.c always call the 'device_init_wakeup(dev,true).
>>> But, anywhere in extcon-usb-gpio.c don't handle the device_wakeup_enable() for dev->power.wakeup.
>>
>>
>> device_init_wakeup()
>> |-> device_wakeup_enable()
>>
>>>
>>> In the extcon-usb-gpio.c, device_may_wakeup(dev) return always 'false'.
>>> If you use the only device_may_wakeup(),
>>> device_may_wakeup() is not able to check whether interrupt is wakeup source or not.
>>>
>>
>> This check is correct and it also will take into account wake up settings changes
>> which can be made through sysfs: /sys/.../devX/power/wakeup
>>
>
> To Grygorii,
>
> You're right. I was mistaken. Again, I analyzed the sequence about wakeup.
> Thanks for your reply.
>
> 1. Register device as wakeup_source.
> device_init_wakeup(dev, true) on probe()
> device_wakeup_enable(dev)
> device_source_register(const char *name)
> struct wakeup_source *ws;
> ws = wakeup_source_create(name)
> if (ws)
> wakeup_source_add(ws);
> ...
> list_add_rcu(&ws->entry, &wakeup_sources);
> ...
> return ws;
>
>
> 2. Register the interrupt as wake_irq
> dev_pm_set_wake_irq(struct device *dev, int irq) on probe()
> struct wake_irq *wirq;
> wirq->dev = dev;
> wirq->irq = irq;
> dev_pm_attach_wake_irq(dev, irq, wirq);
> device_wakeup_attach_irq(*dev, *wakeirq)
> struct wakeup_source *ws;
> ws = dev->power.wakeup;
> ws->wakeirq = wakeirq;
>
>
> 3. Enable irq wake if device is already registed to wakeup_sources.
> dpm_suspend_noirq()
> device_wakeup_arm_wake_irqs()
> list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
> if (ws->wakeirq)
> dev_pm_arm_wake_irq(sw->wakeirq);
> if (device_may_wakeup(wirq->dev))
> enable_irq_wake(wirq->irq);
>
>
> To Roger,
>
> How about using the queue_delayed_work() instead of direct call function?
> Because the spent time of wakeup from suspend state should be fast.
> So, I think that you better to use the queue_delayed_work().
>
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index 118f8ab3be73..f6cbdfe31519 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -186,7 +186,9 @@ static int usb_extcon_resume(struct device *dev)
>
> enable_irq(info->id_irq);
> if (!device_may_wakeup(dev))
> - usb_extcon_detect_cable(&info->wq_detcable.work);
> + queue_delayed_work(system_power_efficient_wq,
> + &info->wq_detcable,
> + info->debounce_jiffies);
Why not to just use queue_work() instead of queue_delayed_work()
as don't need to debounce the input?
cheers,
-roger
>
> return ret;
> }
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-11 11:39 ` Roger Quadros
@ 2016-04-11 13:17 ` Chanwoo Choi
2016-04-11 14:02 ` Roger Quadros
0 siblings, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2016-04-11 13:17 UTC (permalink / raw)
To: Roger Quadros; +Cc: Grygorii Strashko, myungjoo.ham, linux-kernel
Hi Roger,
On Mon, Apr 11, 2016 at 8:39 PM, Roger Quadros <rogerq@ti.com> wrote:
> Chanwoo,
>
> On 11/04/16 14:12, Chanwoo Choi wrote:
>> On 2016년 04월 11일 17:37, Grygorii Strashko wrote:
>>> On 04/11/2016 03:31 AM, Chanwoo Choi wrote:
>>>> Hi Roger,
>>>>
>>>> On 2016년 04월 08일 16:34, Roger Quadros wrote:
>>>>> Pin state might have changed during suspend/resume while
>>>>> our interrupts were disabled and if device doesn't support wakeup.
>>>>>
>>>>> Scan for change during resume for such case.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> ---
>>>>> v2:
>>>>> - only check for state change during resume if device wakeup is not supported
>>>>>
>>>>> drivers/extcon/extcon-usb-gpio.c | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>>>>> index bc61d11..118f8ab 100644
>>>>> --- a/drivers/extcon/extcon-usb-gpio.c
>>>>> +++ b/drivers/extcon/extcon-usb-gpio.c
>>>>> @@ -185,6 +185,8 @@ static int usb_extcon_resume(struct device *dev)
>>>>> int ret = 0;
>>>>>
>>>>> enable_irq(info->id_irq);
>>>>> + if (!device_may_wakeup(dev))
>>>>> + usb_extcon_detect_cable(&info->wq_detcable.work);
>>>>
>>>> The device_may_wakeup() check the following two states:
>>>> - dev->power.can_wakeup - device_init_wakeup() function set the this field.
>>>> - dev->power.wakeup - device_wakeup_enable() function set the this field.
>>>>
>>>> The probe function of extcon-usb-gpio.c always call the 'device_init_wakeup(dev,true).
>>>> But, anywhere in extcon-usb-gpio.c don't handle the device_wakeup_enable() for dev->power.wakeup.
>>>
>>>
>>> device_init_wakeup()
>>> |-> device_wakeup_enable()
>>>
>>>>
>>>> In the extcon-usb-gpio.c, device_may_wakeup(dev) return always 'false'.
>>>> If you use the only device_may_wakeup(),
>>>> device_may_wakeup() is not able to check whether interrupt is wakeup source or not.
>>>>
>>>
>>> This check is correct and it also will take into account wake up settings changes
>>> which can be made through sysfs: /sys/.../devX/power/wakeup
>>>
>>
>> To Grygorii,
>>
>> You're right. I was mistaken. Again, I analyzed the sequence about wakeup.
>> Thanks for your reply.
>>
>> 1. Register device as wakeup_source.
>> device_init_wakeup(dev, true) on probe()
>> device_wakeup_enable(dev)
>> device_source_register(const char *name)
>> struct wakeup_source *ws;
>> ws = wakeup_source_create(name)
>> if (ws)
>> wakeup_source_add(ws);
>> ...
>> list_add_rcu(&ws->entry, &wakeup_sources);
>> ...
>> return ws;
>>
>>
>> 2. Register the interrupt as wake_irq
>> dev_pm_set_wake_irq(struct device *dev, int irq) on probe()
>> struct wake_irq *wirq;
>> wirq->dev = dev;
>> wirq->irq = irq;
>> dev_pm_attach_wake_irq(dev, irq, wirq);
>> device_wakeup_attach_irq(*dev, *wakeirq)
>> struct wakeup_source *ws;
>> ws = dev->power.wakeup;
>> ws->wakeirq = wakeirq;
>>
>>
>> 3. Enable irq wake if device is already registed to wakeup_sources.
>> dpm_suspend_noirq()
>> device_wakeup_arm_wake_irqs()
>> list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
>> if (ws->wakeirq)
>> dev_pm_arm_wake_irq(sw->wakeirq);
>> if (device_may_wakeup(wirq->dev))
>> enable_irq_wake(wirq->irq);
>>
>>
>> To Roger,
>>
>> How about using the queue_delayed_work() instead of direct call function?
>> Because the spent time of wakeup from suspend state should be fast.
>> So, I think that you better to use the queue_delayed_work().
>>
>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>> index 118f8ab3be73..f6cbdfe31519 100644
>> --- a/drivers/extcon/extcon-usb-gpio.c
>> +++ b/drivers/extcon/extcon-usb-gpio.c
>> @@ -186,7 +186,9 @@ static int usb_extcon_resume(struct device *dev)
>>
>> enable_irq(info->id_irq);
>> if (!device_may_wakeup(dev))
>> - usb_extcon_detect_cable(&info->wq_detcable.work);
>> + queue_delayed_work(system_power_efficient_wq,
>> + &info->wq_detcable,
>> + info->debounce_jiffies);
>
> Why not to just use queue_work() instead of queue_delayed_work()
> as don't need to debounce the input?
The use of queue_work() is good.
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-11 13:17 ` Chanwoo Choi
@ 2016-04-11 14:02 ` Roger Quadros
0 siblings, 0 replies; 11+ messages in thread
From: Roger Quadros @ 2016-04-11 14:02 UTC (permalink / raw)
To: cw00.choi; +Cc: Grygorii Strashko, myungjoo.ham, linux-kernel
On 11/04/16 16:17, Chanwoo Choi wrote:
> Hi Roger,
>
> On Mon, Apr 11, 2016 at 8:39 PM, Roger Quadros <rogerq@ti.com> wrote:
>> Chanwoo,
>>
>> On 11/04/16 14:12, Chanwoo Choi wrote:
>>> On 2016년 04월 11일 17:37, Grygorii Strashko wrote:
>>>> On 04/11/2016 03:31 AM, Chanwoo Choi wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On 2016년 04월 08일 16:34, Roger Quadros wrote:
>>>>>> Pin state might have changed during suspend/resume while
>>>>>> our interrupts were disabled and if device doesn't support wakeup.
>>>>>>
>>>>>> Scan for change during resume for such case.
>>>>>>
>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>> ---
>>>>>> v2:
>>>>>> - only check for state change during resume if device wakeup is not supported
>>>>>>
>>>>>> drivers/extcon/extcon-usb-gpio.c | 2 ++
>>>>>> 1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>>>>>> index bc61d11..118f8ab 100644
>>>>>> --- a/drivers/extcon/extcon-usb-gpio.c
>>>>>> +++ b/drivers/extcon/extcon-usb-gpio.c
>>>>>> @@ -185,6 +185,8 @@ static int usb_extcon_resume(struct device *dev)
>>>>>> int ret = 0;
>>>>>>
>>>>>> enable_irq(info->id_irq);
>>>>>> + if (!device_may_wakeup(dev))
>>>>>> + usb_extcon_detect_cable(&info->wq_detcable.work);
>>>>>
>>>>> The device_may_wakeup() check the following two states:
>>>>> - dev->power.can_wakeup - device_init_wakeup() function set the this field.
>>>>> - dev->power.wakeup - device_wakeup_enable() function set the this field.
>>>>>
>>>>> The probe function of extcon-usb-gpio.c always call the 'device_init_wakeup(dev,true).
>>>>> But, anywhere in extcon-usb-gpio.c don't handle the device_wakeup_enable() for dev->power.wakeup.
>>>>
>>>>
>>>> device_init_wakeup()
>>>> |-> device_wakeup_enable()
>>>>
>>>>>
>>>>> In the extcon-usb-gpio.c, device_may_wakeup(dev) return always 'false'.
>>>>> If you use the only device_may_wakeup(),
>>>>> device_may_wakeup() is not able to check whether interrupt is wakeup source or not.
>>>>>
>>>>
>>>> This check is correct and it also will take into account wake up settings changes
>>>> which can be made through sysfs: /sys/.../devX/power/wakeup
>>>>
>>>
>>> To Grygorii,
>>>
>>> You're right. I was mistaken. Again, I analyzed the sequence about wakeup.
>>> Thanks for your reply.
>>>
>>> 1. Register device as wakeup_source.
>>> device_init_wakeup(dev, true) on probe()
>>> device_wakeup_enable(dev)
>>> device_source_register(const char *name)
>>> struct wakeup_source *ws;
>>> ws = wakeup_source_create(name)
>>> if (ws)
>>> wakeup_source_add(ws);
>>> ...
>>> list_add_rcu(&ws->entry, &wakeup_sources);
>>> ...
>>> return ws;
>>>
>>>
>>> 2. Register the interrupt as wake_irq
>>> dev_pm_set_wake_irq(struct device *dev, int irq) on probe()
>>> struct wake_irq *wirq;
>>> wirq->dev = dev;
>>> wirq->irq = irq;
>>> dev_pm_attach_wake_irq(dev, irq, wirq);
>>> device_wakeup_attach_irq(*dev, *wakeirq)
>>> struct wakeup_source *ws;
>>> ws = dev->power.wakeup;
>>> ws->wakeirq = wakeirq;
>>>
>>>
>>> 3. Enable irq wake if device is already registed to wakeup_sources.
>>> dpm_suspend_noirq()
>>> device_wakeup_arm_wake_irqs()
>>> list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
>>> if (ws->wakeirq)
>>> dev_pm_arm_wake_irq(sw->wakeirq);
>>> if (device_may_wakeup(wirq->dev))
>>> enable_irq_wake(wirq->irq);
>>>
>>>
>>> To Roger,
>>>
>>> How about using the queue_delayed_work() instead of direct call function?
>>> Because the spent time of wakeup from suspend state should be fast.
>>> So, I think that you better to use the queue_delayed_work().
>>>
>>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>>> index 118f8ab3be73..f6cbdfe31519 100644
>>> --- a/drivers/extcon/extcon-usb-gpio.c
>>> +++ b/drivers/extcon/extcon-usb-gpio.c
>>> @@ -186,7 +186,9 @@ static int usb_extcon_resume(struct device *dev)
>>>
>>> enable_irq(info->id_irq);
>>> if (!device_may_wakeup(dev))
>>> - usb_extcon_detect_cable(&info->wq_detcable.work);
>>> + queue_delayed_work(system_power_efficient_wq,
>>> + &info->wq_detcable,
>>> + info->debounce_jiffies);
>>
>> Why not to just use queue_work() instead of queue_delayed_work()
>> as don't need to debounce the input?
>
> The use of queue_work() is good.
Unfortunately we can't as info->wq_detcable type is 'struct delayed_work' but
queue_work() expects 'struct work_struct'
I'll just call queue_delayed_work() with 0 delay :)
cheers,
-roger
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-08 7:34 ` [PATCH v2] " Roger Quadros
2016-04-11 0:31 ` Chanwoo Choi
@ 2016-04-11 14:04 ` Roger Quadros
2016-04-11 22:43 ` Chanwoo Choi
1 sibling, 1 reply; 11+ messages in thread
From: Roger Quadros @ 2016-04-11 14:04 UTC (permalink / raw)
To: myungjoo.ham, cw00.choi; +Cc: linux-kernel, Grygorii Strashko, rogerq
Pin state might have changed during suspend/resume while
our interrupts were disabled and if device doesn't support wakeup.
Scan for change during resume for such case.
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v3:
- use queue_delayed_work() instead of directly calling usb_extcon_detect_cable()
v2:
- only check for state change during resume if device wakeup is
drivers/extcon/extcon-usb-gpio.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index bc61d11..bad2159 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -185,6 +185,9 @@ static int usb_extcon_resume(struct device *dev)
int ret = 0;
enable_irq(info->id_irq);
+ if (!device_may_wakeup(dev))
+ queue_delayed_work(system_power_efficient_wq,
+ &info->wq_detcable, 0);
return ret;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] extcon: usb-gpio: Don't miss event during suspend/resume
2016-04-11 14:04 ` [PATCH v3] " Roger Quadros
@ 2016-04-11 22:43 ` Chanwoo Choi
0 siblings, 0 replies; 11+ messages in thread
From: Chanwoo Choi @ 2016-04-11 22:43 UTC (permalink / raw)
To: Roger Quadros, myungjoo.ham; +Cc: linux-kernel, Grygorii Strashko
Hi Roger,
On 2016년 04월 11일 23:04, Roger Quadros wrote:
> Pin state might have changed during suspend/resume while
> our interrupts were disabled and if device doesn't support wakeup.
>
> Scan for change during resume for such case.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> v3:
> - use queue_delayed_work() instead of directly calling usb_extcon_detect_cable()
>
> v2:
> - only check for state change during resume if device wakeup is
>
> drivers/extcon/extcon-usb-gpio.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index bc61d11..bad2159 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -185,6 +185,9 @@ static int usb_extcon_resume(struct device *dev)
> int ret = 0;
>
> enable_irq(info->id_irq);
> + if (!device_may_wakeup(dev))
> + queue_delayed_work(system_power_efficient_wq,
> + &info->wq_detcable, 0);
>
> return ret;
> }
>
Applied it.
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-04-11 22:43 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-06 14:01 [PATCH] extcon: usb-gpio: Don't miss event during suspend/resume Roger Quadros
2016-04-07 23:39 ` Chanwoo Choi
2016-04-08 7:34 ` [PATCH v2] " Roger Quadros
2016-04-11 0:31 ` Chanwoo Choi
2016-04-11 8:37 ` Grygorii Strashko
2016-04-11 11:12 ` Chanwoo Choi
2016-04-11 11:39 ` Roger Quadros
2016-04-11 13:17 ` Chanwoo Choi
2016-04-11 14:02 ` Roger Quadros
2016-04-11 14:04 ` [PATCH v3] " Roger Quadros
2016-04-11 22:43 ` Chanwoo Choi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).