diff mbox

[14/15] gpio: locomo: implement per-pin irq handling

Message ID 1414454528-24240-15-git-send-email-dbaryshkov@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dmitry Baryshkov Oct. 28, 2014, 12:02 a.m. UTC
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>
---
 drivers/gpio/gpio-locomo.c | 158 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)

Comments

Linus Walleij Oct. 31, 2014, 8 a.m. UTC | #1
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
Dmitry Baryshkov Oct. 31, 2014, 9:35 a.m. UTC | #2
2014-10-31 11:00 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> 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.

Thanks, I will take a look.
[Skipped the rest of the comments mostly dedicated to gpiolib_irqchip].
diff mbox

Patch

diff --git a/drivers/gpio/gpio-locomo.c b/drivers/gpio/gpio-locomo.c
index 3b54b07..6ef0fc4 100644
--- a/drivers/gpio/gpio-locomo.c
+++ b/drivers/gpio/gpio-locomo.c
@@ -18,13 +18,18 @@ 
 #include <linux/gpio.h>
 #include <linux/io.h>
 #include <linux/spinlock.h>
+#include <linux/irq.h>
 #include <linux/mfd/locomo.h>
 
+#define LOCOMO_GPIO_NR_IRQS 16
+
 struct locomo_gpio {
 	void __iomem *regs;
+	int irq;
 
 	spinlock_t lock;
 	struct gpio_chip gpio;
+	int irq_base;
 
 	u16 rising_edge;
 	u16 falling_edge;
@@ -114,6 +119,148 @@  static int locomo_gpio_direction_output(struct gpio_chip *chip,
 	return 0;
 }
 
+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;
+}
+
+static void
+locomo_gpio_handler(unsigned int irq, struct irq_desc *desc)
+{
+	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++;
+	}
+}
+
+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);
+}
+
+static void locomo_gpio_mask_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_GIE);
+	r &= ~(0x0001 << (d->irq - lg->irq_base));
+	writew(r, lg->regs + LOCOMO_GIE);
+
+	spin_unlock_irqrestore(&lg->lock, flags);
+}
+
+static void locomo_gpio_unmask_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_GIE);
+	r |= (0x0001 << (d->irq - lg->irq_base));
+	writew(r, lg->regs + LOCOMO_GIE);
+
+	spin_unlock_irqrestore(&lg->lock, flags);
+}
+
+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);
+
+	if (type == IRQ_TYPE_PROBE) {
+		if ((lg->rising_edge | lg->falling_edge) & mask)
+			return 0;
+		type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
+	}
+
+	if (type & IRQ_TYPE_EDGE_RISING)
+		lg->rising_edge |= mask;
+	else
+		lg->rising_edge &= ~mask;
+	if (type & IRQ_TYPE_EDGE_FALLING)
+		lg->falling_edge |= mask;
+	else
+		lg->falling_edge &= ~mask;
+
+	spin_lock_irqsave(&lg->lock, flags);
+
+	writew(lg->rising_edge, lg->regs + LOCOMO_GRIE);
+	writew(lg->falling_edge, lg->regs + LOCOMO_GFIE);
+
+	spin_unlock_irqrestore(&lg->lock, flags);
+
+	return 0;
+}
+
+static struct irq_chip locomo_gpio_chip = {
+	.name		= "LOCOMO-gpio",
+	.irq_ack	= locomo_gpio_ack_irq,
+	.irq_mask	= locomo_gpio_mask_irq,
+	.irq_unmask	= locomo_gpio_unmask_irq,
+	.irq_set_type	= locomo_gpio_type,
+};
+
+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);
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int locomo_gpio_suspend(struct device *dev)
 {
@@ -169,6 +316,10 @@  static int locomo_gpio_probe(struct platform_device *pdev)
 	if (!lg)
 		return -ENOMEM;
 
+	lg->irq = platform_get_irq(pdev, 0);
+	if (lg->irq < 0)
+		return -ENXIO;
+
 	lg->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(lg->regs))
 		return PTR_ERR(lg->regs);
@@ -189,11 +340,14 @@  static int locomo_gpio_probe(struct platform_device *pdev)
 	lg->gpio.get = locomo_gpio_get;
 	lg->gpio.direction_input = locomo_gpio_direction_input;
 	lg->gpio.direction_output = locomo_gpio_direction_output;
+	lg->gpio.to_irq = locomo_gpio_to_irq;
 
 	ret = gpiochip_add(&lg->gpio);
 	if (ret)
 		return ret;
 
+	locomo_gpio_setup_irq(lg);
+
 	return 0;
 }
 
@@ -208,6 +362,10 @@  static int locomo_gpio_remove(struct platform_device *pdev)
 		return ret;
 	}
 
+	irq_set_chained_handler(lg->irq, NULL);
+	irq_set_handler_data(lg->irq, NULL);
+	irq_free_descs(lg->irq_base, LOCOMO_GPIO_NR_IRQS);
+
 	return 0;
 }