[PATCH v3 0/2] Add support for AST2700 INTC driver

[PATCH v3 0/2] Add support for AST2700 INTC driver

[Kevin Chen]

---
v3:
aspeed,ast2700-intc.yaml:
  - Change reg with maxIntems:10 only.
  - Change interrupt with maxIntems:10 only.
  - Add description of INT0 and INTC1.
  - Fix the indentation.

drivers/irqchip/irq-aspeed-intc.c:
  - Change to use CONFIG_ARCH_ASPEED.
  - Fix indentation.
  - Remove irq_domain name assignment.
  - Check all irq numbers first, and set chained.

Kevin Chen (2):
  dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC
  irqchip/aspeed-intc: Add support for AST27XX INTC

 .../aspeed,ast2700-intc.yaml                  |  87 +++++++++++
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-aspeed-intc.c             | 139 ++++++++++++++++++
 3 files changed, 227 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
 create mode 100644 drivers/irqchip/irq-aspeed-intc.c

-- 
2.34.1

[PATCH v3 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Kevin Chen]

The ASPEED AST27XX interrupt controller(INTC) contain second level and
third level interrupt controller.

INTC0:
The second level INTC, which used to assert GIC(#192~#197) if interrupt
in INTC1 asserted. There are 6 GIC interrupt number(#192~#197) used in
one INTC0.

INTC1_x:
The third level INTC, which used to assert GIC(#192~#197) if interrupt in
INTC1 asserted. There are 6 GIC interrupt number(#192~#197) used in one INTC0.

Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
---
 .../aspeed,ast2700-intc.yaml                  | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
new file mode 100644
index 000000000000..650a1f6e1177
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed AST2700 Interrupt Controller
+
+description:
+  This interrupt controller hardware is second level interrupt controller that
+  is hooked to a parent interrupt controller. It's useful to combine multiple
+  interrupt sources into 1 interrupt to parent interrupt controller.
+
+maintainers:
+  - Kevin Chen <kevin_chen@aspeedtech.com>
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2700-intc-ic
+
+  reg:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    const: 2
+
+  interrupts:
+    maxItems: 10
+    description:
+      Depend to which INTC0 or INTC1 used.
+      INTC0 and INTC1 are two kinds of interrupt controller with enable and raw
+      status registers for use.
+      INTC0 is used to assert GIC if interrupt in INTC1 asserted.
+      INTC1 is used to assert INTC0 if interrupt of modules asserted.
+      +-----+   +-------+     +---------+---module0
+      | GIC |---| INTC0 |--+--| INTC1_0 |---module2
+      |     |   |       |  |  |         |---...
+      +-----+   +-------+  |  +---------+---module31
+                           |
+                           |   +---------+---module0
+                           +---| INTC1_1 |---module2
+                           |   |         |---...
+                           |   +---------+---module31
+                          ...
+                           |   +---------+---module0
+                           +---| INTC1_5 |---module2
+                               |         |---...
+                               +---------+---module31
+
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - '#interrupt-cells'
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        interrupt-controller@12101b00 {
+            compatible = "aspeed,ast2700-intc-ic";
+            reg = <0 0x12101b00 0 0x10>;
+            #interrupt-cells = <2>;
+            interrupt-controller;
+            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>;
+        };
+    };
-- 
2.34.1

Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Markus Elfring]

> The ASPEED AST27XX interrupt controller(INTC) contain second level and
…
                                                contains?

…
in INTC1 asserted. There are 6 GIC interrupt number(#192~#197) used in
…
                                             numbers?


Regards,
Markus

Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Rob Herring]

On Wed, Oct 09, 2024 at 07:58:12PM +0800, Kevin Chen wrote:
> The ASPEED AST27XX interrupt controller(INTC) contain second level and
> third level interrupt controller.
> 
> INTC0:
> The second level INTC, which used to assert GIC(#192~#197) if interrupt
> in INTC1 asserted. There are 6 GIC interrupt number(#192~#197) used in
> one INTC0.
> 
> INTC1_x:
> The third level INTC, which used to assert GIC(#192~#197) if interrupt in
> INTC1 asserted. There are 6 GIC interrupt number(#192~#197) used in one INTC0.
> 
> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> ---
>  .../aspeed,ast2700-intc.yaml                  | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> new file mode 100644
> index 000000000000..650a1f6e1177
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed AST2700 Interrupt Controller
> +
> +description:
> +  This interrupt controller hardware is second level interrupt controller that
> +  is hooked to a parent interrupt controller. It's useful to combine multiple
> +  interrupt sources into 1 interrupt to parent interrupt controller.
> +
> +maintainers:
> +  - Kevin Chen <kevin_chen@aspeedtech.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2700-intc-ic
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 2

Describe the meaning of the cells here.

> +
> +  interrupts:
> +    maxItems: 10
> +    description:

You need '|' to preserve formatting.

> +      Depend to which INTC0 or INTC1 used.
> +      INTC0 and INTC1 are two kinds of interrupt controller with enable and raw
> +      status registers for use.
> +      INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> +      INTC1 is used to assert INTC0 if interrupt of modules asserted.
> +      +-----+   +-------+     +---------+---module0
> +      | GIC |---| INTC0 |--+--| INTC1_0 |---module2
> +      |     |   |       |  |  |         |---...
> +      +-----+   +-------+  |  +---------+---module31
> +                           |
> +                           |   +---------+---module0
> +                           +---| INTC1_1 |---module2
> +                           |   |         |---...
> +                           |   +---------+---module31
> +                          ...
> +                           |   +---------+---module0
> +                           +---| INTC1_5 |---module2
> +                               |         |---...
> +                               +---------+---module31
> +
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        interrupt-controller@12101b00 {
> +            compatible = "aspeed,ast2700-intc-ic";
> +            reg = <0 0x12101b00 0 0x10>;
> +            #interrupt-cells = <2>;
> +            interrupt-controller;
> +            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>;
> +        };
> +    };
> -- 
> 2.34.1
> 

RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Kevin Chen]

> > The ASPEED AST27XX interrupt controller(INTC) contain second level and
> > third level interrupt controller.
> >
> > INTC0:
> > The second level INTC, which used to assert GIC(#192~#197) if
> > interrupt in INTC1 asserted. There are 6 GIC interrupt
> > number(#192~#197) used in one INTC0.
> >
> > INTC1_x:
> > The third level INTC, which used to assert GIC(#192~#197) if interrupt
> > in
> > INTC1 asserted. There are 6 GIC interrupt number(#192~#197) used in one
> INTC0.
> >
> > Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> > ---
> >  .../aspeed,ast2700-intc.yaml                  | 87
> +++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-
> > intc.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270
> > 0-intc.yaml
> > b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270
> > 0-intc.yaml
> > new file mode 100644
> > index 000000000000..650a1f6e1177
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,as
> > +++ t2700-intc.yaml
> > @@ -0,0 +1,87 @@
> > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause %YAML 1.2
> > +---
> > +$id:
> > +http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-int
> > +c.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Aspeed AST2700 Interrupt Controller
> > +
> > +description:
> > +  This interrupt controller hardware is second level interrupt
> > +controller that
> > +  is hooked to a parent interrupt controller. It's useful to combine
> > +multiple
> > +  interrupt sources into 1 interrupt to parent interrupt controller.
> > +
> > +maintainers:
> > +  - Kevin Chen <kevin_chen@aspeedtech.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - aspeed,ast2700-intc-ic
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> 
> Describe the meaning of the cells here.
I would change to the following code
+  '#interrupt-cells':
+    const: 2
+    description:
+      The first cell is the IRQ number, the second cell is the trigger
+      type as defined in interrupt.txt in this directory.
> 
> > +
> > +  interrupts:
> > +    maxItems: 10
> > +    description:
> 
> You need '|' to preserve formatting.
OK. Thanks

> 
> > +      Depend to which INTC0 or INTC1 used.
> > +      INTC0 and INTC1 are two kinds of interrupt controller with enable
> and raw
> > +      status registers for use.
> > +      INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> > +      INTC1 is used to assert INTC0 if interrupt of modules asserted.
> > +      +-----+   +-------+     +---------+---module0
> > +      | GIC |---| INTC0 |--+--| INTC1_0 |---module2
> > +      |     |   |       |  |  |         |---...
> > +      +-----+   +-------+  |  +---------+---module31
> > +                           |
> > +                           |   +---------+---module0
> > +                           +---| INTC1_1 |---module2
> > +                           |   |         |---...
> > +                           |   +---------+---module31
> > +                          ...
> > +                           |   +---------+---module0
> > +                           +---| INTC1_5 |---module2
> > +                               |         |---...
> > +                               +---------+---module31
> > +
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupt-controller
> > +  - '#interrupt-cells'
> > +  - interrupts
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    bus {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        interrupt-controller@12101b00 {
> > +            compatible = "aspeed,ast2700-intc-ic";
> > +            reg = <0 0x12101b00 0 0x10>;
> > +            #interrupt-cells = <2>;
> > +            interrupt-controller;
> > +            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>;
> > +        };
> > +    };
> > --
> > 2.34.1
> >

[PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Kevin Chen]

Support for the Aspeed Interrupt Controller found on Aspeed Silicon SoCs,
such as the AST2700, which is arm64 architecture.

To support ASPEED interrupt controller(INTC) maps the internal interrupt
sources of the AST27XX device to an parent interrupt controller.
---
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-aspeed-intc.c | 139 ++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+)
 create mode 100644 drivers/irqchip/irq-aspeed-intc.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e3679ec2b9f7..086911bf4db6 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_MVEBU_SEI)			+= irq-mvebu-sei.o
 obj-$(CONFIG_LS_EXTIRQ)			+= irq-ls-extirq.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o irq-aspeed-scu-ic.o
+obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-intc.o
 obj-$(CONFIG_STM32MP_EXTI)		+= irq-stm32mp-exti.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c
new file mode 100644
index 000000000000..ef1c095ad09e
--- /dev/null
+++ b/drivers/irqchip/irq-aspeed-intc.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  Aspeed Interrupt Controller.
+ *
+ *  Copyright (C) 2023 ASPEED Technology Inc.
+ */
+
+#include <linux/bitops.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
+
+#define INTC_INT_ENABLE_REG	0x00
+#define INTC_INT_STATUS_REG	0x04
+#define IRQS_PER_WORD 32
+
+struct aspeed_intc_ic {
+	void __iomem		*base;
+	raw_spinlock_t		gic_lock;
+	raw_spinlock_t		intc_lock;
+	struct irq_domain	*irq_domain;
+};
+
+static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
+{
+	struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long bit, status;
+
+	chained_irq_enter(chip, desc);
+
+	scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
+		status = readl(intc_ic->base + INTC_INT_STATUS_REG);
+		for_each_set_bit(bit, &status, IRQS_PER_WORD) {
+			generic_handle_domain_irq(intc_ic->irq_domain, bit);
+			writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void aspeed_intc_irq_mask(struct irq_data *data)
+{
+	struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
+	unsigned int mask = readl(intc_ic->base + INTC_INT_ENABLE_REG) & ~BIT(data->hwirq);
+
+	guard(raw_spinlock)(&intc_ic->intc_lock);
+	writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
+}
+
+static void aspeed_intc_irq_unmask(struct irq_data *data)
+{
+	struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
+	unsigned int unmask = readl(intc_ic->base + INTC_INT_ENABLE_REG) | BIT(data->hwirq);
+
+	guard(raw_spinlock)(&intc_ic->intc_lock);
+	writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
+}
+
+static struct irq_chip aspeed_intc_chip = {
+	.name			= "ASPEED INTC",
+	.irq_mask		= aspeed_intc_irq_mask,
+	.irq_unmask		= aspeed_intc_irq_unmask,
+};
+
+static int aspeed_intc_ic_map_irq_domain(struct irq_domain *domain, unsigned int irq,
+					 irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &aspeed_intc_chip, handle_level_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops aspeed_intc_ic_irq_domain_ops = {
+	.map = aspeed_intc_ic_map_irq_domain,
+};
+
+static int __init aspeed_intc_ic_of_init(struct device_node *node,
+					 struct device_node *parent)
+{
+	struct aspeed_intc_ic *intc_ic;
+	int ret = 0;
+	int irq, i;
+
+	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
+	if (!intc_ic)
+		return -ENOMEM;
+
+	intc_ic->base = of_iomap(node, 0);
+	if (!intc_ic->base) {
+		pr_err("Failed to iomap intc_ic base\n");
+		ret = -ENOMEM;
+		goto err_free_ic;
+	}
+	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
+	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
+
+	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
+						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
+	if (!intc_ic->irq_domain) {
+		ret = -ENOMEM;
+		goto err_iounmap;
+	}
+
+	raw_spin_lock_init(&intc_ic->gic_lock);
+	raw_spin_lock_init(&intc_ic->intc_lock);
+
+	/* Check all the irq numbers valid. If not, unmaps all the base and frees the data. */
+	for (i = 0; i < of_irq_count(node); i++) {
+		irq = irq_of_parse_and_map(node, i);
+		if (!irq) {
+			pr_err("Failed to get irq number\n");
+			ret = -EINVAL;
+			goto err_iounmap;
+		}
+	}
+
+	for (i = 0; i < of_irq_count(node); i++) {
+		irq = irq_of_parse_and_map(node, i);
+			irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic);
+	}
+
+	return 0;
+
+err_iounmap:
+	iounmap(intc_ic->base);
+err_free_ic:
+	kfree(intc_ic);
+	return ret;
+}
+
+IRQCHIP_DECLARE(ast2700_intc_ic, "aspeed,ast2700-intc-ic", aspeed_intc_ic_of_init);
-- 
2.34.1

Re: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Markus Elfring]

…
> To support ASPEED interrupt controller(INTC) maps the internal interrupt
> sources of the AST27XX device to an parent interrupt controller.
> ---

* I miss your tag “Signed-off-by”.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc2#n396

* How do you think about to choose an additional imperative wording
  for an improved change description?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc2#n94


…
> +++ b/drivers/irqchip/irq-aspeed-intc.c
> @@ -0,0 +1,139 @@
…
> +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
+{
> +	struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long bit, status;

I suggest to reduce the scopes for three local variables.


> +
> +	chained_irq_enter(chip, desc);

Would you become interested to collaborate with another scoped guard
for this programming interface?
https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchip/chained_irq.h#L13


> +
> +	scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
> +		status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> +		for_each_set_bit(bit, &status, IRQS_PER_WORD) {
> +			generic_handle_domain_irq(intc_ic->irq_domain, bit);
> +			writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}


Regards,
Markus

RE: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Kevin Chen]

Hi Markus,

Thank you for your review. Could you check the message below. Thanks a lot.

> 
> …
> > To support ASPEED interrupt controller(INTC) maps the internal
> > interrupt sources of the AST27XX device to an parent interrupt controller.
> > ---
> 
> * I miss your tag “Signed-off-by”.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
> ntation/process/submitting-patches.rst?h=v6.12-rc2#n396
> 
> * How do you think about to choose an additional imperative wording
>   for an improved change description?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
> ntation/process/submitting-patches.rst?h=v6.12-rc2#n94
> 
> 
> …
> > +++ b/drivers/irqchip/irq-aspeed-intc.c
> > @@ -0,0 +1,139 @@
> …
> > +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
> +{
> > +	struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	unsigned long bit, status;
> 
> I suggest to reduce the scopes for three local variables.
May I check the scopes of bit and status usage?
Variables of bit and status are used in for_each_set_bit.
How could I reduce the scopes?

> 
> 
> > +
> > +	chained_irq_enter(chip, desc);
> 
> Would you become interested to collaborate with another scoped guard for
> this programming interface?
> https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchip/chained
> _irq.h#L13
Maybe like the change in the following?

diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c
index ef1c095ad09e..54d1881c56c6 100644
--- a/drivers/irqchip/irq-aspeed-intc.c
+++ b/drivers/irqchip/irq-aspeed-intc.c
@@ -32,7 +32,7 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
        struct irq_chip *chip = irq_desc_get_chip(desc);
        unsigned long bit, status;

-       chained_irq_enter(chip, desc);
+       guard(chained_irq)(desc);

        scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
                status = readl(intc_ic->base + INTC_INT_STATUS_REG);
@@ -41,8 +41,6 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
                        writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
                }
        }
-
-       chained_irq_exit(chip, desc);
 }

 static void aspeed_intc_irq_mask(struct irq_data *data)
diff --git a/include/linux/irqchip/chained_irq.h b/include/linux/irqchip/chained_irq.h
index dd8b3c476666..399a4ae30205 100644
--- a/include/linux/irqchip/chained_irq.h
+++ b/include/linux/irqchip/chained_irq.h
@@ -38,4 +38,5 @@ static inline void chained_irq_exit(struct irq_chip *chip,
                chip->irq_unmask(&desc->irq_data);
 }

+DEFINE_GUARD (chained_irq, struct irq_desc * , chained_irq_exit ( _T ->irq_data.chip, _T ), chained_irq_enter (_T->irq_data.chip, _T))
 #endif /* __IRQCHIP_CHAINED_IRQ_H */


> 
> 
> > +
> > +	scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
> > +		status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> > +		for_each_set_bit(bit, &status, IRQS_PER_WORD) {
> > +			generic_handle_domain_irq(intc_ic->irq_domain, bit);
> > +			writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
> > +		}
> > +	}
> > +
> > +	chained_irq_exit(chip, desc);
> > +}
> 
> 
> Regards,
> Markus

Re: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Markus Elfring]

>> …
>>> +++ b/drivers/irqchip/irq-aspeed-intc.c
>>> @@ -0,0 +1,139 @@
>> …
>>> +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
>> +{
>>> +	struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
>>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>>> +	unsigned long bit, status;
>>
>> I suggest to reduce the scopes for three local variables.
> May I check the scopes of bit and status usage?
> Variables of bit and status are used in for_each_set_bit.
> How could I reduce the scopes?

I propose to move selected variable definitions into corresponding compound statements
(by using extra curly brackets).
https://refactoring.com/catalog/reduceScopeOfVariable.html


>> Would you become interested to collaborate with another scoped guard for
>> this programming interface?
>> https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchip/chained
>> _irq.h#L13
>
> Maybe like the change in the following?
>
> diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c
> index ef1c095ad09e..54d1881c56c6 100644
> --- a/drivers/irqchip/irq-aspeed-intc.c
> +++ b/drivers/irqchip/irq-aspeed-intc.c
> @@ -32,7 +32,7 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
>         struct irq_chip *chip = irq_desc_get_chip(desc);
>         unsigned long bit, status;
>
> -       chained_irq_enter(chip, desc);
> +       guard(chained_irq)(desc);
>
>         scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
>                 status = readl(intc_ic->base + INTC_INT_STATUS_REG);

Perhaps.


> @@ -41,8 +41,6 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
>                         writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
>                 }
>         }
> -
> -       chained_irq_exit(chip, desc);
>  }
…

Probably, yes.


…
> +++ b/include/linux/irqchip/chained_irq.h
> @@ -38,4 +38,5 @@ static inline void chained_irq_exit(struct irq_chip *chip,
>                 chip->irq_unmask(&desc->irq_data);
>  }
>
> +DEFINE_GUARD (chained_irq, struct irq_desc * , chained_irq_exit ( _T ->irq_data.chip, _T ), chained_irq_enter (_T->irq_data.chip, _T))
…

* Such a macro call looks promising.
  Would you like to omit any space characters before open parentheses?

* Would you like to support scoped guard variants accordingly?


Regards,
Markus

RE: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Kevin Chen]

> >> …
> >>> +++ b/drivers/irqchip/irq-aspeed-intc.c
> >>> @@ -0,0 +1,139 @@
> >> …
> >>> +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
> >> +{
> >>> +	struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
> >>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> >>> +	unsigned long bit, status;
> >>
> >> I suggest to reduce the scopes for three local variables.
> > May I check the scopes of bit and status usage?
> > Variables of bit and status are used in for_each_set_bit.
> > How could I reduce the scopes?
> 
> I propose to move selected variable definitions into corresponding compound
> statements (by using extra curly brackets).
> https://refactoring.com/catalog/reduceScopeOfVariable.html
OK. I moved these two local variables into scoped_guard.

+static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
+{
+       struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
+
+       guard(chained_irq)(desc);
+       scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
+               unsigned long bit, status;
+
+               status = readl(intc_ic->base + INTC_INT_STATUS_REG);
+               for_each_set_bit(bit, &status, IRQS_PER_WORD) {
+                       generic_handle_domain_irq(intc_ic->irq_domain, bit);
+                       writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
+               }
+       }
+}


> 
> 
> >> Would you become interested to collaborate with another scoped guard
> >> for this programming interface?
> >> https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqch
> >> ip/chained
> >> _irq.h#L13
> >
> > Maybe like the change in the following?
> >
> > diff --git a/drivers/irqchip/irq-aspeed-intc.c
> > b/drivers/irqchip/irq-aspeed-intc.c
> > index ef1c095ad09e..54d1881c56c6 100644
> > --- a/drivers/irqchip/irq-aspeed-intc.c
> > +++ b/drivers/irqchip/irq-aspeed-intc.c
> > @@ -32,7 +32,7 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc
> *desc)
> >         struct irq_chip *chip = irq_desc_get_chip(desc);
> >         unsigned long bit, status;
> >
> > -       chained_irq_enter(chip, desc);
> > +       guard(chained_irq)(desc);
> >
> >         scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
> >                 status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> 
> Perhaps.
> 
> 
> > @@ -41,8 +41,6 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc
> *desc)
> >                         writel(BIT(bit), intc_ic->base +
> INTC_INT_STATUS_REG);
> >                 }
> >         }
> > -
> > -       chained_irq_exit(chip, desc);
> >  }
> …
> 
> Probably, yes.
> 
> 
> …
> > +++ b/include/linux/irqchip/chained_irq.h
> > @@ -38,4 +38,5 @@ static inline void chained_irq_exit(struct irq_chip *chip,
> >                 chip->irq_unmask(&desc->irq_data);
> >  }
> >
> > +DEFINE_GUARD (chained_irq, struct irq_desc * , chained_irq_exit ( _T
> > +->irq_data.chip, _T ), chained_irq_enter (_T->irq_data.chip, _T))
> …
> 
> * Such a macro call looks promising.
>   Would you like to omit any space characters before open parentheses?
OK. Fixed.

diff --git a/include/linux/irqchip/chained_irq.h b/include/linux/irqchip/chained_irq.h
index dd8b3c476666..7ee29e478152 100644
--- a/include/linux/irqchip/chained_irq.h
+++ b/include/linux/irqchip/chained_irq.h
@@ -38,4 +38,6 @@ static inline void chained_irq_exit(struct irq_chip *chip,
                chip->irq_unmask(&desc->irq_data);
 }

+DEFINE_GUARD(chained_irq, struct irq_desc *, chained_irq_exit((_T->irq_data.chip), (_T)),
+            chained_irq_enter((_T->irq_data.chip), (_T)))

> 
> * Would you like to support scoped guard variants accordingly?
Do you mean that I need to change the MACRO name for this usage?

> 
> 
> Regards,
> Markus

RE: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Markus Elfring]

> > I propose to move selected variable definitions into corresponding compound
> > statements (by using extra curly brackets).
> > https://refactoring.com/catalog/reduceScopeOfVariable.html
> OK. I moved these two local variables into scoped_guard.

Will development interests grow for further refactorings?


> +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
> +{
> +       struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);

Another update candidate (for scope reduction)?


> +
> +       guard(chained_irq)(desc);

Using another macro call “scoped_guard(…) { … }”?


> +       scoped_guard(raw_spinlock, &intc_ic->gic_lock) {

Would you like to reconsider the proposed macro mixture once more?


> +               unsigned long bit, status;
…

…
> +++ b/include/linux/irqchip/chained_irq.h
> @@ -38,4 +38,6 @@ static inline void chained_irq_exit(struct irq_chip *chip,
>                 chip->irq_unmask(&desc->irq_data);
>  }
> 
> +DEFINE_GUARD(chained_irq, struct irq_desc *, chained_irq_exit((_T->irq_data.chip), (_T)),
> +            chained_irq_enter((_T->irq_data.chip), (_T)))

Would you like to add a #include directive in this header file accordingly?

Regards,
Markus

RE: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Kevin Chen]

> 
> > > I propose to move selected variable definitions into corresponding
> > > compound statements (by using extra curly brackets).
> > > https://refactoring.com/catalog/reduceScopeOfVariable.html
> > OK. I moved these two local variables into scoped_guard.
> 
> Will development interests grow for further refactorings?
Do you have any example for this refactorings?

> 
> 
> > +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) {
> > +       struct aspeed_intc_ic *intc_ic =
> > +irq_desc_get_handler_data(desc);
> 
> Another update candidate (for scope reduction)?
Variable of intc_ic used in "scoped_guard(raw_spinlock, &intc_ic->gic_lock) {".
Or, how can I reduce the scope of intc_ic?

> 
> 
> > +
> > +       guard(chained_irq)(desc);
> 
> Using another macro call “scoped_guard(…) { … }”?
Is it necessary to use scoped_guard(...) {...}?
In the end of aspeed_intc_ic_irq_handler, chained_irq_exit would be called as destructor.
Only one reason I thought is that the chained_irq_exit is needed to be called in the middle of aspeed_intc_ic_irq_handler.

> 
> 
> > +       scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
> 
> Would you like to reconsider the proposed macro mixture once more?
Could I check the reason for once more?

> 
> 
> > +               unsigned long bit, status;
> …
> 
> …
> > +++ b/include/linux/irqchip/chained_irq.h
> > @@ -38,4 +38,6 @@ static inline void chained_irq_exit(struct irq_chip *chip,
> >                 chip->irq_unmask(&desc->irq_data);
> >  }
> >
> > +DEFINE_GUARD(chained_irq, struct irq_desc *,
> chained_irq_exit((_T->irq_data.chip), (_T)),
> > +            chained_irq_enter((_T->irq_data.chip), (_T)))
> 
> Would you like to add a #include directive in this header file accordingly?
Can you give me an example?

> 
> Regards,
> Markus

RE: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Markus Elfring]

…
> > > +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) {
…
> > > +       guard(chained_irq)(desc);
> > 
> > Using another macro call “scoped_guard(…) { … }”?
> Is it necessary to use scoped_guard(...) {...}?

It depends on corresponding case disintions.


> > > +       scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
> > 
> > Would you like to reconsider the proposed macro mixture once more?
> Could I check the reason for once more?

Coding style concerns …?


> > > +++ b/include/linux/irqchip/chained_irq.h
> > > @@ -38,4 +38,6 @@ static inline void chained_irq_exit(struct irq_chip *chip,
> > >                 chip->irq_unmask(&desc->irq_data);
> > >  }
> > >
> > > +DEFINE_GUARD(chained_irq, struct irq_desc *,
> > chained_irq_exit((_T->irq_data.chip), (_T)),
> > > +            chained_irq_enter((_T->irq_data.chip), (_T)))
> > 
> > Would you like to add a #include directive in this header file accordingly?
> Can you give me an example?

See also:
https://elixir.bootlin.com/linux/v6.12-rc3/source/include/linux/device.h#L33

Regards,
Markus

Re: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Thomas Gleixner]

On Wed, Oct 09 2024 at 14:32, Markus Elfring wrote:
>> +
>> +	chained_irq_enter(chip, desc);
>
> Would you become interested to collaborate with another scoped guard
> for this programming interface?

Collaborate in which way? What are you collaborating on?

You are merely asking people to do work which you think is useful. You
can do that, but that does not make it useful.

Making a guard variant of chained_irq_enter/exit needs some thought and
a general plan for cleaning the whole chained irq usage up. It's on the
cleanup list already with quite some other items.

We are not adhoc adding a guard variant because guards are hip right
now. And no this does not need a scoped variant ever.

guards are not the panacea for everything.

> https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchip/chained_irq.h#L13

Please refrain from these silly links. People know to find the functions
on their own.

Kevin, please update the change log, add your SOB and move the local
variables (unsigned long bit, status;) into the scoped_guard() zone.

Leave chained_irq_enter/exit() alone and resubmit.

Thanks,

        tglx

RE: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Kevin Chen]

> >> +
> >> +	chained_irq_enter(chip, desc);
> >
> > Would you become interested to collaborate with another scoped guard
> > for this programming interface?
> 
> Collaborate in which way? What are you collaborating on?
> 
> You are merely asking people to do work which you think is useful. You can do
> that, but that does not make it useful.
> 
> Making a guard variant of chained_irq_enter/exit needs some thought and a
> general plan for cleaning the whole chained irq usage up. It's on the cleanup
> list already with quite some other items.
> 
> We are not adhoc adding a guard variant because guards are hip right now.
> And no this does not need a scoped variant ever.
> 
> guards are not the panacea for everything.
> 
> > https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchi
> > p/chained_irq.h#L13
> 
> Please refrain from these silly links. People know to find the functions on their
> own.
> 
> Kevin, please update the change log, add your SOB and move the local
> variables (unsigned long bit, status;) into the scoped_guard() zone.
OK. I will do this change.

> 
> Leave chained_irq_enter/exit() alone and resubmit.
OK. I will send the v4 patch with chained_irq_enter/exit() alone.

Thanks a lot.

> 
> Thanks,
> 
>         tglx

Re: [v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Markus Elfring]

> Making a guard variant of chained_irq_enter/exit needs some thought and
> a general plan for cleaning the whole chained irq usage up. It's on the
> cleanup list already with quite some other items.

I became also curious how API usage will evolve further here.


> We are not adhoc adding a guard variant because guards are hip right now.

Application interests are growing, aren't they?


> And no this does not need a scoped variant ever.

There are subsystems which seem to prefer such a programming interface occasionally.


> guards are not the panacea for everything.
Their usage might become more popular.

Regards,
Markus

Re: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Dan Carpenter]

On Wed, Oct 09, 2024 at 07:58:13PM +0800, Kevin Chen wrote:
> +static int __init aspeed_intc_ic_of_init(struct device_node *node,
> +					 struct device_node *parent)
> +{
> +	struct aspeed_intc_ic *intc_ic;
> +	int ret = 0;
> +	int irq, i;
> +
> +	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
> +	if (!intc_ic)
> +		return -ENOMEM;
> +
> +	intc_ic->base = of_iomap(node, 0);
> +	if (!intc_ic->base) {
> +		pr_err("Failed to iomap intc_ic base\n");
> +		ret = -ENOMEM;
> +		goto err_free_ic;
> +	}
> +	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
> +	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
> +
> +	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
> +						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
> +	if (!intc_ic->irq_domain) {
> +		ret = -ENOMEM;
> +		goto err_iounmap;
> +	}
> +
> +	raw_spin_lock_init(&intc_ic->gic_lock);
> +	raw_spin_lock_init(&intc_ic->intc_lock);
> +
> +	/* Check all the irq numbers valid. If not, unmaps all the base and frees the data. */
> +	for (i = 0; i < of_irq_count(node); i++) {
> +		irq = irq_of_parse_and_map(node, i);
> +		if (!irq) {
> +			pr_err("Failed to get irq number\n");
> +			ret = -EINVAL;
> +			goto err_iounmap;
> +		}
> +	}
> +
> +	for (i = 0; i < of_irq_count(node); i++) {
> +		irq = irq_of_parse_and_map(node, i);
> +			irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic);

There is an extra tab on this line.

regards,
dan carpenter

> +	}
> +
> +	return 0;

RE: [PATCH v3 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Kevin Chen]

> > +static int __init aspeed_intc_ic_of_init(struct device_node *node,
> > +					 struct device_node *parent)
> > +{
> > +	struct aspeed_intc_ic *intc_ic;
> > +	int ret = 0;
> > +	int irq, i;
> > +
> > +	intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
> > +	if (!intc_ic)
> > +		return -ENOMEM;
> > +
> > +	intc_ic->base = of_iomap(node, 0);
> > +	if (!intc_ic->base) {
> > +		pr_err("Failed to iomap intc_ic base\n");
> > +		ret = -ENOMEM;
> > +		goto err_free_ic;
> > +	}
> > +	writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
> > +	writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
> > +
> > +	intc_ic->irq_domain = irq_domain_add_linear(node, 32,
> > +						    &aspeed_intc_ic_irq_domain_ops, intc_ic);
> > +	if (!intc_ic->irq_domain) {
> > +		ret = -ENOMEM;
> > +		goto err_iounmap;
> > +	}
> > +
> > +	raw_spin_lock_init(&intc_ic->gic_lock);
> > +	raw_spin_lock_init(&intc_ic->intc_lock);
> > +
> > +	/* Check all the irq numbers valid. If not, unmaps all the base and frees
> the data. */
> > +	for (i = 0; i < of_irq_count(node); i++) {
> > +		irq = irq_of_parse_and_map(node, i);
> > +		if (!irq) {
> > +			pr_err("Failed to get irq number\n");
> > +			ret = -EINVAL;
> > +			goto err_iounmap;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < of_irq_count(node); i++) {
> > +		irq = irq_of_parse_and_map(node, i);
> > +			irq_set_chained_handler_and_data(irq,
> aspeed_intc_ic_irq_handler, intc_ic);
> 
> There is an extra tab on this line.
OK. Fixed. Thanks a lot.

> 
> regards,
> dan carpenter
> 
> > +	}
> > +
> > +	return 0;
>