On Mon, 1 Aug 2016, Sebastian Frias wrote:
> Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
> drivers can use") introduced three similar functions:
>
> irq_domain_xlate_onecell()
> irq_domain_xlate_twocell()
> irq_domain_xlate_onetwocell()
>
> yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the
> two previous ones to avoid code duplication.
>
> Fixes: 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
> drivers can use")
That does not fix anything. It optimizes code. We use the "Fixes" tag only
when the existing code is buggy.
> Signed-off-by: Sebastian Frias <[email protected]>
> ---
>
> NOTE: the factored code is not strictly the same in the sense that
> 16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would
> make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".
So the proper way to do that is to split this into two patches:
#1 Change the existing code to do the masking and explain why it is correct.
#2 Refactor the code and get rid of the duplicated implementation.
> Feel free to comment on that matter.
>
> ---
> kernel/irq/irqdomain.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index bee8b02..125a28c 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -839,9 +839,12 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
> {
> if (WARN_ON(intsize < 1))
> return -EINVAL;
> - *out_hwirq = intspec[0];
> - *out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
> - return 0;
> + if (intsize == 1)
> + return irq_domain_xlate_onecell(d, ctrlr, intspec, intsize,
> + out_hwirq, out_type);
> + else
> + return irq_domain_xlate_twocell(d, ctrlr, intspec, intsize,
> + out_hwirq, out_type);
So I really wonder how much of a saving that change is. I wouldn't be
surprised if it would create worse code on some architectures.
Thanks,
tglx
