| Message ID | 20240621225558.280462-1-knaerzche@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: brcmfmac: of: Support interrupts-extended | expand |
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 >> >> >>
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(-)