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

[Krzysztof Kozlowski]

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:
> > On Sat, Oct 05, 2024 at 05:24:20PM +0100, Daniel Golle wrote:
> > > Other than described in commit c94d1783136 ("dt-bindings: net: phy: Make
> > 
> > Please run scripts/checkpatch.pl and fix reported warnings. Then please
> > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
> > Some warnings can be ignored, especially from --strict run, but the code
> > here looks like it needs a fix. Feel free to get in touch if the warning
> > is not clear.
> 
> Sorry about that, I was expecting '--fix-inplace' to take care of that
> but it didn't and I didn't notice. I will address that in a follow-up
> patch.
> 
> > 
> > > LED active-low property common") the absence of the 'active-low'
> > > property means not to touch the polarity settings which are inherited
> > > from reset defaults, the bootloader or bootstrap configuration.
> > > Hence, in order to override a LED pin being active-high in case of the
> > > default, bootloader or bootstrap setting being active-low an additional
> > > property 'active-high' is required.
> > > Document that property and make it mutually exclusive to the existing
> > > 'active-low' property.
> > > 
> > > Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> > > ---
> > >  Documentation/devicetree/bindings/leds/common.yaml | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
> > > index bf9a101e4d42..7c3cd7b7412e 100644
> > > --- a/Documentation/devicetree/bindings/leds/common.yaml
> > > +++ b/Documentation/devicetree/bindings/leds/common.yaml
> > > @@ -202,6 +202,12 @@ properties:
> > >        #trigger-source-cells property in the source node.
> > >      $ref: /schemas/types.yaml#/definitions/phandle-array
> > >  
> > > +  active-high:
> > > +    type: boolean
> > > +    description:
> > > +      Makes LED active high. To turn the LED ON, line needs to be
> > > +      set to high voltage instead of low.
> > 
> > And then we are going to get 2 more bools for other variants...
> 
> I don't see a problem combining 'active-high' or 'active-low' with
> 'inactive-high-impedance' which would be the equivalent of
> 'active-low-tristate' and 'active-high-tristate'.

Oh, I missed that we have already two bool properties. I thought that
there is only active-high.

> 
> > 
> > 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.

Best regards,
Krzysztof