Re: [PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver

[Marc Zyngier <marc.zyngier@arm.com>]

Hi Joachim,

On 02/04/16 17:35, Joachim Eastwood wrote:
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>

As a start, a commit message would be appreciated.

> ---
>  drivers/irqchip/Kconfig                 |   5 +
>  drivers/irqchip/Makefile                |   1 +
>  drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219 ++++++++++++++++++++++++++++++++
>  3 files changed, 225 insertions(+)
>  create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 3e12479..0278837e 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -211,6 +211,11 @@ config KEYSTONE_IRQ
>  		Support for Texas Instruments Keystone 2 IRQ controller IP which
>  		is part of the Keystone 2 IPC mechanism
>  
> +config LPC18XX_GPIO_PINT
> +	bool
> +	select IRQ_DOMAIN
> +	select GENERIC_IRQ_CHIP
> +
>  config MIPS_GIC
>  	bool
>  	select GENERIC_IRQ_IPI
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b03cfcb..bf60e0c 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ)		+= irq-bcm7038-l1.o
>  obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
>  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
>  obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
> +obj-$(CONFIG_LPC18XX_GPIO_PINT)		+= irq-lpc18xx-gpio-pint.o
>  obj-$(CONFIG_MIPS_GIC)			+= irq-mips-gic.o
>  obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o
>  obj-$(CONFIG_ARCH_DIGICOLOR)		+= irq-digicolor.o
> diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
> new file mode 100644
> index 0000000..d53e99b
> --- /dev/null
> +++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
> @@ -0,0 +1,219 @@
> +/*
> + * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
> + *
> + * Copyright (C) 2016 Joachim Eastwood <manabian@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +
> +/* LPC18xx GPIO pin interrupt register offsets */
> +#define LPC18XX_GPIO_PINT_ISEL		0x000
> +#define LPC18XX_GPIO_PINT_SIENR		0x008
> +#define LPC18XX_GPIO_PINT_CIENR		0x00c
> +#define LPC18XX_GPIO_PINT_SIENF		0x014
> +#define LPC18XX_GPIO_PINT_CIENF		0x018
> +#define LPC18XX_GPIO_PINT_IST		0x024
> +
> +#define PINT_MAX_IRQS			32
> +
> +struct lpc18xx_gpio_pint_chip {
> +	struct irq_domain *domain;
> +	void __iomem	  *base;
> +	struct clk	  *clk;
> +	unsigned int	  revmap[];
> +};
> +
> +static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
> +{
> +	struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
> +	unsigned int irq = irq_desc_get_irq(desc);
> +	unsigned int hwirq = pint->revmap[irq];
> +	unsigned int virq;
> +
> +	virq = irq_find_mapping(pint->domain, hwirq);
> +	generic_handle_irq(virq);

Two things here:
- It feels a bit weird that you are indirecting everything through a
cascade interrupt. As you have a 1:1 relationship between the PINT
interrupts and their NVIC counterparts, this would be better described
as a hierarchical irqchip (see drivers/irqchip/irq-vf610-mscm-ir.c for
an example)
- If you decide to stick with the current approach, you're then missing
the usual chained_irq_{enter,exit}.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...