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

[Alex Bee <knaerzche@gmail.com>]

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
>
>