Messages in this thread | | | Date | Tue, 24 Nov 2020 11:15:10 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs |
| |
On 2020-11-24 10:37, Maulik Shah wrote:
[...]
>> static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, >> unsigned function, >> unsigned group) >> { >> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); >> + struct gpio_chip *gc = &pctrl->chip; >> + unsigned int irq = irq_find_mapping(gc->irq.domain, group); >> const struct msm_pingroup *g; >> unsigned long flags; >> u32 val, mask; >> + u32 oldval; >> + u32 old_i; >> int i; >> g = &pctrl->soc->groups[group]; >> @@ -187,15 +215,26 @@ static int msm_pinmux_set_mux(struct pinctrl_dev >> *pctldev, >> if (WARN_ON(i == g->nfuncs)) >> return -EINVAL; >> - raw_spin_lock_irqsave(&pctrl->lock, flags); >> + disable_irq(irq); >> - val = msm_readl_ctl(pctrl, g); >> + raw_spin_lock_irqsave(&pctrl->lock, flags); >> + oldval = val = msm_readl_ctl(pctrl, g); >> val &= ~mask; >> val |= i << g->mux_bit; >> msm_writel_ctl(val, pctrl, g); >> - >> raw_spin_unlock_irqrestore(&pctrl->lock, flags); >> + /* >> + * Clear IRQs if switching to/from GPIO mode since muxing to/from >> + * the GPIO path can cause phantom edges. >> + */ >> + old_i = (oldval & mask) >> g->mux_bit; >> + if (old_i != i && >> + (i == pctrl->soc->gpio_func || old_i == pctrl->soc->gpio_func)) >> + msm_pinctrl_clear_pending_irq(pctrl, group, irq); > > disable_irq() and enable_irq() should be moved inside this if loop. as > only use for this is to mask the IRQ when switching back to gpio IRQ > mode? > > i also don't think we should leave IRQ enabled at the end of this > function by default, probably need to check if IRQ was already > unmasked before disabling it, then only call enable_irq().
Why? It looks to me that this reproduces the behaviour of IRQCHIP_SET_TYPE_MASKED, which is highly desirable. What problem are you trying to address with this?
> >> + >> + enable_irq(irq); >> + >> return 0; >> } >> @@ -456,32 +495,45 @@ static const struct pinconf_ops >> msm_pinconf_ops = { >> static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned >> offset) >> { >> const struct msm_pingroup *g; >> + unsigned int irq = irq_find_mapping(chip->irq.domain, offset); >> struct msm_pinctrl *pctrl = gpiochip_get_data(chip); >> unsigned long flags; >> + u32 oldval; >> u32 val; >> g = &pctrl->soc->groups[offset]; >> + disable_irq(irq); >> + >> raw_spin_lock_irqsave(&pctrl->lock, flags); >> - val = msm_readl_ctl(pctrl, g); >> + oldval = val = msm_readl_ctl(pctrl, g); >> val &= ~BIT(g->oe_bit); >> msm_writel_ctl(val, pctrl, g); >> raw_spin_unlock_irqrestore(&pctrl->lock, flags); >> + if (oldval != val) >> + msm_pinctrl_clear_pending_irq(pctrl, offset, irq); >> + >> + enable_irq(irq); > > i do not think we need disable_irq() and enable_irq() here, changing > direction to input does not mean its being used for interrupt only, it > may be set to use something like Rx mode in UART. > > the client driver should enable IRQ when needed.
And the kernel doesn't expect random interrupts to fire. Again, what are you trying to fix by removing these?
M. -- Jazz is not dead. It just smells funny...
| |