On Mon, Jun 11 2018, Sergio Paracuellos wrote: > This chip support high level and low level interrupts. Those > have to be implemented also to get a complete and clean driver. > > Signed-off-by: Sergio Paracuellos <[email protected]> > --- > drivers/staging/mt7621-gpio/gpio-mt7621.c | 51 > +++++++++++++++++++++++-------- > 1 file changed, 38 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c > b/drivers/staging/mt7621-gpio/gpio-mt7621.c > index 54c18c1..39874cb 100644 > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c > @@ -38,6 +38,8 @@ > * @bank: gpio bank number for the chip > * @rising: mask for rising irqs > * @falling: mask for falling irqs > + * @hlevel: mask for high level irqs > + * @llevel: mask for low level irqs > */ > struct mtk_gc { > struct gpio_chip chip; > @@ -45,6 +47,8 @@ struct mtk_gc { > int bank; > u32 rising; > u32 falling; > + u32 hlevel; > + u32 llevel; > }; > > /** > @@ -184,7 +188,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d) > int bank = pin / MTK_BANK_WIDTH; > struct mtk_gc *rg = &gpio_data->gc_map[bank]; > unsigned long flags; > - u32 rise, fall; > + u32 rise, fall, high, low; > > if (!rg) > return; > @@ -192,8 +196,12 @@ mediatek_gpio_irq_unmask(struct irq_data *d) > spin_lock_irqsave(&rg->lock, flags); > rise = mtk_gpio_r32(rg, GPIO_REG_REDGE); > fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE); > + high = mtk_gpio_r32(rg, GPIO_REG_HLVL); > + low = mtk_gpio_r32(rg, GPIO_REG_LLVL); > mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising)); > mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling)); > + mtk_gpio_w32(rg, GPIO_REG_HLVL, high | (PIN_MASK(pin) & rg->hlevel)); > + mtk_gpio_w32(rg, GPIO_REG_LLVL, low | (PIN_MASK(pin) & rg->llevel)); > spin_unlock_irqrestore(&rg->lock, flags); > } > > @@ -205,7 +213,7 @@ mediatek_gpio_irq_mask(struct irq_data *d) > int bank = pin / MTK_BANK_WIDTH; > struct mtk_gc *rg = &gpio_data->gc_map[bank]; > unsigned long flags; > - u32 rise, fall; > + u32 rise, fall, high, low; > > if (!rg) > return; > @@ -213,8 +221,12 @@ mediatek_gpio_irq_mask(struct irq_data *d) > spin_lock_irqsave(&rg->lock, flags); > rise = mtk_gpio_r32(rg, GPIO_REG_REDGE); > fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE); > + high = mtk_gpio_r32(rg, GPIO_REG_HLVL); > + low = mtk_gpio_r32(rg, GPIO_REG_LLVL); > mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin)); > mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin)); > + mtk_gpio_w32(rg, GPIO_REG_HLVL, high & ~PIN_MASK(pin)); > + mtk_gpio_w32(rg, GPIO_REG_LLVL, low & ~PIN_MASK(pin)); > spin_unlock_irqrestore(&rg->lock, flags); > } > > @@ -231,21 +243,34 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int > type) > return -1; > > if (type == IRQ_TYPE_PROBE) { > - if ((rg->rising | rg->falling) & mask) > + if ((rg->rising | rg->falling | > + rg->hlevel | rg->llevel) & mask) > return 0; > > - type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING; > + type = (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING | > + IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW); > }
This doesn't look right.
IRQ_TYPE_PROBE isn't very well documented and there aren't a lot of
examples of usage, but I think the idea is that if the interrupt type
is already set, then leave it along, otherwise choose a sane default,
which is IRQ_TYPE_EDGE_BOTH
(aka IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING) in all the cases
that I've looked at.
Certainly setting the type to RISING and FALLING and LOW and HIGH cannot
be right as that would cause constant interrupts.
>
> - if (type & IRQ_TYPE_EDGE_RISING)
> - rg->rising |= mask;
> - else
> - rg->rising &= ~mask;
> -
> - if (type & IRQ_TYPE_EDGE_FALLING)
> - rg->falling |= mask;
> - else
> - rg->falling &= ~mask;
> + if (type & IRQ_TYPE_EDGE_RISING ||
> + type & IRQ_TYPE_EDGE_FALLING) {
> + if (type & IRQ_TYPE_EDGE_RISING)
> + rg->rising |= mask;
> + else
> + rg->rising &= ~mask;
> +
> + if (type & IRQ_TYPE_EDGE_FALLING)
> + rg->falling |= mask;
> + else
> + rg->falling &= ~mask;
> + } else {
> + if (type & IRQ_TYPE_LEVEL_HIGH) {
> + rg->hlevel |= mask;
> + rg->llevel &= ~mask;
> + } else {
> + rg->llevel |= mask;
> + rg->hlevel &= ~mask;
> + }
> + }
I wonder if we should be clearing the mask bit for hlevel and llevel
when setting either edge - and clearing the edge bits when setting a
level.
Actually, you might have been right about using a switch statement.
Several drivers have
switch (type & IRQ_TYPE_SENSE_MASK) {
or similar.
The cope with the possiblity that both RISING and FALLING are set, they
have a case for IRQ_TYPE_EDGE_BOTH.
Maybe do
rg->rising &= ~mask;
rg->falling &= ~mask;
rg->hlevel &= ~mask;
rg->llevel &= ~mask;
switch (type & IRQ_TYPE_SENSE_MASK) {
case IRQ_TYPE_EDGE_BOTH:
rg->rising |= mask;
rg->falling |= mask;
break;
....
}
The current code works for my test-cases, but maybe it could be better.
Thanks,
NeilBrown
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
