* [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).