On Sun, Jun 10, 2018 at 07:04:56PM +1000, NeilBrown wrote: > On Sat, Jun 09 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 <sergio.paracuellos@xxxxxxxxx> > > --- > > drivers/staging/mt7621-gpio/gpio-mt7621.c | 57 +++++++++++++++++++++++++------ > > 1 file changed, 46 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c > > index 5d082c8..b0cbb30 100644 > > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c > > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c > > @@ -37,6 +37,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; > > @@ -44,6 +46,8 @@ struct mtk_gc { > > int bank; > > u32 rising; > > u32 falling; > > + u32 hlevel; > > + u32 llevel; > > }; > > > > /** > > @@ -114,7 +118,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; > > @@ -122,10 +126,16 @@ mediatek_gpio_irq_unmask(struct irq_data *d) > > spin_lock_irqsave(&rg->lock, flags); > > rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank)); > > fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank)); > > + high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank)); > > + low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank)); > > mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), > > rise | (PIN_MASK(pin) & rg->rising)); > > mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), > > fall | (PIN_MASK(pin) & rg->falling)); > > + mtk_gpio_w32(rg, GPIO_REG_HLVL(bank), > > + high | (PIN_MASK(pin) & rg->hlevel)); > > + mtk_gpio_w32(rg, GPIO_REG_LLVL(bank), > > + low | (PIN_MASK(pin) & rg->llevel)); > > spin_unlock_irqrestore(&rg->lock, flags); > > } > > > > @@ -137,7 +147,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; > > @@ -145,8 +155,12 @@ mediatek_gpio_irq_mask(struct irq_data *d) > > spin_lock_irqsave(&rg->lock, flags); > > rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank)); > > fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank)); > > + high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank)); > > + low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank)); > > mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin)); > > mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin)); > > + mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), high & ~PIN_MASK(pin)); > > + mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), low & ~PIN_MASK(pin)); > > spin_unlock_irqrestore(&rg->lock, flags); > > } > > > > @@ -163,21 +177,42 @@ 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); > > } > > > > - if (type & IRQ_TYPE_EDGE_RISING) > > + switch (type) { > > + case IRQ_TYPE_EDGE_RISING: > > rg->rising |= mask; > > - else > > - rg->rising &= ~mask; > > - > > - if (type & IRQ_TYPE_EDGE_FALLING) > > - rg->falling |= mask; > > - else > > rg->falling &= ~mask; > > + rg->hlevel &= ~mask; > > + rg->llevel &= ~mask; > > + break; > > + case IRQ_TYPE_EDGE_FALLING: > > + rg->falling |= mask; > > + rg->rising &= ~mask; > > + rg->hlevel &= ~mask; > > + rg->llevel &= ~mask; > > + break; > > + case IRQ_TYPE_LEVEL_HIGH: > > + rg->hlevel |= mask; > > + rg->rising &= ~mask; > > + rg->falling &= mask; > > + rg->llevel &= ~mask; > > + break; > > + case IRQ_TYPE_LEVEL_LOW: > > + rg->llevel |= mask; > > + rg->rising &= ~mask; > > + rg->falling &= mask; > > + rg->hlevel &= ~mask; > > + break; > > Changing this to a switch statement is definitely the wrong thing to do. > Various combinations of bits are possible. > It only makes sense to have HIGH *or* LOW, but it makes lots of sense to > have both RISING and FALLING. Yes, you are right. I'll fix up this to allow both RISING and FALLING as it was before. > It probably doesn't make sense to have both a LEVEL and an EDGE, but I'm > not certain. The chip support both so it seems that it must be also implemented. > > Thanks, > NeilBrown Best regards, Sergio Paracuellos > > > + default: > > + return -EINVAL; > > + } > > > > return 0; > > } > > -- > > 2.7.4 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel