Messages in this thread | | | Subject | Re: [PATCH v2] irqdomain: factorise irq_domain_xlate_onetwocell() | From | Sebastian Frias <> | Date | Tue, 2 Aug 2016 10:31:26 +0200 |
| |
Hi Thomas,
On 08/01/2016 07:07 PM, Thomas Gleixner wrote: > 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.
Ok, I will remove that.
> >> Signed-off-by: Sebastian Frias <sf84@laposte.net> >> --- >> >> 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.
Ok, I can do two patches.
> > >> 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. >
Maybe it does, although I looked at this from the point of view of reducing duplicated code because of the well known issues duplicated code entails. This case is a good example, since the code was duplicated we ended up with slightly different versions of it.
Best regards,
Sebastian
| |