| Message ID | 20250227-04-gpio-irq-threecell-v1-1-4ae4d91baadc@gentoo.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: irq: support describing three-cell interrupts | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
On 2/27/25 5:24 AM, 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. > > Signed-off-by: Yixun Lan <dlan@gentoo.org> > --- > kernel/irq/irqdomain.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > 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? -Alex > + 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; > + > + *out_hwirq = fwspec->param[irq]; > + *out_type = fwspec->param[type] & IRQ_TYPE_SENSE_MASK; > + > return 0; > } > EXPORT_SYMBOL_GPL(irq_domain_translate_twocell); >
On 10:12 Thu 27 Feb , Alex Elder wrote: > On 2/27/25 5:24 AM, 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. > > > > Signed-off-by: Yixun Lan <dlan@gentoo.org> > > --- > > kernel/irq/irqdomain.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > 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 > > > + 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.. > > + > > + *out_hwirq = fwspec->param[irq]; > > + *out_type = fwspec->param[type] & IRQ_TYPE_SENSE_MASK; > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(irq_domain_translate_twocell); > > >
On Thu, Feb 27, 2025 at 9:42 PM Yixun Lan <dlan@gentoo.org> wrote: > > 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 Yeah just rename it twothreecell, that's fine, we will understand it :) Thanks for driving this change, much appreciated! Yours, Linus Walleij
Hi Linus Walleij: On 09:44 Fri 28 Feb , Linus Walleij wrote: > On Thu, Feb 27, 2025 at 9:42 PM Yixun Lan <dlan@gentoo.org> wrote: > > > > 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 > > Yeah just rename it twothreecell, that's fine, we will understand it :) > there will be quite a lot files to touch, which looks a little bit scary but anyway, I'm fine with either way.. $ git grep irq_domain_translate_twocell | cut -f 1 -d ':' | sort -u | wc -l 11 Thomas Gleixner, do you agree with this direction? then I can work on it
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
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) { + 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; + + *out_hwirq = fwspec->param[irq]; + *out_type = fwspec->param[type] & IRQ_TYPE_SENSE_MASK; + return 0; } EXPORT_SYMBOL_GPL(irq_domain_translate_twocell);
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. Signed-off-by: Yixun Lan <dlan@gentoo.org> --- kernel/irq/irqdomain.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)