Hi Linus, On Mon, 2015-03-09 at 14:59 +0100, Linus Walleij wrote: > On Tue, Mar 3, 2015 at 9:47 AM, Alexey Brodkin > <[email protected]> wrote:
> > +++ b/drivers/gpio/gpio-dwapb.c > > @@ -370,6 +370,9 @@ static void dwapb_configure_irqs(struct dwapb_gpio > > *gpio, > > irq_create_mapping(gpio->domain, hwirq); > > > > port->bgc.gc.to_irq = dwapb_gpio_to_irq; > > + > > + /* Reset mask register */ > > + dwapb_write(gpio, GPIO_INTMASK, 0); > > I don't get this. This looks like you just enable all interrupts. The > driver also contains this in .suspend(): DW APB GPIO has 2 separate registers related to interrupts: [1] Mask interrupt register [2] Enable interrupt register So what I do in my patch I unmask all interrupts. But before at least one interrupt is enabled output interrupt line will never get in active state. And by default all interrupts are disabled (reset value = 0). > /* Mask out interrupts */ > dwapb_write(gpio, GPIO_INTMASK, 0xffffffff); > > If *anything* the probe should *mask* all interrupts so that the > .unmask() callback can enable them selectively. I'm going to agree with this statement, but this requires a bit more significant change in driver. I just wanted to fix an issue I discovered on my setup. Interestingly what I observed in my testing that if both enable()/disable() and mask()/unmask() are implemented in driver then only enable()/disable() pair will be actually used. Look at how generic irq_enable() function is implemented - http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/chip.c#n208 --->8--- void irq_enable(struct irq_desc *desc) { irq_state_clr_disabled(desc); if (desc->irq_data.chip->irq_enable) desc->irq_data.chip->irq_enable(&desc->irq_data); else desc->irq_data.chip->irq_unmask(&desc->irq_data); irq_state_clr_masked(desc); } --->8--- > The real problem I think is that struct irq_chip contains > mask()/unmask() callbacks that are not implemented > by this driver. I'd say that mask()/unmask() callbacks are implemented in this driver already. See http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-dwapb.c#n334 --->8--- ct->chip.irq_mask = irq_gc_mask_set_bit; ct->chip.irq_unmask = irq_gc_mask_clr_bit; ct->chip.irq_set_type = dwapb_irq_set_type; ct->chip.irq_enable = dwapb_irq_enable; ct->chip.irq_disable = dwapb_irq_disable; --->8--- It actually uses generic implementation of mask set bit and clear bit: irq_gc_mask_set_bit()/irq_gc_mask_clr_bit() that operate under GPIO_INTMASK register. And I may confirm that these functions correctly set/reset bits in mask register of GPIO controller. > Can you please test the below (untested) patch instead: > > From: Linus Walleij <[email protected]> > Date: Mon, 9 Mar 2015 14:56:18 +0100 > Subject: [PATCH] RFC: gpio: dwapb: handle mask/unmask properly > > This implements the callbacks for masking/unmasking IRQs in the > special IRQ mask/unmask register of the DWAPB GPIO block. > Previously these mask bits were unhandled and relied on > boot-up defaults. > > Reported-by: Alexey Brodkin <[email protected]> > Signed-off-by: Linus Walleij <[email protected]> > --- > drivers/gpio/gpio-dwapb.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 58faf04fce5d..1396f26bac5d 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -158,6 +158,30 @@ static void dwapb_irq_handler(u32 irq, struct > irq_desc *desc) > chip->irq_eoi(irq_desc_get_irq_data(desc)); > } > > +static void dwapb_irq_mask(struct irq_data *d) > +{ > + struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d); > + struct dwapb_gpio *gpio = igc->private; > + > + spin_lock_irqsave(&bgc->lock, flags); > + val = dwapb_read(gpio, GPIO_INTMASK); > + val |= BIT(d->hwirq); > + dwapb_write(gpio, GPIO_INTMASK, val); > + spin_unlock_irqrestore(&bgc->lock, flags); > +} > + > +static void dwapb_irq_unmask(struct irq_data *d) > +{ > + struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d); > + struct dwapb_gpio *gpio = igc->private; > + > + spin_lock_irqsave(&bgc->lock, flags); > + val = dwapb_read(gpio, GPIO_INTMASK); > + val &= ~BIT(d->hwirq); > + dwapb_write(gpio, GPIO_INTMASK, val); > + spin_unlock_irqrestore(&bgc->lock, flags); > +} Why would we need these custom functions if there're already irq_gc_mask_set_bit()/irq_gc_mask_clr_bit() implemented in kernel/irq/generic-chip.c > static void dwapb_irq_enable(struct irq_data *d) > { > struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d); > @@ -302,6 +326,10 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > struct irq_chip_type *ct; > int err, i; > > + /* Mask out and disable all interrupts */ > + dwapb_write(gpio, GPIO_INTMASK, 0xffffffff); > + dwapb_write(gpio, GPIO_INTEN, 0); This looks good to me - it's always a good idea to make sure defaults are set as we expect. > gpio->domain = irq_domain_add_linear(node, ngpio, > &irq_generic_chip_ops, gpio); > if (!gpio->domain) > @@ -334,6 +362,8 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > ct->chip.irq_mask = irq_gc_mask_set_bit; > ct->chip.irq_unmask = irq_gc_mask_clr_bit; > ct->chip.irq_set_type = dwapb_irq_set_type; > + ct->chip.irq_mask = dwapb_irq_mask; > + ct->chip.irq_unmask = dwapb_irq_unmask; Looks like we set "ct->chip.irq_mask" and "ct->chip.irq_unmask" twice, don't we? -Alexey -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
