Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1372548 > unrolled thread

[PATCH] extcon: usb-gpio: Don't miss event during suspend/resume

Back to article view | Back to linux.kernel

Contents

  [PATCH] extcon: usb-gpio: Don't miss event during suspend/resume Roger Quadros <rogerq@ti.com> - 2016-04-06 16:10 +0200
    Re: [PATCH] extcon: usb-gpio: Don't miss event during suspend/resume Chanwoo Choi <cw00.choi@samsung.com> - 2016-04-08 01:40 +0200
    [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume Roger Quadros <rogerq@ti.com> - 2016-04-08 09:40 +0200
      Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume Chanwoo Choi <cw00.choi@samsung.com> - 2016-04-11 02:40 +0200
        Re: [PATCH v2] extcon: usb-gpio: Don't miss event during  suspend/resume Grygorii Strashko <grygorii.strashko@ti.com> - 2016-04-11 10:40 +0200
          Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume Chanwoo Choi <cw00.choi@samsung.com> - 2016-04-11 13:20 +0200
            Re: [PATCH v2] extcon: usb-gpio: Don't miss event during  suspend/resume Roger Quadros <rogerq@ti.com> - 2016-04-11 13:40 +0200
              Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume Chanwoo Choi <cwchoi00@gmail.com> - 2016-04-11 15:20 +0200
                Re: [PATCH v2] extcon: usb-gpio: Don't miss event during  suspend/resume Roger Quadros <rogerq@ti.com> - 2016-04-11 16:10 +0200
      [PATCH v3] extcon: usb-gpio: Don't miss event during suspend/resume Roger Quadros <rogerq@ti.com> - 2016-04-11 16:10 +0200
        Re: [PATCH v3] extcon: usb-gpio: Don't miss event during suspend/resume Chanwoo Choi <cw00.choi@samsung.com> - 2016-04-12 00:50 +0200

#1372548 — [PATCH] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [next] | [standalone]

#1373832

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

[toc] | [prev] | [next] | [standalone]

#1374043 — [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [prev] | [next] | [standalone]

#1375396 — Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [prev] | [next] | [standalone]

#1375611 — Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [prev] | [next] | [standalone]

#1375720 — Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [prev] | [next] | [standalone]

#1375736 — Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [prev] | [next] | [standalone]

#1375829 — Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [prev] | [next] | [standalone]

#1375975 — Re: [PATCH v2] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [prev] | [next] | [standalone]

#1375997 — [PATCH v3] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [prev] | [next] | [standalone]

#1376320 — Re: [PATCH v3] extcon: usb-gpio: Don't miss event during suspend/resume

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web