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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <657c8e53.050a0220.dd6f2.9aaf@mx.google.com>
Date: Fri, 15 Dec 2023 15:22:11 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Tobias Waldekranz <tobias@...dekranz.com>, davem@...emloft.net,
	kuba@...nel.org, linux@...linux.org.uk, kabel@...nel.org,
	hkallweit1@...il.com, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	netdev@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED
 polarity

> > +        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