diff mbox series

[net-next,09/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

Message ID 20230307170046.28917-10-ansuelsmth@gmail.com
State New
Headers
Series net: Add basic LED support for switch/phy |

Commit Message

Christian Marangi March 7, 2023, 5 p.m. UTC 
  Add LEDs definition example for qca8k Switch Family to describe how they
should be defined for a correct usage.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/net/dsa/qca8k.yaml    | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Krzysztof Kozlowski March 8, 2023, 10:58 a.m. UTC | #1 
On 07/03/2023 18:00, Christian Marangi wrote:
> Add LEDs definition example for qca8k Switch Family to describe how they
> should be defined for a correct usage.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Where is the changelog? This was v8 already! What happened with all
review, changes?

Best regards,
Krzysztof
Andrew Lunn March 8, 2023, 1:57 p.m. UTC | #2 
On Wed, Mar 08, 2023 at 11:58:33AM +0100, Krzysztof Kozlowski wrote:
> On 07/03/2023 18:00, Christian Marangi wrote:
> > Add LEDs definition example for qca8k Switch Family to describe how they
> > should be defined for a correct usage.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> 
> Where is the changelog? This was v8 already! What happened with all
> review, changes?

Did you read patch 0?

We have decided to start again, starting small and working up. This
patchset just adds plain, boring LEDs. No acceleration, on hardware
offload. Just on/off, and fixed blink.

What do you think makes the patchset is not bisectable? We are happy
to address such issues, but i did not notice anything.

    Andrew
Krzysztof Kozlowski March 8, 2023, 6:49 p.m. UTC | #3 
On 08/03/2023 14:57, Andrew Lunn wrote:
> On Wed, Mar 08, 2023 at 11:58:33AM +0100, Krzysztof Kozlowski wrote:
>> On 07/03/2023 18:00, Christian Marangi wrote:
>>> Add LEDs definition example for qca8k Switch Family to describe how they
>>> should be defined for a correct usage.
>>>
>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>
>> Where is the changelog? This was v8 already! What happened with all
>> review, changes?
> 
> Did you read patch 0?
> 
> We have decided to start again, starting small and working up. This
> patchset just adds plain, boring LEDs. No acceleration, on hardware
> offload. Just on/off, and fixed blink.

Sure, but the patch is carried over. So what happened with all its
feedback? Was there or was not? How can we know?

> 
> What do you think makes the patchset is not bisectable? We are happy
> to address such issues, but i did not notice anything.

I didn't write anything like that here...

Best regards,
Krzysztof
Christian Marangi March 8, 2023, 7:02 p.m. UTC | #4 
On Wed, Mar 08, 2023 at 07:49:26PM +0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 14:57, Andrew Lunn wrote:
> > On Wed, Mar 08, 2023 at 11:58:33AM +0100, Krzysztof Kozlowski wrote:
> >> On 07/03/2023 18:00, Christian Marangi wrote:
> >>> Add LEDs definition example for qca8k Switch Family to describe how they
> >>> should be defined for a correct usage.
> >>>
> >>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> >>
> >> Where is the changelog? This was v8 already! What happened with all
> >> review, changes?
> > 
> > Did you read patch 0?
> > 
> > We have decided to start again, starting small and working up. This
> > patchset just adds plain, boring LEDs. No acceleration, on hardware
> > offload. Just on/off, and fixed blink.
> 
> Sure, but the patch is carried over. So what happened with all its
> feedback? Was there or was not? How can we know?
>

The history of the old series is a bit sad, not enough review, another
dev asking for a different implementation and me doing an hybrid to
reach a common point (and then disappear intro oblivion)...

Short story is that this current series have nothing related to the HW
offload feature and only in v7 it was asked to put the LED nodes in
ethernet-phy.yaml

I can put in the cover letter of v2 of this series the changelog of the
previous series but they would only be related to other part that are
not related to this.

Just to give you some context and explain why the changelog was dropped.

> > 
> > What do you think makes the patchset is not bisectable? We are happy
> > to address such issues, but i did not notice anything.
> 
> I didn't write anything like that here...
>
Krzysztof Kozlowski March 8, 2023, 7:09 p.m. UTC | #5 
On 08/03/2023 20:02, Christian Marangi wrote:
> On Wed, Mar 08, 2023 at 07:49:26PM +0100, Krzysztof Kozlowski wrote:
>> On 08/03/2023 14:57, Andrew Lunn wrote:
>>> On Wed, Mar 08, 2023 at 11:58:33AM +0100, Krzysztof Kozlowski wrote:
>>>> On 07/03/2023 18:00, Christian Marangi wrote:
>>>>> Add LEDs definition example for qca8k Switch Family to describe how they
>>>>> should be defined for a correct usage.
>>>>>
>>>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>>>
>>>> Where is the changelog? This was v8 already! What happened with all
>>>> review, changes?
>>>
>>> Did you read patch 0?
>>>
>>> We have decided to start again, starting small and working up. This
>>> patchset just adds plain, boring LEDs. No acceleration, on hardware
>>> offload. Just on/off, and fixed blink.
>>
>> Sure, but the patch is carried over. So what happened with all its
>> feedback? Was there or was not? How can we know?
>>
> 
> The history of the old series is a bit sad, not enough review, another
> dev asking for a different implementation and me doing an hybrid to
> reach a common point (and then disappear intro oblivion)...
> 
> Short story is that this current series have nothing related to the HW
> offload feature and only in v7 it was asked to put the LED nodes in
> ethernet-phy.yaml
> 
> I can put in the cover letter of v2 of this series the changelog of the
> previous series but they would only be related to other part that are
> not related to this.
> 
> Just to give you some context and explain why the changelog was dropped.

I am less interested in the changelog of entire patchset but of the
patches which are for me to review. Sending vX as v1 suggests that all
previous review work on this patch could be in some limbo state. Maybe
nothing happened in all previous version, but it's now my task to dig it?

This is why you have "---" for the patch changelog.

Best regards,
Krzysztof
Christian Marangi March 8, 2023, 7:23 p.m. UTC | #6 
On Wed, Mar 08, 2023 at 08:09:34PM +0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 20:02, Christian Marangi wrote:
> > On Wed, Mar 08, 2023 at 07:49:26PM +0100, Krzysztof Kozlowski wrote:
> >> On 08/03/2023 14:57, Andrew Lunn wrote:
> >>> On Wed, Mar 08, 2023 at 11:58:33AM +0100, Krzysztof Kozlowski wrote:
> >>>> On 07/03/2023 18:00, Christian Marangi wrote:
> >>>>> Add LEDs definition example for qca8k Switch Family to describe how they
> >>>>> should be defined for a correct usage.
> >>>>>
> >>>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> >>>>
> >>>> Where is the changelog? This was v8 already! What happened with all
> >>>> review, changes?
> >>>
> >>> Did you read patch 0?
> >>>
> >>> We have decided to start again, starting small and working up. This
> >>> patchset just adds plain, boring LEDs. No acceleration, on hardware
> >>> offload. Just on/off, and fixed blink.
> >>
> >> Sure, but the patch is carried over. So what happened with all its
> >> feedback? Was there or was not? How can we know?
> >>
> > 
> > The history of the old series is a bit sad, not enough review, another
> > dev asking for a different implementation and me doing an hybrid to
> > reach a common point (and then disappear intro oblivion)...
> > 
> > Short story is that this current series have nothing related to the HW
> > offload feature and only in v7 it was asked to put the LED nodes in
> > ethernet-phy.yaml
> > 
> > I can put in the cover letter of v2 of this series the changelog of the
> > previous series but they would only be related to other part that are
> > not related to this.
> > 
> > Just to give you some context and explain why the changelog was dropped.
> 
> I am less interested in the changelog of entire patchset but of the
> patches which are for me to review. Sending vX as v1 suggests that all
> previous review work on this patch could be in some limbo state. Maybe
> nothing happened in all previous version, but it's now my task to dig it?
> 

OK, if you are intrested only on the DT changes I can list:
From v0 to v6:
- There was only qca8k Documentation addition for the leds node.
  Some extra binding were dropped due to implementation change but they
  were only specific to qca8k.

In v7:
- The default-trigger for netdev was introduced (this caused some
  warning for missing netdev trigger for that binding)
- It was suggested and asked to make the LEDs generic and add
  Documentration for phy-ethernet.yaml

In v8:
- The netdev trigger was added to the list of accepted default-trigger
- The Documentation for phy-ethernet.yaml was added

Then we decided to "reboot" the series and:
In v1:
- The default-trigger is dropped (will be introduced later when the work
  for the netdev trigger will be done)
- We use the default state to "keep"

This should be the entire changelog excluding some changed things done
from v0 to v6 with low review and implementation changed at least 3
times during the life of that series.


> This is why you have "---" for the patch changelog.
Andrew Lunn March 8, 2023, 7:42 p.m. UTC | #7 
> Then we decided to "reboot" the series and:
> In v1:
> - The default-trigger is dropped (will be introduced later when the work
>   for the netdev trigger will be done)
> - We use the default state to "keep"

There is one more change. The leds {} property moved from the PHY nodes
into the port nodes, because these are MAC LEDs, not PHY LEDs.

     Andrew
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index 389892592aac..866b3cc73216 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -18,6 +18,8 @@  description:
   PHY it is connected to. In this config, an internal mdio-bus is registered and
   the MDIO master is used for communication. Mixed external and internal
   mdio-bus configurations are not supported by the hardware.
+  Each phy have at least 3 LEDs connected and can be declared
+  using the standard LEDs structure.
 
 properties:
   compatible:
@@ -117,6 +119,7 @@  unevaluatedProperties: false
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
 
     mdio {
         #address-cells = <1>;
@@ -226,6 +229,27 @@  examples:
                     label = "lan1";
                     phy-mode = "internal";
                     phy-handle = <&internal_phy_port1>;
+
+                    leds {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+
+                        led@0 {
+                            reg = <0>;
+                            color = <LED_COLOR_ID_WHITE>;
+                            function = LED_FUNCTION_LAN;
+                            function-enumerator = <1>;
+                            default-state = "keep";
+                        };
+
+                        led@1 {
+                            reg = <1>;
+                            color = <LED_COLOR_ID_AMBER>;
+                            function = LED_FUNCTION_LAN;
+                            function-enumerator = <1>;
+                            default-state = "keep";
+                        };
+                    };
                 };
 
                 port@2 {