diff mbox series

[net-next,v4,1/4] dt-bindings: net: phy: Document new LEDs polarity property

Message ID 20231215212244.1658-2-ansuelsmth@gmail.com
State New
Headers
Series net: phy: generic polarity + LED support for qca808x |

Commit Message

Christian Marangi Dec. 15, 2023, 9:22 p.m. UTC 
  Document new LEDs polarity property to define what mode the LED needs to
be put to turn it on.

Currently supported modes are:

- active-low
- active-high
- active-low-tristate
- active-high-tristate

Mode is optional and if it's not defined, a default value is chosed by
the driver.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
Changes v4:
- Drop support for global active-low
- Rework to polarity option (for marvell10g series support)
Changes v3:
- Out of RFC
Changes v2:
- Add this patch

 .../devicetree/bindings/net/ethernet-phy.yaml         | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Rob Herring Dec. 20, 2023, 3:22 p.m. UTC | #1 
On Fri, Dec 15, 2023 at 10:22:41PM +0100, Christian Marangi wrote:
> Document new LEDs polarity property to define what mode the LED needs to
> be put to turn it on.
> 
> Currently supported modes are:
> 
> - active-low
> - active-high
> - active-low-tristate
> - active-high-tristate

Why is having a polarity unique to LEDs on ethernet PHYs? It's not. We 
already have 'active-low' established on several LED bindings. Please 
move the definition to leds/common.yaml and extend it. I would simply 
add an 'inactive-tristate' boolean property (if there's an actual user). 

I do worry this continues to evolve until we've re-created the pinctrl 
binding...

Rob
Christian Marangi Dec. 20, 2023, 10:53 p.m. UTC | #2 
On Wed, Dec 20, 2023 at 09:22:09AM -0600, Rob Herring wrote:
> On Fri, Dec 15, 2023 at 10:22:41PM +0100, Christian Marangi wrote:
> > Document new LEDs polarity property to define what mode the LED needs to
> > be put to turn it on.
> > 
> > Currently supported modes are:
> > 
> > - active-low
> > - active-high
> > - active-low-tristate
> > - active-high-tristate
> 
> Why is having a polarity unique to LEDs on ethernet PHYs? It's not. We 
> already have 'active-low' established on several LED bindings. Please 
> move the definition to leds/common.yaml and extend it. I would simply 
> add an 'inactive-tristate' boolean property (if there's an actual user). 
>

Should I also drop the active-low from the current schema that have it?

Also we have led-active-low. (should we support both?)

On the marvell10g series we are discussing of using tristate or not. We
notice tristate might be confusing, would it be better to use
inactive-high-impedance ?

> I do worry this continues to evolve until we've re-created the pinctrl 
> binding...
>
Andrew Lunn Dec. 21, 2023, 9:34 a.m. UTC | #3 
> I do worry this continues to evolve until we've re-created the pinctrl 
> binding...

Hi Rob

What is you opinion of the pinctrl binding? Should we just copy parts
of it?

     Andrew
Andrew Lunn Dec. 21, 2023, 9:43 a.m. UTC | #4 
> On the marvell10g series we are discussing of using tristate or not. We
> notice tristate might be confusing, would it be better to use
> inactive-high-impedance ?

The pincfg-node.yaml binding has:

  drive-open-drain:
    oneOf:
      - type: boolean
      - $ref: /schemas/types.yaml#/definitions/uint32
        const: 1    # No known cases of 0
        deprecated: true
    description: drive with open drain

  drive-open-source:
    type: boolean
    description: drive with open source

I'm not sure what the deprecated means. Is it that a value is
deprecated, not the property as a whole?

	Andrew
Conor Dooley Dec. 22, 2023, 3:20 p.m. UTC | #5 
On Thu, Dec 21, 2023 at 10:43:17AM +0100, Andrew Lunn wrote:
> > On the marvell10g series we are discussing of using tristate or not. We
> > notice tristate might be confusing, would it be better to use
> > inactive-high-impedance ?
> 
> The pincfg-node.yaml binding has:
> 
>   drive-open-drain:
>     oneOf:
>       - type: boolean
>       - $ref: /schemas/types.yaml#/definitions/uint32
>         const: 1    # No known cases of 0
>         deprecated: true
>     description: drive with open drain
> 
>   drive-open-source:
>     type: boolean
>     description: drive with open source
> 
> I'm not sure what the deprecated means. Is it that a value is
> deprecated, not the property as a whole?

Yeah, it means that only the boolean form of this property should be
used going forward.

The comment suggests that the value had no meaning in the first place,
and that testing for presence alone has been sufficient all along.
Christian Marangi Dec. 22, 2023, 10:09 p.m. UTC | #6 
On Thu, Dec 21, 2023 at 10:34:41AM +0100, Andrew Lunn wrote:
> > I do worry this continues to evolve until we've re-created the pinctrl 
> > binding...
> 
> Hi Rob
> 
> What is you opinion of the pinctrl binding? Should we just copy parts
> of it?
>

Hi,

I have the new series ready but I'm not sure pincfg-node have useful
property.

What we should use from there? From what I can see only output-low would
be useful to us. I didn't find a way to handle the inactive mode.

Should I send the new series so we can continue the discussion there or
better to wait for Rob feedback?
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 8fb2a6ee7e5b..282bf18f50fd 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -225,6 +225,17 @@  properties:
               driver dependent and required for ports that define multiple
               LED for the same port.
 
+          polarity:
+            description: |
+              Electrical polarity and drive type for the LED to turn it
+              on.
+            $ref: /schemas/types.yaml#/definitions/string
+            enum:
+              - active-low
+              - active-high
+              - active-low-tristate
+              - active-high-tristate
+
         required:
           - reg