Message-ID: <868rldf5qx.wl-maz@kernel.org>
Date:   Tue, 18 Oct 2022 17:36:54 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Firas Ashkar <firas.ashkar@...oirfairelinux.com>,
        Arnd Bergmann <arnd@...db.de>
Cc:     alex@...riz.org.uk, tglx@...utronix.de,
        linux-kernel@...r.kernel.org, inux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] irqchip: add TS7800v1 fpga based controller driver

+Arnd

On Tue, 18 Oct 2022 14:30:34 +0100,
Firas Ashkar <firas.ashkar@...oirfairelinux.com> wrote:
> 
> 1. add TS-7800v1 fpga based irq controller driver, and
> 2. add related memory and irq resources
> 
> By default only mapped FPGA interrupts will be chained/multiplexed, these
> chained interrupts will then be available to other device drivers to
> request via the corresponding Linux IRQs.
> 
> $ cat /proc/cpuinfo
> processor	: 0
> model name	: Feroceon rev 0 (v5l)
> BogoMIPS	: 333.33
> Features	: swp half thumb fastmult edsp
> CPU implementer	: 0x41
> CPU architecture: 5TEJ
> CPU variant	: 0x0
> CPU part	: 0x926
> CPU revision	: 0
> 
> Hardware	: Technologic Systems TS-78xx SBC
> Revision	: 0000
> Serial		: 0000000000000000
> $
> 
> $ cat /proc/interrupts
>            CPU0
>   1:        902  orion_irq     Level     orion_tick
>   4:        795  orion_irq     Level     ttyS0
>  13:          0  orion_irq     Level     ehci_hcd:usb2
>  18:          0  orion_irq     Level     ehci_hcd:usb1
>  22:         69  orion_irq     Level     eth0
>  23:        171  orion_irq     Level     orion-mdio
>  29:          0  orion_irq     Level     mv_crypto
>  31:          2  orion_irq     Level     mv_xor.0
>  65:          1  ts7800-irqc   0 Edge      ts-dmac-cpupci
> Err:          0
> $
> 
> $ cat /proc/iomem
> 00000000-07ffffff : System RAM
>   00008000-00978fff : Kernel code
>   00d20000-00e54f97 : Kernel data
> e8000044-e8000047 : timeriomem_rng
>   e8000044-e8000047 : timeriomem_rng timeriomem_rng
> e8000200-e80002ff : ts_irqc
>   e8000200-e80002ff : ts7800-irqc ts_irqc
> e8000400-e80004ff : ts_dmac
>   e8000400-e80004ff : ts7800-dmac ts_dmac
> e8000804-e8000807 : gen_nand
>   e8000804-e8000807 : gen_nand gen_nand
> e8000808-e8000808 : rtc-m48t86
>   e8000808-e8000808 : rtc-m48t86 rtc-m48t86
> e800080c-e800080c : rtc-m48t86
>   e800080c-e800080c : rtc-m48t86 rtc-m48t86
> f1012000-f10120ff : serial8250.0
>   f1012000-f101201f : serial
> f1012100-f10121ff : serial8250.1
>   f1012100-f101211f : serial
> f1020108-f102010b : orion_wdt
> f1020300-f1020303 : orion_wdt
> f1020400-f102040f : ts_bridge
>   f1020400-f102040f : ts7800-irqc ts_bridge
> f1050000-f1050fff : orion-ehci.0
>   f1050000-f1050fff : orion-ehci.0 orion-ehci.0
> f1060900-f10609ff : xor 0 low
> f1060b00-f1060bff : xor 0 high
> f1072000-f1075fff : ge00 base
>   f1072004-f1072087 : ge00 mvmdio base
> f1080000-f1084fff : sata base
> f1090000-f109ffff : regs
>   f1090000-f109ffff : mv_crypto regs
> f10a0000-f10a0fff : orion-ehci.1
>   f10a0000-f10a0fff : orion-ehci.1 orion-ehci.1
> f2200000-f2201fff : sram
>   f2200000-f2201fff : mv_crypto sram
> $
> 
> $ uname -a
> Linux ts-7800 6.1.0-rc1 #2 PREEMPT Mon Oct 17 11:19:12 EDT 2022 armv5tel
>  GNU/Linux
> $

I don't see how useful this information is. Please limit the commit
message to the provided functionality.

> 
> Signed-off-by: Firas Ashkar <firas.ashkar@...oirfairelinux.com>
> ---
> :100644 100644 2f4fe3ca5c1a ed8378893208 M	arch/arm/mach-orion5x/ts78xx-fpga.h
> :100644 100644 af810e7ccd79 d319a68b7160 M	arch/arm/mach-orion5x/ts78xx-setup.c
> :100644 100644 7ef9f5e696d3 d184fb435c5d M	drivers/irqchip/Kconfig
> :100644 100644 87b49a10962c b022eece2042 M	drivers/irqchip/Makefile
> :000000 100644 000000000000 178f43548b2a A	drivers/irqchip/irq-ts7800.c

How did you generated this patch?

>  arch/arm/mach-orion5x/ts78xx-fpga.h  |   1 +
>  arch/arm/mach-orion5x/ts78xx-setup.c |  55 +++++
>  drivers/irqchip/Kconfig              |  12 +
>  drivers/irqchip/Makefile             |   1 +
>  drivers/irqchip/irq-ts7800.c         | 319 +++++++++++++++++++++++++++
>  5 files changed, 388 insertions(+)
> 
> diff --git a/arch/arm/mach-orion5x/ts78xx-fpga.h b/arch/arm/mach-orion5x/ts78xx-fpga.h
> index 2f4fe3ca5c1a..ed8378893208 100644
> --- a/arch/arm/mach-orion5x/ts78xx-fpga.h
> +++ b/arch/arm/mach-orion5x/ts78xx-fpga.h
> @@ -32,6 +32,7 @@ struct fpga_devices {
>  	struct fpga_device	ts_rtc;
>  	struct fpga_device	ts_nand;
>  	struct fpga_device	ts_rng;
> +	struct fpga_device	ts_irqc;
>  };
>  
>  struct ts78xx_fpga_data {
> diff --git a/arch/arm/mach-orion5x/ts78xx-setup.c b/arch/arm/mach-orion5x/ts78xx-setup.c
> index af810e7ccd79..d319a68b7160 100644
> --- a/arch/arm/mach-orion5x/ts78xx-setup.c
> +++ b/arch/arm/mach-orion5x/ts78xx-setup.c
> @@ -322,6 +322,49 @@ static void ts78xx_ts_rng_unload(void)
>  	platform_device_del(&ts78xx_ts_rng_device);
>  }
>  
> +/*****************************************************************************
> + * fpga irq controller
> + ****************************************************************************/
> +#define TS_IRQC_CTRL (TS78XX_FPGA_REGS_PHYS_BASE + 0x200)
> +#define TS_BRIDGE_CTRL (ORION5X_REGS_PHYS_BASE + 0x20400)
> +#define TS_IRQC_PARENT 0x2
> +static struct resource ts_irqc_resources[] = {
> +	DEFINE_RES_MEM_NAMED(TS_IRQC_CTRL, 0x100, "ts_irqc"),
> +	DEFINE_RES_MEM_NAMED(TS_BRIDGE_CTRL, 0x10, "ts_bridge"),
> +	DEFINE_RES_IRQ_NAMED(TS_IRQC_PARENT, "ts_irqc_parent"),
> +};
> +
> +static struct platform_device ts_irqc_device = {
> +	.name = "ts7800-irqc",
> +	.id = -1,
> +	.resource = ts_irqc_resources,
> +	.num_resources = ARRAY_SIZE(ts_irqc_resources),
> +};
> +
> +static int ts_irqc_load(void)
> +{
> +	int rc;
> +
> +	if (ts78xx_fpga.supports.ts_irqc.init == 0) {
> +		rc = platform_device_register(&ts_irqc_device);
> +		if (!rc)
> +			ts78xx_fpga.supports.ts_irqc.init = 1;
> +	} else {
> +		rc = platform_device_add(&ts_irqc_device);
> +	}
> +
> +	if (rc)
> +		pr_info("FPGA based IRQ controller could not be registered: %d\n",
> +			rc);
> +
> +	return rc;
> +}
> +
> +static void ts_irqc_unload(void)
> +{
> +	platform_device_del(&ts_irqc_device);
> +}
> +
>  /*****************************************************************************
>   * FPGA 'hotplug' support code
>   ****************************************************************************/
> @@ -330,6 +373,7 @@ static void ts78xx_fpga_devices_zero_init(void)
>  	ts78xx_fpga.supports.ts_rtc.init = 0;
>  	ts78xx_fpga.supports.ts_nand.init = 0;
>  	ts78xx_fpga.supports.ts_rng.init = 0;
> +	ts78xx_fpga.supports.ts_irqc.init = 0;
>  }
>  
>  static void ts78xx_fpga_supports(void)
> @@ -348,6 +392,7 @@ static void ts78xx_fpga_supports(void)
>  		ts78xx_fpga.supports.ts_rtc.present = 1;
>  		ts78xx_fpga.supports.ts_nand.present = 1;
>  		ts78xx_fpga.supports.ts_rng.present = 1;
> +		ts78xx_fpga.supports.ts_irqc.present = 1;
>  		break;
>  	default:
>  		/* enable devices if magic matches */
> @@ -358,11 +403,13 @@ static void ts78xx_fpga_supports(void)
>  			ts78xx_fpga.supports.ts_rtc.present = 1;
>  			ts78xx_fpga.supports.ts_nand.present = 1;
>  			ts78xx_fpga.supports.ts_rng.present = 1;
> +			ts78xx_fpga.supports.ts_irqc.present = 1;
>  			break;
>  		default:
>  			ts78xx_fpga.supports.ts_rtc.present = 0;
>  			ts78xx_fpga.supports.ts_nand.present = 0;
>  			ts78xx_fpga.supports.ts_rng.present = 0;
> +			ts78xx_fpga.supports.ts_irqc.present = 0;
>  		}
>  	}
>  }
> @@ -371,6 +418,12 @@ static int ts78xx_fpga_load_devices(void)
>  {
>  	int tmp, ret = 0;
>  
> +	if (ts78xx_fpga.supports.ts_irqc.present == 1) {
> +		tmp = ts_irqc_load();
> +		if (tmp)
> +			ts78xx_fpga.supports.ts_irqc.present = 0;
> +		ret |= tmp;
> +	}
>  	if (ts78xx_fpga.supports.ts_rtc.present == 1) {
>  		tmp = ts78xx_ts_rtc_load();
>  		if (tmp)
> @@ -402,6 +455,8 @@ static int ts78xx_fpga_unload_devices(void)
>  		ts78xx_ts_nand_unload();
>  	if (ts78xx_fpga.supports.ts_rng.present == 1)
>  		ts78xx_ts_rng_unload();
> +	if (ts78xx_fpga.supports.ts_irqc.present == 1)
> +		ts_irqc_unload();
>  
>  	return 0;
>  }

I am absolutely *NOT* keen on adding more non-DT stuff in this day and
age. The DT effort has been going on for over 10 years, and maybe it
is time that this board makes the jump before we add anything new to
it, specially given that there is a DT board for this platform.

Arnd, what's your call on this?

> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 7ef9f5e696d3..d184fb435c5d 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -290,6 +290,18 @@ config TS4800_IRQ
>  	help
>  	  Support for the TS-4800 FPGA IRQ controller
>  
> +config TS7800_IRQ
> +	tristate "TS-7800 IRQ controller"
> +	select IRQ_DOMAIN
> +	depends on HAS_IOMEM
> +	depends on MACH_TS78XX
> +	help
> +	  Support for TS-7800 FPGA based IRQ controller.
> +	  This IRQ controller acts as a multiplexer chaining mapped
> +	  FPGA interrupts to a single system bridge interrupt.
> +
> +	  If you have an FPGA IP corresponding to this driver, say Y or M here.
> +

How can the user tell they have this IP or another one? Can they
freely load it? Can the make *changes* to it?

>  config VERSATILE_FPGA_IRQ
>  	bool
>  	select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 87b49a10962c..b022eece2042 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
>  obj-$(CONFIG_ST_IRQCHIP)		+= irq-st.o
>  obj-$(CONFIG_TB10X_IRQC)		+= irq-tb10x.o
>  obj-$(CONFIG_TS4800_IRQ)		+= irq-ts4800.o
> +obj-$(CONFIG_TS7800_IRQ)		+= irq-ts7800.o
>  obj-$(CONFIG_XTENSA)			+= irq-xtensa-pic.o
>  obj-$(CONFIG_XTENSA_MX)			+= irq-xtensa-mx.o
>  obj-$(CONFIG_XILINX_INTC)		+= irq-xilinx-intc.o
> diff --git a/drivers/irqchip/irq-ts7800.c b/drivers/irqchip/irq-ts7800.c
> new file mode 100644
> index 000000000000..178f43548b2a
> --- /dev/null
> +++ b/drivers/irqchip/irq-ts7800.c
> @@ -0,0 +1,319 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * FPGA based IRQ contorller driver for TS-7800v1
> + *
> + * Copyright (C) 2022 Savoir-faire Linux Inc.
> + *
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +
> +#define DRIVER_NAME "ts7800-irqc"
> +
> +#define IRQ_MASK_REG 0x4
> +#define IRQ_STATUS_REG 0x8
> +#define TS7800_IRQ_NUM 0x20
> +
> +/*
> + * FPGA IRQ Controller, list of all mappable/chainable interrupts
> + *
> + * IRQ  Description
> + * ---  -----------
> + * 0x0  dma_cpu_pci_dack_o
> + * 0x1  dma_fpga_dack_o
> + * 0x2  SD Busy#
> + * 0x3  isa_irq3
> + * 0x4  isa_irq4
> + * 0x5  isa_irq5
> + * 0x6  isa_irq6
> + * 0x7  isa_irq7
> + * 0x8  Reserved
> + * 0x9  isa_irq9
> + * 0xa  isa_irq10
> + * 0xb  isa_irq11
> + * 0xc  isa_irq12
> + * 0xd  isa_irq14
> + * 0xe  isa_irq15
> + * 0x10:0x19  tsuart_irqs
> + *
> + * To map or enable a specific FPGA interrupt, add its corresponding number to 'enabled_mappings'.
> + * Example:
> + *  1. For 'dma_cpu_pci_dack_o' (FPGA based DMA controller), add 0x0 to 'enabled_mappings'
> + *  2. For 'tsuart_irqs' (FPGA based UARTs), add their FPGA interrupt range of 0x10-0x19
> + *     to 'enabled_mappings'
> + *
> + * By default only mapped/enabled interrupts will be chained/multiplexed,
> + * these chained interrupts will then be available to other device drivers to request via the
> + * corresponding Linux IRQs.
> + *
> + * Each mapped FPGA interrupt will have a corresponding Linux IRQ, this new IRQ will be appended
> + * to the system's current list of Linux IRQs, on TS78xx, the first mapped FPGA interrupt will have
> + * a corresponding new Linux IRQ starting at 65. The next mapped FPGA interrupt will have a Linux
> + * IRQ of 66 and so forth.
> + *
> + * Example-1:
> + *  In order for a device driver, say the FPGA based DMA controller driver 'TS-DMAC',
> + *  to use either of the corresponding FPGA interrupts 'dma_cpu_pci_dack_o' and 'dma_fpga_dack_o',
> + *  the 'TS-DMAC' driver has to:
> + *  1. add FPGA interrupt numbers 0x0 and 0x1 to 'enabled_mappings', in this file, and
> + *  2. request the corresponding Linux IRQ numbers 65 and 66 respectively in its implementation.
> + *
> + * Eample-2:
> + *  In order for another device driver, say the FPGA based 'TSUART' driver, to use the related FPGA
> + *  interrupts, the 'TSUART' driver has to:
> + *  1. append FPGA interrupt numbers 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19 to
> + *     'enabled_mappings', in this file, and
> + *  2. request the corresponding Linux IRQ numbers, these IRQs could start from 65 till 74, if no
> + *     previous FPGA interrupts where mapped. However, if other FPGA interrupts, say those of
> + *     'TS-DMAC' in Example-1 were also mapped/enabled, then the correct corresponding Linux IRQs
> + *     would start from 67 till 76.
> + */

See why this really should be a DT-based driver? And please keep
comments to less than 80 chars per line.

> +
> +static irq_hw_number_t enabled_mappings[] = { 0x0 };
> +
> +struct ts7800_irq_data {
> +	int mpp7_virq;
> +	void __iomem *base;
> +	void __iomem *bridge;
> +	struct irq_domain *domain;
> +	struct irq_chip irq_chip;
> +};
> +
> +static void ts7800_irq_mask(struct irq_data *d)
> +{
> +	struct ts7800_irq_data *data = irq_data_get_irq_chip_data(d);
> +	u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
> +	u32 mask_bits = 1 << d->hwirq;
> +
> +	writel(fpga_mask_reg & ~mask_bits, data->base + IRQ_MASK_REG);
> +	writel(fpga_mask_reg & ~mask_bits, data->bridge + IRQ_MASK_REG);

I know your HW is UP, but seeing these RMW sequences without a lock
makes me jump. You probably want to either forbid this driver on SMP
systems, or add the required locks.

> +}
> +
> +static void ts7800_irq_unmask(struct irq_data *d)
> +{
> +	struct ts7800_irq_data *data = irq_data_get_irq_chip_data(d);
> +	u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
> +	u32 mask_bits = 1 << d->hwirq;
> +
> +	writel(fpga_mask_reg | mask_bits, data->base + IRQ_MASK_REG);
> +	writel(fpga_mask_reg | mask_bits, data->bridge + IRQ_MASK_REG);
> +}
> +
> +static int ts7800_irqdomain_map(struct irq_domain *d, unsigned int irq,
> +				irq_hw_number_t hwirq)
> +{
> +	struct ts7800_irq_data *data = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &data->irq_chip, handle_simple_irq);

Are you sure of this? Edge and level interrupts have very different
semantics, and handle_simple_irq seems like a massive cop-out.

> +	irq_set_chip_data(irq, data);
> +	irq_set_noprobe(irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops ts7800_ic_ops = {
> +	.map = ts7800_irqdomain_map,
> +	.xlate = irq_domain_xlate_onecell,

I don't really get the usage model of this thing. Who provides the
interrupt specifier?

> +};
> +
> +static void ts7800_ic_chained_handle_irq(struct irq_desc *desc)
> +{
> +	struct ts7800_irq_data *data = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	u32 mask_bits = readl(data->base + IRQ_STATUS_REG);
> +	u32 status = mask_bits;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	if (unlikely(status == 0)) {
> +		handle_bad_irq(desc);
> +		goto out;
> +	}
> +
> +	do {
> +		unsigned int bit = __ffs(status);
> +		int irq = irq_find_mapping(data->domain, bit);
> +
> +		status &= ~(1 << bit);
> +
> +		if (irq && (mask_bits & BIT(bit))) {
> +			u32 x32 = readl(data->bridge);
> +
> +			generic_handle_irq(irq);

Please use generic handle_dopmain_irq().

> +
> +			x32 &= ~(mask_bits & BIT(bit));
> +			writel(x32, data->bridge);

What does this write do? If this is an Ack of the interrupt, you've
just lost an edge.

> +		}
> +	} while (status);
> +
> +out:
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int ts7800_ic_probe(struct platform_device *pdev)
> +{
> +	struct ts7800_irq_data *data = NULL;
> +	struct irq_chip *irq_chip = NULL;
> +	struct resource *mem_res = NULL, *brdg_res = NULL, *irq_res = NULL;
> +	unsigned int irqdomain;
> +	int i, ret;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (IS_ERR_OR_NULL(data)) {
> +		dev_err(&pdev->dev,
> +			"Failed to allocate TS7800 data, error %ld\n",
> +			PTR_ERR(data));
> +		ret = PTR_ERR(data);
> +		goto devm_kzalloc_err;
> +	}
> +
> +	mem_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ts_irqc");
> +	if (IS_ERR_OR_NULL(mem_res)) {
> +		dev_err(&pdev->dev,
> +			"Failed to get platform memory resource, error %ld\n",
> +			PTR_ERR(mem_res));
> +		ret = PTR_ERR(mem_res);
> +		goto pltfrm_get_res_mem_err;
> +	}
> +
> +	data->base = devm_ioremap_resource(&pdev->dev, mem_res);
> +	if (IS_ERR_OR_NULL(data->base)) {
> +		dev_err(&pdev->dev,
> +			"Failed to IO map mem-region %s, error %ld\n",
> +			mem_res->name, PTR_ERR(data->base));
> +		ret = PTR_ERR(data->base);
> +		goto devm_ioremap_res_mem_err;
> +	}
> +
> +	brdg_res =
> +		platform_get_resource_byname(pdev, IORESOURCE_MEM, "ts_bridge");

Don't break assignments.

> +	if (IS_ERR_OR_NULL(brdg_res)) {
> +		dev_err(&pdev->dev,
> +			"Failed to get platform bridge resource, error %ld\n",
> +			PTR_ERR(brdg_res));
> +		ret = PTR_ERR(brdg_res);
> +		goto pltfrm_get_res_brdg_err;
> +	}
> +
> +	data->bridge = devm_ioremap_resource(&pdev->dev, brdg_res);
> +	if (IS_ERR_OR_NULL(data->bridge)) {
> +		dev_err(&pdev->dev,
> +			"Failed to IO map bridge-region %s, error %ld\n",
> +			mem_res->name, PTR_ERR(data->bridge));
> +		ret = PTR_ERR(data->bridge);
> +		goto devm_ioremap_res_brdge_err;
> +	}
> +
> +	irq_res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
> +					       "ts_irqc_parent");
> +	if (IS_ERR_OR_NULL(irq_res)) {
> +		dev_err(&pdev->dev,
> +			"Failed to get platform parent irq resource, error %ld\n",
> +			PTR_ERR(irq_res));
> +		ret = PTR_ERR(irq_res);
> +		goto pltfrm_get_res_irq_err;
> +	}
> +
> +	data->mpp7_virq = irq_res->start;
> +	writel(0x0, data->base + IRQ_MASK_REG);
> +	writel(0x0, data->bridge + IRQ_MASK_REG);
> +	writel(0x0, data->bridge);
> +
> +	irq_chip = &data->irq_chip;
> +	irq_chip->name = dev_name(&pdev->dev);

No. We don't pretty-print these things. And really, the irq_chip
structure should be as static as possible (if possible const).

> +	irq_chip->irq_mask = ts7800_irq_mask;
> +	irq_chip->irq_unmask = ts7800_irq_unmask;
> +
> +	data->domain = irq_domain_add_linear(pdev->dev.of_node, TS7800_IRQ_NUM,
> +					     &ts7800_ic_ops, data);
> +	if (IS_ERR_OR_NULL(data->domain)) {
> +		dev_err(&pdev->dev, "cannot add IRQ domain\n");
> +		ret = PTR_ERR(data->domain);
> +		goto irq_domain_add_linear_err;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(enabled_mappings); ++i) {
> +		irqdomain =
> +			irq_create_mapping(data->domain, enabled_mappings[i]);
> +	}
> +
> +	irq_set_chained_handler_and_data(data->mpp7_virq,
> +					 ts7800_ic_chained_handle_irq, data);
> +
> +	irq_set_status_flags(data->mpp7_virq,
> +			     IRQ_DISABLE_UNLAZY | IRQ_LEVEL | IRQ_NOPROBE);

Why the IRQ_DISABLE_UNLAZY flag?

> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +
> +irq_domain_add_linear_err:
> +pltfrm_get_res_irq_err:
> +	devm_iounmap(&pdev->dev, data->bridge);
> +
> +devm_ioremap_res_brdge_err:
> +pltfrm_get_res_brdg_err:
> +	devm_iounmap(&pdev->dev, data->base);
> +
> +devm_ioremap_res_mem_err:
> +pltfrm_get_res_mem_err:
> +	devm_kfree(&pdev->dev, data);
> +
> +devm_kzalloc_err:
> +	return ret;
> +}
> +
> +static int ts7800_ic_remove(struct platform_device *pdev)
> +{
> +	struct ts7800_irq_data *data = platform_get_drvdata(pdev);
> +	int i;
> +
> +	if (!IS_ERR_OR_NULL(data)) {
> +		irq_set_chained_handler_and_data(data->mpp7_virq, NULL, NULL);
> +
> +		for (i = 0; i < ARRAY_SIZE(enabled_mappings); ++i)
> +			irq_dispose_mapping(irq_find_mapping(
> +				data->domain, enabled_mappings[i]));
> +
> +		irq_domain_remove(data->domain);
> +	}
> +
> +	return 0;
> +}

We don't remove interrupt controllers. What happens if someone still
had a mapping?

> +
> +static const struct platform_device_id ts7800v1_ic_ids[] = {
> +	{
> +		.name = DRIVER_NAME,
> +	},
> +	{
> +		/* sentinel */
> +	}
> +};
> +
> +MODULE_DEVICE_TABLE(platform, ts7800v1_ic_ids);
> +
> +static struct platform_driver ts7800_ic_driver = {
> +	.probe  = ts7800_ic_probe,
> +	.remove = ts7800_ic_remove,
> +	.id_table	= ts7800v1_ic_ids,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +};
> +module_platform_driver(ts7800_ic_driver);

Please use the existing helpers.

> +
> +MODULE_ALIAS("platform:ts7800-irqc");
> +MODULE_DESCRIPTION("TS-7800v1 FPGA based IRQ controller Driver");
> +MODULE_AUTHOR("Firas Ashkar <firas.ashkar@...oirfairelinux.com>");
> +MODULE_LICENSE("GPL");

	M.

-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists