mbox series

[0/3] drivers/irqchip: Add NXP INTMUX interrupt

Message ID 1576653615-27954-1-git-send-email-qiangqing.zhang@nxp.com
Headers show
Series drivers/irqchip: Add NXP INTMUX interrupt | expand

Message

Joakim Zhang Dec. 18, 2019, 7:20 a.m. UTC
This patch set adds driver for NXP INTMUX interrupt controller.

Joakim Zhang (2):
  dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer
  drivers/irqchip: enable INTMUX interrupt controller driver

Shengjiu Wang (1):
  drivers/irqchip: add NXP INTMUX interrupt multiplexer support

 .../interrupt-controller/fsl,intmux.txt       |  34 +++
 drivers/irqchip/Kconfig                       |   6 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-imx-intmux.c              | 220 ++++++++++++++++++
 4 files changed, 261 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
 create mode 100644 drivers/irqchip/irq-imx-intmux.c

Comments

Marc Zyngier Dec. 18, 2019, 9:37 a.m. UTC | #1
On 2019-12-18 07:20, Joakim Zhang wrote:
> From: Shengjiu Wang <shengjiu.wang@nxp.com>
>
> The intmux module is used to output internal interrupt in subsystem
> to system with 32-to-8 configuration. It has several multiplex
> channels depends on system.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/irqchip/irq-imx-intmux.c | 220 
> +++++++++++++++++++++++++++++++
>  1 file changed, 220 insertions(+)
>  create mode 100644 drivers/irqchip/irq-imx-intmux.c
>
> diff --git a/drivers/irqchip/irq-imx-intmux.c
> b/drivers/irqchip/irq-imx-intmux.c
> new file mode 100644
> index 000000000000..fa24b968f30b
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-intmux.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2017 NXP
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/spinlock.h>
> +
> +#define CHANCSR(n)	(0x0 + 0x40 * n)
> +#define CHANVEC(n)	(0x4 + 0x40 * n)

These two macros are unused.

> +#define CHANIER(n)	(0x10 + (0x40 * n))
> +#define CHANIPR(n)	(0x20 + (0x40 * n))
> +
> +struct intmux_irqchip_data {
> +	int chanidx;
> +	int irq;
> +	struct irq_domain *domain;
> +	unsigned int irqstat;

It would make things a bit readable if you aligned the various fields:

struct intmux_irqchip_data {
         int                  chanidx;
         int                  irq;
         struct irq_domain    *domain;
[...]
};

> +};
> +
> +struct intmux_data {
> +	spinlock_t lock;
> +	struct platform_device *pdev;
> +	void __iomem *regs;
> +	struct clk *ipg_clk;
> +	int channum;
> +	struct intmux_irqchip_data irqchip_data[];
> +};
> +
> +static void imx_intmux_irq_mask(struct irq_data *d)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> +	u32 idx = irqchip_data->chanidx;
> +	struct intmux_data *intmux_data = container_of(irqchip_data,
> +				struct intmux_data, irqchip_data[idx]);
> +	void __iomem *reg;
> +	u32 val;
> +
> +	spin_lock(&intmux_data->lock);

This is racy. you could take an interrupt while executing 
disable_irq(),
which calls this. In turn, the interrupt handler will try to acquire 
this
lock -> deadlock.

Please turn this into its _irqsave version.

> +	reg = intmux_data->regs + CHANIER(idx);
> +	val = readl_relaxed(reg);
> +	/* disable the interrupt source of this channel */
> +	val &= ~(1 << d->hwirq);

         val &= ~BIT(d->hwirq);

> +	writel_relaxed(val, reg);
> +	spin_unlock(&intmux_data->lock);
> +}
> +
> +static void imx_intmux_irq_unmask(struct irq_data *d)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> +	u32 idx = irqchip_data->chanidx;
> +	struct intmux_data *intmux_data = container_of(irqchip_data,
> +				struct intmux_data, irqchip_data[idx]);
> +	void __iomem *reg;
> +	u32 val;
> +
> +	spin_lock(&intmux_data->lock);
> +	reg = intmux_data->regs + CHANIER(idx);
> +	val = readl_relaxed(reg);
> +	/* enable the interrupt source of this channel */
> +	val |= 1 << d->hwirq;
> +	writel_relaxed(val, reg);
> +	spin_unlock(&intmux_data->lock);
> +}
> +
> +static struct irq_chip imx_intmux_irq_chip = {
> +	.name		= "intmux",
> +	.irq_mask	= imx_intmux_irq_mask,
> +	.irq_unmask	= imx_intmux_irq_unmask,
> +};
> +
> +static int imx_intmux_irq_map(struct irq_domain *h, unsigned int 
> irq,
> +			      irq_hw_number_t hwirq)
> +{
> +	irq_set_status_flags(irq, IRQ_LEVEL);
> +	irq_set_chip_data(irq, h->host_data);
> +	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, 
> handle_level_irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops imx_intmux_domain_ops = {
> +	.map		= imx_intmux_irq_map,
> +	.xlate		= irq_domain_xlate_onecell,
> +};
> +
> +static void imx_intmux_update_irqstat(struct intmux_irqchip_data
> *irqchip_data)
> +{
> +	int i = irqchip_data->chanidx;
> +	struct intmux_data *intmux_data = container_of(irqchip_data,
> +				struct intmux_data, irqchip_data[i]);
> +
> +	/* read the interrupt source pending status of this channel */
> +	irqchip_data->irqstat = readl_relaxed(intmux_data->regs + 
> CHANIPR(i));

Why does it need to be stored into the data structure, instead of
sinply being returned by the function?

> +}
> +
> +static void imx_intmux_irq_handler(struct irq_desc *desc)
> +{
> +	struct intmux_irqchip_data *irqchip_data = 
> irq_desc_get_handler_data(desc);
> +	int pos, virq;
> +
> +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +
> +	imx_intmux_update_irqstat(irqchip_data);
> +
> +	for_each_set_bit(pos, (unsigned long *)&irqchip_data->irqstat, 32) 
> {

This is broken on big-endian. Never cast a smaller type into unsigned 
long
if you're going to use any of the bit iterators.

> +		virq = irq_find_mapping(irqchip_data->domain, pos);
> +		if (virq)
> +			generic_handle_irq(virq);
> +	}
> +
> +	chained_irq_exit(irq_desc_get_chip(desc), desc);
> +}
> +
> +static int imx_intmux_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct intmux_data *intmux_data;
> +	int channum;
> +	int i, ret;
> +
> +	ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
> +	if (ret)
> +		channum = 1;
> +
> +	intmux_data = devm_kzalloc(&pdev->dev, sizeof(*intmux_data) +
> +				   channum * sizeof(intmux_data->irqchip_data[0]),
> +				   GFP_KERNEL);
> +	if (!intmux_data)
> +		return -ENOMEM;
> +
> +	intmux_data->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(intmux_data->regs)) {
> +		dev_err(&pdev->dev, "failed to initialize reg\n");
> +		return PTR_ERR(intmux_data->regs);
> +	}
> +
> +	intmux_data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> +	if (IS_ERR(intmux_data->ipg_clk)) {
> +		ret = PTR_ERR(intmux_data->ipg_clk);
> +		dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	intmux_data->channum = channum;
> +	intmux_data->pdev = pdev;

What is the point of keeping track of this? The only instance where you
go from MUX to device is just below, and you already have the device
at hand.

> +	spin_lock_init(&intmux_data->lock);
> +
> +	ret = clk_prepare_enable(intmux_data->ipg_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < channum; i++) {
> +		intmux_data->irqchip_data[i].chanidx = i;
> +		intmux_data->irqchip_data[i].irq = platform_get_irq(pdev, i);
> +		if (intmux_data->irqchip_data[i].irq <= 0) {
> +			dev_err(&pdev->dev, "failed to get irq\n");
> +			return -ENODEV;
> +		}
> +
> +		intmux_data->irqchip_data[i].domain = irq_domain_add_linear(
> +						np,
> +						32,
> +						&imx_intmux_domain_ops,
> +						&intmux_data->irqchip_data[i]);

Please indent this in a readable manner. If you need an intermediate 
variable,
so be it. Or have a long line if you want, but don't write things like 
this.

> +		if (!intmux_data->irqchip_data[i].domain) {
> +			dev_err(&intmux_data->pdev->dev,
> +				"failed to create IRQ domain\n");
> +			return -ENOMEM;
> +		}
> +
> +		irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
> +						 imx_intmux_irq_handler,
> +						 &intmux_data->irqchip_data[i]);

Shouldn't you initialize the HW to some sane state here? Like having
having all interrupts masked?

> +	}
> +
> +	platform_set_drvdata(pdev, intmux_data);
> +
> +	return 0;
> +}
> +
> +static int imx_intmux_remove(struct platform_device *pdev)
> +{
> +	struct intmux_data *intmux_data = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < intmux_data->channum; i++) {
> +		irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
> +						 NULL, NULL);

Same thing here. Shouldn't you make sure that no interrupt can fire 
anymore?

> +
> +		irq_domain_remove(intmux_data->irqchip_data[i].domain);
> +	}
> +
> +	platform_set_drvdata(pdev, NULL);
> +	clk_disable_unprepare(intmux_data->ipg_clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id imx_intmux_id[] = {
> +	{ .compatible = "fsl,imx-intmux", },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver imx_intmux_driver = {
> +	.driver = {
> +		.name = "imx-intmux",
> +		.of_match_table = imx_intmux_id,
> +	},
> +	.probe = imx_intmux_probe,
> +	.remove = imx_intmux_remove,
> +};
> +builtin_platform_driver(imx_intmux_driver);

Thanks,

         M.
Marc Zyngier Dec. 18, 2019, 9:40 a.m. UTC | #2
On 2019-12-18 07:20, Joakim Zhang wrote:
> Enable INTMUX interrupt controller driver for i.MX platforms.
>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/irqchip/Kconfig  | 6 ++++++
>  drivers/irqchip/Makefile | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index ba152954324b..7e2b1e9d0b45 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -457,6 +457,12 @@ config IMX_IRQSTEER
>  	help
>  	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
>
> +config IMX_INTMUX
> +	def_bool y if ARCH_MXC
> +	select IRQ_DOMAIN
> +	help
> +	  Support for the i.MX INTMUX interrupt multiplexer.
> +
>  config LS1X_IRQ
>  	bool "Loongson-1 Interrupt Controller"
>  	depends on MACH_LOONGSON32
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e806dda690ea..af976a79d1fb 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -100,6 +100,7 @@ obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
>  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>  obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
> +obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
>  obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
>  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
>  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o

Please merge this with the previous patch, it doesn't really warrant
a separate patch.

Thanks,

         M.
Joakim Zhang Dec. 18, 2019, 10:11 a.m. UTC | #3
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2019年12月18日 17:38
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; robh+dt@kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de; S.j.
> Wang <shengjiu.wang@nxp.com>; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; Andy Duan <fugang.duan@nxp.com>;
> Aisheng Dong <aisheng.dong@nxp.com>
> Subject: Re: [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt
> multiplexer support
> 
> On 2019-12-18 07:20, Joakim Zhang wrote:
> > From: Shengjiu Wang <shengjiu.wang@nxp.com>
> >
> > The intmux module is used to output internal interrupt in subsystem to
> > system with 32-to-8 configuration. It has several multiplex channels
> > depends on system.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/irqchip/irq-imx-intmux.c | 220
> > +++++++++++++++++++++++++++++++
> >  1 file changed, 220 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-imx-intmux.c
> >
> > diff --git a/drivers/irqchip/irq-imx-intmux.c
> > b/drivers/irqchip/irq-imx-intmux.c
> > new file mode 100644
> > index 000000000000..fa24b968f30b
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-intmux.c
> > @@ -0,0 +1,220 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright 2017 NXP
> > +
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
> > +#include <linux/kernel.h> #include <linux/module.h> #include
> > +<linux/of_platform.h> #include <linux/spinlock.h>
> > +
> > +#define CHANCSR(n)	(0x0 + 0x40 * n)
> > +#define CHANVEC(n)	(0x4 + 0x40 * n)
> 
> These two macros are unused.
Hi Marc,

Yes, we defined these two macros and have not used yet. I will remove it firstly in V2.

> > +#define CHANIER(n)	(0x10 + (0x40 * n))
> > +#define CHANIPR(n)	(0x20 + (0x40 * n))
> > +
> > +struct intmux_irqchip_data {
> > +	int chanidx;
> > +	int irq;
> > +	struct irq_domain *domain;
> > +	unsigned int irqstat;
> 
> It would make things a bit readable if you aligned the various fields:
> 
> struct intmux_irqchip_data {
>          int                  chanidx;
>          int                  irq;
>          struct irq_domain    *domain;
> [...]
> };
Ok, I will do it in V2.

Thanks.

Best Regards,
Joakim Zhang
> 
> > +};
> > +
> > +struct intmux_data {
> > +	spinlock_t lock;
> > +	struct platform_device *pdev;
> > +	void __iomem *regs;
> > +	struct clk *ipg_clk;
> > +	int channum;
> > +	struct intmux_irqchip_data irqchip_data[]; };
> > +
> > +static void imx_intmux_irq_mask(struct irq_data *d) {
> > +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> > +	u32 idx = irqchip_data->chanidx;
> > +	struct intmux_data *intmux_data = container_of(irqchip_data,
> > +				struct intmux_data, irqchip_data[idx]);
> > +	void __iomem *reg;
> > +	u32 val;
> > +
> > +	spin_lock(&intmux_data->lock);
> 
> This is racy. you could take an interrupt while executing disable_irq(), which
> calls this. In turn, the interrupt handler will try to acquire this lock -> deadlock.
> 
> Please turn this into its _irqsave version.
> 
> > +	reg = intmux_data->regs + CHANIER(idx);
> > +	val = readl_relaxed(reg);
> > +	/* disable the interrupt source of this channel */
> > +	val &= ~(1 << d->hwirq);
> 
>          val &= ~BIT(d->hwirq);
> 
> > +	writel_relaxed(val, reg);
> > +	spin_unlock(&intmux_data->lock);
> > +}
> > +
> > +static void imx_intmux_irq_unmask(struct irq_data *d) {
> > +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> > +	u32 idx = irqchip_data->chanidx;
> > +	struct intmux_data *intmux_data = container_of(irqchip_data,
> > +				struct intmux_data, irqchip_data[idx]);
> > +	void __iomem *reg;
> > +	u32 val;
> > +
> > +	spin_lock(&intmux_data->lock);
> > +	reg = intmux_data->regs + CHANIER(idx);
> > +	val = readl_relaxed(reg);
> > +	/* enable the interrupt source of this channel */
> > +	val |= 1 << d->hwirq;
> > +	writel_relaxed(val, reg);
> > +	spin_unlock(&intmux_data->lock);
> > +}
> > +
> > +static struct irq_chip imx_intmux_irq_chip = {
> > +	.name		= "intmux",
> > +	.irq_mask	= imx_intmux_irq_mask,
> > +	.irq_unmask	= imx_intmux_irq_unmask,
> > +};
> > +
> > +static int imx_intmux_irq_map(struct irq_domain *h, unsigned int
> > irq,
> > +			      irq_hw_number_t hwirq)
> > +{
> > +	irq_set_status_flags(irq, IRQ_LEVEL);
> > +	irq_set_chip_data(irq, h->host_data);
> > +	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip,
> > handle_level_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops imx_intmux_domain_ops = {
> > +	.map		= imx_intmux_irq_map,
> > +	.xlate		= irq_domain_xlate_onecell,
> > +};
> > +
> > +static void imx_intmux_update_irqstat(struct intmux_irqchip_data
> > *irqchip_data)
> > +{
> > +	int i = irqchip_data->chanidx;
> > +	struct intmux_data *intmux_data = container_of(irqchip_data,
> > +				struct intmux_data, irqchip_data[i]);
> > +
> > +	/* read the interrupt source pending status of this channel */
> > +	irqchip_data->irqstat = readl_relaxed(intmux_data->regs +
> > CHANIPR(i));
> 
> Why does it need to be stored into the data structure, instead of
> sinply being returned by the function?
> 
> > +}
> > +
> > +static void imx_intmux_irq_handler(struct irq_desc *desc)
> > +{
> > +	struct intmux_irqchip_data *irqchip_data =
> > irq_desc_get_handler_data(desc);
> > +	int pos, virq;
> > +
> > +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> > +
> > +	imx_intmux_update_irqstat(irqchip_data);
> > +
> > +	for_each_set_bit(pos, (unsigned long *)&irqchip_data->irqstat, 32)
> > {
> 
> This is broken on big-endian. Never cast a smaller type into unsigned
> long
> if you're going to use any of the bit iterators.
> 
> > +		virq = irq_find_mapping(irqchip_data->domain, pos);
> > +		if (virq)
> > +			generic_handle_irq(virq);
> > +	}
> > +
> > +	chained_irq_exit(irq_desc_get_chip(desc), desc);
> > +}
> > +
> > +static int imx_intmux_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct intmux_data *intmux_data;
> > +	int channum;
> > +	int i, ret;
> > +
> > +	ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
> > +	if (ret)
> > +		channum = 1;
> > +
> > +	intmux_data = devm_kzalloc(&pdev->dev, sizeof(*intmux_data) +
> > +				   channum * sizeof(intmux_data->irqchip_data[0]),
> > +				   GFP_KERNEL);
> > +	if (!intmux_data)
> > +		return -ENOMEM;
> > +
> > +	intmux_data->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(intmux_data->regs)) {
> > +		dev_err(&pdev->dev, "failed to initialize reg\n");
> > +		return PTR_ERR(intmux_data->regs);
> > +	}
> > +
> > +	intmux_data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> > +	if (IS_ERR(intmux_data->ipg_clk)) {
> > +		ret = PTR_ERR(intmux_data->ipg_clk);
> > +		dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	intmux_data->channum = channum;
> > +	intmux_data->pdev = pdev;
> 
> What is the point of keeping track of this? The only instance where you
> go from MUX to device is just below, and you already have the device
> at hand.
> 
> > +	spin_lock_init(&intmux_data->lock);
> > +
> > +	ret = clk_prepare_enable(intmux_data->ipg_clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < channum; i++) {
> > +		intmux_data->irqchip_data[i].chanidx = i;
> > +		intmux_data->irqchip_data[i].irq = platform_get_irq(pdev, i);
> > +		if (intmux_data->irqchip_data[i].irq <= 0) {
> > +			dev_err(&pdev->dev, "failed to get irq\n");
> > +			return -ENODEV;
> > +		}
> > +
> > +		intmux_data->irqchip_data[i].domain = irq_domain_add_linear(
> > +						np,
> > +						32,
> > +						&imx_intmux_domain_ops,
> > +						&intmux_data->irqchip_data[i]);
> 
> Please indent this in a readable manner. If you need an intermediate
> variable,
> so be it. Or have a long line if you want, but don't write things like
> this.
> 
> > +		if (!intmux_data->irqchip_data[i].domain) {
> > +			dev_err(&intmux_data->pdev->dev,
> > +				"failed to create IRQ domain\n");
> > +			return -ENOMEM;
> > +		}
> > +
> > +
> 	irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
> > +						 imx_intmux_irq_handler,
> > +						 &intmux_data->irqchip_data[i]);
> 
> Shouldn't you initialize the HW to some sane state here? Like having
> having all interrupts masked?
> 
> > +	}
> > +
> > +	platform_set_drvdata(pdev, intmux_data);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_intmux_remove(struct platform_device *pdev)
> > +{
> > +	struct intmux_data *intmux_data = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	for (i = 0; i < intmux_data->channum; i++) {
> > +
> 	irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
> > +						 NULL, NULL);
> 
> Same thing here. Shouldn't you make sure that no interrupt can fire
> anymore?
> 
> > +
> > +		irq_domain_remove(intmux_data->irqchip_data[i].domain);
> > +	}
> > +
> > +	platform_set_drvdata(pdev, NULL);
> > +	clk_disable_unprepare(intmux_data->ipg_clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id imx_intmux_id[] = {
> > +	{ .compatible = "fsl,imx-intmux", },
> > +	{ /* sentinel */ },
> > +};
> > +
> > +static struct platform_driver imx_intmux_driver = {
> > +	.driver = {
> > +		.name = "imx-intmux",
> > +		.of_match_table = imx_intmux_id,
> > +	},
> > +	.probe = imx_intmux_probe,
> > +	.remove = imx_intmux_remove,
> > +};
> > +builtin_platform_driver(imx_intmux_driver);
> 
> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...
Joakim Zhang Dec. 18, 2019, 10:11 a.m. UTC | #4
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2019年12月18日 17:41
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; robh+dt@kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de; S.j.
> Wang <shengjiu.wang@nxp.com>; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; Andy Duan <fugang.duan@nxp.com>;
> Aisheng Dong <aisheng.dong@nxp.com>
> Subject: Re: [PATCH 3/3] drivers/irqchip: enable INTMUX interrupt controller
> driver
> 
> On 2019-12-18 07:20, Joakim Zhang wrote:
> > Enable INTMUX interrupt controller driver for i.MX platforms.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/irqchip/Kconfig  | 6 ++++++
> >  drivers/irqchip/Makefile | 1 +
> >  2 files changed, 7 insertions(+)
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > ba152954324b..7e2b1e9d0b45 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -457,6 +457,12 @@ config IMX_IRQSTEER
> >  	help
> >  	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
> >
> > +config IMX_INTMUX
> > +	def_bool y if ARCH_MXC
> > +	select IRQ_DOMAIN
> > +	help
> > +	  Support for the i.MX INTMUX interrupt multiplexer.
> > +
> >  config LS1X_IRQ
> >  	bool "Loongson-1 Interrupt Controller"
> >  	depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
> > e806dda690ea..af976a79d1fb 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -100,6 +100,7 @@ obj-$(CONFIG_CSKY_MPINTC)		+=
> irq-csky-mpintc.o
> >  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
> >  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
> >  obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
> > +obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
> >  obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
> >  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
> >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> 
> Please merge this with the previous patch, it doesn't really warrant a separate
> patch.
> 
> Thanks,

Got it. Thanks.

Best Regards,
Joakim Zhang
>          M.
> --
> Jazz is not dead. It just smells funny...