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

["Marek Behún" <kabel@kernel.org>]

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