| Message ID | 55F6AC87.4040301@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Adding dtor & linux-input as we are now discussing the elan trackpad driver... On Mon, Sep 14, 2015 at 7:16 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 12/09/15 10:50, maoguang meng wrote: >> hi Sudeep: >> >> I test flowlling your blow suggestions,but the system can not be woken. >> >> beacuse,mtk_eint_suspend will mask it.As we know if eint wakeup system >> it must be unmasked before enter suspend flow. >> >> e.x >> >> static int __maybe_unused elan_suspend(struct device *dev) >> { >> struct i2c_client *client = to_i2c_client(dev); >> struct elan_tp_data *data = i2c_get_clientdata(client); >> int ret; >> >> /* >> * We are taking the mutex to make sure sysfs operations are >> * complete before we attempt to bring the device into low[er] >> * power mode. >> */ >> ret = mutex_lock_interruptible(&data->sysfs_mutex); >> if (ret) >> return ret; >> >> disable_irq(client->irq); >> >> if (device_may_wakeup(dev)) { >> ret = elan_sleep(data); >> /* Enable wake from IRQ */ >> data->irq_wake = (enable_irq_wake(client->irq) == 0); > > This is wrong assumption in the driver. enable_irq_wake doesn't > implicitly enable the IRQ. So the disable_irq should be moved to else. > And the resume patch also needs to be fixed accordingly, otherwise you > may get unbalanced irq. But this should not be the reason for fixing the > pinctrl suspend/resume. > > Regards, > Sudeep > > --->8 > > diff --git a/drivers/input/mouse/elan_i2c_core.c > b/drivers/input/mouse/elan_i2c_core.c > index fa945304b9a5..7de26b04f45c 100644 > --- a/drivers/input/mouse/elan_i2c_core.c > +++ b/drivers/input/mouse/elan_i2c_core.c > @@ -1117,13 +1117,13 @@ static int __maybe_unused elan_suspend(struct > device *dev) > if (ret) > return ret; > > - disable_irq(client->irq); > - > if (device_may_wakeup(dev)) { > ret = elan_sleep(data); > /* Enable wake from IRQ */ > data->irq_wake = (enable_irq_wake(client->irq) == 0); > } else { > + disable_irq(client->irq); > + > ret = elan_disable_power(data); > }
On Mon, Sep 14, 2015 at 4:16 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 12/09/15 10:50, maoguang meng wrote: >> hi Sudeep: >> >> I test flowlling your blow suggestions,but the system can not be woken. >> >> beacuse,mtk_eint_suspend will mask it.As we know if eint wakeup system >> it must be unmasked before enter suspend flow. >> >> e.x >> >> static int __maybe_unused elan_suspend(struct device *dev) >> { >> struct i2c_client *client = to_i2c_client(dev); >> struct elan_tp_data *data = i2c_get_clientdata(client); >> int ret; >> >> /* >> * We are taking the mutex to make sure sysfs operations are >> * complete before we attempt to bring the device into low[er] >> * power mode. >> */ >> ret = mutex_lock_interruptible(&data->sysfs_mutex); >> if (ret) >> return ret; >> >> disable_irq(client->irq); >> >> if (device_may_wakeup(dev)) { >> ret = elan_sleep(data); >> /* Enable wake from IRQ */ >> data->irq_wake = (enable_irq_wake(client->irq) == 0); > > This is wrong assumption in the driver. enable_irq_wake doesn't > implicitly enable the IRQ. So the disable_irq should be moved to else. > And the resume patch also needs to be fixed accordingly, otherwise you > may get unbalanced irq. But this should not be the reason for fixing the > pinctrl suspend/resume. > Elan driver does not want to enable servicing IRQs, it just wants to configure them as wakeup sources. Hence the current elan_suspend() is fine. When system wakes up and the device is resumed and the driver is ready to service interrupts it will enable IRQ again. IOW enable_irq_wake() and enable_irq() are 2 completely different calls and it is perfectly fine to disable IRQ and then ebale it as a wakeup source. Thanks.
On 15/09/15 03:52, Dmitry Torokhov wrote: > On Mon, Sep 14, 2015 at 4:16 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: [...] >> >> This is wrong assumption in the driver. enable_irq_wake doesn't >> implicitly enable the IRQ. So the disable_irq should be moved to else. >> And the resume patch also needs to be fixed accordingly, otherwise you >> may get unbalanced irq. But this should not be the reason for fixing the >> pinctrl suspend/resume. >> > > Elan driver does not want to enable servicing IRQs, it just wants to > configure them as wakeup sources. Hence the current elan_suspend() is > fine. When system wakes up and the device is resumed and the driver is > ready to service interrupts it will enable IRQ again. > Fair enough. But I am struggling to understand how this fits into existing IRQ infrastructure. Few controllers that don't have wakeup source configuration facility can set IRQCHIP_SKIP_SET_WAKE and just leave the interrupts enabled in suspend path to wake it up. So IMO, the above strategy might not work on such controllers. > IOW enable_irq_wake() and enable_irq() are 2 completely different > calls and it is perfectly fine to disable IRQ and then ebale it as a > wakeup source. I agree that they are entirely different APIs, I am not sure if we can support different interrupt controller with such strategy. Since the irq/pm core handle disabling device IRQs and section "System Wakeup Interrupts, enable_irq_wake() and disable_irq_wake()" in Documentation/power/suspend-and-interrupts.txt gives me different understanding, we can check with tglx on how to handle this. Regards, Sudeep
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c index fa945304b9a5..7de26b04f45c 100644 --- a/drivers/input/mouse/elan_i2c_core.c +++ b/drivers/input/mouse/elan_i2c_core.c @@ -1117,13 +1117,13 @@ static int __maybe_unused elan_suspend(struct device *dev) if (ret) return ret; - disable_irq(client->irq); - if (device_may_wakeup(dev)) { ret = elan_sleep(data); /* Enable wake from IRQ */ data->irq_wake = (enable_irq_wake(client->irq) == 0); } else { + disable_irq(client->irq); + ret = elan_disable_power(data); }