* [PATCH] pinctrl: intel: Clear interrupt status in unmask callback
@ 2019-04-22 4:45 Kai-Heng Feng
[not found] ` <Ld5HCy5--3-1@tutanota.com>
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Kai-Heng Feng @ 2019-04-22 4:45 UTC (permalink / raw)
To: mika.westerberg, andriy.shevchenko
Cc: linus.walleij, hotwater438, hdegoede, linux-gpio, linux-kernel,
Kai-Heng Feng
Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was
added because clearing interrupt status bit is required to avoid
unexpected behavior.
Turns out the unmask callback also needs the fix, which can solve weird
IRQ triggering issues on I2C touchpad ELAN1200.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
drivers/pinctrl/intel/pinctrl-intel.c | 35 ++++-----------------------
1 file changed, 5 insertions(+), 30 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 3b1818184207..53878604537e 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -913,35 +913,6 @@ static void intel_gpio_irq_ack(struct irq_data *d)
}
}
-static void intel_gpio_irq_enable(struct irq_data *d)
-{
- struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
- struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
- const struct intel_community *community;
- const struct intel_padgroup *padgrp;
- int pin;
-
- pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), &community, &padgrp);
- if (pin >= 0) {
- unsigned int gpp, gpp_offset, is_offset;
- unsigned long flags;
- u32 value;
-
- gpp = padgrp->reg_num;
- gpp_offset = padgroup_offset(padgrp, pin);
- is_offset = community->is_offset + gpp * 4;
-
- raw_spin_lock_irqsave(&pctrl->lock, flags);
- /* Clear interrupt status first to avoid unexpected interrupt */
- writel(BIT(gpp_offset), community->regs + is_offset);
-
- value = readl(community->regs + community->ie_offset + gpp * 4);
- value |= BIT(gpp_offset);
- writel(value, community->regs + community->ie_offset + gpp * 4);
- raw_spin_unlock_irqrestore(&pctrl->lock, flags);
- }
-}
-
static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -963,6 +934,11 @@ static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
reg = community->regs + community->ie_offset + gpp * 4;
raw_spin_lock_irqsave(&pctrl->lock, flags);
+
+ /* Clear interrupt status first to avoid unexpected interrupt */
+ if (!mask)
+ writel(BIT(gpp_offset), community->regs + community->is_offset + gpp * 4);
+
value = readl(reg);
if (mask)
value &= ~BIT(gpp_offset);
@@ -1106,7 +1082,6 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
static struct irq_chip intel_gpio_irqchip = {
.name = "intel-gpio",
- .irq_enable = intel_gpio_irq_enable,
.irq_ack = intel_gpio_irq_ack,
.irq_mask = intel_gpio_irq_mask,
.irq_unmask = intel_gpio_irq_unmask,
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
[parent not found: <Ld5HCy5--3-1@tutanota.com>]
* Re: [PATCH] pinctrl: intel: Clear interrupt status in unmask callback
[not found] ` <Ld5HCy5--3-1@tutanota.com>
@ 2019-04-23 4:57 ` Kai-Heng Feng
[not found] ` <Ld8QGex--3-1@tutanota.com>
0 siblings, 1 reply; 9+ messages in thread
From: Kai-Heng Feng @ 2019-04-23 4:57 UTC (permalink / raw)
To: hotwater438
Cc: Mika Westerberg, Andy Shevchenko, linus.walleij, Hdegoede,
linux-gpio, Linux Kernel
Hi,
at 02:22, <hotwater438@tutanota.com> <hotwater438@tutanota.com> wrote:
> Hi.
> I've just applied this patch, and touchpad woorks smoothly, but suspend
> issue is still present.
> After suspend, i2c_hid module bursts i2c_hid i2c-ELAN1200:00:
> i2c_hid_get_input: incomplete report (16/65535) messages (more than 50
> reports/sec).
> In dmesg I can see a frequency of reporting every 0.0007 - 0.001 dmesg
> time units.
What’s the default suspend mode on the platform?
This is a common issue for system that defaults to Suspend-to-idle, but S3
is in use.
The root cause is that the power of the touchpad doesn’t get cut off during
S3 by platform firmware.
Do you also see this issue if S2I is in use?
Kai-Heng
>
> Though I can sucessfully restart module and after restarting it works as
> good as it was.
>
> So suspend issue is still present.
>
> Regards,
> Vladislav.
>
>
> Apr 22, 2019, 7:45 AM by kai.heng.feng@canonical.com:
> Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was
> added because clearing interrupt status bit is required to avoid
> unexpected behavior.
>
> Turns out the unmask callback also needs the fix, which can solve weird
> IRQ triggering issues on I2C touchpad ELAN1200.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> drivers/pinctrl/intel/pinctrl-intel.c | 35 ++++-----------------------
> 1 file changed, 5 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
> b/drivers/pinctrl/intel/pinctrl-intel.c
> index 3b1818184207..53878604537e 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -913,35 +913,6 @@ static void intel_gpio_irq_ack(struct irq_data *d)
> }
> }
>
> -static void intel_gpio_irq_enable(struct irq_data *d)
> -{
> - struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> - struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> - const struct intel_community *community;
> - const struct intel_padgroup *padgrp;
> - int pin;
> -
> - pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), &community, &padgrp);
> - if (pin >= 0) {
> - unsigned int gpp, gpp_offset, is_offset;
> - unsigned long flags;
> - u32 value;
> -
> - gpp = padgrp->reg_num;
> - gpp_offset = padgroup_offset(padgrp, pin);
> - is_offset = community->is_offset + gpp * 4;
> -
> - raw_spin_lock_irqsave(&pctrl->lock, flags);
> - /* Clear interrupt status first to avoid unexpected interrupt */
> - writel(BIT(gpp_offset), community->regs + is_offset);
> -
> - value = readl(community->regs + community->ie_offset + gpp * 4);
> - value |= BIT(gpp_offset);
> - writel(value, community->regs + community->ie_offset + gpp * 4);
> - raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> - }
> -}
> -
> static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -963,6 +934,11 @@ static void intel_gpio_irq_mask_unmask(struct
> irq_data *d, bool mask)
> reg = community->regs + community->ie_offset + gpp * 4;
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
> +
> + /* Clear interrupt status first to avoid unexpected interrupt */
> + if (!mask)
> + writel(BIT(gpp_offset), community->regs + community->is_offset +
> gpp * 4);
> +
> value = readl(reg);
> if (mask)
> value &= ~BIT(gpp_offset);
> @@ -1106,7 +1082,6 @@ static irqreturn_t intel_gpio_irq(int irq, void
> *data)
>
> static struct irq_chip intel_gpio_irqchip = {
> .name = "intel-gpio",
> - .irq_enable = intel_gpio_irq_enable,
> .irq_ack = intel_gpio_irq_ack,
> .irq_mask = intel_gpio_irq_mask,
> .irq_unmask = intel_gpio_irq_unmask,
> --
> 2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pinctrl: intel: Clear interrupt status in unmask callback
2019-04-22 4:45 [PATCH] pinctrl: intel: Clear interrupt status in unmask callback Kai-Heng Feng
[not found] ` <Ld5HCy5--3-1@tutanota.com>
@ 2019-04-25 9:11 ` Mika Westerberg
2019-04-26 21:47 ` Andy Shevchenko
2 siblings, 0 replies; 9+ messages in thread
From: Mika Westerberg @ 2019-04-25 9:11 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: andriy.shevchenko, linus.walleij, hotwater438, hdegoede,
linux-gpio, linux-kernel
On Mon, Apr 22, 2019 at 12:45:39PM +0800, Kai-Heng Feng wrote:
> Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was
> added because clearing interrupt status bit is required to avoid
> unexpected behavior.
>
> Turns out the unmask callback also needs the fix, which can solve weird
> IRQ triggering issues on I2C touchpad ELAN1200.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pinctrl: intel: Clear interrupt status in unmask callback
2019-04-22 4:45 [PATCH] pinctrl: intel: Clear interrupt status in unmask callback Kai-Heng Feng
[not found] ` <Ld5HCy5--3-1@tutanota.com>
2019-04-25 9:11 ` Mika Westerberg
@ 2019-04-26 21:47 ` Andy Shevchenko
2019-04-29 9:16 ` Kai-Heng Feng
2 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2019-04-26 21:47 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: mika.westerberg, linus.walleij, hotwater438, hdegoede,
linux-gpio, linux-kernel
On Mon, Apr 22, 2019 at 12:45:39PM +0800, Kai-Heng Feng wrote:
> Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was
> added because clearing interrupt status bit is required to avoid
> unexpected behavior.
>
> Turns out the unmask callback also needs the fix, which can solve weird
> IRQ triggering issues on I2C touchpad ELAN1200.
> -static void intel_gpio_irq_enable(struct irq_data *d)
> -{
> - struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> - struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> - const struct intel_community *community;
> - const struct intel_padgroup *padgrp;
> - int pin;
> -
> - pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), &community, &padgrp);
> - if (pin >= 0) {
> - unsigned int gpp, gpp_offset, is_offset;
> - unsigned long flags;
> - u32 value;
> -
> - gpp = padgrp->reg_num;
> - gpp_offset = padgroup_offset(padgrp, pin);
> - is_offset = community->is_offset + gpp * 4;
> -
> - raw_spin_lock_irqsave(&pctrl->lock, flags);
> - /* Clear interrupt status first to avoid unexpected interrupt */
> - writel(BIT(gpp_offset), community->regs + is_offset);
> -
> - value = readl(community->regs + community->ie_offset + gpp * 4);
> - value |= BIT(gpp_offset);
> - writel(value, community->regs + community->ie_offset + gpp * 4);
> - raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> - }
> -}
> -
> static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -963,6 +934,11 @@ static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
> reg = community->regs + community->ie_offset + gpp * 4;
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
> +
> + /* Clear interrupt status first to avoid unexpected interrupt */
> + if (!mask)
Can we do this unconditionally?
> + writel(BIT(gpp_offset), community->regs + community->is_offset + gpp * 4);
I would rather prefer to follow the below pattern, like
reg = ...;
writel(..., reg);
or, to decrease calculus under spin lock, something like
reg = ->regs + gpp * 4;
writel(..., reg + is_offset);
readl(reg + ie_offset);
etc.
> +
> value = readl(reg);
> if (mask)
> value &= ~BIT(gpp_offset);
> @@ -1106,7 +1082,6 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
>
> static struct irq_chip intel_gpio_irqchip = {
> .name = "intel-gpio",
> - .irq_enable = intel_gpio_irq_enable,
Is it possible scenario when IRQ enable is called, but not masking callbacks?
For _AEI or GPE?
> .irq_ack = intel_gpio_irq_ack,
> .irq_mask = intel_gpio_irq_mask,
> .irq_unmask = intel_gpio_irq_unmask,
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pinctrl: intel: Clear interrupt status in unmask callback
2019-04-26 21:47 ` Andy Shevchenko
@ 2019-04-29 9:16 ` Kai-Heng Feng
2019-04-29 13:13 ` Andy Shevchenko
0 siblings, 1 reply; 9+ messages in thread
From: Kai-Heng Feng @ 2019-04-29 9:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: mika.westerberg, linus.walleij, hotwater438, hdegoede,
linux-gpio, linux-kernel
at 05:47, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Apr 22, 2019 at 12:45:39PM +0800, Kai-Heng Feng wrote:
>> Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was
>> added because clearing interrupt status bit is required to avoid
>> unexpected behavior.
>>
>> Turns out the unmask callback also needs the fix, which can solve weird
>> IRQ triggering issues on I2C touchpad ELAN1200.
>
>> -static void intel_gpio_irq_enable(struct irq_data *d)
>> -{
>> - struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> - struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
>> - const struct intel_community *community;
>> - const struct intel_padgroup *padgrp;
>> - int pin;
>> -
>> - pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), &community, &padgrp);
>> - if (pin >= 0) {
>> - unsigned int gpp, gpp_offset, is_offset;
>> - unsigned long flags;
>> - u32 value;
>> -
>> - gpp = padgrp->reg_num;
>> - gpp_offset = padgroup_offset(padgrp, pin);
>> - is_offset = community->is_offset + gpp * 4;
>> -
>> - raw_spin_lock_irqsave(&pctrl->lock, flags);
>> - /* Clear interrupt status first to avoid unexpected interrupt */
>> - writel(BIT(gpp_offset), community->regs + is_offset);
>> -
>> - value = readl(community->regs + community->ie_offset + gpp * 4);
>> - value |= BIT(gpp_offset);
>> - writel(value, community->regs + community->ie_offset + gpp * 4);
>> - raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>> - }
>> -}
>> -
>> static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
>> {
>> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> @@ -963,6 +934,11 @@ static void intel_gpio_irq_mask_unmask(struct
>> irq_data *d, bool mask)
>> reg = community->regs + community->ie_offset + gpp * 4;
>>
>> raw_spin_lock_irqsave(&pctrl->lock, flags);
>> +
>> + /* Clear interrupt status first to avoid unexpected interrupt */
>
>> + if (!mask)
>
> Can we do this unconditionally?
Yes I think so.
>
>> + writel(BIT(gpp_offset), community->regs +
>> community->is_offset + gpp * 4);
>
> I would rather prefer to follow the below pattern, like
>
> reg = ...;
> writel(..., reg);
>
> or, to decrease calculus under spin lock, something like
>
> reg = ->regs + gpp * 4;
>
> writel(..., reg + is_offset);
>
> readl(reg + ie_offset);
>
> etc.
Ok, will do.
>
>> +
>> value = readl(reg);
>> if (mask)
>> value &= ~BIT(gpp_offset);
>> @@ -1106,7 +1082,6 @@ static irqreturn_t intel_gpio_irq(int irq, void
>> *data)
>>
>> static struct irq_chip intel_gpio_irqchip = {
>> .name = "intel-gpio",
>
>> - .irq_enable = intel_gpio_irq_enable,
>
> Is it possible scenario when IRQ enable is called, but not masking
> callbacks?
> For _AEI or GPE?
I am unfamiliar with both of them, what are the callbacks to be used for
_AEI and GPE case?
Seems like both gpiolib and irqchip call irq_unmask() when irq_enable() is
absent.
Kai-Heng
>
>> .irq_ack = intel_gpio_irq_ack,
>> .irq_mask = intel_gpio_irq_mask,
>> .irq_unmask = intel_gpio_irq_unmask,
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pinctrl: intel: Clear interrupt status in unmask callback
2019-04-29 9:16 ` Kai-Heng Feng
@ 2019-04-29 13:13 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2019-04-29 13:13 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: mika.westerberg, linus.walleij, hotwater438, hdegoede,
linux-gpio, linux-kernel
On Mon, Apr 29, 2019 at 05:16:16PM +0800, Kai-Heng Feng wrote:
> at 05:47, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Apr 22, 2019 at 12:45:39PM +0800, Kai-Heng Feng wrote:
> > > Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was
> > > added because clearing interrupt status bit is required to avoid
> > > unexpected behavior.
> > >
> > > Turns out the unmask callback also needs the fix, which can solve weird
> > > IRQ triggering issues on I2C touchpad ELAN1200.
> > Is it possible scenario when IRQ enable is called, but not masking
> > callbacks?
> > For _AEI or GPE?
>
> I am unfamiliar with both of them, what are the callbacks to be used for
> _AEI and GPE case?
> Seems like both gpiolib and irqchip call irq_unmask() when irq_enable() is
> absent.
Yes, that's correct, thank you for double checking.
* @irq_enable: enable the interrupt (defaults to chip->unmask if NULL)
Wait for v2 with mentioned earlier changes and gathered tags.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-04-29 13:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-22 4:45 [PATCH] pinctrl: intel: Clear interrupt status in unmask callback Kai-Heng Feng
[not found] ` <Ld5HCy5--3-1@tutanota.com>
2019-04-23 4:57 ` Kai-Heng Feng
[not found] ` <Ld8QGex--3-1@tutanota.com>
2019-04-23 9:08 ` Mika Westerberg
[not found] ` <Ld8ZLG1--3-1@tutanota.com>
2019-04-23 9:47 ` Kai-Heng Feng
[not found] ` <Ld9SLAo--3-1@tutanota.com>
2019-04-25 5:16 ` Kai Heng Feng
2019-04-25 9:11 ` Mika Westerberg
2019-04-26 21:47 ` Andy Shevchenko
2019-04-29 9:16 ` Kai-Heng Feng
2019-04-29 13:13 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).