[PATCH net-next 0/4] net: dsa: realtek: fix LED support for rtl8366

[PATCH net-next 0/4] net: dsa: realtek: fix LED support for rtl8366

[Luiz Angelo Daros de Luca]

This series fixes the LED support for rtl8366. The existing code was not
tested in a device with switch LEDs and it was using a flawed logic.

The driver now keeps the default LED configuration if nothing requests a
different behavior. This may be enough for most devices. This can be
achieved either by omitting the LED from the device-tree or configuring
all LEDs in a group with the default state set to "keep".

The hardware trigger for LEDs in Realtek switches is shared among all
LEDs in a group. This behavior doesn't align well with the Linux LED
API, which controls LEDs individually. Once the OS changes the
brightness of a LED in a group still triggered by the hardware, the
entire group switches to software-controlled LEDs. This shared behavior
also prevents offloading the trigger to the hardware as it would require
an orchestration between LEDs in a group, not currently present in the
LED API.

The assertion of device hardware reset during driver removal was removed
because it was causing an issue with the LED release code. Devres
devices are released after the driver's removal is executed. Asserting
the reset at that point was causing timeout errors during LED release
when it attempted to turn off the LED.
 

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
Changes in v1:
- Rebased on new relatek DSA drivers
- Improved commit messages
- Added commit to remove the reset assert during .remove
- Link to RFC: https://lore.kernel.org/r/20240106184651.3665-1-luizluca@gmail.com

---
Luiz Angelo Daros de Luca (4):
      dt-bindings: net: dsa: realtek: describe LED usage
      net: dsa: realtek: keep default LED state in rtl8366rb
      net: dsa: realtek: do not assert reset on remove
      net: dsa: realtek: add LED drivers for rtl8366rb

 .../devicetree/bindings/net/dsa/realtek.yaml       |  46 +++
 drivers/net/dsa/realtek/rtl8366rb.c                | 341 ++++++++++++++++-----
 drivers/net/dsa/realtek/rtl83xx.c                  |   7 +-
 3 files changed, 306 insertions(+), 88 deletions(-)
---
base-commit: d7e14e534493328cc5f67baaff2b0c23d32b0a57
change-id: 20240212-realtek-led-a665ae39a9c5

Best regards,
-- 
Luiz Angelo Daros de Luca <luizluca@gmail.com>

[PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Luiz Angelo Daros de Luca]

Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
LED reg property will indicate its LED group.

An example of LED usage was included in an existing switch example.

Cc: devicetree@vger.kernel.org
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 .../devicetree/bindings/net/dsa/realtek.yaml       | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index 70b6bda3cf98..45c1656b3fae 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -108,6 +108,32 @@ properties:
       compatible:
         const: realtek,smi-mdio
 
+patternProperties:
+  '^(ethernet-)?ports$':
+    type: object
+    additionalProperties: true
+
+    patternProperties:
+      '^(ethernet-)?port@[0-6]$':
+        type: object
+        additionalProperties: true
+
+        properties:
+          leds:
+            description:
+              "LEDs associated with this port"
+
+            patternProperties:
+              '^led@[a-f0-9]+$':
+                type: object
+                additionalProperties: true
+
+                properties:
+                  reg:
+                    description:
+                      "reg indicates the LED group for this LED"
+                    enum: [0, 1, 2, 3]
+
 if:
   required:
     - reg
@@ -144,6 +170,7 @@ unevaluatedProperties: false
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
     #include <dt-bindings/interrupt-controller/irq.h>
 
     platform {
@@ -190,6 +217,25 @@ examples:
                                     reg = <4>;
                                     label = "wan";
                                     phy-handle = <&phy4>;
+
+                                    leds {
+                                            #address-cells = <1>;
+                                            #size-cells = <0>;
+
+                                            led@0 {
+                                                    reg = <0>;
+                                                    color = <LED_COLOR_ID_GREEN>;
+                                                    function = LED_FUNCTION_WAN;
+                                                    default-state = "keep";
+                                            };
+
+                                            led@3 {
+                                                    reg = <3>;
+                                                    color = <LED_COLOR_ID_AMBER>;
+                                                    function = LED_FUNCTION_WAN;
+                                                    default-state = "keep";
+                                            };
+                                    };
                             };
                             port@5 {
                                     reg = <5>;

-- 
2.44.0

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Krzysztof Kozlowski]

On 10/03/2024 05:51, Luiz Angelo Daros de Luca wrote:
> Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> LED reg property will indicate its LED group.
> 

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). 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), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.

> An example of LED usage was included in an existing switch example.
> 
> Cc: devicetree@vger.kernel.org

Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
commit msg. There is no single need to store automated output of
get_maintainers.pl in the git log. It can be easily re-created at any
given time, thus its presence in the git history is redundant and
obfuscates the log.

If you need it for your own patch management purposes, keep it under the
--- separator.

> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  .../devicetree/bindings/net/dsa/realtek.yaml       | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index 70b6bda3cf98..45c1656b3fae 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -108,6 +108,32 @@ properties:
>        compatible:
>          const: realtek,smi-mdio
>  
> +patternProperties:
> +  '^(ethernet-)?ports$':
> +    type: object
> +    additionalProperties: true
> +
> +    patternProperties:
> +      '^(ethernet-)?port@[0-6]$':
> +        type: object
> +        additionalProperties: true
> +
> +        properties:
> +          leds:

type: object
additionalProperties: false

> +            description:
> +              "LEDs associated with this port"

Drop quotes.

> +
> +            patternProperties:
> +              '^led@[a-f0-9]+$':

[0-3]

> +                type: object
> +                additionalProperties: true

This cannot be 'true'. Which then will point you to errors and missing
ref to leds schema and need to use unevaluatedProperties: false.


> +
> +                properties:
> +                  reg:
> +                    description:
> +                      "reg indicates the LED group for this LED"

Drop quotes


Best regards,
Krzysztof

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Luiz Angelo Daros de Luca]

Hi Krzysztof

> On 10/03/2024 05:51, Luiz Angelo Daros de Luca wrote:
> > Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> > LED reg property will indicate its LED group.
> >
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). 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), work on fork of kernel
> (don't, instead use mainline) or you ignore some maintainers (really
> don't). Just use b4 and everything should be fine, although remember
> about `b4 prep --auto-to-cc` if you added new patches to the patchset.
>
> > An example of LED usage was included in an existing switch example.
> >
> > Cc: devicetree@vger.kernel.org
>
> Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
> commit msg. There is no single need to store automated output of
> get_maintainers.pl in the git log. It can be easily re-created at any
> given time, thus its presence in the git history is redundant and
> obfuscates the log.

It is a left-over before I adopted b4. I'll do the cleanup.

>

> > +patternProperties:
> > +  '^(ethernet-)?ports$':
> > +    type: object
> > +    additionalProperties: true
> > +
> > +    patternProperties:
> > +      '^(ethernet-)?port@[0-6]$':
> > +        type: object
> > +        additionalProperties: true
> > +
> > +        properties:
> > +          leds:
>
> type: object
> additionalProperties: false
>
> > +            description:
> > +              "LEDs associated with this port"
>
> Drop quotes.

At some in my frequent system upgrades (rolling release), it
uninstalled the yamllint

warning: python package 'yamllint' not installed, skipping

It should have catched that before.

>
> > +
> > +            patternProperties:
> > +              '^led@[a-f0-9]+$':
>
> [0-3]

leds are already defined for a port. I'm just trying to add a
restriction to allow only 0-3 leds and use that to identify the group.
These suggestions will redefine the leds property, forcing me to
declare #address-cells, #size-cells for leds and reference the led
schema in led@[0-3]. Is there a way to just add a constraint to what
is already present?

>
> > +                type: object
> > +                additionalProperties: true
>
> This cannot be 'true'. Which then will point you to errors and missing
> ref to leds schema and need to use unevaluatedProperties: false.
>
>
> > +
> > +                properties:
> > +                  reg:
> > +                    description:
> > +                      "reg indicates the LED group for this LED"
>
> Drop quotes
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Krzysztof Kozlowski]

On 21/03/2024 12:56, Luiz Angelo Daros de Luca wrote:
> 
>>
>>> +
>>> +            patternProperties:
>>> +              '^led@[a-f0-9]+$':
>>
>> [0-3]
> 
> leds are already defined for a port. I'm just trying to add a
> restriction to allow only 0-3 leds and use that to identify the group.

Where is the restriction, in your original patch?

> These suggestions will redefine the leds property, forcing me to

How? I really do not see it.

> declare #address-cells, #size-cells for leds and reference the led
> schema in led@[0-3]. Is there a way to just add a constraint to what
> is already present?

I don't follow.

> 
>>
>>> +                type: object
>>> +                additionalProperties: true
>>
>> This cannot be 'true'. Which then will point you to errors and missing
>> ref to leds schema and need to use unevaluatedProperties: false.
Best regards,
Krzysztof

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Luiz Angelo Daros de Luca]

Hi Krzysztof,

> >>
> >>> +
> >>> +            patternProperties:
> >>> +              '^led@[a-f0-9]+$':
> >>
> >> [0-3]
> >
> > leds are already defined for a port. I'm just trying to add a
> > restriction to allow only 0-3 leds and use that to identify the group.
>
> Where is the restriction, in your original patch?

I tried to limit the led index to [0-3] (from the original
'^led@[a-f0-9]+$') and reg also to [0-3] (originally not constrained).

>
> > These suggestions will redefine the leds property, forcing me to
>
> How? I really do not see it.

I thought it was including the previous object definition when I
mentioned the same property again. However, the
"unevaluatedProperties: false" made it clear that it is actually
replacing the previous declaration. Sorry for my lack of experience.

> > declare #address-cells, #size-cells for leds and reference the led
> > schema in led@[0-3]. Is there a way to just add a constraint to what
> > is already present?
>
> I don't follow.

I would like to somehow add a restriction without replacing the
existing subnode definition. I'm not sure if the YAML scheme can
modify an heritaged sub-sub-property without redefining it all over
again or if the parent object requires a specific subobject property.
Anyway, the discussion ended up concluding that it was not worth it to
add such complexity for this situation.

Thank you for your time.

>
> >
> >>
> >>> +                type: object
> >>> +                additionalProperties: true
> >>
> >> This cannot be 'true'. Which then will point you to errors and missing
> >> ref to leds schema and need to use unevaluatedProperties: false.
> Best regards,
> Krzysztof
>

Regards,

Luiz

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Krzysztof Kozlowski]

On 24/03/2024 03:10, Luiz Angelo Daros de Luca wrote:
> Hi Krzysztof,
> 
>>>>
>>>>> +
>>>>> +            patternProperties:
>>>>> +              '^led@[a-f0-9]+$':
>>>>
>>>> [0-3]
>>>
>>> leds are already defined for a port. I'm just trying to add a
>>> restriction to allow only 0-3 leds and use that to identify the group.
>>
>> Where is the restriction, in your original patch?
> 
> I tried to limit the led index to [0-3] (from the original
> '^led@[a-f0-9]+$') and reg also to [0-3] (originally not constrained).

Where? I asked where, not what you tried to do. I don't think you tried
to add any restriction on 0-3 leds.

Best regards,
Krzysztof

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Andrew Lunn]

On Sun, Mar 10, 2024 at 01:51:58AM -0300, Luiz Angelo Daros de Luca wrote:
> Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> LED reg property will indicate its LED group.
> +                properties:
> +                  reg:
> +                    description:
> +                      "reg indicates the LED group for this LED"
> +                    enum: [0, 1, 2, 3]

If this identifies the group, what identifies the actual LED? There
are four of them. How do i say LEDs 0 and 1 are unused, 2 and 3 are
wired to LEDs?

It would be much more usual for reg to be the LED number for the
port. And there then be a group property indicating what group the LED
belongs to. However, i'm wondering, is group fixed? Do we actually
need it in DT, or can there just be a table in the driver which maps
port:led to group?

	 Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Luiz Angelo Daros de Luca]

Hi Andrew,

Em dom., 10 de mar. de 2024 às 15:02, Andrew Lunn <andrew@lunn.ch> escreveu:
>> On Sun, Mar 10, 2024 at 01:51:58AM -0300, Luiz Angelo Daros de Luca wrote:
> > Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> > LED reg property will indicate its LED group.
> > +                properties:
> > +                  reg:
> > +                    description:
> > +                      "reg indicates the LED group for this LED"
> > +                    enum: [0, 1, 2, 3]
>
> If this identifies the group, what identifies the actual LED? There
> are four of them. How do i say LEDs 0 and 1 are unused, 2 and 3 are
> wired to LEDs?

Do we need to mention a specific HW is not present? I guess we just
omit it, like the led@1 and led@2 in the example.

> It would be much more usual for reg to be the LED number for the
> port. And there then be a group property indicating what group the LED
> belongs to. However, i'm wondering, is group fixed? Do we actually
> need it in DT, or can there just be a table in the driver which maps
> port:led to group?

The LED groups are a fixed HW feature. The chip has dedicated pins for
each LED in each group. Those pins might have an alternative usage (as
an GPIO) but you cannot change a LED from a group to another in SW.
That's why I thought it was a thing to be described in the DT.

>          Andrew

Regards,

Luiz

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Linus Walleij]

On Sun, Mar 10, 2024 at 5:52 AM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> LED reg property will indicate its LED group.
>
> An example of LED usage was included in an existing switch example.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

But... is this patch even needed at all?

On the top of Documentation/devicetree/bindings/net/dsa/realtek.yaml:

allOf:
  - $ref: dsa.yaml#/$defs/ethernet-ports

In Documentation/devicetree/bindings/net/dsa/dsa.yaml:

$defs:
  ethernet-ports:
    description: A DSA switch without any extra port properties
    $ref: '#'

    patternProperties:
      "^(ethernet-)?ports$":
        patternProperties:
          "^(ethernet-)?port@[0-9a-f]+$":
            description: Ethernet switch ports
            $ref: dsa-port.yaml#
            unevaluatedProperties: false

(NB, just "ports" is fine.)
In Documentation/devicetree/bindings/net/dsa/dsa-port.yaml:

$ref: /schemas/net/ethernet-switch-port.yaml#

In Documentation/devicetree/bindings/net/ethernet-switch-port.yaml:

$ref: ethernet-controller.yaml#

In Documentation/devicetree/bindings/net/ethernet-controller.yaml:

  leds:
    description:
      Describes the LEDs associated by Ethernet Controller.
      These LEDs are not integrated in the PHY and PHY doesn't have any
      control on them. Ethernet Controller regs are used to control
      these defined LEDs.

    type: object

    properties:
      '#address-cells':
        const: 1

      '#size-cells':
        const: 0

    patternProperties:
      '^led@[a-f0-9]+$':
        $ref: /schemas/leds/common.yaml#

        properties:
          reg:
            maxItems: 1
            description:
              This define the LED index in the PHY or the MAC. It's really
              driver dependent and required for ports that define multiple
              LED for the same port.

        required:
          - reg

        unevaluatedProperties: false

    additionalProperties: false

Try to introduce small errors in your DTS leds node and it should warn!

I'm not claiming that above include chain is "easy to read"... It makes
perfect sense to machines but maybe is a bit to construed for human
readers.

What you should do instead (IMO) is to just keep the last part that
adds a leds node example.

Yours,
Linus Walleij

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Luiz Angelo Daros de Luca]

Hi Linus,

>
> > Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> > LED reg property will indicate its LED group.
> >
> > An example of LED usage was included in an existing switch example.
> >
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
>
> But... is this patch even needed at all?

Yes, leds is a property already supported. The only purpose for this
patch is to limit led numbers to 0-3 and describe that the reg number
is the group.

I don't know if there is a better way to add restrictions to something
already defined. Anyway, if that is not necessary, I can drop it.

Regards,

Luiz

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Andrew Lunn]

> +                properties:
> +                  reg:
> +                    description:
> +                      "reg indicates the LED group for this LED"
> +                    enum: [0, 1, 2, 3]

Having read the code, i don't think the binding needs to say anything
about the group. It is implicit. Port LED 0 is in group 0. Port LED 1
is in group 1. So there is nothing here which is outside of the
standard LED binding. So as Linus said, i don't think you need any
YAML at all, the DSA port binding should be sufficient.

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Luiz Angelo Daros de Luca]

Hi Andrew,

>
> > +                properties:
> > +                  reg:
> > +                    description:
> > +                      "reg indicates the LED group for this LED"
> > +                    enum: [0, 1, 2, 3]
>
> Having read the code, i don't think the binding needs to say anything
> about the group. It is implicit. Port LED 0 is in group 0. Port LED 1
> is in group 1. So there is nothing here which is outside of the
> standard LED binding. So as Linus said, i don't think you need any
> YAML at all, the DSA port binding should be sufficient.

The only purpose for this patch is to limit led numbers to 0-3 and
describe that the reg number is the group. If that is not important,
I'll just drop it.

Regards,

Luiz

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Rob Herring]

On Sun, 10 Mar 2024 01:51:58 -0300, Luiz Angelo Daros de Luca wrote:
> Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> LED reg property will indicate its LED group.
> 
> An example of LED usage was included in an existing switch example.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  .../devicetree/bindings/net/dsa/realtek.yaml       | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/dsa/realtek.yaml:121:15: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/net/dsa/realtek.yaml:131:23: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240310-realtek-led-v1-1-4d9813ce938e@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: realtek: describe LED usage

[Luiz Angelo Daros de Luca]

Hi Rob,

> On Sun, 10 Mar 2024 01:51:58 -0300, Luiz Angelo Daros de Luca wrote:
> > Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> > LED reg property will indicate its LED group.
> >
> > An example of LED usage was included in an existing switch example.
> >
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > ---
> >  .../devicetree/bindings/net/dsa/realtek.yaml       | 46 ++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/net/dsa/realtek.yaml:121:15: [error] string value is redundantly quoted with any quotes (quoted-strings)
> ./Documentation/devicetree/bindings/net/dsa/realtek.yaml:131:23: [error] string value is redundantly quoted with any quotes (quoted-strings)

Thanks for the tip. I just noticed my yamllint got uninstalled,
probably related to a python upgrade, and I missed the warning.

warning: python package 'yamllint' not installed, skipping

Sorry for the fuss.

Regards,

Luiz