Re: [PATCH net-next 1/4] dt-bindings: leds: add 'active-high' property

[Daniel Golle]

On Mon, Oct 07, 2024 at 08:38:27AM +0200, Krzysztof Kozlowski wrote:
> On Sun, Oct 06, 2024 at 02:04:35PM +0100, Daniel Golle wrote:
> > On Sun, Oct 06, 2024 at 02:44:44PM +0200, Krzysztof Kozlowski wrote:
> > > I think this should be just string enum, see marvell,marvell10g.yaml
> > 
> > I found the vendor-specific 'marvell,polarity' property in
> > https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231214201442.660447-5-tobias@xxxxxxxxxxxxxx/
> > 
> > However, I can't find that file in any Linux tree.
> > 
> > Looking at the suggested patch on patchwork, I got a few questions on
> > how to deal with the situation as of today:
> > 
> > So should the existing support for the 'active-low' and
> > 'inactive-high-impedance' properties be replaced by that string enum?
> > Or should the string property be interpreted in addition to the
> > bools defined in leds/common.yaml?
> > 
> > Should the string property be defined for each PHY or should we move
> > it into a common file?
> > 
> > If so, should that common file also be leds/common.yaml or should we
> > create a new file only for PHY LEDs instead?
> > 
> > Sorry for being confused, I don't mind going down what ever path to have
> > LED polarity configurable properly in DT.
> 
> Let's ignore my idea.
> 
> However I still wonder whether your choice for lack of properties is
> appropriate. Lack of properties as "bootloader default" means it can
> change. Why would anyone prefer to keep bootloader default? The wiring
> is fixed - it's never "we design PCB based on bootloader, so with new
> bootloader we will change PCB"?
> 
> And if you meant bootstrapping through some hardwired configuration,
> then again it is known and defined.

I agree, and my original intention was to just always apply polarity
settings and force people to correctly declare them in DT.
However, that would break DT compatibility on devices not making use
of those properties and relying only on strapping or bootloader
defaults. See also RFC discussed here:

https://patchwork.kernel.org/project/netdevbpf/patch/473d62f268f2a317fd81d0f38f15d2f2f98e2451.1728056697.git.daniel@xxxxxxxxxxxxxx/