On Wed, Jul 06, 2016 at 10:57:19AM +0200, Linus Walleij wrote:
> > +static irqreturn_t wcove_gpio_irq_handler(int irq, void *data)
> > +{
> > + int pending;
> > + unsigned int p0, p1, virq, gpio;
> > + struct wcove_gpio *wg = data;
> > +
> > + if (regmap_read(wg->regmap, IRQ_STATUS_OFFSET + 0, &p0) ||
> > + regmap_read(wg->regmap, IRQ_STATUS_OFFSET + 1, &p1)) {
>
> Why can't you use regmap_bulk_read() here?
Will fix this in v5.
>
> > + dev_err(wg->chip.parent, "%s(): regmap_read() failed.\n",
> > + __func__);
> > + return IRQ_NONE;
> > + }
> > +
> > + pending = p0 | (p1 << 8);
> > +
> > + for (gpio = 0; gpio < WCOVE_GPIO_NUM; gpio++) {
> > + if (pending & BIT(gpio)) {
> > + virq = irq_find_mapping(wg->chip.irqdomain, gpio);
> > + handle_nested_irq(virq);
> > + }
> > + }
> > +
> > + regmap_write(wg->regmap, IRQ_STATUS_OFFSET + 0, p0);
> > + regmap_write(wg->regmap, IRQ_STATUS_OFFSET + 1, p1);
>
> Use regmap_bulk_write()?
Will fix this in v5.
>
> Also you're ignoring the return error code. Check it and dev_err() if
> it fails.
Yes, will fix.
>
> This loop seems like it could miss interrupts happening while
> processing. Especially edge interrupts, and thatr will lead to serious
> bugs later.
>
> Please consider the following construction:
>
> 1. read status register
> 2. Any IRQs active?
> 2.1 No IRQs active: if this is the FIRST iteration, exit with IRQ_NONE
> 2.2 No IRQs active If this the second iteration or later, exit with
> IRQ_HANDLED
> 2.3 IRQs active, continue
> 2. Find first active IRQ
> 3. Handle first active IRQ
> 4. ACK the first active IRQ by writing the status register
> 5. Reiterate from 1
>
> This way, if two IRQs happen at the same time, or if a new IRQ appears
> while you're inside the interrupt handler, it gets served.
I agree. Writing to status register should be done bit by bit, instead of
one write for all bits. Will fix this in v5.
>
> > +static void wcove_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > +{
> > + struct wcove_gpio *wg = gpiochip_get_data(chip);
> > + int gpio, offset, group;
> > + unsigned int ctlo, ctli, irq_mask, irq_status;
> > +
> > + for (gpio = 0; gpio < WCOVE_GPIO_NUM; gpio++) {
> > + group = gpio < GROUP0_NR_IRQS ? 0 : 1;
> > + regmap_read(wg->regmap, to_reg(gpio, CTRL_OUT), &ctlo);
> > + regmap_read(wg->regmap, to_reg(gpio, CTRL_IN), &ctli);
> > + regmap_read(wg->regmap, IRQ_MASK_OFFSET + group, &irq_mask);
> > + regmap_read(wg->regmap, IRQ_STATUS_OFFSET + group,
> > &irq_status);
>
> Ignoring error codes. Fix this.
Will Fix in v5.
>
> > + gpiochip_irqchip_add(&wg->chip, &wcove_irqchip, 0,
> > + handle_simple_irq, IRQ_TYPE_NONE);
>
> Reexamine the use of handle_simple_irq() here. We have two kinds of
> irq hardware: those with one register for ACKing and reading the status
> of an IRQ, and those with two registers for it: one where you ACK the
> IRQ (so it can immediately re-trigger) and one to read the status of
> whether it happened. Sometimes different handling is needed for
> levek and edge IRQs even (c.f. gpio-pl061.c).
>
> Only the hardware with just one register for both things should use
> handle_simple_irq(). This seems to be the case here but I want you
> to verify.
I will check and fix if it's needed.
>
> Yours,
> Linus Walleij
Thanks for your review.
