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-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 19 Dec 2023 11:13:43 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: 20231214201442.660447-5-tobias@...dekranz.com
Cc: Andrew Lunn <andrew@...n.ch>, 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

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