| Message ID | 20240310-realtek-led-v1-1-4d9813ce938e@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | net: dsa: realtek: fix LED support for rtl8366 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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
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
> + 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
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.
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 >
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
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
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
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
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
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
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
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>;
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(+)