Re: [alsa-devel] [PATCH 14/15] gpio: locomo: implement per-pin irq handling

On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov dbaryshkov@gmail.com wrote:

LoCoMo has a possibility to generate per-GPIO edge irqs. Support for that was there in old locomo driver, got 'cleaned up' during old driver IRQ cascading cleanup and is now reimplemented. It is expected that SL-5500 (collie) will use locomo gpio irqs for mmc detection irq.

Signed-off-by: Dmitry Eremin-Solenikov dbaryshkov@gmail.com

Please don't use open-coded IRQ handling like this, we are moving away from that.

In Kconfig,

select GPIOLIB_IRQCHIP

and look at the other drivers selecting this for inspiration. There is even some documentation in Documentation/gpio/driver.txt

You will find that it cuts down a lot of overhead from your driver and does everything in the right way in a central place.

struct locomo_gpio { void __iomem *regs;

•   int irq;
  
    spinlock_t lock;
    struct gpio_chip gpio;
  

•   int irq_base;
  

gpiolib irqchip helpers uses irqdomain to do all this debasing and rebasing for you. Go with that.

+static int locomo_gpio_to_irq(struct gpio_chip *chip, unsigned offset) +{

•   struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
  

•   return lg->irq_base + offset;
  

+}

And it implements .to_irq() in the gpiolib core.

+static void +locomo_gpio_handler(unsigned int irq, struct irq_desc *desc)

It's locomo_gpio_irq_handler() right?

+{

•   u16 req;
  

•   struct locomo_gpio *lg = irq_get_handler_data(irq);
  

•   int i = lg->irq_base;
  

•   req = readw(lg->regs + LOCOMO_GIR) &
  

•         readw(lg->regs + LOCOMO_GPD);
  

•   while (req) {
  

•           if (req & 1)
  

•                   generic_handle_irq(i);
  

•           req >>= 1;
  

•           i++;
  

•   }
  

Same thing as the MFD device, look closer at how you construct the IRQ handling loop, so the register gets re-read each iteration.

+static void locomo_gpio_ack_irq(struct irq_data *d) +{

•   struct locomo_gpio *lg = irq_data_get_irq_chip_data(d);
  

•   unsigned long flags;
  

•   u16 r;
  

•   spin_lock_irqsave(&lg->lock, flags);
  

•   r = readw(lg->regs + LOCOMO_GWE);
  

•   r |= (0x0001 << (d->irq - lg->irq_base));
  

•   writew(r, lg->regs + LOCOMO_GWE);
  

•   r = readw(lg->regs + LOCOMO_GIS);
  

•   r &= ~(0x0001 << (d->irq - lg->irq_base));
  

•   writew(r, lg->regs + LOCOMO_GIS);
  

•   r = readw(lg->regs + LOCOMO_GWE);
  

•   r &= ~(0x0001 << (d->irq - lg->irq_base));
  

•   writew(r, lg->regs + LOCOMO_GWE);
  

•   spin_unlock_irqrestore(&lg->lock, flags);
  

+}

I really wonder if this locking is needed around these regioster accesses. It seems more like a habit than like something that is actually needed. Think it over.

*irqsave* versions of spinlocks are definately wrong in the irqchip callbacks, if you give it a minute I think you quickly realize why.

+static int locomo_gpio_type(struct irq_data *d, unsigned int type) +{

•   unsigned int mask;
  

•   struct locomo_gpio *lg = irq_data_get_irq_chip_data(d);
  

•   unsigned long flags;
  

•   mask = 1 << (d->irq - lg->irq_base);
  

This should just use d->hwirq with irqdomain implemented correctly.

(...)

+static void locomo_gpio_setup_irq(struct locomo_gpio *lg) +{

•   int irq;
  

•   lg->irq_base = irq_alloc_descs(-1, 0, LOCOMO_GPIO_NR_IRQS, -1);
  

•   /* Install handlers for IRQ_LOCOMO_* */
  

•   for (irq = lg->irq_base;
  

•                   irq < lg->irq_base + LOCOMO_GPIO_NR_IRQS;
  

•                   irq++) {
  

•           irq_set_chip_and_handler(irq, &locomo_gpio_chip,
  

•                           handle_edge_irq);
  

•           irq_set_chip_data(irq, lg);
  

•           set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
  

•   }
  

•   /*
  

•    * Install handler for IRQ_LOCOMO_HW.
  

•    */
  

•   irq_set_handler_data(lg->irq, lg);
  

•   irq_set_chained_handler(lg->irq, locomo_gpio_handler);
  

+}

All this gets redundant with gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().

Yours, Linus Walleij