[PATCH v2 0/2] gpio: irq: support describing three-cell interrupts

[PATCH v2 0/2] gpio: irq: support describing three-cell interrupts

[Yixun Lan]

In this patch [1], the GPIO controller add support for describing
hardware with a three-cell scheme:

    gpios = <&gpio instance offset flags>;

It also result describing interrupts in three-cell as this in DT:

    node {
            interrupt-parent = <&gpio>;
            interrupts = <instance hwirq irqflag>;
    }

This series try to extend describing interrupts with three-cell scheme.

The first patch will add capability for parsing irq number and flag
from last two cells which eventually will support the three-cells
interrupt, the second patch support finding irqdomain according to
interrupt instance index.

Link: https://lore.kernel.org/all/20250225-gpio-ranges-fourcell-v3-0-860382ba4713@linaro.org [1]
Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
Changes in v2:
- introduce generic irq_domain_translate_cells(), other inline cells function 
- hide the OF-specific things into gpiolib-of.c|h
- Link to v1: https://lore.kernel.org/r/20250227-04-gpio-irq-threecell-v1-0-4ae4d91baadc@gentoo.org

---
Yixun Lan (2):
      irqdomain: support three-cell scheme interrupts
      gpiolib: support parsing gpio three-cell interrupts scheme

 drivers/gpio/gpiolib-of.c |  8 +++++++
 drivers/gpio/gpiolib-of.h |  6 +++++
 drivers/gpio/gpiolib.c    | 23 +++++++++++++++----
 include/linux/irqdomain.h | 37 +++++++++++++++++++++++--------
 kernel/irq/irqdomain.c    | 56 +++++++++++++++++++----------------------------
 5 files changed, 83 insertions(+), 47 deletions(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250227-04-gpio-irq-threecell-66e1e073c806
prerequisite-change-id: 20250217-gpio-ranges-fourcell-85888ad219da:v3
prerequisite-patch-id: 9d4c8b05cc56d25bfb93f3b06420ba6e93340d31
prerequisite-patch-id: 7949035abd05ec02a9426bb17819d9108e66e0d7

Best regards,
-- 
Yixun Lan

[PATCH v2 1/2] irqdomain: support three-cell scheme interrupts

[Yixun Lan]

The is a prerequisite patch to support parsing three-cell
interrupts which encoded as <instance hwirq irqflag>,
the translate function will always retrieve irq number and
flag from last two cells.

In this patch, we introduce a generic interrupt cells translation
function, others functions will be inline version.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 include/linux/irqdomain.h | 37 +++++++++++++++++++++++--------
 kernel/irq/irqdomain.c    | 56 +++++++++++++++++++----------------------------
 2 files changed, 50 insertions(+), 43 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index e432b6a12a32f9f16ec1ea2fa8e24a649d55caae..d96796263a2e177140f928cb369656a44dd45dda 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -572,15 +572,34 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d, struct device_node *ctrlr,
 			const u32 *intspec, unsigned int intsize,
 			irq_hw_number_t *out_hwirq, unsigned int *out_type);
 
-int irq_domain_translate_twocell(struct irq_domain *d,
-				 struct irq_fwspec *fwspec,
-				 unsigned long *out_hwirq,
-				 unsigned int *out_type);
-
-int irq_domain_translate_onecell(struct irq_domain *d,
-				 struct irq_fwspec *fwspec,
-				 unsigned long *out_hwirq,
-				 unsigned int *out_type);
+int irq_domain_translate_cells(struct irq_domain *d,
+			       struct irq_fwspec *fwspec,
+			       unsigned long *out_hwirq,
+			       unsigned int *out_type);
+
+static inline int irq_domain_translate_onecell(struct irq_domain *d,
+					       struct irq_fwspec *fwspec,
+					       unsigned long *out_hwirq,
+					       unsigned int *out_type)
+{
+	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
+}
+
+static inline int irq_domain_translate_twocell(struct irq_domain *d,
+					       struct irq_fwspec *fwspec,
+					       unsigned long *out_hwirq,
+					       unsigned int *out_type)
+{
+	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
+}
+
+static inline int irq_domain_translate_threecell(struct irq_domain *d,
+						 struct irq_fwspec *fwspec,
+						 unsigned long *out_hwirq,
+						 unsigned int *out_type)
+{
+	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
+}
 
 /* IPI functions */
 int irq_reserve_ipi(struct irq_domain *domain, const struct cpumask *dest);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index ec6d8e72d980f604ded2bfa2143420e0e0095920..8d3b357b7dedbb2c274d4761c315e430b1d35610 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1171,50 +1171,38 @@ const struct irq_domain_ops irq_domain_simple_ops = {
 EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
 
 /**
- * irq_domain_translate_onecell() - Generic translate for direct one cell
+ * irq_domain_translate_cells() - Generic translate for up to three cells
  * bindings
  * @d:		Interrupt domain involved in the translation
  * @fwspec:	The firmware interrupt specifier to translate
  * @out_hwirq:	Pointer to storage for the hardware interrupt number
  * @out_type:	Pointer to storage for the interrupt type
  */
-int irq_domain_translate_onecell(struct irq_domain *d,
-				 struct irq_fwspec *fwspec,
-				 unsigned long *out_hwirq,
-				 unsigned int *out_type)
+int irq_domain_translate_cells(struct irq_domain *d,
+			       struct irq_fwspec *fwspec,
+			       unsigned long *out_hwirq,
+			       unsigned int *out_type)
 {
-	if (WARN_ON(fwspec->param_count < 1))
-		return -EINVAL;
-	*out_hwirq = fwspec->param[0];
-	*out_type = IRQ_TYPE_NONE;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
+	unsigned int cells = fwspec->param_count;
 
-/**
- * irq_domain_translate_twocell() - Generic translate for direct two cell
- * bindings
- * @d:		Interrupt domain involved in the translation
- * @fwspec:	The firmware interrupt specifier to translate
- * @out_hwirq:	Pointer to storage for the hardware interrupt number
- * @out_type:	Pointer to storage for the interrupt type
- *
- * Device Tree IRQ specifier translation function which works with two cell
- * bindings where the cell values map directly to the hwirq number
- * and linux irq flags.
- */
-int irq_domain_translate_twocell(struct irq_domain *d,
-				 struct irq_fwspec *fwspec,
-				 unsigned long *out_hwirq,
-				 unsigned int *out_type)
-{
-	if (WARN_ON(fwspec->param_count < 2))
+	switch (cells) {
+	case 1:
+		*out_hwirq = fwspec->param[0];
+		*out_type = IRQ_TYPE_NONE;
+		return 0;
+	case 2 ... 3:
+		/*
+		 * For multi cell translations the hardware interrupt number
+		 * and type are in the last two cells.
+		 */
+		*out_hwirq = fwspec->param[cells - 2];
+		*out_type = fwspec->param[cells - 1] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	default:
 		return -EINVAL;
-	*out_hwirq = fwspec->param[0];
-	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
-	return 0;
+	}
 }
-EXPORT_SYMBOL_GPL(irq_domain_translate_twocell);
+EXPORT_SYMBOL_GPL(irq_domain_translate_cells);
 
 int irq_domain_alloc_descs(int virq, unsigned int cnt, irq_hw_number_t hwirq,
 			   int node, const struct irq_affinity_desc *affinity)

-- 
2.48.1

[PATCH v2 2/2] gpiolib: support parsing gpio three-cell interrupts scheme

[Yixun Lan]

gpio irq which using three-cell scheme should always call
instance_match() function to find the correct irqdomain.

The select() function will be called with !DOMAIN_BUS_ANY,
so for specific gpio irq driver, it need to set bus token
explicitly, something like:
  irq_domain_update_bus_token(girq->domain, DOMAIN_BUS_WIRED);

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 drivers/gpio/gpiolib-of.c |  8 ++++++++
 drivers/gpio/gpiolib-of.h |  6 ++++++
 drivers/gpio/gpiolib.c    | 23 +++++++++++++++++++----
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 2e537ee979f3e2b6e8d5f86f3e121a66f2a8e083..e19904569fb1b71c1fff237132d17050ef02b074 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -1187,3 +1187,11 @@ void of_gpiochip_remove(struct gpio_chip *chip)
 {
 	of_node_put(dev_of_node(&chip->gpiodev->dev));
 }
+
+bool of_gpiochip_instance_match(struct gpio_chip *gc, unsigned int index)
+{
+	if (gc->of_node_instance_match)
+		return gc->of_node_instance_match(gc, index);
+
+	return false;
+}
diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
index 16d6ac8cb156c02232ea868b755bbdc46c78e3c7..3eebfac290c571e3b90e4437295db8eaacb021a3 100644
--- a/drivers/gpio/gpiolib-of.h
+++ b/drivers/gpio/gpiolib-of.h
@@ -22,6 +22,7 @@ struct gpio_desc *of_find_gpio(struct device_node *np,
 			       unsigned long *lookupflags);
 int of_gpiochip_add(struct gpio_chip *gc);
 void of_gpiochip_remove(struct gpio_chip *gc);
+bool of_gpiochip_instance_match(struct gpio_chip *gc, unsigned int index);
 int of_gpio_count(const struct fwnode_handle *fwnode, const char *con_id);
 #else
 static inline struct gpio_desc *of_find_gpio(struct device_node *np,
@@ -33,6 +34,11 @@ static inline struct gpio_desc *of_find_gpio(struct device_node *np,
 }
 static inline int of_gpiochip_add(struct gpio_chip *gc) { return 0; }
 static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
+static inline bool of_gpiochip_instance_match(struct gpio_chip *gc,
+					      unsigned int index)
+{
+	return false;
+}
 static inline int of_gpio_count(const struct fwnode_handle *fwnode,
 				const char *con_id)
 {
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 679ed764cb143c4b3357106de1570e8d38441372..266be465b9103c17861a0d76f2dfbf1f1de3a073 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1449,10 +1449,9 @@ static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d,
 						   unsigned long *hwirq,
 						   unsigned int *type)
 {
-	/* We support standard DT translation */
-	if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
-		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
-	}
+	/* We support standard DT translation up to three cells */
+	if (is_of_node(fwspec->fwnode))
+		return irq_domain_translate_cells(d, fwspec, hwirq, type);
 
 	/* This is for board files and others not using DT */
 	if (is_fwnode_irqchip(fwspec->fwnode)) {
@@ -1754,9 +1753,25 @@ static void gpiochip_irq_unmap(struct irq_domain *d, unsigned int irq)
 	irq_set_chip_data(irq, NULL);
 }
 
+static int gpiochip_irq_select(struct irq_domain *d, struct irq_fwspec *fwspec,
+			enum irq_domain_bus_token bus_token)
+{
+	struct fwnode_handle *fwnode = fwspec->fwnode;
+	struct gpio_chip *gc = d->host_data;
+	unsigned int index = fwspec->param[0];
+
+	if (fwspec->param_count == 3 && is_of_node(fwnode))
+		return of_gpiochip_instance_match(gc, index);
+
+	/* Fallback for twocells */
+	return ((fwnode != NULL) && (d->fwnode == fwnode) &&
+		(d->bus_token == bus_token));
+}
+
 static const struct irq_domain_ops gpiochip_domain_ops = {
 	.map	= gpiochip_irq_map,
 	.unmap	= gpiochip_irq_unmap,
+	.select	= gpiochip_irq_select,
 	/* Virtually all GPIO irqchips are twocell:ed */
 	.xlate	= irq_domain_xlate_twocell,
 };

-- 
2.48.1

Re: [PATCH v2 2/2] gpiolib: support parsing gpio three-cell interrupts scheme

[Yixun Lan]

Hi Linus, Thomas:

On 07:15 Sun 02 Mar     , Yixun Lan wrote:
> gpio irq which using three-cell scheme should always call
> instance_match() function to find the correct irqdomain.
> 
> The select() function will be called with !DOMAIN_BUS_ANY,
> so for specific gpio irq driver, it need to set bus token
> explicitly, something like:
>   irq_domain_update_bus_token(girq->domain, DOMAIN_BUS_WIRED);
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  drivers/gpio/gpiolib-of.c |  8 ++++++++
>  drivers/gpio/gpiolib-of.h |  6 ++++++
>  drivers/gpio/gpiolib.c    | 23 +++++++++++++++++++----
>  3 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 2e537ee979f3e2b6e8d5f86f3e121a66f2a8e083..e19904569fb1b71c1fff237132d17050ef02b074 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -1187,3 +1187,11 @@ void of_gpiochip_remove(struct gpio_chip *chip)
>  {
>  	of_node_put(dev_of_node(&chip->gpiodev->dev));
>  }
> +
> +bool of_gpiochip_instance_match(struct gpio_chip *gc, unsigned int index)
> +{
> +	if (gc->of_node_instance_match)
> +		return gc->of_node_instance_match(gc, index);
> +
> +	return false;
> +}
> diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
> index 16d6ac8cb156c02232ea868b755bbdc46c78e3c7..3eebfac290c571e3b90e4437295db8eaacb021a3 100644
> --- a/drivers/gpio/gpiolib-of.h
> +++ b/drivers/gpio/gpiolib-of.h
> @@ -22,6 +22,7 @@ struct gpio_desc *of_find_gpio(struct device_node *np,
>  			       unsigned long *lookupflags);
>  int of_gpiochip_add(struct gpio_chip *gc);
>  void of_gpiochip_remove(struct gpio_chip *gc);
> +bool of_gpiochip_instance_match(struct gpio_chip *gc, unsigned int index);
>  int of_gpio_count(const struct fwnode_handle *fwnode, const char *con_id);
>  #else
>  static inline struct gpio_desc *of_find_gpio(struct device_node *np,
> @@ -33,6 +34,11 @@ static inline struct gpio_desc *of_find_gpio(struct device_node *np,
>  }
>  static inline int of_gpiochip_add(struct gpio_chip *gc) { return 0; }
>  static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
> +static inline bool of_gpiochip_instance_match(struct gpio_chip *gc,
> +					      unsigned int index)
> +{
> +	return false;
> +}
>  static inline int of_gpio_count(const struct fwnode_handle *fwnode,
>  				const char *con_id)
>  {
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 679ed764cb143c4b3357106de1570e8d38441372..266be465b9103c17861a0d76f2dfbf1f1de3a073 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1449,10 +1449,9 @@ static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d,
>  						   unsigned long *hwirq,
>  						   unsigned int *type)
>  {
> -	/* We support standard DT translation */
> -	if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
> -		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
> -	}
> +	/* We support standard DT translation up to three cells */
> +	if (is_of_node(fwspec->fwnode))
> +		return irq_domain_translate_cells(d, fwspec, hwirq, type);

I'm not sure if you like this way for calling generic cells parser here,
as it should work for 1, 2, 3 cells model, which save a few lines meanwhile

otherwise we end something like

	if (is_of_node(fwspec->fwnode) {
		if (fwspec->param_count == 2) {
			return irq_domain_translate_twocell(d, fwspec, hwirq, type);
		if (fwspec->param_count == 3) {
			return irq_domain_translate_threecell(d, fwspec, hwirq, type);
	}
>  
>  	/* This is for board files and others not using DT */
>  	if (is_fwnode_irqchip(fwspec->fwnode)) {
> @@ -1754,9 +1753,25 @@ static void gpiochip_irq_unmap(struct irq_domain *d, unsigned int irq)
>  	irq_set_chip_data(irq, NULL);
>  }
>  
> +static int gpiochip_irq_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> +			enum irq_domain_bus_token bus_token)
> +{
> +	struct fwnode_handle *fwnode = fwspec->fwnode;
> +	struct gpio_chip *gc = d->host_data;
> +	unsigned int index = fwspec->param[0];
> +
> +	if (fwspec->param_count == 3 && is_of_node(fwnode))
> +		return of_gpiochip_instance_match(gc, index);
> +
> +	/* Fallback for twocells */
> +	return ((fwnode != NULL) && (d->fwnode == fwnode) &&
> +		(d->bus_token == bus_token));
> +}
> +
>  static const struct irq_domain_ops gpiochip_domain_ops = {
>  	.map	= gpiochip_irq_map,
>  	.unmap	= gpiochip_irq_unmap,
> +	.select	= gpiochip_irq_select,
>  	/* Virtually all GPIO irqchips are twocell:ed */
>  	.xlate	= irq_domain_xlate_twocell,
>  };
> 
> -- 
> 2.48.1
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Re: [PATCH v2 1/2] irqdomain: support three-cell scheme interrupts

[Thomas Gleixner]

On Sun, Mar 02 2025 at 07:15, Yixun Lan wrote:
> The is a prerequisite patch to support parsing three-cell
> interrupts which encoded as <instance hwirq irqflag>,
> the translate function will always retrieve irq number and
> flag from last two cells.
>
> In this patch, we introduce a generic interrupt cells translation
> function, others functions will be inline version.

Please read:

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
  https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes

> +int irq_domain_translate_cells(struct irq_domain *d,
> +			       struct irq_fwspec *fwspec,
> +			       unsigned long *out_hwirq,
> +			       unsigned int *out_type);

Please get rid of the extra line breaks. You have 100 (99) characters available.

> +static inline int irq_domain_translate_onecell(struct irq_domain *d,
> +					       struct irq_fwspec *fwspec,
> +					       unsigned long *out_hwirq,
> +					       unsigned int *out_type)
> +{
> +	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
> +}
> +
> +static inline int irq_domain_translate_twocell(struct irq_domain *d,
> +					       struct irq_fwspec *fwspec,
> +					       unsigned long *out_hwirq,
> +					       unsigned int *out_type)
> +{
> +	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
> +}
> +
> +static inline int irq_domain_translate_threecell(struct irq_domain *d,
> +						 struct irq_fwspec *fwspec,
> +						 unsigned long *out_hwirq,
> +						 unsigned int *out_type)
> +{
> +	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
> +}

What's this for? It's not used. The onecell/twocell wrappers are just
there to keep the current code working.
  
> +int irq_domain_translate_cells(struct irq_domain *d,
> +			       struct irq_fwspec *fwspec,
> +			       unsigned long *out_hwirq,
> +			       unsigned int *out_type)

Please remove the extra line breaks.

int irq_domain_translate_cells(struct irq_domain *d, struct irq_fwspec *fwspec,
			       unsigned long *out_hwirq, unsigned int *out_type)

is perfectly fine.

>  {
> -	if (WARN_ON(fwspec->param_count < 1))
> -		return -EINVAL;
> -	*out_hwirq = fwspec->param[0];
> -	*out_type = IRQ_TYPE_NONE;
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
> +	unsigned int cells = fwspec->param_count;
>  
> -/**
> - * irq_domain_translate_twocell() - Generic translate for direct two cell
> - * bindings
> - * @d:		Interrupt domain involved in the translation
> - * @fwspec:	The firmware interrupt specifier to translate
> - * @out_hwirq:	Pointer to storage for the hardware interrupt number
> - * @out_type:	Pointer to storage for the interrupt type
> - *
> - * Device Tree IRQ specifier translation function which works with two cell
> - * bindings where the cell values map directly to the hwirq number
> - * and linux irq flags.
> - */
> -int irq_domain_translate_twocell(struct irq_domain *d,
> -				 struct irq_fwspec *fwspec,
> -				 unsigned long *out_hwirq,
> -				 unsigned int *out_type)
> -{
> -	if (WARN_ON(fwspec->param_count < 2))
> +	switch (cells) {
> +	case 1:
> +		*out_hwirq = fwspec->param[0];
> +		*out_type = IRQ_TYPE_NONE;
> +		return 0;
> +	case 2 ... 3:

I have second thoughts about this when looking deeper.

The current one/two cell implementations validate that param_count is at
least the number of parameters. Which means that the parameter count
could be larger, but only evaluates the first one or the first two.

I have no idea whether this matters or not, but arguably a two cell
fwspec could be successfully fed into translate_onecell(), no?

And that triggers a related question.

Why is the three cell translation not following the one/two cell scheme
and has the parameters at the same place (index 0,1), i.e. adding the
extra information at the end? That makes sense to me as the extra cell
is obviously not directly related to the interrupt mapping.

Thanks,

        tglx

Re: [PATCH v2 1/2] irqdomain: support three-cell scheme interrupts

[Yixun Lan]

Hello, Thomas Gleixner:

On 19:30 Sun 02 Mar     , Thomas Gleixner wrote:
> On Sun, Mar 02 2025 at 07:15, Yixun Lan wrote:
> > The is a prerequisite patch to support parsing three-cell
> > interrupts which encoded as <instance hwirq irqflag>,
> > the translate function will always retrieve irq number and
> > flag from last two cells.
> >
> > In this patch, we introduce a generic interrupt cells translation
> > function, others functions will be inline version.
> 
> Please read:
> 
>   https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>   https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes
> 
> > +int irq_domain_translate_cells(struct irq_domain *d,
> > +			       struct irq_fwspec *fwspec,
> > +			       unsigned long *out_hwirq,
> > +			       unsigned int *out_type);
> 
> Please get rid of the extra line breaks. You have 100 (99) characters available.
> 
> > +static inline int irq_domain_translate_onecell(struct irq_domain *d,
> > +					       struct irq_fwspec *fwspec,
> > +					       unsigned long *out_hwirq,
> > +					       unsigned int *out_type)
> > +{
> > +	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
> > +}
> > +
> > +static inline int irq_domain_translate_twocell(struct irq_domain *d,
> > +					       struct irq_fwspec *fwspec,
> > +					       unsigned long *out_hwirq,
> > +					       unsigned int *out_type)
> > +{
> > +	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
> > +}
> > +
> > +static inline int irq_domain_translate_threecell(struct irq_domain *d,
> > +						 struct irq_fwspec *fwspec,
> > +						 unsigned long *out_hwirq,
> > +						 unsigned int *out_type)
> > +{
> > +	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
> > +}
> 
> What's this for? It's not used. The onecell/twocell wrappers are just
> there to keep the current code working.
>   
> > +int irq_domain_translate_cells(struct irq_domain *d,
> > +			       struct irq_fwspec *fwspec,
> > +			       unsigned long *out_hwirq,
> > +			       unsigned int *out_type)
> 
> Please remove the extra line breaks.
> 
> int irq_domain_translate_cells(struct irq_domain *d, struct irq_fwspec *fwspec,
> 			       unsigned long *out_hwirq, unsigned int *out_type)
> 
> is perfectly fine.
> 
> >  {
> > -	if (WARN_ON(fwspec->param_count < 1))
> > -		return -EINVAL;
> > -	*out_hwirq = fwspec->param[0];
> > -	*out_type = IRQ_TYPE_NONE;
> > -	return 0;
> > -}
> > -EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
> > +	unsigned int cells = fwspec->param_count;
> >  
> > -/**
> > - * irq_domain_translate_twocell() - Generic translate for direct two cell
> > - * bindings
> > - * @d:		Interrupt domain involved in the translation
> > - * @fwspec:	The firmware interrupt specifier to translate
> > - * @out_hwirq:	Pointer to storage for the hardware interrupt number
> > - * @out_type:	Pointer to storage for the interrupt type
> > - *
> > - * Device Tree IRQ specifier translation function which works with two cell
> > - * bindings where the cell values map directly to the hwirq number
> > - * and linux irq flags.
> > - */
> > -int irq_domain_translate_twocell(struct irq_domain *d,
> > -				 struct irq_fwspec *fwspec,
> > -				 unsigned long *out_hwirq,
> > -				 unsigned int *out_type)
> > -{
> > -	if (WARN_ON(fwspec->param_count < 2))
> > +	switch (cells) {
> > +	case 1:
> > +		*out_hwirq = fwspec->param[0];
> > +		*out_type = IRQ_TYPE_NONE;
> > +		return 0;
> > +	case 2 ... 3:
> 
> I have second thoughts about this when looking deeper.
> 
> The current one/two cell implementations validate that param_count is at
> least the number of parameters. Which means that the parameter count
> could be larger, but only evaluates the first one or the first two.
> 
> I have no idea whether this matters or not, but arguably a two cell
> fwspec could be successfully fed into translate_onecell(), no?
> 
> And that triggers a related question.
> 
> Why is the three cell translation not following the one/two cell scheme
> and has the parameters at the same place (index 0,1), i.e. adding the
> extra information at the end? That makes sense to me as the extra cell
> is obviously not directly related to the interrupt mapping.

this from gpio DT
 gpios = <&gpio instance offset flags>;

I think we currently just following the scheme with gpio cells order
scheme, which is (index(instance) offset flag..), the index and offset
are parameters to locate the irq which can easily derive from global
gpio pin number, so I thought it's more intuitive to group them 
orderly together..

But you really raise a good suggestion here, if we follow appending extra
information at the end, then it will make threecell translate scheme
compatible with twocell, which mean we don't really need extra function
to prase, and can eventually drop this patch, I personally like this direction.

hi, Linus Walleij, what do you think on this?

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Re: [PATCH v2 1/2] irqdomain: support three-cell scheme interrupts

[Linus Walleij]

On Mon, Mar 3, 2025 at 1:40 PM Yixun Lan <dlan@gentoo.org> wrote:
> On 19:30 Sun 02 Mar     , Thomas Gleixner wrote:

> > Why is the three cell translation not following the one/two cell scheme
> > and has the parameters at the same place (index 0,1), i.e. adding the
> > extra information at the end? That makes sense to me as the extra cell
> > is obviously not directly related to the interrupt mapping.
>
> I think we currently just following the scheme with gpio cells order
> scheme, which is (index(instance) offset flag..), the index and offset
> are parameters to locate the irq which can easily derive from global
> gpio pin number, so I thought it's more intuitive to group them
> orderly together..

Right, the DT bindings are mainly for human consumption, and the
cells are positioned in left-to-right intuitive order.

If they were only for machines it would be another issue, but it's
people who have to write and maintain these files.

For example, in a library a machine could arrange books by
first letter in the title, then by second letter in the title etc, but
that would be very confusing for humans who expect to find
them in author order.

There are many examples of this in the DT bindings.

Yours,
Linus Walleij

Re: [PATCH v2 1/2] irqdomain: support three-cell scheme interrupts

[Yixun Lan]

Hi Linus Walleij:

On 08:31 Tue 04 Mar     , Linus Walleij wrote:
> On Mon, Mar 3, 2025 at 1:40 PM Yixun Lan <dlan@gentoo.org> wrote:
> > On 19:30 Sun 02 Mar     , Thomas Gleixner wrote:
> 
> > > Why is the three cell translation not following the one/two cell scheme
> > > and has the parameters at the same place (index 0,1), i.e. adding the
> > > extra information at the end? That makes sense to me as the extra cell
> > > is obviously not directly related to the interrupt mapping.
> >
> > I think we currently just following the scheme with gpio cells order
> > scheme, which is (index(instance) offset flag..), the index and offset
> > are parameters to locate the irq which can easily derive from global
> > gpio pin number, so I thought it's more intuitive to group them
> > orderly together..
> 
> Right, the DT bindings are mainly for human consumption, and the
> cells are positioned in left-to-right intuitive order.
> 
> If they were only for machines it would be another issue, but it's
> people who have to write and maintain these files.
> 
> For example, in a library a machine could arrange books by
> first letter in the title, then by second letter in the title etc, but
> that would be very confusing for humans who expect to find
> them in author order.
> 
> There are many examples of this in the DT bindings.
> 
Ok, I got your idea.. thanks
I will rework the patch to address Thomas's concern

> Yours,
> Linus Walleij
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Re: [PATCH v2 2/2] gpiolib: support parsing gpio three-cell interrupts scheme

[Linus Walleij]

On Sun, Mar 2, 2025 at 12:16 AM Yixun Lan <dlan@gentoo.org> wrote:

> gpio irq which using three-cell scheme should always call
> instance_match() function to find the correct irqdomain.
>
> The select() function will be called with !DOMAIN_BUS_ANY,
> so for specific gpio irq driver, it need to set bus token
> explicitly, something like:
>   irq_domain_update_bus_token(girq->domain, DOMAIN_BUS_WIRED);
>
> Signed-off-by: Yixun Lan <dlan@gentoo.org>

I read up on the bus token code and it seems to do the right thing!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 1/2] irqdomain: support three-cell scheme interrupts

[Yixun Lan]

Hi Thomas:

On 19:30 Sun 02 Mar     , Thomas Gleixner wrote:
> On Sun, Mar 02 2025 at 07:15, Yixun Lan wrote:
> > The is a prerequisite patch to support parsing three-cell
> > interrupts which encoded as <instance hwirq irqflag>,
> > the translate function will always retrieve irq number and
> > flag from last two cells.
> >
> > In this patch, we introduce a generic interrupt cells translation
> > function, others functions will be inline version.
> 
> Please read:
> 
>   https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>   https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes
> 
> > +int irq_domain_translate_cells(struct irq_domain *d,
> > +			       struct irq_fwspec *fwspec,
> > +			       unsigned long *out_hwirq,
> > +			       unsigned int *out_type);
> 
> Please get rid of the extra line breaks. You have 100 (99) characters available.
> 
> > +static inline int irq_domain_translate_onecell(struct irq_domain *d,
> > +					       struct irq_fwspec *fwspec,
> > +					       unsigned long *out_hwirq,
> > +					       unsigned int *out_type)
> > +{
> > +	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
> > +}
> > +
> > +static inline int irq_domain_translate_twocell(struct irq_domain *d,
> > +					       struct irq_fwspec *fwspec,
> > +					       unsigned long *out_hwirq,
> > +					       unsigned int *out_type)
> > +{
> > +	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
> > +}
> > +
> > +static inline int irq_domain_translate_threecell(struct irq_domain *d,
> > +						 struct irq_fwspec *fwspec,
> > +						 unsigned long *out_hwirq,
> > +						 unsigned int *out_type)
> > +{
> > +	return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type);
> > +}
> 
> What's this for? It's not used. The onecell/twocell wrappers are just
> there to keep the current code working.
>   
> > +int irq_domain_translate_cells(struct irq_domain *d,
> > +			       struct irq_fwspec *fwspec,
> > +			       unsigned long *out_hwirq,
> > +			       unsigned int *out_type)
> 
> Please remove the extra line breaks.
> 
> int irq_domain_translate_cells(struct irq_domain *d, struct irq_fwspec *fwspec,
> 			       unsigned long *out_hwirq, unsigned int *out_type)
> 
> is perfectly fine.
> 
> >  {
> > -	if (WARN_ON(fwspec->param_count < 1))
> > -		return -EINVAL;
> > -	*out_hwirq = fwspec->param[0];
> > -	*out_type = IRQ_TYPE_NONE;
> > -	return 0;
> > -}
> > -EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
> > +	unsigned int cells = fwspec->param_count;
> >  
> > -/**
> > - * irq_domain_translate_twocell() - Generic translate for direct two cell
> > - * bindings
> > - * @d:		Interrupt domain involved in the translation
> > - * @fwspec:	The firmware interrupt specifier to translate
> > - * @out_hwirq:	Pointer to storage for the hardware interrupt number
> > - * @out_type:	Pointer to storage for the interrupt type
> > - *
> > - * Device Tree IRQ specifier translation function which works with two cell
> > - * bindings where the cell values map directly to the hwirq number
> > - * and linux irq flags.
> > - */
> > -int irq_domain_translate_twocell(struct irq_domain *d,
> > -				 struct irq_fwspec *fwspec,
> > -				 unsigned long *out_hwirq,
> > -				 unsigned int *out_type)
> > -{
> > -	if (WARN_ON(fwspec->param_count < 2))
> > +	switch (cells) {
> > +	case 1:
> > +		*out_hwirq = fwspec->param[0];
> > +		*out_type = IRQ_TYPE_NONE;
> > +		return 0;
> > +	case 2 ... 3:
> 
> I have second thoughts about this when looking deeper.
> 
> The current one/two cell implementations validate that param_count is at
> least the number of parameters. Which means that the parameter count
> could be larger, but only evaluates the first one or the first two.
> 
> I have no idea whether this matters or not, but arguably a two cell
> fwspec could be successfully fed into translate_onecell(), no?
> 
I think this isn't a problem, the function translate_onecell() or twocell()
will be called explicitly, so they will parse cells with only wanted number
but ignore additional one

when come to this, I do think there is problem in my previous patch
that implicitly extend the translate_twocell() to parse three cells,
I will try to fix this in next version by introducing a threecell()
or translate_twothreecell() function for corresponding cases.

> And that triggers a related question.
> 
> Why is the three cell translation not following the one/two cell scheme
> and has the parameters at the same place (index 0,1), i.e. adding the
> extra information at the end? That makes sense to me as the extra cell
> is obviously not directly related to the interrupt mapping.
> 
> Thanks,
> 
>         tglx

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55