[PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Tobias Waldekranz]

Hardware supports multiple ways of driving attached LEDs, but this is
not configurable via any sample-at-reset pins - rather it must be set
via software.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 .../bindings/net/marvell,marvell10g.yaml      | 60 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/marvell,marvell10g.yaml

diff --git a/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml b/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
new file mode 100644
index 000000000000..37ff7fdfdd3d
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/marvell,marvell10g.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell Alaska X 10G Ethernet PHY
+
+maintainers:
+  - Tobias Waldekranz <tobias@waldekranz.com>
+
+description: |
+  Bindings for Marvell Alaska X 10G Ethernet PHYs
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+properties:
+  leds:
+    type: object
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      '^led@[a-f0-9]+$':
+        $ref: /schemas/leds/common.yaml#
+
+        properties:
+          marvell,polarity:
+            description: |
+              Electrical polarity and drive type for this LED. In the
+              active state, hardware may drive the pin either low or
+              high. In the inactive state, the pin can either be
+              driven to the opposite logic level, or be tristated.
+            $ref: /schemas/types.yaml#/definitions/string
+            enum:
+              - active-low
+              - active-high
+              - active-low-tristate
+              - active-high-tristate
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy@0 {
+            reg = <0>;
+
+            marvell,polarity = "active-low-tristate";
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index a151988646fe..2def66789f9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12849,6 +12849,7 @@ M:	Russell King <linux@armlinux.org.uk>
 M:	Marek Behún <kabel@kernel.org>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
 F:	drivers/net/phy/marvell10g.c
 
 MARVELL MVEBU THERMAL DRIVER
-- 
2.34.1

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Krzysztof Kozlowski]

On 14/12/2023 21:14, Tobias Waldekranz wrote:
> Hardware supports multiple ways of driving attached LEDs, but this is
> not configurable via any sample-at-reset pins - rather it must be set
> via software.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  .../bindings/net/marvell,marvell10g.yaml      | 60 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml b/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
> new file mode 100644
> index 000000000000..37ff7fdfdd3d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/marvell,marvell10g.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Alaska X 10G Ethernet PHY
> +
> +maintainers:
> +  - Tobias Waldekranz <tobias@waldekranz.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Bindings for Marvell Alaska X 10G Ethernet PHYs

Drop Bindings for and describe the hardware. You are repeating title, so
it is useless.

> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +properties:

How is this schema selected/applied? I guess you have exactly the same
problem as recently talked about other ethernet PHY bindings.

See:
https://lore.kernel.org/linux-devicetree/20231209014828.28194-1-ansuelsmth@gmail.com/

> +  leds:
> +    type: object
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      '^led@[a-f0-9]+$':
> +        $ref: /schemas/leds/common.yaml#

Are you sure you need to repeat all this?

> +
> +        properties:
> +          marvell,polarity:
> +            description: |
> +              Electrical polarity and drive type for this LED. In the
> +              active state, hardware may drive the pin either low or
> +              high. In the inactive state, the pin can either be
> +              driven to the opposite logic level, or be tristated.
> +            $ref: /schemas/types.yaml#/definitions/string
> +            enum:
> +              - active-low
> +              - active-high
> +              - active-low-tristate
> +              - active-high-tristate
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy@0 {
> +            reg = <0>;
> +
> +            marvell,polarity = "active-low-tristate";

It is clearly visible here that your schema is an no-op. You do not
allow such property in the phy, but in leds!



Best regards,
Krzysztof

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Andrew Lunn]

> +        properties:
> +          marvell,polarity:
> +            description: |
> +              Electrical polarity and drive type for this LED. In the
> +              active state, hardware may drive the pin either low or
> +              high. In the inactive state, the pin can either be
> +              driven to the opposite logic level, or be tristated.
> +            $ref: /schemas/types.yaml#/definitions/string
> +            enum:
> +              - active-low
> +              - active-high
> +              - active-low-tristate
> +              - active-high-tristate

Christian is working on adding a generic active-low property, which
any PHY LED could use. The assumption being if the bool property is
not present, it defaults to active-high.

So we should consider, how popular are these two tristate values? Is
this a Marvell only thing, or do other PHYs also have them? Do we want
to make them part of the generic PHY led binding? Also, is an enum the
correct representation? Maybe tristate should be another bool
property? Hi/Low and tristate seem to be orthogonal, so maybe two
properties would make it cleaner with respect to generic properties?

Please work with Christian on this.

Thanks
	Andrew

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Christian Marangi]

> > +        properties:
> > +          marvell,polarity:
> > +            description: |
> > +              Electrical polarity and drive type for this LED. In the
> > +              active state, hardware may drive the pin either low or
> > +              high. In the inactive state, the pin can either be
> > +              driven to the opposite logic level, or be tristated.
> > +            $ref: /schemas/types.yaml#/definitions/string
> > +            enum:
> > +              - active-low
> > +              - active-high
> > +              - active-low-tristate
> > +              - active-high-tristate
> 
> Christian is working on adding a generic active-low property, which
> any PHY LED could use. The assumption being if the bool property is
> not present, it defaults to active-high.
> 

Hi, it was pointed out this series sorry for not noticing before.

> So we should consider, how popular are these two tristate values? Is
> this a Marvell only thing, or do other PHYs also have them? Do we want
> to make them part of the generic PHY led binding? Also, is an enum the
> correct representation? Maybe tristate should be another bool
> property? Hi/Low and tristate seem to be orthogonal, so maybe two
> properties would make it cleaner with respect to generic properties?

For parsing it would make it easier to have the thing split.

But on DT I feel an enum like it's done here might be more clear.

Assuming the property define the LED polarity, it would make sense
to have a single one instead of a sum of boolean.

The boolean idea might be problematic in the future for device that
devisates from what we expect.

Example: A device set the LED to active-high by default and we want a
way in DT to define active-low. With the boolean idea of having
"active-high" and assume active-low if not defined we would have to put
active-high in every PHY node (to reflect the default settings)

Having a property instead permits us to support more case.

Ideally on code side we would have an enum that map the string to the
different modes and we would pass to a .led_set_polarity the enum.
(or if we really want a bitmask)


If we feel tristate is special enough we can consider leaving that
specific to marvell (something like marvell,led-tristate)

But if we notice it's more generic then we will have to keep
compatibility for both.

> 
> Please work with Christian on this.

Think since the current idea is to support this in the LED api with set
polarity either the 2 series needs to be merged or the polarity part
needs to be detached and submitted later until we sort the generic way
to set it?

-- 
	Ansuel

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Andrew Lunn]

On Fri, Dec 15, 2023 at 03:22:11PM +0100, Christian Marangi wrote:
> > > +        properties:
> > > +          marvell,polarity:
> > > +            description: |
> > > +              Electrical polarity and drive type for this LED. In the
> > > +              active state, hardware may drive the pin either low or
> > > +              high. In the inactive state, the pin can either be
> > > +              driven to the opposite logic level, or be tristated.
> > > +            $ref: /schemas/types.yaml#/definitions/string
> > > +            enum:
> > > +              - active-low
> > > +              - active-high
> > > +              - active-low-tristate
> > > +              - active-high-tristate
> > 
> > Christian is working on adding a generic active-low property, which
> > any PHY LED could use. The assumption being if the bool property is
> > not present, it defaults to active-high.
> > 
> 
> Hi, it was pointed out this series sorry for not noticing before.
> 
> > So we should consider, how popular are these two tristate values? Is
> > this a Marvell only thing, or do other PHYs also have them? Do we want
> > to make them part of the generic PHY led binding? Also, is an enum the
> > correct representation? Maybe tristate should be another bool
> > property? Hi/Low and tristate seem to be orthogonal, so maybe two
> > properties would make it cleaner with respect to generic properties?
> 
> For parsing it would make it easier to have the thing split.
> 
> But on DT I feel an enum like it's done here might be more clear.

I took a look at a datasheet for a standalone 1G Marvell PHY. It has
the same capabilities. So this is something which can be reused by a
few devices.

So an enum in DT, and an enum for the API to the PHY driver seems like
a good idea. I doubt there will be too many more variants, but it does
give us an easy way to add more values.

	Andrew

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Christian Marangi]

On Fri, Dec 15, 2023 at 03:22:11PM +0100, Christian Marangi wrote:
> > > +        properties:
> > > +          marvell,polarity:
> > > +            description: |
> > > +              Electrical polarity and drive type for this LED. In the
> > > +              active state, hardware may drive the pin either low or
> > > +              high. In the inactive state, the pin can either be
> > > +              driven to the opposite logic level, or be tristated.
> > > +            $ref: /schemas/types.yaml#/definitions/string
> > > +            enum:
> > > +              - active-low
> > > +              - active-high
> > > +              - active-low-tristate
> > > +              - active-high-tristate
> > 
> > Christian is working on adding a generic active-low property, which
> > any PHY LED could use. The assumption being if the bool property is
> > not present, it defaults to active-high.
> > 
> 
> Hi, it was pointed out this series sorry for not noticing before.
> 
> > So we should consider, how popular are these two tristate values? Is
> > this a Marvell only thing, or do other PHYs also have them? Do we want
> > to make them part of the generic PHY led binding? Also, is an enum the
> > correct representation? Maybe tristate should be another bool
> > property? Hi/Low and tristate seem to be orthogonal, so maybe two
> > properties would make it cleaner with respect to generic properties?
> 
> For parsing it would make it easier to have the thing split.
> 
> But on DT I feel an enum like it's done here might be more clear.
> 
> Assuming the property define the LED polarity, it would make sense
> to have a single one instead of a sum of boolean.
> 
> The boolean idea might be problematic in the future for device that
> devisates from what we expect.
> 
> Example: A device set the LED to active-high by default and we want a
> way in DT to define active-low. With the boolean idea of having
> "active-high" and assume active-low if not defined we would have to put
> active-high in every PHY node (to reflect the default settings)
> 
> Having a property instead permits us to support more case.
> 
> Ideally on code side we would have an enum that map the string to the
> different modes and we would pass to a .led_set_polarity the enum.
> (or if we really want a bitmask)
> 
> 
> If we feel tristate is special enough we can consider leaving that
> specific to marvell (something like marvell,led-tristate)
> 
> But if we notice it's more generic then we will have to keep
> compatibility for both.
> 
> > 
> > Please work with Christian on this.
> 
> Think since the current idea is to support this in the LED api with set
> polarity either the 2 series needs to be merged or the polarity part
> needs to be detached and submitted later until we sort the generic way
> to set it?
>

Hi Andrew,

I asked some further info to Tobias. With a better look at the
Documentation, it was notice that tristate is only to drive the LED off.

So to drive LED ON:
- active-low
- active-high

And to drive LED OFF:
- low
- high
- tristate

I feel introducing description to how to drive the LED inactive might be
too much.

Would it be ok to have something like

polarity:
- "active-low"
- "active-high"

And a bool with "marvel,led-inactive-tristate" specific to this PHY?

In alternative we can list all the modes as done in the qca808x series
currently proposed. (more flexible for other PHY and expandable but can
pose the risk of bloating the property with all kind of modes)

PHY driver wise, with the set_polarity function or even probe function
they can handle this with a priv struct and operate in the
set_brightness function for these special handling.

-- 
	Ansuel

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Marek Behún]

On Tue, 19 Dec 2023 11:13:43 +0100
Christian Marangi <ansuelsmth@gmail.com> wrote:

> On Fri, Dec 15, 2023 at 03:22:11PM +0100, Christian Marangi wrote:
> > > > +        properties:
> > > > +          marvell,polarity:
> > > > +            description: |
> > > > +              Electrical polarity and drive type for this LED. In the
> > > > +              active state, hardware may drive the pin either low or
> > > > +              high. In the inactive state, the pin can either be
> > > > +              driven to the opposite logic level, or be tristated.
> > > > +            $ref: /schemas/types.yaml#/definitions/string
> > > > +            enum:
> > > > +              - active-low
> > > > +              - active-high
> > > > +              - active-low-tristate
> > > > +              - active-high-tristate  
> > > 
> > > Christian is working on adding a generic active-low property, which
> > > any PHY LED could use. The assumption being if the bool property is
> > > not present, it defaults to active-high.
> > >   
> > 
> > Hi, it was pointed out this series sorry for not noticing before.
> >   
> > > So we should consider, how popular are these two tristate values? Is
> > > this a Marvell only thing, or do other PHYs also have them? Do we want
> > > to make them part of the generic PHY led binding? Also, is an enum the
> > > correct representation? Maybe tristate should be another bool
> > > property? Hi/Low and tristate seem to be orthogonal, so maybe two
> > > properties would make it cleaner with respect to generic properties?  
> > 
> > For parsing it would make it easier to have the thing split.
> > 
> > But on DT I feel an enum like it's done here might be more clear.
> > 
> > Assuming the property define the LED polarity, it would make sense
> > to have a single one instead of a sum of boolean.
> > 
> > The boolean idea might be problematic in the future for device that
> > devisates from what we expect.
> > 
> > Example: A device set the LED to active-high by default and we want a
> > way in DT to define active-low. With the boolean idea of having
> > "active-high" and assume active-low if not defined we would have to put
> > active-high in every PHY node (to reflect the default settings)
> > 
> > Having a property instead permits us to support more case.
> > 
> > Ideally on code side we would have an enum that map the string to the
> > different modes and we would pass to a .led_set_polarity the enum.
> > (or if we really want a bitmask)
> > 
> > 
> > If we feel tristate is special enough we can consider leaving that
> > specific to marvell (something like marvell,led-tristate)
> > 
> > But if we notice it's more generic then we will have to keep
> > compatibility for both.
> >   
> > > 
> > > Please work with Christian on this.  
> > 
> > Think since the current idea is to support this in the LED api with set
> > polarity either the 2 series needs to be merged or the polarity part
> > needs to be detached and submitted later until we sort the generic way
> > to set it?
> >  
> 
> Hi Andrew,
> 
> I asked some further info to Tobias. With a better look at the
> Documentation, it was notice that tristate is only to drive the LED off.
> 
> So to drive LED ON:
> - active-low
> - active-high
> 
> And to drive LED OFF:
> - low
> - high
> - tristate
> 
> I feel introducing description to how to drive the LED inactive might be
> too much.
> 
> Would it be ok to have something like
> 
> polarity:
> - "active-low"
> - "active-high"
> 
> And a bool with "marvel,led-inactive-tristate" specific to this PHY?
* marvell

The "tristate" in LED off state means high impendance (or
alternatively: open, Z), see:
  https://en.wikipedia.org/wiki/Three-state_logic

Marvell calling this high impedance state "tristate" is IMO confusing,
since "tristate" means 3 state logic, the three states being:
- connected to high voltage
- connected to low voltage
- not connected to any voltage

I would propose something like
  inactive-hi-z;
or even better
  inactive-high-impedance;

Krzysztof, what do you think?

Marek

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Christian Marangi]

On Tue, Dec 19, 2023 at 11:58:07AM +0100, Marek Behún wrote:
> On Tue, 19 Dec 2023 11:13:43 +0100
> Christian Marangi <ansuelsmth@gmail.com> wrote:
> 
> > On Fri, Dec 15, 2023 at 03:22:11PM +0100, Christian Marangi wrote:
> > > > > +        properties:
> > > > > +          marvell,polarity:
> > > > > +            description: |
> > > > > +              Electrical polarity and drive type for this LED. In the
> > > > > +              active state, hardware may drive the pin either low or
> > > > > +              high. In the inactive state, the pin can either be
> > > > > +              driven to the opposite logic level, or be tristated.
> > > > > +            $ref: /schemas/types.yaml#/definitions/string
> > > > > +            enum:
> > > > > +              - active-low
> > > > > +              - active-high
> > > > > +              - active-low-tristate
> > > > > +              - active-high-tristate  
> > > > 
> > > > Christian is working on adding a generic active-low property, which
> > > > any PHY LED could use. The assumption being if the bool property is
> > > > not present, it defaults to active-high.
> > > >   
> > > 
> > > Hi, it was pointed out this series sorry for not noticing before.
> > >   
> > > > So we should consider, how popular are these two tristate values? Is
> > > > this a Marvell only thing, or do other PHYs also have them? Do we want
> > > > to make them part of the generic PHY led binding? Also, is an enum the
> > > > correct representation? Maybe tristate should be another bool
> > > > property? Hi/Low and tristate seem to be orthogonal, so maybe two
> > > > properties would make it cleaner with respect to generic properties?  
> > > 
> > > For parsing it would make it easier to have the thing split.
> > > 
> > > But on DT I feel an enum like it's done here might be more clear.
> > > 
> > > Assuming the property define the LED polarity, it would make sense
> > > to have a single one instead of a sum of boolean.
> > > 
> > > The boolean idea might be problematic in the future for device that
> > > devisates from what we expect.
> > > 
> > > Example: A device set the LED to active-high by default and we want a
> > > way in DT to define active-low. With the boolean idea of having
> > > "active-high" and assume active-low if not defined we would have to put
> > > active-high in every PHY node (to reflect the default settings)
> > > 
> > > Having a property instead permits us to support more case.
> > > 
> > > Ideally on code side we would have an enum that map the string to the
> > > different modes and we would pass to a .led_set_polarity the enum.
> > > (or if we really want a bitmask)
> > > 
> > > 
> > > If we feel tristate is special enough we can consider leaving that
> > > specific to marvell (something like marvell,led-tristate)
> > > 
> > > But if we notice it's more generic then we will have to keep
> > > compatibility for both.
> > >   
> > > > 
> > > > Please work with Christian on this.  
> > > 
> > > Think since the current idea is to support this in the LED api with set
> > > polarity either the 2 series needs to be merged or the polarity part
> > > needs to be detached and submitted later until we sort the generic way
> > > to set it?
> > >  
> > 
> > Hi Andrew,
> > 
> > I asked some further info to Tobias. With a better look at the
> > Documentation, it was notice that tristate is only to drive the LED off.
> > 
> > So to drive LED ON:
> > - active-low
> > - active-high
> > 
> > And to drive LED OFF:
> > - low
> > - high
> > - tristate
> > 
> > I feel introducing description to how to drive the LED inactive might be
> > too much.
> > 
> > Would it be ok to have something like
> > 
> > polarity:
> > - "active-low"
> > - "active-high"
> > 
> > And a bool with "marvel,led-inactive-tristate" specific to this PHY?
> * marvell
> 
> The "tristate" in LED off state means high impendance (or
> alternatively: open, Z), see:
>   https://en.wikipedia.org/wiki/Three-state_logic
> 
> Marvell calling this high impedance state "tristate" is IMO confusing,
> since "tristate" means 3 state logic, the three states being:
> - connected to high voltage
> - connected to low voltage
> - not connected to any voltage
> 
> I would propose something like
>   inactive-hi-z;
> or even better
>   inactive-high-impedance;
> 
> Krzysztof, what do you think?

Considering we want to use a property called polarity that might intend
the full configuration of the LED.

Wonder if
- active-low
- active-high
- active-low-open
- active-high-open

And describe them that in
- active-low
- active-high

low or high voltage is used for the other pin.

And for active-low-open and active-high-open the other pin is not
connected.

But maybe open might be even confusing (since I don't think they are not
connected bu as you said just attached to something high impedance.

-- 
	Ansuel