Re: [PATCH] dt-bindings: leds: leds-gpio: fix & extend node regex

[Marek Behun <marek.behun@nic.cz>]

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;
 };