diff mbox series

[4/5] dt-bindings: net: wireless: ath9k: add new options

Message ID 20240905180928.382090-5-rosenp@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: ath9k: remove platform_data support | expand

Commit Message

Rosen Penev Sept. 5, 2024, 6:09 p.m. UTC
These platform_data options are now available for OF.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Krzysztof Kozlowski Sept. 5, 2024, 9:29 p.m. UTC | #1
On 05/09/2024 20:09, Rosen Penev wrote:
> These platform_data options are now available for OF.

This explains nothing. Describe the hardware, not driver.

> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

> ---
>  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> index 0e5412cff2bc..5c293d558a94 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> @@ -44,6 +44,16 @@ properties:
>  
>    ieee80211-freq-limit: true
>  
> +  qca,led-active-high:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates that the LED pin is active high instead of low

Where is DTS user of it?

Best regards,
Krzysztof
Krzysztof Kozlowski Sept. 5, 2024, 9:33 p.m. UTC | #2
On 05/09/2024 20:09, Rosen Penev wrote:
> These platform_data options are now available for OF.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> index 0e5412cff2bc..5c293d558a94 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> @@ -44,6 +44,16 @@ properties:
>  
>    ieee80211-freq-limit: true
>  
> +  qca,led-active-high:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates that the LED pin is active high instead of low
> +
> +  qca,led-pin:

So this all looks like re-inventing standard GPIOs, at least for devices
using GPIOLIB. Your commit msg needs proper rationale.

Best regards,
Krzysztof
Rosen Penev Sept. 5, 2024, 10:35 p.m. UTC | #3
On Thu, Sep 5, 2024 at 2:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 05/09/2024 20:09, Rosen Penev wrote:
> > These platform_data options are now available for OF.
>
> This explains nothing. Describe the hardware, not driver.
>
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.
>
> Please kindly resend and include all necessary To/Cc entries.
Will do.
> </form letter>
>
> > ---
> >  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > index 0e5412cff2bc..5c293d558a94 100644
> > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > @@ -44,6 +44,16 @@ properties:
> >
> >    ieee80211-freq-limit: true
> >
> > +  qca,led-active-high:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Indicates that the LED pin is active high instead of low
>
> Where is DTS user of it?
None at this time. There are only downstream users.

This maps to the led_active_high in ath9k_platform_device , which gets
removed in a subsequent commit.

drivers/net/wireless/mediatek/mt76/mac80211.c uses led-sources as the
name for the same purpose. Maybe rename?

>
> Best regards,
> Krzysztof
>
Rosen Penev Sept. 5, 2024, 10:38 p.m. UTC | #4
On Thu, Sep 5, 2024 at 2:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 05/09/2024 20:09, Rosen Penev wrote:
> > These platform_data options are now available for OF.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > index 0e5412cff2bc..5c293d558a94 100644
> > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > @@ -44,6 +44,16 @@ properties:
> >
> >    ieee80211-freq-limit: true
> >
> > +  qca,led-active-high:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Indicates that the LED pin is active high instead of low
> > +
> > +  qca,led-pin:
>
> So this all looks like re-inventing standard GPIOs, at least for devices
> using GPIOLIB. Your commit msg needs proper rationale.
git grep led_classdev_register drivers/net/wireless/ | wc -l
16

Both old and new drivers register their own GPIO devices to control LEDs.
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Sept. 6, 2024, 7:18 a.m. UTC | #5
On 06/09/2024 00:35, Rosen Penev wrote:
> On Thu, Sep 5, 2024 at 2:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 05/09/2024 20:09, Rosen Penev wrote:
>>> These platform_data options are now available for OF.
>>
>> This explains nothing. Describe the hardware, not driver.
>>
>>>
>>> Signed-off-by: Rosen Penev <rosenp@gmail.com>
>>
>> <form letter>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC. It might happen, that command when run on an older
>> kernel, gives you outdated entries. Therefore please be sure you base
>> your patches on recent Linux kernel.
>>
>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
>> people, so fix your workflow. Tools might also fail if you work on some
>> ancient tree (don't, instead use mainline) or work on fork of kernel
>> (don't, instead use mainline). Just use b4 and everything should be
>> fine, although remember about `b4 prep --auto-to-cc` if you added new
>> patches to the patchset.
>>
>> You missed at least devicetree list (maybe more), so this won't be
>> tested by automated tooling. Performing review on untested code might be
>> a waste of time.
>>
>> Please kindly resend and include all necessary To/Cc entries.
> Will do.
>> </form letter>
>>
>>> ---
>>>  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
>>> index 0e5412cff2bc..5c293d558a94 100644
>>> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
>>> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
>>> @@ -44,6 +44,16 @@ properties:
>>>
>>>    ieee80211-freq-limit: true
>>>
>>> +  qca,led-active-high:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description:
>>> +      Indicates that the LED pin is active high instead of low
>>
>> Where is DTS user of it?
> None at this time. There are only downstream users.

We do not add stuff for downstream.

> 
> This maps to the led_active_high in ath9k_platform_device , which gets
> removed in a subsequent commit.
> 
> drivers/net/wireless/mediatek/mt76/mac80211.c uses led-sources as the
> name for the same purpose. Maybe rename?

Again, you add undocumented, unused compatibles along with unused
properties. We don't care about downstream kernels. You need upstream users.

Best regards,
Krzysztof
Rosen Penev Sept. 6, 2024, 7:49 p.m. UTC | #6
On Fri, Sep 6, 2024 at 12:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 06/09/2024 00:35, Rosen Penev wrote:
> > On Thu, Sep 5, 2024 at 2:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 05/09/2024 20:09, Rosen Penev wrote:
> >>> These platform_data options are now available for OF.
> >>
> >> This explains nothing. Describe the hardware, not driver.
> >>
> >>>
> >>> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> >>
> >> <form letter>
> >> Please use scripts/get_maintainers.pl to get a list of necessary people
> >> and lists to CC. It might happen, that command when run on an older
> >> kernel, gives you outdated entries. Therefore please be sure you base
> >> your patches on recent Linux kernel.
> >>
> >> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> >> people, so fix your workflow. Tools might also fail if you work on some
> >> ancient tree (don't, instead use mainline) or work on fork of kernel
> >> (don't, instead use mainline). Just use b4 and everything should be
> >> fine, although remember about `b4 prep --auto-to-cc` if you added new
> >> patches to the patchset.
> >>
> >> You missed at least devicetree list (maybe more), so this won't be
> >> tested by automated tooling. Performing review on untested code might be
> >> a waste of time.
> >>
> >> Please kindly resend and include all necessary To/Cc entries.
> > Will do.
> >> </form letter>
> >>
> >>> ---
> >>>  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
> >>>  1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> >>> index 0e5412cff2bc..5c293d558a94 100644
> >>> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> >>> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> >>> @@ -44,6 +44,16 @@ properties:
> >>>
> >>>    ieee80211-freq-limit: true
> >>>
> >>> +  qca,led-active-high:
> >>> +    $ref: /schemas/types.yaml#/definitions/flag
> >>> +    description:
> >>> +      Indicates that the LED pin is active high instead of low
> >>
> >> Where is DTS user of it?
> > None at this time. There are only downstream users.
>
> We do not add stuff for downstream.
>
> >
> > This maps to the led_active_high in ath9k_platform_device , which gets
> > removed in a subsequent commit.
> >
> > drivers/net/wireless/mediatek/mt76/mac80211.c uses led-sources as the
> > name for the same purpose. Maybe rename?
>
> Again, you add undocumented, unused compatibles along with unused
> properties. We don't care about downstream kernels. You need upstream users.
OK. Will remove in v2.
>
> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
index 0e5412cff2bc..5c293d558a94 100644
--- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
@@ -44,6 +44,16 @@  properties:
 
   ieee80211-freq-limit: true
 
+  qca,led-active-high:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates that the LED pin is active high instead of low
+
+  qca,led-pin:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      Sets the GPIO number for the LED pin instead of the default
+
   qca,no-eeprom:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
@@ -75,6 +85,8 @@  examples:
         reg = <0 0 0 0 0>;
         interrupts = <3>;
         qca,no-eeprom;
+        qca,led-active-high;
+        qca,led-pin = /bits/ 8 <11>;
       };
     };
   - |