2021-01-25 03:19:18

by mtk15103

[permalink] [raw]
Subject: [PATCH] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt

When flipping the polarity will be generated interrupt under certain
circumstances, but GPIO external signal has not changed.
Then, mask the interrupt before polarity setting, and clear the
unexpected interrupt after trigger type setting completed.

Signed-off-by: Hailong Fan <[email protected]>
---
[V2]
---
drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c
index 22736f60c16c..0042f32c7e7e 100644
--- a/drivers/pinctrl/mediatek/mtk-eint.c
+++ b/drivers/pinctrl/mediatek/mtk-eint.c
@@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d)
static int mtk_eint_set_type(struct irq_data *d, unsigned int type)
{
struct mtk_eint *eint = irq_data_get_irq_chip_data(d);
+ bool unmasked;
u32 mask = BIT(d->hwirq & 0x1f);
void __iomem *reg;

@@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type)
else
eint->dual_edge[d->hwirq] = 0;

+ if (!mtk_eint_get_mask(eint, d->hwirq)) {
+ mtk_eint_mask(d);
+ unmasked = true;
+ } else {
+ unmasked = false;
+ }
+
if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) {
reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr);
writel(mask, reg);
@@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type)
writel(mask, reg);
}

- if (eint->dual_edge[d->hwirq])
- mtk_eint_flip_edge(eint, d->hwirq);
+ mtk_eint_ack(d);
+ if (unmasked)
+ mtk_eint_unmask(d);

return 0;
}
--
2.18.0


2021-01-25 03:26:11

by Nicolas Boichat

[permalink] [raw]
Subject: Re: [PATCH] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt

On Mon, Jan 25, 2021 at 11:15 AM Hailong Fan <[email protected]> wrote:
>
> When flipping the polarity will be generated interrupt under certain
> circumstances, but GPIO external signal has not changed.
> Then, mask the interrupt before polarity setting, and clear the
> unexpected interrupt after trigger type setting completed.

I'd add a short note about why you remove mtk_eint_flip_edge, that is,
because mtk_eint_unmask already calls it.

>
> Signed-off-by: Hailong Fan <[email protected]>
> ---
> [V2]
> ---
> drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c
> index 22736f60c16c..0042f32c7e7e 100644
> --- a/drivers/pinctrl/mediatek/mtk-eint.c
> +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d)
> static int mtk_eint_set_type(struct irq_data *d, unsigned int type)
> {
> struct mtk_eint *eint = irq_data_get_irq_chip_data(d);
> + bool unmasked;

Well, this is true if the interrupt has been masked (or, equivalently,
if we need to unmask it later).

So I think either "masked" or "unmask" are better as variable names.

> u32 mask = BIT(d->hwirq & 0x1f);
> void __iomem *reg;
>
> @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type)
> else
> eint->dual_edge[d->hwirq] = 0;
>
> + if (!mtk_eint_get_mask(eint, d->hwirq)) {
> + mtk_eint_mask(d);
> + unmasked = true;
> + } else {
> + unmasked = false;
> + }
> +
> if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) {
> reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr);
> writel(mask, reg);
> @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type)
> writel(mask, reg);
> }
>
> - if (eint->dual_edge[d->hwirq])
> - mtk_eint_flip_edge(eint, d->hwirq);
> + mtk_eint_ack(d);
> + if (unmasked)
> + mtk_eint_unmask(d);
>
> return 0;
> }
> --
> 2.18.0

2021-01-25 04:22:38

by mtk15103

[permalink] [raw]
Subject: Re: [PATCH] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt

On Mon, 2021-01-25 at 11:24 +0800, Nicolas Boichat wrote:
> On Mon, Jan 25, 2021 at 11:15 AM Hailong Fan <[email protected]> wrote:
> >
> > When flipping the polarity will be generated interrupt under certain
> > circumstances, but GPIO external signal has not changed.
> > Then, mask the interrupt before polarity setting, and clear the
> > unexpected interrupt after trigger type setting completed.
>
> I'd add a short note about why you remove mtk_eint_flip_edge, that is,
> because mtk_eint_unmask already calls it.
Add it on V3, thx.

> >
> > Signed-off-by: Hailong Fan <[email protected]>
> > ---
> > [V2]
> > ---
> > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c
> > index 22736f60c16c..0042f32c7e7e 100644
> > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d)
> > static int mtk_eint_set_type(struct irq_data *d, unsigned int type)
> > {
> > struct mtk_eint *eint = irq_data_get_irq_chip_data(d);
> > + bool unmasked;
>
> Well, this is true if the interrupt has been masked (or, equivalently,
> if we need to unmask it later).
>
> So I think either "masked" or "unmask" are better as variable names.
Yes, masked is better.
>
> > u32 mask = BIT(d->hwirq & 0x1f);
> > void __iomem *reg;
> >
> > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type)
> > else
> > eint->dual_edge[d->hwirq] = 0;
> >
> > + if (!mtk_eint_get_mask(eint, d->hwirq)) {
> > + mtk_eint_mask(d);
> > + unmasked = true;
> > + } else {
> > + unmasked = false;
> > + }
> > +
> > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) {
> > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr);
> > writel(mask, reg);
> > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type)
> > writel(mask, reg);
> > }
> >
> > - if (eint->dual_edge[d->hwirq])
> > - mtk_eint_flip_edge(eint, d->hwirq);
> > + mtk_eint_ack(d);
> > + if (unmasked)
> > + mtk_eint_unmask(d);
> >
> > return 0;
> > }
> > --
> > 2.18.0