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

[Thomas Gleixner <tglx@linutronix.de>]

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