Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 15/09/15 03:52, Dmitry Torokhov wrote:

On Mon, Sep 14, 2015 at 4:16 AM, Sudeep Holla  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 15/09/15 03:52, Dmitry Torokhov wrote:

On Mon, Sep 14, 2015 at 4:16 AM, Sudeep Holla  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Dmitry Torokhov]

On Mon, Sep 14, 2015 at 4:16 AM, Sudeep Holla  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(>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.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Daniel Kurtz]

Adding dtor & linux-input as we are now discussing the elan trackpad driver...

On Mon, Sep 14, 2015 at 7:16 PM, Sudeep Holla  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(>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);
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

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(>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);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

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(>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);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Daniel Kurtz]

Adding dtor & linux-input as we are now discussing the elan trackpad driver...

On Mon, Sep 14, 2015 at 7:16 PM, Sudeep Holla  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(>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);
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Dmitry Torokhov]

On Mon, Sep 14, 2015 at 4:16 AM, Sudeep Holla  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(>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.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[maoguang meng]

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(>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);
} else {
ret = elan_disable_power(data);
}

mutex_unlock(>sysfs_mutex);
return ret;
}

thanks

Best Regards,

Maoguang

On Wed, 2015-09-09 at 00:50 +0800, Sudeep Holla wrote:
> 
> On 08/09/15 10:28, Sudeep Holla wrote:
> >
> >
> > On 06/09/15 11:39, maoguang meng wrote:
> >> On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:
> >>> Hi maoguang,
> >>>
> >>> On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  
> >>> wrote:
> 
> 
>  On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:
> >
> > From: Maoguang Meng 
> >
> > This patch implement irq_set_wake to get who is wakeup source and
> > setup on suspend resume.
> >
> > Signed-off-by: Maoguang Meng 
> >
> >>> [snip]
> >>>
> >>> Can you please respond to Sudeep's questions:
> >>>
>  Does this pinmux controller:
> 
>  1. Support wake-up configuration ? If not, you need to use
>   IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
>   mask_{set,clear} if the same registers are used for {en,dis}able
> >>
> >> YES.
> >> we can call enable_irq_wake(irq) to config this irq as a wake-up
> >> source.
> >>
> >
> > No that doesn't answer my question. Yes you can always call
> > enable_irq_wake(irq) as along as you have irq_set_wake implemented.
> > But my question was does this pinmux controller support wake-up
> > configuration.
> >
> > IMHO, by looking at the implementation I can confirm *NO*, it doesn't.
> > So please stop copy-pasting the implementation from other drivers.
> > The reason is you operate on the same mask_{set,clear} which you use
> > to {en,dis}able the interrupts which means you don't have to do anything
> > to configure an interrupt as a wakeup source other than just keeping
> > them enabled. Hopefully this clarifies.
> >
> 
>  2. Is in always on domain ? If not, you need save/restore only to
>   resume back the functionality. Generally we can set
>   IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
>   disabled during suspend and re-enabled in resume path. You just
>   save/restore raw values without tracking the wake-up source.
> >>
> >> YES.
> >>
> >
> > Again *YES* for what part of my question. If it's always-on, then you
> > don't need this suspend/resume callbacks at all, so I assume the answer
> > is *NO*.
> >
> 
> IIUC this pinmux controller, something like below should work.
> 
> 
> >8
> 
>  From 7c26992bce047efb66a6ba8d2ffec2b272499df7 Mon Sep 17 00:00:00 2001
> From: Sudeep Holla 
> Date: Tue, 8 Sep 2015 17:35:38 +0100
> Subject: [PATCH] pinctrl: mediatek: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND
> 
> The mediatek pinmux controller doesn't provides any facility to configure
> the wakeup sources. So instead of providing redundant irq_set_wake
> functionality, SKIP_SET_WAKE could be set to it. Also there's no need to
> maintain 2 sets of masks.
> 
> This patch enables IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND and
> also removes wake_mask.
> 
> Cc: Linus Walleij 
> Cc: Matthias Brugger 
> Cc: Hongzhou Yang 
> Cc: Yingjoe Chen 
> Cc: Maoguang Meng 
> Cc: Chaotian Jing 
> Cc: linux-g...@vger.kernel.org
> Cc: linux-media...@lists.infradead.org
> Signed-off-by: Sudeep Holla 
> ---
>   drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 24 
> +---
>   drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  1 -
>   2 files changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index 7726c6caaf83..d8e194a5bb31 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> @@ -1063,20 +1063,6 @@ static int mtk_eint_set_type(struct irq_data *d,
>   return 0;
>   }
> 
> -static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> -{
> - struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> - int shift = d->hwirq & 0x1f;
> - int reg = d->hwirq >> 5;
> 

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[maoguang meng]

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(>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);
} else {
ret = elan_disable_power(data);
}

mutex_unlock(>sysfs_mutex);
return ret;
}

thanks

Best Regards,

Maoguang

On Wed, 2015-09-09 at 00:50 +0800, Sudeep Holla wrote:
> 
> On 08/09/15 10:28, Sudeep Holla wrote:
> >
> >
> > On 06/09/15 11:39, maoguang meng wrote:
> >> On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:
> >>> Hi maoguang,
> >>>
> >>> On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  
> >>> wrote:
> 
> 
>  On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:
> >
> > From: Maoguang Meng 
> >
> > This patch implement irq_set_wake to get who is wakeup source and
> > setup on suspend resume.
> >
> > Signed-off-by: Maoguang Meng 
> >
> >>> [snip]
> >>>
> >>> Can you please respond to Sudeep's questions:
> >>>
>  Does this pinmux controller:
> 
>  1. Support wake-up configuration ? If not, you need to use
>   IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
>   mask_{set,clear} if the same registers are used for {en,dis}able
> >>
> >> YES.
> >> we can call enable_irq_wake(irq) to config this irq as a wake-up
> >> source.
> >>
> >
> > No that doesn't answer my question. Yes you can always call
> > enable_irq_wake(irq) as along as you have irq_set_wake implemented.
> > But my question was does this pinmux controller support wake-up
> > configuration.
> >
> > IMHO, by looking at the implementation I can confirm *NO*, it doesn't.
> > So please stop copy-pasting the implementation from other drivers.
> > The reason is you operate on the same mask_{set,clear} which you use
> > to {en,dis}able the interrupts which means you don't have to do anything
> > to configure an interrupt as a wakeup source other than just keeping
> > them enabled. Hopefully this clarifies.
> >
> 
>  2. Is in always on domain ? If not, you need save/restore only to
>   resume back the functionality. Generally we can set
>   IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
>   disabled during suspend and re-enabled in resume path. You just
>   save/restore raw values without tracking the wake-up source.
> >>
> >> YES.
> >>
> >
> > Again *YES* for what part of my question. If it's always-on, then you
> > don't need this suspend/resume callbacks at all, so I assume the answer
> > is *NO*.
> >
> 
> IIUC this pinmux controller, something like below should work.
> 
> 
> >8
> 
>  From 7c26992bce047efb66a6ba8d2ffec2b272499df7 Mon Sep 17 00:00:00 2001
> From: Sudeep Holla 
> Date: Tue, 8 Sep 2015 17:35:38 +0100
> Subject: [PATCH] pinctrl: mediatek: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND
> 
> The mediatek pinmux controller doesn't provides any facility to configure
> the wakeup sources. So instead of providing redundant irq_set_wake
> functionality, SKIP_SET_WAKE could be set to it. Also there's no need to
> maintain 2 sets of masks.
> 
> This patch enables IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND and
> also removes wake_mask.
> 
> Cc: Linus Walleij 
> Cc: Matthias Brugger 
> Cc: Hongzhou Yang 
> Cc: Yingjoe Chen 
> Cc: Maoguang Meng 
> Cc: Chaotian Jing 
> Cc: linux-g...@vger.kernel.org
> Cc: linux-media...@lists.infradead.org
> Signed-off-by: Sudeep Holla 
> ---
>   drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 24 
> +---
>   drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  1 -
>   2 files changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index 7726c6caaf83..d8e194a5bb31 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> @@ -1063,20 +1063,6 @@ static 

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 11/09/15 12:22, Chung-Yih Wang (王崇懿) wrote:

Hi Sudeep and Maoguang,

Please correct me if I am wrong. I think the wake_mask Maoguang
implemented is the wake-up configuration and it is how he disabled
other unwanted interrupt sources(e.g. audio jacket insertion) during
suspend.



OK, you are right, I think I now understand the issue. I misread the
code initially thinking the suspend/resume are implemented as
syscore_ops but they are standard device pm ops.


   With Sudeep's patch which we had similar one before, the system got
waken up by audio jack insertion which we don't want. Maoguang tried
to implement wake_mask as the wake-up configuration to keep track of
effective wakeup sources(i.e. those who makes enable_irq_wake) and
write the wake-up configuration in mtk_eint_suspend(). What is your
suggestion to address this issue? Thanks!



One option is to convert them to *_noirq callbacks assuming all the
users of this pinctrl irqchip have sanely implemented their
suspend/resume and don't trigger interrupts between dpm_suspend and
suspend_device_irqs. What do you think ?

Regards,
Sudeep

>8

@@ -1130,8 +1130,8 @@ static int mtk_eint_resume(struct device *device)
 }

 const struct dev_pm_ops mtk_eint_pm_ops = {
-   .suspend = mtk_eint_suspend,
-   .resume = mtk_eint_resume,
+   .suspend_noirq = mtk_eint_suspend,
+   .resume_noirq = mtk_eint_resume,
 };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[王崇懿]

Hi Sudeep and Maoguang,

   Please correct me if I am wrong. I think the wake_mask Maoguang
implemented is the wake-up configuration and it is how he disabled
other unwanted interrupt sources(e.g. audio jacket insertion) during
suspend.

  With Sudeep's patch which we had similar one before, the system got
waken up by audio jack insertion which we don't want. Maoguang tried
to implement wake_mask as the wake-up configuration to keep track of
effective wakeup sources(i.e. those who makes enable_irq_wake) and
write the wake-up configuration in mtk_eint_suspend(). What is your
suggestion to address this issue? Thanks!

On Wed, Sep 9, 2015 at 12:50 AM, Sudeep Holla  wrote:
>
>
> On 08/09/15 10:28, Sudeep Holla wrote:
>>
>>
>>
>> On 06/09/15 11:39, maoguang meng wrote:
>>>
>>> On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:

 Hi maoguang,

 On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla 
 wrote:
>
>
>
> On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:
>>
>>
>> From: Maoguang Meng 
>>
>> This patch implement irq_set_wake to get who is wakeup source and
>> setup on suspend resume.
>>
>> Signed-off-by: Maoguang Meng 
>>
 [snip]

 Can you please respond to Sudeep's questions:

> Does this pinmux controller:
>
> 1. Support wake-up configuration ? If not, you need to use
>  IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
>  mask_{set,clear} if the same registers are used for {en,dis}able
>>>
>>>
>>> YES.
>>> we can call enable_irq_wake(irq) to config this irq as a wake-up
>>> source.
>>>
>>
>> No that doesn't answer my question. Yes you can always call
>> enable_irq_wake(irq) as along as you have irq_set_wake implemented.
>> But my question was does this pinmux controller support wake-up
>> configuration.
>>
>> IMHO, by looking at the implementation I can confirm *NO*, it doesn't.
>> So please stop copy-pasting the implementation from other drivers.
>> The reason is you operate on the same mask_{set,clear} which you use
>> to {en,dis}able the interrupts which means you don't have to do anything
>> to configure an interrupt as a wakeup source other than just keeping
>> them enabled. Hopefully this clarifies.
>>
>
> 2. Is in always on domain ? If not, you need save/restore only to
>  resume back the functionality. Generally we can set
>  IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
>  disabled during suspend and re-enabled in resume path. You just
>  save/restore raw values without tracking the wake-up source.
>>>
>>>
>>> YES.
>>>
>>
>> Again *YES* for what part of my question. If it's always-on, then you
>> don't need this suspend/resume callbacks at all, so I assume the answer
>> is *NO*.
>>
>
> IIUC this pinmux controller, something like below should work.
>
>
> >8
>
> From 7c26992bce047efb66a6ba8d2ffec2b272499df7 Mon Sep 17 00:00:00 2001
> From: Sudeep Holla 
> Date: Tue, 8 Sep 2015 17:35:38 +0100
> Subject: [PATCH] pinctrl: mediatek: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND
>
> The mediatek pinmux controller doesn't provides any facility to configure
> the wakeup sources. So instead of providing redundant irq_set_wake
> functionality, SKIP_SET_WAKE could be set to it. Also there's no need to
> maintain 2 sets of masks.
>
> This patch enables IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND and
> also removes wake_mask.
>
> Cc: Linus Walleij 
> Cc: Matthias Brugger 
> Cc: Hongzhou Yang 
> Cc: Yingjoe Chen 
> Cc: Maoguang Meng 
> Cc: Chaotian Jing 
> Cc: linux-g...@vger.kernel.org
> Cc: linux-media...@lists.infradead.org
> Signed-off-by: Sudeep Holla 
> ---
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 24 +---
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  1 -
>  2 files changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index 7726c6caaf83..d8e194a5bb31 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> @@ -1063,20 +1063,6 @@ static int mtk_eint_set_type(struct irq_data *d,
> return 0;
>  }
>
> -static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> -{
> -   struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> -   int shift = d->hwirq & 0x1f;
> -   int reg = d->hwirq >> 5;
> -
> -   if (on)
> -   pctl->wake_mask[reg] |= BIT(shift);
> -   else
> -   pctl->wake_mask[reg] &= ~BIT(shift);
> -
> -   return 0;
> -}
> -
>  static void mtk_eint_chip_write_mask(const struct mtk_eint_offsets *chip,
> void __iomem *eint_reg_base, u32 *buf)
>  {
> @@ -1112,7 +1098,6 @@ static int mtk_eint_suspend(struct device *device)
>
> reg = pctl->eint_reg_base;
> mtk_eint_chip_read_mask(eint_offsets, reg, pctl->cur_mask);
> 

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[王崇懿]

Hi Sudeep and Maoguang,

   Please correct me if I am wrong. I think the wake_mask Maoguang
implemented is the wake-up configuration and it is how he disabled
other unwanted interrupt sources(e.g. audio jacket insertion) during
suspend.

  With Sudeep's patch which we had similar one before, the system got
waken up by audio jack insertion which we don't want. Maoguang tried
to implement wake_mask as the wake-up configuration to keep track of
effective wakeup sources(i.e. those who makes enable_irq_wake) and
write the wake-up configuration in mtk_eint_suspend(). What is your
suggestion to address this issue? Thanks!

On Wed, Sep 9, 2015 at 12:50 AM, Sudeep Holla  wrote:
>
>
> On 08/09/15 10:28, Sudeep Holla wrote:
>>
>>
>>
>> On 06/09/15 11:39, maoguang meng wrote:
>>>
>>> On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:

 Hi maoguang,

 On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla 
 wrote:
>
>
>
> On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:
>>
>>
>> From: Maoguang Meng 
>>
>> This patch implement irq_set_wake to get who is wakeup source and
>> setup on suspend resume.
>>
>> Signed-off-by: Maoguang Meng 
>>
 [snip]

 Can you please respond to Sudeep's questions:

> Does this pinmux controller:
>
> 1. Support wake-up configuration ? If not, you need to use
>  IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
>  mask_{set,clear} if the same registers are used for {en,dis}able
>>>
>>>
>>> YES.
>>> we can call enable_irq_wake(irq) to config this irq as a wake-up
>>> source.
>>>
>>
>> No that doesn't answer my question. Yes you can always call
>> enable_irq_wake(irq) as along as you have irq_set_wake implemented.
>> But my question was does this pinmux controller support wake-up
>> configuration.
>>
>> IMHO, by looking at the implementation I can confirm *NO*, it doesn't.
>> So please stop copy-pasting the implementation from other drivers.
>> The reason is you operate on the same mask_{set,clear} which you use
>> to {en,dis}able the interrupts which means you don't have to do anything
>> to configure an interrupt as a wakeup source other than just keeping
>> them enabled. Hopefully this clarifies.
>>
>
> 2. Is in always on domain ? If not, you need save/restore only to
>  resume back the functionality. Generally we can set
>  IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
>  disabled during suspend and re-enabled in resume path. You just
>  save/restore raw values without tracking the wake-up source.
>>>
>>>
>>> YES.
>>>
>>
>> Again *YES* for what part of my question. If it's always-on, then you
>> don't need this suspend/resume callbacks at all, so I assume the answer
>> is *NO*.
>>
>
> IIUC this pinmux controller, something like below should work.
>
>
> >8
>
> From 7c26992bce047efb66a6ba8d2ffec2b272499df7 Mon Sep 17 00:00:00 2001
> From: Sudeep Holla 
> Date: Tue, 8 Sep 2015 17:35:38 +0100
> Subject: [PATCH] pinctrl: mediatek: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND
>
> The mediatek pinmux controller doesn't provides any facility to configure
> the wakeup sources. So instead of providing redundant irq_set_wake
> functionality, SKIP_SET_WAKE could be set to it. Also there's no need to
> maintain 2 sets of masks.
>
> This patch enables IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND and
> also removes wake_mask.
>
> Cc: Linus Walleij 
> Cc: Matthias Brugger 
> Cc: Hongzhou Yang 
> Cc: Yingjoe Chen 
> Cc: Maoguang Meng 
> Cc: Chaotian Jing 
> Cc: linux-g...@vger.kernel.org
> Cc: linux-media...@lists.infradead.org
> Signed-off-by: Sudeep Holla 
> ---
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 24 +---
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  1 -
>  2 files changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index 7726c6caaf83..d8e194a5bb31 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> @@ -1063,20 +1063,6 @@ static int mtk_eint_set_type(struct irq_data *d,
> return 0;
>  }
>
> -static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> -{
> -   struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> -   int shift = d->hwirq & 0x1f;
> -   int reg = d->hwirq >> 5;
> -
> -   if (on)
> -   pctl->wake_mask[reg] |= BIT(shift);
> -   else
> -   pctl->wake_mask[reg] &= ~BIT(shift);
> -
> -   return 0;
> -}
> -
>  static void 

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 11/09/15 12:22, Chung-Yih Wang (王崇懿) wrote:

Hi Sudeep and Maoguang,

Please correct me if I am wrong. I think the wake_mask Maoguang
implemented is the wake-up configuration and it is how he disabled
other unwanted interrupt sources(e.g. audio jacket insertion) during
suspend.



OK, you are right, I think I now understand the issue. I misread the
code initially thinking the suspend/resume are implemented as
syscore_ops but they are standard device pm ops.


   With Sudeep's patch which we had similar one before, the system got
waken up by audio jack insertion which we don't want. Maoguang tried
to implement wake_mask as the wake-up configuration to keep track of
effective wakeup sources(i.e. those who makes enable_irq_wake) and
write the wake-up configuration in mtk_eint_suspend(). What is your
suggestion to address this issue? Thanks!



One option is to convert them to *_noirq callbacks assuming all the
users of this pinctrl irqchip have sanely implemented their
suspend/resume and don't trigger interrupts between dpm_suspend and
suspend_device_irqs. What do you think ?

Regards,
Sudeep

>8

@@ -1130,8 +1130,8 @@ static int mtk_eint_resume(struct device *device)
 }

 const struct dev_pm_ops mtk_eint_pm_ops = {
-   .suspend = mtk_eint_suspend,
-   .resume = mtk_eint_resume,
+   .suspend_noirq = mtk_eint_suspend,
+   .resume_noirq = mtk_eint_resume,
 };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 08/09/15 10:28, Sudeep Holla wrote:



On 06/09/15 11:39, maoguang meng wrote:

On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:

Hi maoguang,

On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  wrote:



On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:


From: Maoguang Meng 

This patch implement irq_set_wake to get who is wakeup source and
setup on suspend resume.

Signed-off-by: Maoguang Meng 


[snip]

Can you please respond to Sudeep's questions:


Does this pinmux controller:

1. Support wake-up configuration ? If not, you need to use
 IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
 mask_{set,clear} if the same registers are used for {en,dis}able


YES.
we can call enable_irq_wake(irq) to config this irq as a wake-up
source.



No that doesn't answer my question. Yes you can always call
enable_irq_wake(irq) as along as you have irq_set_wake implemented.
But my question was does this pinmux controller support wake-up
configuration.

IMHO, by looking at the implementation I can confirm *NO*, it doesn't.
So please stop copy-pasting the implementation from other drivers.
The reason is you operate on the same mask_{set,clear} which you use
to {en,dis}able the interrupts which means you don't have to do anything
to configure an interrupt as a wakeup source other than just keeping
them enabled. Hopefully this clarifies.



2. Is in always on domain ? If not, you need save/restore only to
 resume back the functionality. Generally we can set
 IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
 disabled during suspend and re-enabled in resume path. You just
 save/restore raw values without tracking the wake-up source.


YES.



Again *YES* for what part of my question. If it's always-on, then you
don't need this suspend/resume callbacks at all, so I assume the answer
is *NO*.



IIUC this pinmux controller, something like below should work.


>8

From 7c26992bce047efb66a6ba8d2ffec2b272499df7 Mon Sep 17 00:00:00 2001
From: Sudeep Holla 
Date: Tue, 8 Sep 2015 17:35:38 +0100
Subject: [PATCH] pinctrl: mediatek: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

The mediatek pinmux controller doesn't provides any facility to configure
the wakeup sources. So instead of providing redundant irq_set_wake
functionality, SKIP_SET_WAKE could be set to it. Also there's no need to
maintain 2 sets of masks.

This patch enables IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND and
also removes wake_mask.

Cc: Linus Walleij 
Cc: Matthias Brugger 
Cc: Hongzhou Yang 
Cc: Yingjoe Chen 
Cc: Maoguang Meng 
Cc: Chaotian Jing 
Cc: linux-g...@vger.kernel.org
Cc: linux-media...@lists.infradead.org
Signed-off-by: Sudeep Holla 
---
 drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 24 
+---

 drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  1 -
 2 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c

index 7726c6caaf83..d8e194a5bb31 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
@@ -1063,20 +1063,6 @@ static int mtk_eint_set_type(struct irq_data *d,
return 0;
 }

-static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
-{
-   struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
-   int shift = d->hwirq & 0x1f;
-   int reg = d->hwirq >> 5;
-
-   if (on)
-   pctl->wake_mask[reg] |= BIT(shift);
-   else
-   pctl->wake_mask[reg] &= ~BIT(shift);
-
-   return 0;
-}
-
 static void mtk_eint_chip_write_mask(const struct mtk_eint_offsets *chip,
void __iomem *eint_reg_base, u32 *buf)
 {
@@ -1112,7 +1098,6 @@ static int mtk_eint_suspend(struct device *device)

reg = pctl->eint_reg_base;
mtk_eint_chip_read_mask(eint_offsets, reg, pctl->cur_mask);
-   mtk_eint_chip_write_mask(eint_offsets, reg, pctl->wake_mask);

return 0;
 }
@@ -1153,9 +1138,9 @@ static struct irq_chip mtk_pinctrl_irq_chip = {
.irq_unmask = mtk_eint_unmask,
.irq_ack = mtk_eint_ack,
.irq_set_type = mtk_eint_set_type,
-   .irq_set_wake = mtk_eint_irq_set_wake,
.irq_request_resources = mtk_pinctrl_irq_request_resources,
.irq_release_resources = mtk_pinctrl_irq_release_resources,
+   .flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
 };

 static unsigned int mtk_eint_init(struct mtk_pinctrl *pctl)
@@ -1393,13 +1378,6 @@ int mtk_pctrl_init(struct platform_device *pdev,
}

ports_buf = pctl->devdata->eint_offsets.ports;
-   pctl->wake_mask = devm_kcalloc(>dev, ports_buf,
-   sizeof(*pctl->wake_mask), GFP_KERNEL);
-   if (!pctl->wake_mask) {
-   ret = -ENOMEM;
-   goto chip_error;
-   }
-
pctl->cur_mask = devm_kcalloc(>dev, ports_buf,

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 06/09/15 11:39, maoguang meng wrote:

On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:

Hi maoguang,

On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  wrote:



On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:


From: Maoguang Meng 

This patch implement irq_set_wake to get who is wakeup source and
setup on suspend resume.

Signed-off-by: Maoguang Meng 


[snip]

Can you please respond to Sudeep's questions:


Does this pinmux controller:

1. Support wake-up configuration ? If not, you need to use
IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
mask_{set,clear} if the same registers are used for {en,dis}able


   YES.
   we can call enable_irq_wake(irq) to config this irq as a wake-up
source.



No that doesn't answer my question. Yes you can always call
enable_irq_wake(irq) as along as you have irq_set_wake implemented.
But my question was does this pinmux controller support wake-up
configuration.

IMHO, by looking at the implementation I can confirm *NO*, it doesn't.
So please stop copy-pasting the implementation from other drivers.
The reason is you operate on the same mask_{set,clear} which you use
to {en,dis}able the interrupts which means you don't have to do anything
to configure an interrupt as a wakeup source other than just keeping
them enabled. Hopefully this clarifies.



2. Is in always on domain ? If not, you need save/restore only to
resume back the functionality. Generally we can set
IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
disabled during suspend and re-enabled in resume path. You just
save/restore raw values without tracking the wake-up source.


   YES.



Again *YES* for what part of my question. If it's always-on, then you
don't need this suspend/resume callbacks at all, so I assume the answer
is *NO*.



Also I see that no care is taken to set the port irq as wake enable
source. It may work with current mainline, but won't with -next. Please
ensure the port irq to the parent interrupt controller remains
enabled(i.e set as wake).


As you know, we do not care the port irq as wake enbale source,
when enter suspend we think no matter what the port irq is
enabled/disabled which are not affected as a wake-up source,
Because eint has a line to receive SPM.



I understand that, but for proper wakeup functionality you need to do
configure the parent if you want to identify and handle that wakeup
interrupt. If you don't care then it should be fine.


For example,after suspend,press power key(use eint5) will generate an
electrical signal which is send to spm by eint.
and then spm wake cpu.


Yes I got that, as I mentioned above, I think you don't need to identify
and handle the wakeup interrupt. E.g. if it's a keyboard key press, you
may need to identify the key pressed, so it needs to be handled
properly. In your case, I assume it's just power button whose main
function is to wake/boot and that's already achieved when you are woken up.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 06/09/15 11:39, maoguang meng wrote:

On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:

Hi maoguang,

On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  wrote:



On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:


From: Maoguang Meng 

This patch implement irq_set_wake to get who is wakeup source and
setup on suspend resume.

Signed-off-by: Maoguang Meng 


[snip]

Can you please respond to Sudeep's questions:


Does this pinmux controller:

1. Support wake-up configuration ? If not, you need to use
IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
mask_{set,clear} if the same registers are used for {en,dis}able


   YES.
   we can call enable_irq_wake(irq) to config this irq as a wake-up
source.



No that doesn't answer my question. Yes you can always call
enable_irq_wake(irq) as along as you have irq_set_wake implemented.
But my question was does this pinmux controller support wake-up
configuration.

IMHO, by looking at the implementation I can confirm *NO*, it doesn't.
So please stop copy-pasting the implementation from other drivers.
The reason is you operate on the same mask_{set,clear} which you use
to {en,dis}able the interrupts which means you don't have to do anything
to configure an interrupt as a wakeup source other than just keeping
them enabled. Hopefully this clarifies.



2. Is in always on domain ? If not, you need save/restore only to
resume back the functionality. Generally we can set
IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
disabled during suspend and re-enabled in resume path. You just
save/restore raw values without tracking the wake-up source.


   YES.



Again *YES* for what part of my question. If it's always-on, then you
don't need this suspend/resume callbacks at all, so I assume the answer
is *NO*.



Also I see that no care is taken to set the port irq as wake enable
source. It may work with current mainline, but won't with -next. Please
ensure the port irq to the parent interrupt controller remains
enabled(i.e set as wake).


As you know, we do not care the port irq as wake enbale source,
when enter suspend we think no matter what the port irq is
enabled/disabled which are not affected as a wake-up source,
Because eint has a line to receive SPM.



I understand that, but for proper wakeup functionality you need to do
configure the parent if you want to identify and handle that wakeup
interrupt. If you don't care then it should be fine.


For example,after suspend,press power key(use eint5) will generate an
electrical signal which is send to spm by eint.
and then spm wake cpu.


Yes I got that, as I mentioned above, I think you don't need to identify
and handle the wakeup interrupt. E.g. if it's a keyboard key press, you
may need to identify the key pressed, so it needs to be handled
properly. In your case, I assume it's just power button whose main
function is to wake/boot and that's already achieved when you are woken up.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 08/09/15 10:28, Sudeep Holla wrote:



On 06/09/15 11:39, maoguang meng wrote:

On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:

Hi maoguang,

On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  wrote:



On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:


From: Maoguang Meng 

This patch implement irq_set_wake to get who is wakeup source and
setup on suspend resume.

Signed-off-by: Maoguang Meng 


[snip]

Can you please respond to Sudeep's questions:


Does this pinmux controller:

1. Support wake-up configuration ? If not, you need to use
 IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
 mask_{set,clear} if the same registers are used for {en,dis}able


YES.
we can call enable_irq_wake(irq) to config this irq as a wake-up
source.



No that doesn't answer my question. Yes you can always call
enable_irq_wake(irq) as along as you have irq_set_wake implemented.
But my question was does this pinmux controller support wake-up
configuration.

IMHO, by looking at the implementation I can confirm *NO*, it doesn't.
So please stop copy-pasting the implementation from other drivers.
The reason is you operate on the same mask_{set,clear} which you use
to {en,dis}able the interrupts which means you don't have to do anything
to configure an interrupt as a wakeup source other than just keeping
them enabled. Hopefully this clarifies.



2. Is in always on domain ? If not, you need save/restore only to
 resume back the functionality. Generally we can set
 IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
 disabled during suspend and re-enabled in resume path. You just
 save/restore raw values without tracking the wake-up source.


YES.



Again *YES* for what part of my question. If it's always-on, then you
don't need this suspend/resume callbacks at all, so I assume the answer
is *NO*.



IIUC this pinmux controller, something like below should work.


>8

From 7c26992bce047efb66a6ba8d2ffec2b272499df7 Mon Sep 17 00:00:00 2001
From: Sudeep Holla 
Date: Tue, 8 Sep 2015 17:35:38 +0100
Subject: [PATCH] pinctrl: mediatek: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

The mediatek pinmux controller doesn't provides any facility to configure
the wakeup sources. So instead of providing redundant irq_set_wake
functionality, SKIP_SET_WAKE could be set to it. Also there's no need to
maintain 2 sets of masks.

This patch enables IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND and
also removes wake_mask.

Cc: Linus Walleij 
Cc: Matthias Brugger 
Cc: Hongzhou Yang 
Cc: Yingjoe Chen 
Cc: Maoguang Meng 
Cc: Chaotian Jing 
Cc: linux-g...@vger.kernel.org
Cc: linux-media...@lists.infradead.org
Signed-off-by: Sudeep Holla 
---
 drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 24 
+---

 drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  1 -
 2 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c

index 7726c6caaf83..d8e194a5bb31 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
@@ -1063,20 +1063,6 @@ static int mtk_eint_set_type(struct irq_data *d,
return 0;
 }

-static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
-{
-   struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
-   int shift = d->hwirq & 0x1f;
-   int reg = d->hwirq >> 5;
-
-   if (on)
-   pctl->wake_mask[reg] |= BIT(shift);
-   else
-   pctl->wake_mask[reg] &= ~BIT(shift);
-
-   return 0;
-}
-
 static void mtk_eint_chip_write_mask(const struct mtk_eint_offsets *chip,
void __iomem *eint_reg_base, u32 *buf)
 {
@@ -1112,7 +1098,6 @@ static int mtk_eint_suspend(struct device *device)

reg = pctl->eint_reg_base;
mtk_eint_chip_read_mask(eint_offsets, reg, pctl->cur_mask);
-   mtk_eint_chip_write_mask(eint_offsets, reg, pctl->wake_mask);

return 0;
 }
@@ -1153,9 +1138,9 @@ static struct irq_chip mtk_pinctrl_irq_chip = {
.irq_unmask = mtk_eint_unmask,
.irq_ack = mtk_eint_ack,
.irq_set_type = mtk_eint_set_type,
-   .irq_set_wake = mtk_eint_irq_set_wake,
.irq_request_resources = mtk_pinctrl_irq_request_resources,
.irq_release_resources = mtk_pinctrl_irq_release_resources,
+   .flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
 };

 static unsigned int mtk_eint_init(struct mtk_pinctrl *pctl)
@@ -1393,13 +1378,6 @@ int mtk_pctrl_init(struct platform_device *pdev,
}

ports_buf = pctl->devdata->eint_offsets.ports;
-   pctl->wake_mask = devm_kcalloc(>dev, ports_buf,
-  

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[maoguang meng]

On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:
> Hi maoguang,
> 
> On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  wrote:
> >
> >
> > On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:
> >>
> >> From: Maoguang Meng 
> >>
> >> This patch implement irq_set_wake to get who is wakeup source and
> >> setup on suspend resume.
> >>
> >> Signed-off-by: Maoguang Meng 
> >>
> [snip]
> 
> Can you please respond to Sudeep's questions:
> 
> > Does this pinmux controller:
> >
> > 1. Support wake-up configuration ? If not, you need to use
> >IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
> >mask_{set,clear} if the same registers are used for {en,dis}able

  YES.
  we can call enable_irq_wake(irq) to config this irq as a wake-up
source.

> >
> > 2. Is in always on domain ? If not, you need save/restore only to
> >resume back the functionality. Generally we can set
> >IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
> >disabled during suspend and re-enabled in resume path. You just
> >save/restore raw values without tracking the wake-up source.

  YES.

> >
> > Also I see that no care is taken to set the port irq as wake enable
> > source. It may work with current mainline, but won't with -next. Please
> > ensure the port irq to the parent interrupt controller remains
> > enabled(i.e set as wake).

As you know, we do not care the port irq as wake enbale source,
when enter suspend we think no matter what the port irq is
enabled/disabled which are not affected as a wake-up source,
Because eint has a line to receive SPM.

For example,after suspend,press power key(use eint5) will generate an
electrical signal which is send to spm by eint.
and then spm wake cpu.
  
> >
> > Regards,
> > Sudeep

Regards,
Maoguang

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[maoguang meng]

On Wed, 2015-09-02 at 14:02 +0800, Daniel Kurtz wrote:
> Hi maoguang,
> 
> On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  wrote:
> >
> >
> > On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:
> >>
> >> From: Maoguang Meng 
> >>
> >> This patch implement irq_set_wake to get who is wakeup source and
> >> setup on suspend resume.
> >>
> >> Signed-off-by: Maoguang Meng 
> >>
> [snip]
> 
> Can you please respond to Sudeep's questions:
> 
> > Does this pinmux controller:
> >
> > 1. Support wake-up configuration ? If not, you need to use
> >IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
> >mask_{set,clear} if the same registers are used for {en,dis}able

  YES.
  we can call enable_irq_wake(irq) to config this irq as a wake-up
source.

> >
> > 2. Is in always on domain ? If not, you need save/restore only to
> >resume back the functionality. Generally we can set
> >IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
> >disabled during suspend and re-enabled in resume path. You just
> >save/restore raw values without tracking the wake-up source.

  YES.

> >
> > Also I see that no care is taken to set the port irq as wake enable
> > source. It may work with current mainline, but won't with -next. Please
> > ensure the port irq to the parent interrupt controller remains
> > enabled(i.e set as wake).

As you know, we do not care the port irq as wake enbale source,
when enter suspend we think no matter what the port irq is
enabled/disabled which are not affected as a wake-up source,
Because eint has a line to receive SPM.

For example,after suspend,press power key(use eint5) will generate an
electrical signal which is send to spm by eint.
and then spm wake cpu.
  
> >
> > Regards,
> > Sudeep

Regards,
Maoguang

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Daniel Kurtz]

Hi maoguang,

On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  wrote:
>
>
> On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:
>>
>> From: Maoguang Meng 
>>
>> This patch implement irq_set_wake to get who is wakeup source and
>> setup on suspend resume.
>>
>> Signed-off-by: Maoguang Meng 
>>
[snip]

Can you please respond to Sudeep's questions:

> Does this pinmux controller:
>
> 1. Support wake-up configuration ? If not, you need to use
>IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
>mask_{set,clear} if the same registers are used for {en,dis}able
>
> 2. Is in always on domain ? If not, you need save/restore only to
>resume back the functionality. Generally we can set
>IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
>disabled during suspend and re-enabled in resume path. You just
>save/restore raw values without tracking the wake-up source.
>
> Also I see that no care is taken to set the port irq as wake enable
> source. It may work with current mainline, but won't with -next. Please
> ensure the port irq to the parent interrupt controller remains
> enabled(i.e set as wake).

>
> Regards,
> Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Daniel Kurtz]

Hi maoguang,

On Tue, Aug 25, 2015 at 12:27 AM, Sudeep Holla  wrote:
>
>
> On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:
>>
>> From: Maoguang Meng 
>>
>> This patch implement irq_set_wake to get who is wakeup source and
>> setup on suspend resume.
>>
>> Signed-off-by: Maoguang Meng 
>>
[snip]

Can you please respond to Sudeep's questions:

> Does this pinmux controller:
>
> 1. Support wake-up configuration ? If not, you need to use
>IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
>mask_{set,clear} if the same registers are used for {en,dis}able
>
> 2. Is in always on domain ? If not, you need save/restore only to
>resume back the functionality. Generally we can set
>IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
>disabled during suspend and re-enabled in resume path. You just
>save/restore raw values without tracking the wake-up source.
>
> Also I see that no care is taken to set the port irq as wake enable
> source. It may work with current mainline, but won't with -next. Please
> ensure the port irq to the parent interrupt controller remains
> enabled(i.e set as wake).

>
> Regards,
> Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Linus Walleij]

On Fri, Aug 14, 2015 at 10:38 AM,   wrote:

> From: Maoguang Meng 
>
> This patch implement irq_set_wake to get who is wakeup source and
> setup on suspend resume.
>
> Signed-off-by: Maoguang Meng 
>
> ---
> changes since v3:
> -add a comment in mtk_eint_chip_read_mask.
> -delete ALIGN when allocate eint_offsets.ports.
> -fix unrelated change.

Patch applied with Daniel's, Yingjoe's and Hongzhu's tags.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Linus Walleij]

On Fri, Aug 14, 2015 at 10:38 AM,  maoguang.m...@mediatek.com wrote:

 From: Maoguang Meng maoguang.m...@mediatek.com

 This patch implement irq_set_wake to get who is wakeup source and
 setup on suspend resume.

 Signed-off-by: Maoguang Meng maoguang.m...@mediatek.com

 ---
 changes since v3:
 -add a comment in mtk_eint_chip_read_mask.
 -delete ALIGN when allocate eint_offsets.ports.
 -fix unrelated change.

Patch applied with Daniel's, Yingjoe's and Hongzhu's tags.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:

From: Maoguang Meng 

This patch implement irq_set_wake to get who is wakeup source and
setup on suspend resume.

Signed-off-by: Maoguang Meng 

---
changes since v3:
-add a comment in mtk_eint_chip_read_mask.
-delete ALIGN when allocate eint_offsets.ports.
-fix unrelated change.

changes since v2:
-modify irq_wake to handle irq wakeup source.
-allocate two buffers separately.
-fix some codestyle.

Changes since v1:
-implement irq_wake handler.
---

  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 ++-
  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
  3 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
index d0c811d..ad27184 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
@@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
.driver = {
.name = "mediatek-mt8173-pinctrl",
.of_match_table = mt8173_pctrl_match,
+   .pm = _eint_pm_ops,
},
  };

diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
index ad1ea16..fe34ce9 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
@@ -33,6 +33,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 

  #include "../core.h"
@@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
return 0;
  }

+static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+   struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
+   int shift = d->hwirq & 0x1f;
+   int reg = d->hwirq >> 5;
+
+   if (on)
+   pctl->wake_mask[reg] |= BIT(shift);
+   else
+   pctl->wake_mask[reg] &= ~BIT(shift);
+
+   return 0;
+}


Does this pinmux controller:

1. Support wake-up configuration ? If not, you need to use
   IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
   mask_{set,clear} if the same registers are used for {en,dis}able

2. Is in always on domain ? If not, you need save/restore only to
   resume back the functionality. Generally we can set  
   IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
   disabled during suspend and re-enabled in resume path. You just
   save/restore raw values without tracking the wake-up source.

Also I see that no care is taken to set the port irq as wake enable
source. It may work with current mainline, but won't with -next. Please
ensure the port irq to the parent interrupt controller remains
enabled(i.e set as wake).

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Sudeep Holla]

On 14/08/15 09:38, maoguang.m...@mediatek.com wrote:

From: Maoguang Meng maoguang.m...@mediatek.com

This patch implement irq_set_wake to get who is wakeup source and
setup on suspend resume.

Signed-off-by: Maoguang Meng maoguang.m...@mediatek.com

---
changes since v3:
-add a comment in mtk_eint_chip_read_mask.
-delete ALIGN when allocate eint_offsets.ports.
-fix unrelated change.

changes since v2:
-modify irq_wake to handle irq wakeup source.
-allocate two buffers separately.
-fix some codestyle.

Changes since v1:
-implement irq_wake handler.
---

  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 ++-
  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
  3 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
index d0c811d..ad27184 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
@@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
.driver = {
.name = mediatek-mt8173-pinctrl,
.of_match_table = mt8173_pctrl_match,
+   .pm = mtk_eint_pm_ops,
},
  };

diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
index ad1ea16..fe34ce9 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
@@ -33,6 +33,7 @@
  #include linux/mfd/syscon.h
  #include linux/delay.h
  #include linux/interrupt.h
+#include linux/pm.h
  #include dt-bindings/pinctrl/mt65xx.h

  #include ../core.h
@@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
return 0;
  }

+static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+   struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
+   int shift = d-hwirq  0x1f;
+   int reg = d-hwirq  5;
+
+   if (on)
+   pctl-wake_mask[reg] |= BIT(shift);
+   else
+   pctl-wake_mask[reg] = ~BIT(shift);
+
+   return 0;
+}


Does this pinmux controller:

1. Support wake-up configuration ? If not, you need to use
   IRQCHIP_SKIP_SET_WAKE. I don't see any value in writing the
   mask_{set,clear} if the same registers are used for {en,dis}able

2. Is in always on domain ? If not, you need save/restore only to
   resume back the functionality. Generally we can set  
   IRQCHIP_MASK_ON_SUSPEND to ensure non-wake-up interrupts are
   disabled during suspend and re-enabled in resume path. You just
   save/restore raw values without tracking the wake-up source.

Also I see that no care is taken to set the port irq as wake enable
source. It may work with current mainline, but won't with -next. Please
ensure the port irq to the parent interrupt controller remains
enabled(i.e set as wake).

Regards,
Sudeep
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Hongzhou Yang]

On Mon, 2015-08-17 at 21:25 +0800, Yingjoe Chen wrote:
> On Mon, 2015-08-17 at 17:09 +0800, Daniel Kurtz wrote:
> > On Mon, Aug 17, 2015 at 3:52 PM, Yingjoe Chen  
> > wrote:
> > > On Fri, 2015-08-14 at 16:38 +0800, maoguang.m...@mediatek.com wrote:
> > >> From: Maoguang Meng 
> > >>
> > >> This patch implement irq_set_wake to get who is wakeup source and
> > >> setup on suspend resume.
> > >>
> > >> Signed-off-by: Maoguang Meng 
> > >>
> > >> ---
> > >> changes since v3:
> > >> -add a comment in mtk_eint_chip_read_mask.
> > >> -delete ALIGN when allocate eint_offsets.ports.
> > >> -fix unrelated change.
> > >>
> > >> changes since v2:
> > >> -modify irq_wake to handle irq wakeup source.
> > >> -allocate two buffers separately.
> > >> -fix some codestyle.
> > >>
> > >> Changes since v1:
> > >> -implement irq_wake handler.
> > >> ---
> > >>
> > >>  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
> > >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
> > >> ++-
> > >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
> > >>  3 files changed, 95 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
> > >> b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> > >> index d0c811d..ad27184 100644
> > >> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> > >> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> > >> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
> > >>   .driver = {
> > >>   .name = "mediatek-mt8173-pinctrl",
> > >>   .of_match_table = mt8173_pctrl_match,
> > >> + .pm = _eint_pm_ops,
> > >>   },
> > >>  };
> > >>
> > >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
> > >> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > >> index ad1ea16..fe34ce9 100644
> > >> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > >> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> > >> @@ -33,6 +33,7 @@
> > >>  #include 
> > >>  #include 
> > >>  #include 
> > >> +#include 
> > >>  #include 
> > >>
> > >>  #include "../core.h"
> > >> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
> > >>   return 0;
> > >>  }
> > >>
> > >> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> > >> +{
> > >> + struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> > >> + int shift = d->hwirq & 0x1f;
> > >> + int reg = d->hwirq >> 5;
> > >> +
> > >> + if (on)
> > >> + pctl->wake_mask[reg] |= BIT(shift);
> > >> + else
> > >> + pctl->wake_mask[reg] &= ~BIT(shift);
> > >> +
> > >> + return 0;
> > >> +}
> > >
> > > Hi Maoguang,
> > >
> > > You changed from set_bit/clear_bit to this, but didn't add any locking.
> > > Since this is basic read/modify/write, is it OK to do it without
> > > locking?
> > 
> > I believe calling .irq_set_wake() concurrently with itself is
> > protected by irq_get_desc_buslock():
> > 
> >  int irq_set_irq_wake(unsigned int irq, unsigned int on)
> >  {
> >  unsigned long flags;
> >  struct irq_desc *desc = irq_get_desc_buslock(irq, ,
> > IRQ_GET_DESC_CHECK_GLOBAL);
> >  int ret = 0;
> > ...
> >  ret = set_irq_wake_real(irq, on);
> > ...
> >  irq_put_desc_busunlock(desc, flags);
> >  return ret;
> >  }
> 
> Hi Dan,
> 
> You are right, irq_get_desc_buslock will acquire desc lock even when
> irq_chip didn't provide irq_bus_lock callback. So this should be OK.
> Sorry for the noise.
> 
> For this patch:
> 
> Acked-by: Yingjoe Chen 
> 
> Joe.C
> 


Acked-by: Hongzhou Yang 

Thanks.
Hongzhou


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Yingjoe Chen]

On Mon, 2015-08-17 at 17:09 +0800, Daniel Kurtz wrote:
> On Mon, Aug 17, 2015 at 3:52 PM, Yingjoe Chen  
> wrote:
> > On Fri, 2015-08-14 at 16:38 +0800, maoguang.m...@mediatek.com wrote:
> >> From: Maoguang Meng 
> >>
> >> This patch implement irq_set_wake to get who is wakeup source and
> >> setup on suspend resume.
> >>
> >> Signed-off-by: Maoguang Meng 
> >>
> >> ---
> >> changes since v3:
> >> -add a comment in mtk_eint_chip_read_mask.
> >> -delete ALIGN when allocate eint_offsets.ports.
> >> -fix unrelated change.
> >>
> >> changes since v2:
> >> -modify irq_wake to handle irq wakeup source.
> >> -allocate two buffers separately.
> >> -fix some codestyle.
> >>
> >> Changes since v1:
> >> -implement irq_wake handler.
> >> ---
> >>
> >>  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
> >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
> >> ++-
> >>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
> >>  3 files changed, 95 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
> >> b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> >> index d0c811d..ad27184 100644
> >> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> >> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> >> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
> >>   .driver = {
> >>   .name = "mediatek-mt8173-pinctrl",
> >>   .of_match_table = mt8173_pctrl_match,
> >> + .pm = _eint_pm_ops,
> >>   },
> >>  };
> >>
> >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
> >> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> >> index ad1ea16..fe34ce9 100644
> >> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> >> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> >> @@ -33,6 +33,7 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >> +#include 
> >>  #include 
> >>
> >>  #include "../core.h"
> >> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
> >>   return 0;
> >>  }
> >>
> >> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> >> +{
> >> + struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> >> + int shift = d->hwirq & 0x1f;
> >> + int reg = d->hwirq >> 5;
> >> +
> >> + if (on)
> >> + pctl->wake_mask[reg] |= BIT(shift);
> >> + else
> >> + pctl->wake_mask[reg] &= ~BIT(shift);
> >> +
> >> + return 0;
> >> +}
> >
> > Hi Maoguang,
> >
> > You changed from set_bit/clear_bit to this, but didn't add any locking.
> > Since this is basic read/modify/write, is it OK to do it without
> > locking?
> 
> I believe calling .irq_set_wake() concurrently with itself is
> protected by irq_get_desc_buslock():
> 
>  int irq_set_irq_wake(unsigned int irq, unsigned int on)
>  {
>  unsigned long flags;
>  struct irq_desc *desc = irq_get_desc_buslock(irq, ,
> IRQ_GET_DESC_CHECK_GLOBAL);
>  int ret = 0;
> ...
>  ret = set_irq_wake_real(irq, on);
> ...
>  irq_put_desc_busunlock(desc, flags);
>  return ret;
>  }

Hi Dan,

You are right, irq_get_desc_buslock will acquire desc lock even when
irq_chip didn't provide irq_bus_lock callback. So this should be OK.
Sorry for the noise.

For this patch:

Acked-by: Yingjoe Chen 

Joe.C



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Daniel Kurtz]

On Mon, Aug 17, 2015 at 3:52 PM, Yingjoe Chen  wrote:
> On Fri, 2015-08-14 at 16:38 +0800, maoguang.m...@mediatek.com wrote:
>> From: Maoguang Meng 
>>
>> This patch implement irq_set_wake to get who is wakeup source and
>> setup on suspend resume.
>>
>> Signed-off-by: Maoguang Meng 
>>
>> ---
>> changes since v3:
>> -add a comment in mtk_eint_chip_read_mask.
>> -delete ALIGN when allocate eint_offsets.ports.
>> -fix unrelated change.
>>
>> changes since v2:
>> -modify irq_wake to handle irq wakeup source.
>> -allocate two buffers separately.
>> -fix some codestyle.
>>
>> Changes since v1:
>> -implement irq_wake handler.
>> ---
>>
>>  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
>>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
>> ++-
>>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
>>  3 files changed, 95 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
>> b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
>> index d0c811d..ad27184 100644
>> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
>> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
>> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
>>   .driver = {
>>   .name = "mediatek-mt8173-pinctrl",
>>   .of_match_table = mt8173_pctrl_match,
>> + .pm = _eint_pm_ops,
>>   },
>>  };
>>
>> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
>> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
>> index ad1ea16..fe34ce9 100644
>> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
>> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
>> @@ -33,6 +33,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>
>>  #include "../core.h"
>> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
>>   return 0;
>>  }
>>
>> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
>> +{
>> + struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
>> + int shift = d->hwirq & 0x1f;
>> + int reg = d->hwirq >> 5;
>> +
>> + if (on)
>> + pctl->wake_mask[reg] |= BIT(shift);
>> + else
>> + pctl->wake_mask[reg] &= ~BIT(shift);
>> +
>> + return 0;
>> +}
>
> Hi Maoguang,
>
> You changed from set_bit/clear_bit to this, but didn't add any locking.
> Since this is basic read/modify/write, is it OK to do it without
> locking?

I believe calling .irq_set_wake() concurrently with itself is
protected by irq_get_desc_buslock():

 int irq_set_irq_wake(unsigned int irq, unsigned int on)
 {
 unsigned long flags;
 struct irq_desc *desc = irq_get_desc_buslock(irq, ,
IRQ_GET_DESC_CHECK_GLOBAL);
 int ret = 0;
...
 ret = set_irq_wake_real(irq, on);
...
 irq_put_desc_busunlock(desc, flags);
 return ret;
 }


I'm not 100% sure about the .suspend/.resume paths, but I don't think
they can occur during .irq_set_wake(), either.
Nor were they protected by the set_bit/clear_bit implementation.

-Dan


>
> Joe.C
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Yingjoe Chen]

On Fri, 2015-08-14 at 16:38 +0800, maoguang.m...@mediatek.com wrote:
> From: Maoguang Meng 
> 
> This patch implement irq_set_wake to get who is wakeup source and
> setup on suspend resume.
> 
> Signed-off-by: Maoguang Meng 
> 
> ---
> changes since v3:
> -add a comment in mtk_eint_chip_read_mask.
> -delete ALIGN when allocate eint_offsets.ports.
> -fix unrelated change.
> 
> changes since v2:
> -modify irq_wake to handle irq wakeup source.
> -allocate two buffers separately.
> -fix some codestyle.
> 
> Changes since v1:
> -implement irq_wake handler.
> ---
> 
>  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
> ++-
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
>  3 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
> b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> index d0c811d..ad27184 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
>   .driver = {
>   .name = "mediatek-mt8173-pinctrl",
>   .of_match_table = mt8173_pctrl_match,
> + .pm = _eint_pm_ops,
>   },
>  };
>  
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index ad1ea16..fe34ce9 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "../core.h"
> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
>   return 0;
>  }
>  
> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> + struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> + int shift = d->hwirq & 0x1f;
> + int reg = d->hwirq >> 5;
> +
> + if (on)
> + pctl->wake_mask[reg] |= BIT(shift);
> + else
> + pctl->wake_mask[reg] &= ~BIT(shift);
> +
> + return 0;
> +}

Hi Maoguang,

You changed from set_bit/clear_bit to this, but didn't add any locking.
Since this is basic read/modify/write, is it OK to do it without
locking?

Joe.C


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Hongzhou Yang]

On Mon, 2015-08-17 at 21:25 +0800, Yingjoe Chen wrote:
 On Mon, 2015-08-17 at 17:09 +0800, Daniel Kurtz wrote:
  On Mon, Aug 17, 2015 at 3:52 PM, Yingjoe Chen yingjoe.c...@mediatek.com 
  wrote:
   On Fri, 2015-08-14 at 16:38 +0800, maoguang.m...@mediatek.com wrote:
   From: Maoguang Meng maoguang.m...@mediatek.com
  
   This patch implement irq_set_wake to get who is wakeup source and
   setup on suspend resume.
  
   Signed-off-by: Maoguang Meng maoguang.m...@mediatek.com
  
   ---
   changes since v3:
   -add a comment in mtk_eint_chip_read_mask.
   -delete ALIGN when allocate eint_offsets.ports.
   -fix unrelated change.
  
   changes since v2:
   -modify irq_wake to handle irq wakeup source.
   -allocate two buffers separately.
   -fix some codestyle.
  
   Changes since v1:
   -implement irq_wake handler.
   ---
  
drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
   ++-
drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
3 files changed, 95 insertions(+), 1 deletion(-)
  
   diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
   b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
   index d0c811d..ad27184 100644
   --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
   +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
   @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
 .driver = {
 .name = mediatek-mt8173-pinctrl,
 .of_match_table = mt8173_pctrl_match,
   + .pm = mtk_eint_pm_ops,
 },
};
  
   diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
   b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
   index ad1ea16..fe34ce9 100644
   --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
   +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
   @@ -33,6 +33,7 @@
#include linux/mfd/syscon.h
#include linux/delay.h
#include linux/interrupt.h
   +#include linux/pm.h
#include dt-bindings/pinctrl/mt65xx.h
  
#include ../core.h
   @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
 return 0;
}
  
   +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
   +{
   + struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
   + int shift = d-hwirq  0x1f;
   + int reg = d-hwirq  5;
   +
   + if (on)
   + pctl-wake_mask[reg] |= BIT(shift);
   + else
   + pctl-wake_mask[reg] = ~BIT(shift);
   +
   + return 0;
   +}
  
   Hi Maoguang,
  
   You changed from set_bit/clear_bit to this, but didn't add any locking.
   Since this is basic read/modify/write, is it OK to do it without
   locking?
  
  I believe calling .irq_set_wake() concurrently with itself is
  protected by irq_get_desc_buslock():
  
   int irq_set_irq_wake(unsigned int irq, unsigned int on)
   {
   unsigned long flags;
   struct irq_desc *desc = irq_get_desc_buslock(irq, flags,
  IRQ_GET_DESC_CHECK_GLOBAL);
   int ret = 0;
  ...
   ret = set_irq_wake_real(irq, on);
  ...
   irq_put_desc_busunlock(desc, flags);
   return ret;
   }
 
 Hi Dan,
 
 You are right, irq_get_desc_buslock will acquire desc lock even when
 irq_chip didn't provide irq_bus_lock callback. So this should be OK.
 Sorry for the noise.
 
 For this patch:
 
 Acked-by: Yingjoe Chen yingjoe.c...@mediatek.com
 
 Joe.C
 


Acked-by: Hongzhou Yang hongzhou.y...@mediatek.com

Thanks.
Hongzhou


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Daniel Kurtz]

On Mon, Aug 17, 2015 at 3:52 PM, Yingjoe Chen yingjoe.c...@mediatek.com wrote:
 On Fri, 2015-08-14 at 16:38 +0800, maoguang.m...@mediatek.com wrote:
 From: Maoguang Meng maoguang.m...@mediatek.com

 This patch implement irq_set_wake to get who is wakeup source and
 setup on suspend resume.

 Signed-off-by: Maoguang Meng maoguang.m...@mediatek.com

 ---
 changes since v3:
 -add a comment in mtk_eint_chip_read_mask.
 -delete ALIGN when allocate eint_offsets.ports.
 -fix unrelated change.

 changes since v2:
 -modify irq_wake to handle irq wakeup source.
 -allocate two buffers separately.
 -fix some codestyle.

 Changes since v1:
 -implement irq_wake handler.
 ---

  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
 ++-
  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
  3 files changed, 95 insertions(+), 1 deletion(-)

 diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
 b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
 index d0c811d..ad27184 100644
 --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
 +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
 @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
   .driver = {
   .name = mediatek-mt8173-pinctrl,
   .of_match_table = mt8173_pctrl_match,
 + .pm = mtk_eint_pm_ops,
   },
  };

 diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
 b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
 index ad1ea16..fe34ce9 100644
 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
 +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
 @@ -33,6 +33,7 @@
  #include linux/mfd/syscon.h
  #include linux/delay.h
  #include linux/interrupt.h
 +#include linux/pm.h
  #include dt-bindings/pinctrl/mt65xx.h

  #include ../core.h
 @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
   return 0;
  }

 +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
 +{
 + struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
 + int shift = d-hwirq  0x1f;
 + int reg = d-hwirq  5;
 +
 + if (on)
 + pctl-wake_mask[reg] |= BIT(shift);
 + else
 + pctl-wake_mask[reg] = ~BIT(shift);
 +
 + return 0;
 +}

 Hi Maoguang,

 You changed from set_bit/clear_bit to this, but didn't add any locking.
 Since this is basic read/modify/write, is it OK to do it without
 locking?

I believe calling .irq_set_wake() concurrently with itself is
protected by irq_get_desc_buslock():

 int irq_set_irq_wake(unsigned int irq, unsigned int on)
 {
 unsigned long flags;
 struct irq_desc *desc = irq_get_desc_buslock(irq, flags,
IRQ_GET_DESC_CHECK_GLOBAL);
 int ret = 0;
...
 ret = set_irq_wake_real(irq, on);
...
 irq_put_desc_busunlock(desc, flags);
 return ret;
 }


I'm not 100% sure about the .suspend/.resume paths, but I don't think
they can occur during .irq_set_wake(), either.
Nor were they protected by the set_bit/clear_bit implementation.

-Dan



 Joe.C


 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Yingjoe Chen]

On Mon, 2015-08-17 at 17:09 +0800, Daniel Kurtz wrote:
 On Mon, Aug 17, 2015 at 3:52 PM, Yingjoe Chen yingjoe.c...@mediatek.com 
 wrote:
  On Fri, 2015-08-14 at 16:38 +0800, maoguang.m...@mediatek.com wrote:
  From: Maoguang Meng maoguang.m...@mediatek.com
 
  This patch implement irq_set_wake to get who is wakeup source and
  setup on suspend resume.
 
  Signed-off-by: Maoguang Meng maoguang.m...@mediatek.com
 
  ---
  changes since v3:
  -add a comment in mtk_eint_chip_read_mask.
  -delete ALIGN when allocate eint_offsets.ports.
  -fix unrelated change.
 
  changes since v2:
  -modify irq_wake to handle irq wakeup source.
  -allocate two buffers separately.
  -fix some codestyle.
 
  Changes since v1:
  -implement irq_wake handler.
  ---
 
   drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
   drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
  ++-
   drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
   3 files changed, 95 insertions(+), 1 deletion(-)
 
  diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
  b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
  index d0c811d..ad27184 100644
  --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
  +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
  @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
.driver = {
.name = mediatek-mt8173-pinctrl,
.of_match_table = mt8173_pctrl_match,
  + .pm = mtk_eint_pm_ops,
},
   };
 
  diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
  b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
  index ad1ea16..fe34ce9 100644
  --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
  +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
  @@ -33,6 +33,7 @@
   #include linux/mfd/syscon.h
   #include linux/delay.h
   #include linux/interrupt.h
  +#include linux/pm.h
   #include dt-bindings/pinctrl/mt65xx.h
 
   #include ../core.h
  @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
return 0;
   }
 
  +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
  +{
  + struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
  + int shift = d-hwirq  0x1f;
  + int reg = d-hwirq  5;
  +
  + if (on)
  + pctl-wake_mask[reg] |= BIT(shift);
  + else
  + pctl-wake_mask[reg] = ~BIT(shift);
  +
  + return 0;
  +}
 
  Hi Maoguang,
 
  You changed from set_bit/clear_bit to this, but didn't add any locking.
  Since this is basic read/modify/write, is it OK to do it without
  locking?
 
 I believe calling .irq_set_wake() concurrently with itself is
 protected by irq_get_desc_buslock():
 
  int irq_set_irq_wake(unsigned int irq, unsigned int on)
  {
  unsigned long flags;
  struct irq_desc *desc = irq_get_desc_buslock(irq, flags,
 IRQ_GET_DESC_CHECK_GLOBAL);
  int ret = 0;
 ...
  ret = set_irq_wake_real(irq, on);
 ...
  irq_put_desc_busunlock(desc, flags);
  return ret;
  }

Hi Dan,

You are right, irq_get_desc_buslock will acquire desc lock even when
irq_chip didn't provide irq_bus_lock callback. So this should be OK.
Sorry for the noise.

For this patch:

Acked-by: Yingjoe Chen yingjoe.c...@mediatek.com

Joe.C



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Yingjoe Chen]

On Fri, 2015-08-14 at 16:38 +0800, maoguang.m...@mediatek.com wrote:
 From: Maoguang Meng maoguang.m...@mediatek.com
 
 This patch implement irq_set_wake to get who is wakeup source and
 setup on suspend resume.
 
 Signed-off-by: Maoguang Meng maoguang.m...@mediatek.com
 
 ---
 changes since v3:
 -add a comment in mtk_eint_chip_read_mask.
 -delete ALIGN when allocate eint_offsets.ports.
 -fix unrelated change.
 
 changes since v2:
 -modify irq_wake to handle irq wakeup source.
 -allocate two buffers separately.
 -fix some codestyle.
 
 Changes since v1:
 -implement irq_wake handler.
 ---
 
  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
 ++-
  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
  3 files changed, 95 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
 b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
 index d0c811d..ad27184 100644
 --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
 +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
 @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
   .driver = {
   .name = mediatek-mt8173-pinctrl,
   .of_match_table = mt8173_pctrl_match,
 + .pm = mtk_eint_pm_ops,
   },
  };
  
 diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
 b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
 index ad1ea16..fe34ce9 100644
 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
 +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
 @@ -33,6 +33,7 @@
  #include linux/mfd/syscon.h
  #include linux/delay.h
  #include linux/interrupt.h
 +#include linux/pm.h
  #include dt-bindings/pinctrl/mt65xx.h
  
  #include ../core.h
 @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
   return 0;
  }
  
 +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
 +{
 + struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
 + int shift = d-hwirq  0x1f;
 + int reg = d-hwirq  5;
 +
 + if (on)
 + pctl-wake_mask[reg] |= BIT(shift);
 + else
 + pctl-wake_mask[reg] = ~BIT(shift);
 +
 + return 0;
 +}

Hi Maoguang,

You changed from set_bit/clear_bit to this, but didn't add any locking.
Since this is basic read/modify/write, is it OK to do it without
locking?

Joe.C


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Daniel Kurtz]

On Fri, Aug 14, 2015 at 4:38 PM,  wrote:
>
> From: Maoguang Meng 
>
> This patch implement irq_set_wake to get who is wakeup source and
> setup on suspend resume.
>
> Signed-off-by: Maoguang Meng 

Reviewed-by: Daniel Kurtz 

Thanks!
-Dan

>
> ---
> changes since v3:
> -add a comment in mtk_eint_chip_read_mask.
> -delete ALIGN when allocate eint_offsets.ports.
> -fix unrelated change.
>
> changes since v2:
> -modify irq_wake to handle irq wakeup source.
> -allocate two buffers separately.
> -fix some codestyle.
>
> Changes since v1:
> -implement irq_wake handler.
> ---
>
>  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
> ++-
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
>  3 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
> b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> index d0c811d..ad27184 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
> .driver = {
> .name = "mediatek-mt8173-pinctrl",
> .of_match_table = mt8173_pctrl_match,
> +   .pm = _eint_pm_ops,
> },
>  };
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index ad1ea16..fe34ce9 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include "../core.h"
> @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
> return 0;
>  }
>
> +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +   struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> +   int shift = d->hwirq & 0x1f;
> +   int reg = d->hwirq >> 5;
> +
> +   if (on)
> +   pctl->wake_mask[reg] |= BIT(shift);
> +   else
> +   pctl->wake_mask[reg] &= ~BIT(shift);
> +
> +   return 0;
> +}
> +
> +static void mtk_eint_chip_write_mask(const struct mtk_eint_offsets *chip,
> +   void __iomem *eint_reg_base, u32 *buf)
> +{
> +   int port;
> +   void __iomem *reg;
> +
> +   for (port = 0; port < chip->ports; port++) {
> +   reg = eint_reg_base + (port << 2);
> +   writel_relaxed(~buf[port], reg + chip->mask_set);
> +   writel_relaxed(buf[port], reg + chip->mask_clr);
> +   }
> +}
> +
> +static void mtk_eint_chip_read_mask(const struct mtk_eint_offsets *chip,
> +   void __iomem *eint_reg_base, u32 *buf)
> +{
> +   int port;
> +   void __iomem *reg;
> +
> +   for (port = 0; port < chip->ports; port++) {
> +   reg = eint_reg_base + chip->mask + (port << 2);
> +   buf[port] = ~readl_relaxed(reg);
> +   /* Mask is 0 when irq is enabled, and 1 when disabled. */
> +   }
> +}
> +
> +static int mtk_eint_suspend(struct device *device)
> +{
> +   void __iomem *reg;
> +   struct mtk_pinctrl *pctl = dev_get_drvdata(device);
> +   const struct mtk_eint_offsets *eint_offsets =
> +   >devdata->eint_offsets;
> +
> +   reg = pctl->eint_reg_base;
> +   mtk_eint_chip_read_mask(eint_offsets, reg, pctl->cur_mask);
> +   mtk_eint_chip_write_mask(eint_offsets, reg, pctl->wake_mask);
> +
> +   return 0;
> +}
> +
> +static int mtk_eint_resume(struct device *device)
> +{
> +   struct mtk_pinctrl *pctl = dev_get_drvdata(device);
> +   const struct mtk_eint_offsets *eint_offsets =
> +   >devdata->eint_offsets;
> +
> +   mtk_eint_chip_write_mask(eint_offsets,
> +   pctl->eint_reg_base, pctl->cur_mask);
> +
> +   return 0;
> +}
> +
> +const struct dev_pm_ops mtk_eint_pm_ops = {
> +   .suspend = mtk_eint_suspend,
> +   .resume = mtk_eint_resume,
> +};
> +
>  static void mtk_eint_ack(struct irq_data *d)
>  {
> struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> @@ -1076,10 +1148,12 @@ static void mtk_eint_ack(struct irq_data *d)
>
>  static struct irq_chip mtk_pinctrl_irq_chip = {
> .name = "mt-eint",
> +   .irq_disable = mtk_eint_mask,
> .irq_mask = mtk_eint_mask,
> .irq_unmask = mtk_eint_unmask,
> .irq_ack = mtk_eint_ack,
> .irq_set_type = mtk_eint_set_type,
> +   .irq_set_wake = mtk_eint_irq_set_wake,
> .irq_request_resources = mtk_pinctrl_irq_request_resources,
> .irq_release_resources = mtk_pinctrl_irq_release_resources,
>  };
> @@ -1217,7 +1291,7 @@ int mtk_pctrl_init(struct platform_device *pdev,
> struct device_node *np = pdev->dev.of_node, *node;
> struct property *prop;
> struct resource *res;
> -   int i, ret, irq;
> +   int 

Re: [PATCH v4] pinctrl: mediatek: Implement wake handler and suspend resume

[Daniel Kurtz]

On Fri, Aug 14, 2015 at 4:38 PM, maoguang.m...@mediatek.com wrote:

 From: Maoguang Meng maoguang.m...@mediatek.com

 This patch implement irq_set_wake to get who is wakeup source and
 setup on suspend resume.

 Signed-off-by: Maoguang Meng maoguang.m...@mediatek.com

Reviewed-by: Daniel Kurtz djku...@chromium.org

Thanks!
-Dan


 ---
 changes since v3:
 -add a comment in mtk_eint_chip_read_mask.
 -delete ALIGN when allocate eint_offsets.ports.
 -fix unrelated change.

 changes since v2:
 -modify irq_wake to handle irq wakeup source.
 -allocate two buffers separately.
 -fix some codestyle.

 Changes since v1:
 -implement irq_wake handler.
 ---

  drivers/pinctrl/mediatek/pinctrl-mt8173.c |  1 +
  drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 91 
 ++-
  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |  4 ++
  3 files changed, 95 insertions(+), 1 deletion(-)

 diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c 
 b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
 index d0c811d..ad27184 100644
 --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
 +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
 @@ -385,6 +385,7 @@ static struct platform_driver mtk_pinctrl_driver = {
 .driver = {
 .name = mediatek-mt8173-pinctrl,
 .of_match_table = mt8173_pctrl_match,
 +   .pm = mtk_eint_pm_ops,
 },
  };

 diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
 b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
 index ad1ea16..fe34ce9 100644
 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
 +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
 @@ -33,6 +33,7 @@
  #include linux/mfd/syscon.h
  #include linux/delay.h
  #include linux/interrupt.h
 +#include linux/pm.h
  #include dt-bindings/pinctrl/mt65xx.h

  #include ../core.h
 @@ -1062,6 +1063,77 @@ static int mtk_eint_set_type(struct irq_data *d,
 return 0;
  }

 +static int mtk_eint_irq_set_wake(struct irq_data *d, unsigned int on)
 +{
 +   struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
 +   int shift = d-hwirq  0x1f;
 +   int reg = d-hwirq  5;
 +
 +   if (on)
 +   pctl-wake_mask[reg] |= BIT(shift);
 +   else
 +   pctl-wake_mask[reg] = ~BIT(shift);
 +
 +   return 0;
 +}
 +
 +static void mtk_eint_chip_write_mask(const struct mtk_eint_offsets *chip,
 +   void __iomem *eint_reg_base, u32 *buf)
 +{
 +   int port;
 +   void __iomem *reg;
 +
 +   for (port = 0; port  chip-ports; port++) {
 +   reg = eint_reg_base + (port  2);
 +   writel_relaxed(~buf[port], reg + chip-mask_set);
 +   writel_relaxed(buf[port], reg + chip-mask_clr);
 +   }
 +}
 +
 +static void mtk_eint_chip_read_mask(const struct mtk_eint_offsets *chip,
 +   void __iomem *eint_reg_base, u32 *buf)
 +{
 +   int port;
 +   void __iomem *reg;
 +
 +   for (port = 0; port  chip-ports; port++) {
 +   reg = eint_reg_base + chip-mask + (port  2);
 +   buf[port] = ~readl_relaxed(reg);
 +   /* Mask is 0 when irq is enabled, and 1 when disabled. */
 +   }
 +}
 +
 +static int mtk_eint_suspend(struct device *device)
 +{
 +   void __iomem *reg;
 +   struct mtk_pinctrl *pctl = dev_get_drvdata(device);
 +   const struct mtk_eint_offsets *eint_offsets =
 +   pctl-devdata-eint_offsets;
 +
 +   reg = pctl-eint_reg_base;
 +   mtk_eint_chip_read_mask(eint_offsets, reg, pctl-cur_mask);
 +   mtk_eint_chip_write_mask(eint_offsets, reg, pctl-wake_mask);
 +
 +   return 0;
 +}
 +
 +static int mtk_eint_resume(struct device *device)
 +{
 +   struct mtk_pinctrl *pctl = dev_get_drvdata(device);
 +   const struct mtk_eint_offsets *eint_offsets =
 +   pctl-devdata-eint_offsets;
 +
 +   mtk_eint_chip_write_mask(eint_offsets,
 +   pctl-eint_reg_base, pctl-cur_mask);
 +
 +   return 0;
 +}
 +
 +const struct dev_pm_ops mtk_eint_pm_ops = {
 +   .suspend = mtk_eint_suspend,
 +   .resume = mtk_eint_resume,
 +};
 +
  static void mtk_eint_ack(struct irq_data *d)
  {
 struct mtk_pinctrl *pctl = irq_data_get_irq_chip_data(d);
 @@ -1076,10 +1148,12 @@ static void mtk_eint_ack(struct irq_data *d)

  static struct irq_chip mtk_pinctrl_irq_chip = {
 .name = mt-eint,
 +   .irq_disable = mtk_eint_mask,
 .irq_mask = mtk_eint_mask,
 .irq_unmask = mtk_eint_unmask,
 .irq_ack = mtk_eint_ack,
 .irq_set_type = mtk_eint_set_type,
 +   .irq_set_wake = mtk_eint_irq_set_wake,
 .irq_request_resources = mtk_pinctrl_irq_request_resources,
 .irq_release_resources = mtk_pinctrl_irq_release_resources,
  };
 @@ -1217,7 +1291,7 @@ int mtk_pctrl_init(struct platform_device *pdev,
 struct device_node *np = pdev-dev.of_node, *node;
 struct property *prop;
 struct resource *res;
 -   int i, ret, irq;
 +