Re: [PATCH] wifi: brcmfmac: of: Support interrupts-extended

[Alex Bee]

Am 22.06.24 um 19:46 schrieb Arend Van Spriel:
> On June 22, 2024 4:07:40 PM Alex Bee <knaerzche@xxxxxxxxx> 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@xxxxxxxxxxxx> wrote:
> > >
> > > > On June 22, 2024 12:56:02 AM Alex Bee <knaerzche@xxxxxxxxx> 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