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

[Thomas Gleixner <tglx@xxxxxxxxxxxxx>]

On Thu, Feb 27 2025 at 20:41, Yixun Lan wrote:
> On 10:12 Thu 27 Feb     , Alex Elder wrote:
>> On 2/27/25 5:24 AM, Yixun Lan wrote:
>> > 
>> > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> > index ec6d8e72d980f604ded2bfa2143420e0e0095920..cb874ab5e54a4763d601122becd63b6d759e55d2 100644
>> > --- a/kernel/irq/irqdomain.c
>> > +++ b/kernel/irq/irqdomain.c
>> > @@ -1208,10 +1208,17 @@ int irq_domain_translate_twocell(struct irq_domain *d,
>> >   				 unsigned long *out_hwirq,
>> >   				 unsigned int *out_type)
>> >   {
>> 
>> This function is meant for "twocell".  There is also another function
>> irq_domain_translate_onecell().  Why don't you just create
>> irq_domain_translate_threecell" instead?
>> 
> good question!
>
> it's too many changes for adding "threecell" which I thought not worth
> the effort, or maybe we can rename the function to *twothreecell()?
>
> I'm not sure which way to go is the best, ideas from maintainer are
> welcome

We really want to have explicit functions for two and three cells.
 
>> > +	u32 irq, type;
>> > +
>> >   	if (WARN_ON(fwspec->param_count < 2))
>> >   		return -EINVAL;
>> > -	*out_hwirq = fwspec->param[0];
>> > -	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>> > +
>> > +	irq = fwspec->param_count - 2;
>> > +	type = fwspec->param_count - 1;
> no matter two or three cell, it's always parse the last two cells,
> virtually they are same syntax, which can reuse the *_translate_twocell()
> function perfectly..

Yes, that works but the code is completely non-obvious. So what you
really want is something like this:

int irq_domain_translate_cells(struct irq_domain *d, unsigned long *hwirq,
			       unsigned int *type)
{
        unsigned int cells = fwspec->param_count;

        switch (cells) {
       	case 1:
                *hwirq = fwspec->param[0];
                *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.
	         */
		*hwirq = fwspec->param[cells - 2];
        	*type = fwspec->param[cells - 1] & IRQ_TYPE_SENSE_MASK;
                return 0;
        default:
   		return -EINVAL;
        }
}

Then have inline helpers:

static inline int irq_domain_translate_XXXcell(struct irq_domain *d, unsigned long *hwirq,
					       unsigned int *type)
{
        return irq_domain_translate_cells(d, hwirq, type);
}

That avoids changing all call sites at once and merges the one cell
translation into it.

You get the idea....

Thanks,

        tglx