On Tue, Dec 19, 2023 at 11:58:07AM +0100, Marek Behún wrote: > 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? Considering we want to use a property called polarity that might intend the full configuration of the LED. Wonder if - active-low - active-high - active-low-open - active-high-open And describe them that in - active-low - active-high low or high voltage is used for the other pin. And for active-low-open and active-high-open the other pin is not connected. But maybe open might be even confusing (since I don't think they are not connected bu as you said just attached to something high impedance. -- Ansuel
