| Message ID | 20240621225558.280462-1-knaerzche@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: brcmfmac: of: Support interrupts-extended | expand |
On June 22, 2024 12:56:02 AM Alex Bee <knaerzche@gmail.com> wrote: > This "new" version of defining external interrupts is around for a very > long time now and supported and preferred by irq_of_parse_and_map > respectively of_irq_parse_one. > > Support it in brcmfmac as well by checking if either "interrupts" or > "interrupts-extended" property exists as indication if irq_of_parse_and_map > should be called. All very interesting, but why should we add code for something that is not specified in the bindings documentation? NAK (for now). Feel free to update the bindings document. Regards, Arend
On June 22, 2024 3:38:32 PM Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > On June 22, 2024 12:56:02 AM Alex Bee <knaerzche@gmail.com> wrote: > >> This "new" version of defining external interrupts is around for a very >> long time now and supported and preferred by irq_of_parse_and_map >> respectively of_irq_parse_one. >> >> Support it in brcmfmac as well by checking if either "interrupts" or >> "interrupts-extended" property exists as indication if irq_of_parse_and_map >> should be called. > > All very interesting, but why should we add code for something that is not > specified in the bindings documentation? > > NAK (for now). Feel free to update the bindings document. So looked up the interrupts-extended definition: The "interrupts-extended" property is a special form; useful when a node needs to reference multiple interrupt parents or a different interrupt parent than the inherited one. Each entry in this property contains both the parent phandle and the interrupt specifier. Given that brcmfmac device will only have one interrupt item defined there is no need to use it. If someone can give a good argument to support it please chime in. Regards, Arend
Am 22.06.24 um 15:49 schrieb Arend Van Spriel: > On June 22, 2024 3:38:32 PM Arend Van Spriel > <arend.vanspriel@broadcom.com> wrote: > >> On June 22, 2024 12:56:02 AM Alex Bee <knaerzche@gmail.com> wrote: >> >>> This "new" version of defining external interrupts is around for a very >>> long time now and supported and preferred by irq_of_parse_and_map >>> respectively of_irq_parse_one. >>> >>> Support it in brcmfmac as well by checking if either "interrupts" or >>> "interrupts-extended" property exists as indication if >>> irq_of_parse_and_map >>> should be called. >> >> All very interesting, but why should we add code for something that >> is not >> specified in the bindings documentation? >> >> NAK (for now). Feel free to update the bindings document. > Sure, if you insist on it, I can update the bindings document. So far not many (no) users did that, as it is clearly specified as an alternative in devicetree/bindings/interrupt-controller/interrupts.txt (sure, it's not yet converted to yaml yet). > So looked up the interrupts-extended definition: > > The "interrupts-extended" property is a special form; useful when a > node needs > to reference multiple interrupt parents or a different interrupt > parent than > the inherited one. Each entry in this property contains both the > parent phandle > and the interrupt specifier. > They point in this particular case is not how many interrupts exsist, but "... or a different interrupt parent than the inherited ..." which is almost always the case for brcmfmac, as it usually specifies a gpio controller as interrupt parent. So: ... interrupt-parent = <&gpio0>; interrupts = <RK_PA0 IRQ_TYPE_LEVEL_HIGH>; ... gets to (a single line): ... interrupts-extended: = <&gpio0 RK_PA0 IRQ_TYPE_LEVEL_HIGH>; ... Which is a much nicer form, imho. And by the way for instance arch/arm/boot/dts/qcom/qcom-apq8026-lg-lenok.dts already uses it that way and the interrupt will currently not picked up (at least not by this driver). Alex > Given that brcmfmac device will only have one interrupt item defined > there is no need to use it. If someone can give a good argument to > support it please chime in. > > Regards, > Arend > > >
Am 22.06.24 um 16:07 schrieb Alex Bee: > > Am 22.06.24 um 15:49 schrieb Arend Van Spriel: >> On June 22, 2024 3:38:32 PM Arend Van Spriel >> <arend.vanspriel@broadcom.com> wrote: >> >>> On June 22, 2024 12:56:02 AM Alex Bee <knaerzche@gmail.com> wrote: >>> >>>> This "new" version of defining external interrupts is around for a >>>> very >>>> long time now and supported and preferred by irq_of_parse_and_map >>>> respectively of_irq_parse_one. >>>> >>>> Support it in brcmfmac as well by checking if either "interrupts" or >>>> "interrupts-extended" property exists as indication if >>>> irq_of_parse_and_map >>>> should be called. >>> >>> All very interesting, but why should we add code for something that >>> is not >>> specified in the bindings documentation? >>> >>> NAK (for now). Feel free to update the bindings document. >> > Sure, if you insist on it, I can update the bindings document. So far not > many (no) users did that, as it is clearly specified as an alternative in > devicetree/bindings/interrupt-controller/interrupts.txt (sure, it's > not yet > converted to yaml yet). If you are worried about schema validation: Some magic will accept both "interrupts" and "interrupts-extended" if only "interrupts" is specified in the binding. Not sure that happens. >> So looked up the interrupts-extended definition: >> >> The "interrupts-extended" property is a special form; useful when a >> node needs >> to reference multiple interrupt parents or a different interrupt >> parent than >> the inherited one. Each entry in this property contains both the >> parent phandle >> and the interrupt specifier. >> > They point in this particular case is not how many interrupts exsist, but > "... or a different interrupt parent than > the inherited ..." which is almost always the case for brcmfmac, as it > usually specifies a gpio controller as interrupt parent. So: > Maybe I should resend with a verbosity-increased commit message? > ... > interrupt-parent = <&gpio0>; > interrupts = <RK_PA0 IRQ_TYPE_LEVEL_HIGH>; > ... > > gets to (a single line): > ... > interrupts-extended: = <&gpio0 RK_PA0 IRQ_TYPE_LEVEL_HIGH>; > ... > > Which is a much nicer form, imho. > And by the way for instance > arch/arm/boot/dts/qcom/qcom-apq8026-lg-lenok.dts already uses it that way > and the interrupt will currently not picked up (at least not by this > driver). > > Alex > >> Given that brcmfmac device will only have one interrupt item defined >> there is no need to use it. If someone can give a good argument to >> support it please chime in. >> >> Regards, >> Arend >> >> >>
On June 22, 2024 4:07:40 PM Alex Bee <knaerzche@gmail.com> wrote: > Am 22.06.24 um 15:49 schrieb Arend Van Spriel: >> On June 22, 2024 3:38:32 PM Arend Van Spriel >> <arend.vanspriel@broadcom.com> wrote: >> >>> On June 22, 2024 12:56:02 AM Alex Bee <knaerzche@gmail.com> wrote: >>> >>>> This "new" version of defining external interrupts is around for a very >>>> long time now and supported and preferred by irq_of_parse_and_map >>>> respectively of_irq_parse_one. >>>> >>>> Support it in brcmfmac as well by checking if either "interrupts" or >>>> "interrupts-extended" property exists as indication if >>>> irq_of_parse_and_map >>>> should be called. >>> >>> All very interesting, but why should we add code for something that >>> is not >>> specified in the bindings documentation? >>> >>> NAK (for now). Feel free to update the bindings document. > Sure, if you insist on it, I can update the bindings document. So far not > many (no) users did that, as it is clearly specified as an alternative in > devicetree/bindings/interrupt-controller/interrupts.txt (sure, it's not yet > converted to yaml yet). Right. So in my opinion that should be handled by the interrupt subsystem. Hence I dived into irq_of_parse_and_map(). I would suggest to open code that: /* make sure there are interrupts defined in the node */ - if (!of_property_present(np, "interrupts")) + if (of_irq_parse_one(...)) return; irq = irq_create_of_mapping(...); > Which is a much nicer form, imho. > And by the way for instance > arch/arm/boot/dts/qcom/qcom-apq8026-lg-lenok.dts already uses it that way > and the interrupt will currently not picked up (at least not by this > driver). I expected the "nicer" argument would be thrown in at some point. Esthetics are never a technical argument, but let's not debate that ;-) Hopefully you can agree with my suggestion. Regards, Arend
Am 22.06.24 um 19:46 schrieb Arend Van Spriel: > On June 22, 2024 4:07:40 PM Alex Bee <knaerzche@gmail.com> wrote: > >> Am 22.06.24 um 15:49 schrieb Arend Van Spriel: >>> On June 22, 2024 3:38:32 PM Arend Van Spriel >>> <arend.vanspriel@broadcom.com> wrote: >>> >>>> On June 22, 2024 12:56:02 AM Alex Bee <knaerzche@gmail.com> wrote: >>>> >>>>> This "new" version of defining external interrupts is around for a >>>>> very >>>>> long time now and supported and preferred by irq_of_parse_and_map >>>>> respectively of_irq_parse_one. >>>>> >>>>> Support it in brcmfmac as well by checking if either "interrupts" or >>>>> "interrupts-extended" property exists as indication if >>>>> irq_of_parse_and_map >>>>> should be called. >>>> >>>> All very interesting, but why should we add code for something that >>>> is not >>>> specified in the bindings documentation? >>>> >>>> NAK (for now). Feel free to update the bindings document. >> Sure, if you insist on it, I can update the bindings document. So far >> not >> many (no) users did that, as it is clearly specified as an >> alternative in >> devicetree/bindings/interrupt-controller/interrupts.txt (sure, it's >> not yet >> converted to yaml yet). > > Right. So in my opinion that should be handled by the interrupt > subsystem. Hence I dived into irq_of_parse_and_map(). I would suggest > to open code that: > And as you can see: It's already handled by the interrupt subsystem - all what prevents it from working in it's intended behavior is this strange of_property_present check. > /* make sure there are interrupts defined in the node */ > - if (!of_property_present(np, "interrupts")) > + if (of_irq_parse_one(...)) > return; > Agreed. That's even better - I also didn't fully understand why this of_property_present was chosen in the first place. Actually I wanted to send something similar first with only calling of_irq_parse_one and return early if it fails, but the result doesn't allow to differentiate between "no interrupt defined" and "interrupt mapping failed" - so open coding it seems required, unfortunately.. > irq = irq_create_of_mapping(...); > >> Which is a much nicer form, imho. >> And by the way for instance >> arch/arm/boot/dts/qcom/qcom-apq8026-lg-lenok.dts already uses it that >> way >> and the interrupt will currently not picked up (at least not by this >> driver). > > I expected the "nicer" argument would be thrown in at some point. > Esthetics are never a technical argument, but let's not debate that > ;-) Hopefully you can agree with my suggestion. > I wouldn't want the 'nicer'-argument to be an argument, as I don't like that either: so sorry for that. My point was: Why wouldn’t we support it in brcmfmac also? So will resend with you suggestion applied: Remove !of_property_present check completely and do the two steps of_irq_parse_one and irq_create_of_mapping separately. Alex > Regards, > Arend > >
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c index e406e11481a6..6cdc941552e3 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c @@ -129,7 +129,8 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, sdio->drive_strength = val; /* make sure there are interrupts defined in the node */ - if (!of_property_present(np, "interrupts")) + if (!of_property_present(np, "interrupts") && + !of_property_present(np, "interrupts-extended")) return; irq = irq_of_parse_and_map(np, 0);
This "new" version of defining external interrupts is around for a very long time now and supported and preferred by irq_of_parse_and_map respectively of_irq_parse_one. Support it in brcmfmac as well by checking if either "interrupts" or "interrupts-extended" property exists as indication if irq_of_parse_and_map should be called. Signed-off-by: Alex Bee <knaerzche@gmail.com> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)