On Tue, 19 Dec 2023 11:13:43 +0100 Christian Marangi <ansuelsmth@xxxxxxxxx> 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
