On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov
<dbaryshkov@xxxxxxxxx> 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@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
