| Message ID | 20210310070025.9150-1-zajec5@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | dt-bindings: leds: leds-gpio: fix & extend node regex | expand |
On Wed, 10 Mar 2021 08:00:25 +0100 Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > The old regex allowed only 1 character to follow the "led-" prefix which > was most likely just an overlook. Fix it and while at it allow dashes in > node names. It allows more meaningful names and it helpful e.g. when > having the same function name with 2 different colors. For example: > 1. led-power-white > 2. led-power-blue > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > Documentation/devicetree/bindings/leds/leds-gpio.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-gpio.yaml > index 7ad2baeda0b0..ae46a43e480f 100644 > --- a/Documentation/devicetree/bindings/leds/leds-gpio.yaml > +++ b/Documentation/devicetree/bindings/leds/leds-gpio.yaml > @@ -21,7 +21,7 @@ properties: > patternProperties: > # The first form is preferred, but fall back to just 'led' anywhere in the > # node name to at least catch some child nodes. > - "(^led-[0-9a-f]$|led)": > + "(^led-[0-9a-f][0-9a-f-]*$|led)": Why not use +, like everywhere else? "(^led-[0-9a-f]+$|led)"
On 12.03.2021 08:44, Marek Behun wrote: > On Wed, 10 Mar 2021 08:00:25 +0100 > Rafał Miłecki <zajec5@gmail.com> wrote: > >> From: Rafał Miłecki <rafal@milecki.pl> >> >> The old regex allowed only 1 character to follow the "led-" prefix which >> was most likely just an overlook. Fix it and while at it allow dashes in >> node names. It allows more meaningful names and it helpful e.g. when >> having the same function name with 2 different colors. For example: >> 1. led-power-white >> 2. led-power-blue >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> Documentation/devicetree/bindings/leds/leds-gpio.yaml | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-gpio.yaml >> index 7ad2baeda0b0..ae46a43e480f 100644 >> --- a/Documentation/devicetree/bindings/leds/leds-gpio.yaml >> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.yaml >> @@ -21,7 +21,7 @@ properties: >> patternProperties: >> # The first form is preferred, but fall back to just 'led' anywhere in the >> # node name to at least catch some child nodes. >> - "(^led-[0-9a-f]$|led)": >> + "(^led-[0-9a-f][0-9a-f-]*$|led)": > > Why not use +, like everywhere else? > "(^led-[0-9a-f]+$|led)" 1. Your regex doesn't allow dashes. I described that in commit message. 2. If I use one range and +, that will allow unwanted names like "led--power"
On Fri, 12 Mar 2021 08:52:16 +0100 Rafał Miłecki <zajec5@gmail.com> wrote: > On 12.03.2021 08:44, Marek Behun wrote: > > On Wed, 10 Mar 2021 08:00:25 +0100 > > Rafał Miłecki <zajec5@gmail.com> wrote: > > > >> From: Rafał Miłecki <rafal@milecki.pl> > >> > >> The old regex allowed only 1 character to follow the "led-" prefix which > >> was most likely just an overlook. Fix it and while at it allow dashes in > >> node names. It allows more meaningful names and it helpful e.g. when > >> having the same function name with 2 different colors. For example: > >> 1. led-power-white > >> 2. led-power-blue > >> > >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >> --- > >> Documentation/devicetree/bindings/leds/leds-gpio.yaml | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-gpio.yaml > >> index 7ad2baeda0b0..ae46a43e480f 100644 > >> --- a/Documentation/devicetree/bindings/leds/leds-gpio.yaml > >> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.yaml > >> @@ -21,7 +21,7 @@ properties: > >> patternProperties: > >> # The first form is preferred, but fall back to just 'led' anywhere in the > >> # node name to at least catch some child nodes. > >> - "(^led-[0-9a-f]$|led)": > >> + "(^led-[0-9a-f][0-9a-f-]*$|led)": > > > > Why not use +, like everywhere else? > > "(^led-[0-9a-f]+$|led)" > > 1. Your regex doesn't allow dashes. I described that in commit message. Ah, I confess I did not read the commit message. My fault. > 2. If I use one range and +, that will allow unwanted names like "led--power" But this can happen anyway. Your regex will match for example "led-deaf------beef". Moreover you give as an example names led-power-white led-power-blue but the regex only allows hexadecimal characters, ie led-dead-beef led-1f-3 The idea is that the string after "led-" is a hexadecimal address. Names like led-power-white shouldn't be used, as far as I understand. Marek
On 12.03.2021 09:23, Marek Behun wrote: > On Fri, 12 Mar 2021 08:52:16 +0100 > Rafał Miłecki <zajec5@gmail.com> wrote: > >> On 12.03.2021 08:44, Marek Behun wrote: >>> On Wed, 10 Mar 2021 08:00:25 +0100 >>> Rafał Miłecki <zajec5@gmail.com> wrote: >>> >>>> From: Rafał Miłecki <rafal@milecki.pl> >>>> >>>> The old regex allowed only 1 character to follow the "led-" prefix which >>>> was most likely just an overlook. Fix it and while at it allow dashes in >>>> node names. It allows more meaningful names and it helpful e.g. when >>>> having the same function name with 2 different colors. For example: >>>> 1. led-power-white >>>> 2. led-power-blue >>>> >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>> --- >>>> Documentation/devicetree/bindings/leds/leds-gpio.yaml | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-gpio.yaml >>>> index 7ad2baeda0b0..ae46a43e480f 100644 >>>> --- a/Documentation/devicetree/bindings/leds/leds-gpio.yaml >>>> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.yaml >>>> @@ -21,7 +21,7 @@ properties: >>>> patternProperties: >>>> # The first form is preferred, but fall back to just 'led' anywhere in the >>>> # node name to at least catch some child nodes. >>>> - "(^led-[0-9a-f]$|led)": >>>> + "(^led-[0-9a-f][0-9a-f-]*$|led)": >>> >>> Why not use +, like everywhere else? >>> "(^led-[0-9a-f]+$|led)" >> >> 1. Your regex doesn't allow dashes. I described that in commit message. > > Ah, I confess I did not read the commit message. My fault. > >> 2. If I use one range and +, that will allow unwanted names like "led--power" > > But this can happen anyway. Your regex will match for example > "led-deaf------beef". You're right. I probably was overthinking that ;) > Moreover you give as an example names > led-power-white > led-power-blue > but the regex only allows hexadecimal characters, ie > led-dead-beef > led-1f-3 > > The idea is that the string after "led-" is a hexadecimal address. > Names like > led-power-white > shouldn't be used, as far as I understand. Oops! 1. My regex was meant to be [0-9][a-z-][0-9][a-z-]+ 2. I totally missed that nodename should contain hex num and not a name My patch is based on bad binding understanding. So as I understand it now, the point of led hex number is to enumerate nodes. That way we avoid: ERROR (duplicate_node_names): /leds/led: Duplicate node name I'm just wondering if there is some cleaner solution than using those led-0, led-1, led-2, led-3, led-4 (...) names. Would that be acceptable to use address with GPIO number? Example: leds { compatible = "gpio-leds"; led@6 { gpios = <&mpc8572 6 GPIO_ACTIVE_HIGH>; color = <LED_COLOR_ID_RED>; }; led@7 { gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>; color = <LED_COLOR_ID_GREEN>; }; };
On Fri, 12 Mar 2021 10:12:26 +0100 Rafał Miłecki <zajec5@gmail.com> wrote: > On 12.03.2021 09:23, Marek Behun wrote: > > On Fri, 12 Mar 2021 08:52:16 +0100 > > Rafał Miłecki <zajec5@gmail.com> wrote: > > > >> On 12.03.2021 08:44, Marek Behun wrote: > >>> On Wed, 10 Mar 2021 08:00:25 +0100 > >>> Rafał Miłecki <zajec5@gmail.com> wrote: > >>> > >>>> From: Rafał Miłecki <rafal@milecki.pl> > >>>> > >>>> The old regex allowed only 1 character to follow the "led-" prefix which > >>>> was most likely just an overlook. Fix it and while at it allow dashes in > >>>> node names. It allows more meaningful names and it helpful e.g. when > >>>> having the same function name with 2 different colors. For example: > >>>> 1. led-power-white > >>>> 2. led-power-blue > >>>> > >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >>>> --- > >>>> Documentation/devicetree/bindings/leds/leds-gpio.yaml | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-gpio.yaml > >>>> index 7ad2baeda0b0..ae46a43e480f 100644 > >>>> --- a/Documentation/devicetree/bindings/leds/leds-gpio.yaml > >>>> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.yaml > >>>> @@ -21,7 +21,7 @@ properties: > >>>> patternProperties: > >>>> # The first form is preferred, but fall back to just 'led' anywhere in the > >>>> # node name to at least catch some child nodes. > >>>> - "(^led-[0-9a-f]$|led)": > >>>> + "(^led-[0-9a-f][0-9a-f-]*$|led)": > >>> > >>> Why not use +, like everywhere else? > >>> "(^led-[0-9a-f]+$|led)" > >> > >> 1. Your regex doesn't allow dashes. I described that in commit message. > > > > Ah, I confess I did not read the commit message. My fault. > > > >> 2. If I use one range and +, that will allow unwanted names like "led--power" > > > > But this can happen anyway. Your regex will match for example > > "led-deaf------beef". > > You're right. I probably was overthinking that ;) > > > > Moreover you give as an example names > > led-power-white > > led-power-blue > > but the regex only allows hexadecimal characters, ie > > led-dead-beef > > led-1f-3 > > > > The idea is that the string after "led-" is a hexadecimal address. > > Names like > > led-power-white > > shouldn't be used, as far as I understand. > > Oops! > 1. My regex was meant to be [0-9][a-z-][0-9][a-z-]+ > 2. I totally missed that nodename should contain hex num and not a name > > My patch is based on bad binding understanding. > > > So as I understand it now, the point of led hex number is to enumerate > nodes. That way we avoid: > ERROR (duplicate_node_names): /leds/led: Duplicate node name > > > I'm just wondering if there is some cleaner solution than using those > led-0, led-1, led-2, led-3, led-4 (...) names. > > Would that be acceptable to use address with GPIO number? Example: > > leds { > compatible = "gpio-leds"; > led@6 { > gpios = <&mpc8572 6 GPIO_ACTIVE_HIGH>; > color = <LED_COLOR_ID_RED>; > }; > led@7 { > gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>; > color = <LED_COLOR_ID_GREEN>; > }; > }; I don't know. This is a question for Rob Herring. But why is led-0, led-1, led-2 not good enough? You can still define function via the function property: led-7 { gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>; color = <LED_COLOR_ID_GREEN>; function = LED_FUNCTION_POWER; };
[Rob: please kindly comment on nodes numbering] On 12.03.2021 10:26, Marek Behun wrote: > On Fri, 12 Mar 2021 10:12:26 +0100 > Rafał Miłecki <zajec5@gmail.com> wrote: > >> On 12.03.2021 09:23, Marek Behun wrote: >>> On Fri, 12 Mar 2021 08:52:16 +0100 >>> Rafał Miłecki <zajec5@gmail.com> wrote: >>> >>>> On 12.03.2021 08:44, Marek Behun wrote: >>>>> On Wed, 10 Mar 2021 08:00:25 +0100 >>>>> Rafał Miłecki <zajec5@gmail.com> wrote: >>>>> >>>>>> From: Rafał Miłecki <rafal@milecki.pl> >>>>>> >>>>>> The old regex allowed only 1 character to follow the "led-" prefix which >>>>>> was most likely just an overlook. Fix it and while at it allow dashes in >>>>>> node names. It allows more meaningful names and it helpful e.g. when >>>>>> having the same function name with 2 different colors. For example: >>>>>> 1. led-power-white >>>>>> 2. led-power-blue >>>>>> >>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>>>> --- >>>>>> Documentation/devicetree/bindings/leds/leds-gpio.yaml | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-gpio.yaml >>>>>> index 7ad2baeda0b0..ae46a43e480f 100644 >>>>>> --- a/Documentation/devicetree/bindings/leds/leds-gpio.yaml >>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.yaml >>>>>> @@ -21,7 +21,7 @@ properties: >>>>>> patternProperties: >>>>>> # The first form is preferred, but fall back to just 'led' anywhere in the >>>>>> # node name to at least catch some child nodes. >>>>>> - "(^led-[0-9a-f]$|led)": >>>>>> + "(^led-[0-9a-f][0-9a-f-]*$|led)": >>>>> >>>>> Why not use +, like everywhere else? >>>>> "(^led-[0-9a-f]+$|led)" >>>> >>>> 1. Your regex doesn't allow dashes. I described that in commit message. >>> >>> Ah, I confess I did not read the commit message. My fault. >>> >>>> 2. If I use one range and +, that will allow unwanted names like "led--power" >>> >>> But this can happen anyway. Your regex will match for example >>> "led-deaf------beef". >> >> You're right. I probably was overthinking that ;) >> >> >>> Moreover you give as an example names >>> led-power-white >>> led-power-blue >>> but the regex only allows hexadecimal characters, ie >>> led-dead-beef >>> led-1f-3 >>> >>> The idea is that the string after "led-" is a hexadecimal address. >>> Names like >>> led-power-white >>> shouldn't be used, as far as I understand. >> >> Oops! >> 1. My regex was meant to be [0-9][a-z-][0-9][a-z-]+ >> 2. I totally missed that nodename should contain hex num and not a name >> >> My patch is based on bad binding understanding. >> >> >> So as I understand it now, the point of led hex number is to enumerate >> nodes. That way we avoid: >> ERROR (duplicate_node_names): /leds/led: Duplicate node name >> >> >> I'm just wondering if there is some cleaner solution than using those >> led-0, led-1, led-2, led-3, led-4 (...) names. >> >> Would that be acceptable to use address with GPIO number? Example: >> >> leds { >> compatible = "gpio-leds"; >> led@6 { >> gpios = <&mpc8572 6 GPIO_ACTIVE_HIGH>; >> color = <LED_COLOR_ID_RED>; >> }; >> led@7 { >> gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>; >> color = <LED_COLOR_ID_GREEN>; >> }; >> }; > > I don't know. This is a question for Rob Herring. > But why is led-0, led-1, led-2 not good enough? > You can still define function via the function property: > led-7 { > gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>; > color = <LED_COLOR_ID_GREEN>; > function = LED_FUNCTION_POWER; > }; 1. I found is a bit unnatural 2. Inserting/removing single LED may result in renaming nodes Nothing really serious, I'm just wondering if that could be slightly improved.
On Wed, Mar 10, 2021 at 08:00:25AM +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > The old regex allowed only 1 character to follow the "led-" prefix which > was most likely just an overlook. Indeed. > Fix it and while at it allow dashes in > node names. It allows more meaningful names and it helpful e.g. when > having the same function name with 2 different colors. For example: > 1. led-power-white > 2. led-power-blue No, node names are supposed to be generic and reflect the class of device. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > Documentation/devicetree/bindings/leds/leds-gpio.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-gpio.yaml > index 7ad2baeda0b0..ae46a43e480f 100644 > --- a/Documentation/devicetree/bindings/leds/leds-gpio.yaml > +++ b/Documentation/devicetree/bindings/leds/leds-gpio.yaml > @@ -21,7 +21,7 @@ properties: > patternProperties: > # The first form is preferred, but fall back to just 'led' anywhere in the > # node name to at least catch some child nodes. > - "(^led-[0-9a-f]$|led)": > + "(^led-[0-9a-f][0-9a-f-]*$|led)": > type: object > > $ref: common.yaml# > -- > 2.26.2 >
On 16.03.2021 23:31, Rob Herring wrote: > On Wed, Mar 10, 2021 at 08:00:25AM +0100, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> The old regex allowed only 1 character to follow the "led-" prefix which >> was most likely just an overlook. > > Indeed. > >> Fix it and while at it allow dashes in >> node names. It allows more meaningful names and it helpful e.g. when >> having the same function name with 2 different colors. For example: >> 1. led-power-white >> 2. led-power-blue > > No, node names are supposed to be generic and reflect the class of > device. There was some extra discussion on this patch that has ended up with a question about numbering nodes. Current binding assumes that nodes should be numbered with independent suffix numbers like: led-0 { }; led-1 { }; led-2 { }; Do you think this could / should be improved somehow? One option I was thinking about was using: led@0 { }; led@5 { }; where numbers ("0", "5") should match GPIO numbers. Is that a valid solution and does it improve things to make it worth it?
On Tue, Mar 16, 2021 at 5:25 PM Rafał Miłecki <rafal@milecki.pl> wrote: > > On 16.03.2021 23:31, Rob Herring wrote: > > On Wed, Mar 10, 2021 at 08:00:25AM +0100, Rafał Miłecki wrote: > >> From: Rafał Miłecki <rafal@milecki.pl> > >> > >> The old regex allowed only 1 character to follow the "led-" prefix which > >> was most likely just an overlook. > > > > Indeed. > > > >> Fix it and while at it allow dashes in > >> node names. It allows more meaningful names and it helpful e.g. when > >> having the same function name with 2 different colors. For example: > >> 1. led-power-white > >> 2. led-power-blue > > > > No, node names are supposed to be generic and reflect the class of > > device. > > There was some extra discussion on this patch that has ended up with a question about numbering nodes. > > Current binding assumes that nodes should be numbered with independent suffix numbers like: > led-0 { }; > led-1 { }; > led-2 { }; > > Do you think this could / should be improved somehow? No, we have other ways for meaningful names (label, color, function, etc.). > One option I was thinking about was using: > led@0 { }; > led@5 { }; > where numbers ("0", "5") should match GPIO numbers. > > Is that a valid solution and does it improve things to make it worth it? What if you have <gpioa 1> and <gpiob 1>? The cells in a consumer for a provider are opaque to the consumer. Rob
On 23.03.2021 23:02, Rob Herring wrote: > On Tue, Mar 16, 2021 at 5:25 PM Rafał Miłecki <rafal@milecki.pl> wrote: >> >> On 16.03.2021 23:31, Rob Herring wrote: >>> On Wed, Mar 10, 2021 at 08:00:25AM +0100, Rafał Miłecki wrote: >>>> From: Rafał Miłecki <rafal@milecki.pl> >>>> >>>> The old regex allowed only 1 character to follow the "led-" prefix which >>>> was most likely just an overlook. >>> >>> Indeed. >>> >>>> Fix it and while at it allow dashes in >>>> node names. It allows more meaningful names and it helpful e.g. when >>>> having the same function name with 2 different colors. For example: >>>> 1. led-power-white >>>> 2. led-power-blue >>> >>> No, node names are supposed to be generic and reflect the class of >>> device. >> >> There was some extra discussion on this patch that has ended up with a question about numbering nodes. >> >> Current binding assumes that nodes should be numbered with independent suffix numbers like: >> led-0 { }; >> led-1 { }; >> led-2 { }; >> >> Do you think this could / should be improved somehow? > > No, we have other ways for meaningful names (label, color, function, etc.). > >> One option I was thinking about was using: >> led@0 { }; >> led@5 { }; >> where numbers ("0", "5") should match GPIO numbers. >> >> Is that a valid solution and does it improve things to make it worth it? > > What if you have <gpioa 1> and <gpiob 1>? > > The cells in a consumer for a provider are opaque to the consumer. Thanks a lot for helping me understand that, it make sense ofc.
diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.yaml b/Documentation/devicetree/bindings/leds/leds-gpio.yaml index 7ad2baeda0b0..ae46a43e480f 100644 --- a/Documentation/devicetree/bindings/leds/leds-gpio.yaml +++ b/Documentation/devicetree/bindings/leds/leds-gpio.yaml @@ -21,7 +21,7 @@ properties: patternProperties: # The first form is preferred, but fall back to just 'led' anywhere in the # node name to at least catch some child nodes. - "(^led-[0-9a-f]$|led)": + "(^led-[0-9a-f][0-9a-f-]*$|led)": type: object $ref: common.yaml#