From: Christian Marangi <ansuelsmth@gmail•com>
To: "Marek Behún" <kabel@kernel•org>
Cc: krzysztof.kozlowski+dt@linaro•org,
20231214201442.660447-5-tobias@waldekranz•com,
Andrew Lunn <andrew@lunn•ch>,
Tobias Waldekranz <tobias@waldekranz•com>,
davem@davemloft•net, kuba@kernel•org, linux@armlinux•org.uk,
hkallweit1@gmail•com, robh+dt@kernel•org, conor+dt@kernel•org,
netdev@vger•kernel.org, devicetree@vger•kernel.org
Subject: Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity
Date: Tue, 19 Dec 2023 20:43:41 +0100 [thread overview]
Message-ID: <658216e1.5d0a0220.ef7e8.7ff0@mx.google.com> (raw)
In-Reply-To: <20231219115807.22c22694@dellmb>
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@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?
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
next prev parent reply other threads:[~2023-12-19 22:19 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-15 14:22 [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity Christian Marangi
2023-12-16 12:25 ` Andrew Lunn
2023-12-19 10:13 ` Christian Marangi
2023-12-19 10:58 ` Marek Behún
2023-12-19 19:43 ` Christian Marangi [this message]
-- strict thread matches above, loose matches on Subject: below --
2023-12-14 20:14 [PATCH net-next 0/4] net: phy: marvell10g: Firmware loading and LED support for 88X3310 Tobias Waldekranz
2023-12-14 20:14 ` [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity Tobias Waldekranz
2023-12-15 8:47 ` Krzysztof Kozlowski
2023-12-15 11:19 ` Andrew Lunn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=658216e1.5d0a0220.ef7e8.7ff0@mx.google.com \
--to=ansuelsmth@gmail$(echo .)com \
--cc=20231214201442.660447-5-tobias@waldekranz$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=conor+dt@kernel$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=devicetree@vger$(echo .)kernel.org \
--cc=hkallweit1@gmail$(echo .)com \
--cc=kabel@kernel$(echo .)org \
--cc=krzysztof.kozlowski+dt@linaro$(echo .)org \
--cc=kuba@kernel$(echo .)org \
--cc=linux@armlinux$(echo .)org.uk \
--cc=netdev@vger$(echo .)kernel.org \
--cc=robh+dt@kernel$(echo .)org \
--cc=tobias@waldekranz$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox