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

[Yixun Lan <dlan@gentoo.org>]

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