| Message ID | e91ca84ac836fc40c94c52733f8fc607bcbed64c.1728145095.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/4] dt-bindings: leds: add 'active-high' property | expand |
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. > 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@makrotopia.org> > --- > 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 think this should be just string enum, see marvell,marvell10g.yaml Best regards, Krzysztof
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@makrotopia.org> > > --- > > 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'. > > 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@waldekranz.com/ 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.
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@makrotopia.org> > > > --- > > > 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@waldekranz.com/ > > 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
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@waldekranz.com/ > > > > 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@makrotopia.org/
On Mon, Oct 07, 2024 at 12:30:53PM +0100, Daniel Golle wrote: > 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@waldekranz.com/ > > > > > > 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@makrotopia.org/ > I see that the series was marked as "Not Applicable" in patchwork. What is the reason for that? To me it looks like it can be applied on today's net-next cleanly: [daniel@box linux.git]$ git fetch https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next From https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next * branch HEAD -> FETCH_HEAD [daniel@box linux.git]$ git checkout FETCH_HEAD HEAD is now at 6607c17c6c5e net: mana: Enable debugfs files for MANA device [daniel@box linux.git]$ wget -q -O - https://patchwork.kernel.org/series/895863/mbox/ | git am Applying: dt-bindings: leds: add 'active-high' property Applying: net: phy: support 'active-high' property for PHY LEDs Applying: net: phy: aquantia: correctly describe LED polarity override Applying: net: phy: mxl-gpy: correctly describe LED polarity [daniel@box linux.git]$ Or did I misunderstand the meaning of "Not Applicable"? If so, please clarify.
On Wed, 9 Oct 2024 14:32:45 +0100 Daniel Golle wrote:
> What is the reason for that?
No idea.
But we do need an ack from device tree maintainers, I don't see one.
On 05/10/2024 18:24, Daniel Golle wrote: > Other than described in commit c94d1783136 ("dt-bindings: net: phy: Make > 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@makrotopia.org> > --- > 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. > + > active-low: > type: boolean > description: > @@ -225,6 +231,14 @@ properties: > Maximum timeout in microseconds after which the flash LED is turned off. > Required for flash LED nodes with configurable timeout. > > +allOf: > + - if: > + required: > + - active-low > + then: > + properties: > + active-high: false I read prior discussion, so indeed that is safest bet. With the commit SHA fixed: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
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. + active-low: type: boolean description: @@ -225,6 +231,14 @@ properties: Maximum timeout in microseconds after which the flash LED is turned off. Required for flash LED nodes with configurable timeout. +allOf: + - if: + required: + - active-low + then: + properties: + active-high: false + additionalProperties: true examples:
Other than described in commit c94d1783136 ("dt-bindings: net: phy: Make 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@makrotopia.org> --- Documentation/devicetree/bindings/leds/common.yaml | 14 ++++++++++++++ 1 file changed, 14 insertions(+)