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 (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: phy: generic polarity + LED support for qca808x | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

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 (Arm) 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