| Message ID | 1560764090-22740-1-git-send-email-neeraju@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pinctrl: qcom: Add irq_enable callback for msm gpio | expand |
On Mon, Jun 17, 2019 at 11:35 AM Neeraj Upadhyay <neeraju@codeaurora.org> wrote: > From: Srinivas Ramana <sramana@codeaurora.org> > > Introduce the irq_enable callback which will be same as irq_unmask > except that it will also clear the status bit before unmask. > > This will help in clearing any erroneous interrupts that would > have got latched when the interrupt is not in use. > > There may be devices like UART which can use the same gpio line > for data rx as well as a wakeup gpio when in suspend. The data that > was flowing on the line may latch the interrupt and when we enable > the interrupt before going to suspend, this would trigger the > unexpected interrupt. This change helps clearing the interrupt > so that these unexpected interrupts gets cleared. > > Signed-off-by: Srinivas Ramana <sramana@codeaurora.org> > Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> Overall this looks good to me, waiting for Bjorn's review. > Changes since v1: > - Extracted common code into __msm_gpio_irq_unmask(). Please don't name functions __like __that. > -static void msm_gpio_irq_unmask(struct irq_data *d) > +static void __msm_gpio_irq_unmask(struct irq_data *d, bool status_clear) Instead of __unclear __underscore __semantic use something really descriptive like static void msm_gpio_irq_clear_irq() That is what it does, right? Other than that it looks fine. Yours, Linus Walleij
On 6/25/19 2:28 PM, Linus Walleij wrote: > On Mon, Jun 17, 2019 at 11:35 AM Neeraj Upadhyay <neeraju@codeaurora.org> wrote: > >> From: Srinivas Ramana <sramana@codeaurora.org> >> >> Introduce the irq_enable callback which will be same as irq_unmask >> except that it will also clear the status bit before unmask. >> >> This will help in clearing any erroneous interrupts that would >> have got latched when the interrupt is not in use. >> >> There may be devices like UART which can use the same gpio line >> for data rx as well as a wakeup gpio when in suspend. The data that >> was flowing on the line may latch the interrupt and when we enable >> the interrupt before going to suspend, this would trigger the >> unexpected interrupt. This change helps clearing the interrupt >> so that these unexpected interrupts gets cleared. >> >> Signed-off-by: Srinivas Ramana <sramana@codeaurora.org> >> Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> > Overall this looks good to me, waiting for Bjorn's review. > >> Changes since v1: >> - Extracted common code into __msm_gpio_irq_unmask(). > Please don't name functions __like __that. > >> -static void msm_gpio_irq_unmask(struct irq_data *d) >> +static void __msm_gpio_irq_unmask(struct irq_data *d, bool status_clear) > Instead of __unclear __underscore __semantic use something > really descriptive like > > static void msm_gpio_irq_clear_irq() > > That is what it does, right? Is below ok? as it clears (if status_clear set) and then unmasks irq static void msm_gpio_irq_clear_unmask() > > Other than that it looks fine. > > Yours, > Linus Walleij
On Tue, Jun 25, 2019 at 12:29 PM Neeraj Upadhyay <neeraju@codeaurora.org> wrote: > On 6/25/19 2:28 PM, Linus Walleij wrote: > > Please don't name functions __like __that. > > > >> -static void msm_gpio_irq_unmask(struct irq_data *d) > >> +static void __msm_gpio_irq_unmask(struct irq_data *d, bool status_clear) > > Instead of __unclear __underscore __semantic use something > > really descriptive like > > > > static void msm_gpio_irq_clear_irq() > > > > That is what it does, right? > > Is below ok? as it clears (if status_clear set) and then unmasks irq > > static void msm_gpio_irq_clear_unmask() Sure thing! Thanks. Yours, Linus Walleij
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 6e319bc..2a127f0 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -729,7 +729,7 @@ static void msm_gpio_irq_mask(struct irq_data *d) raw_spin_unlock_irqrestore(&pctrl->lock, flags); } -static void msm_gpio_irq_unmask(struct irq_data *d) +static void __msm_gpio_irq_unmask(struct irq_data *d, bool status_clear) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct msm_pinctrl *pctrl = gpiochip_get_data(gc); @@ -741,6 +741,17 @@ static void msm_gpio_irq_unmask(struct irq_data *d) raw_spin_lock_irqsave(&pctrl->lock, flags); + if (status_clear) { + /* + * clear the interrupt status bit before unmask to avoid + * any erroneous interrupts that would have got latched + * when the interrupt is not in use. + */ + val = msm_readl_intr_status(pctrl, g); + val &= ~BIT(g->intr_status_bit); + msm_writel_intr_status(val, pctrl, g); + } + val = msm_readl_intr_cfg(pctrl, g); val |= BIT(g->intr_raw_status_bit); val |= BIT(g->intr_enable_bit); @@ -751,6 +762,17 @@ static void msm_gpio_irq_unmask(struct irq_data *d) raw_spin_unlock_irqrestore(&pctrl->lock, flags); } +static void msm_gpio_irq_enable(struct irq_data *d) +{ + + __msm_gpio_irq_unmask(d, true); +} + +static void msm_gpio_irq_unmask(struct irq_data *d) +{ + __msm_gpio_irq_unmask(d, false); +} + static void msm_gpio_irq_ack(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); @@ -978,6 +1000,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl); pctrl->irq_chip.name = "msmgpio"; + pctrl->irq_chip.irq_enable = msm_gpio_irq_enable; pctrl->irq_chip.irq_mask = msm_gpio_irq_mask; pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask; pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;