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...
