[PATCH 0/6] some LED patches

[PATCH 0/6] some LED patches

[Rasmus Villemoes]

I wanted to add support for ti,lp5562, and found an old submission
from Doug. While trying to modify that to work in current U-Boot, I
found a problem with the "move label handling to core" patches.

Patch 1 is an attempt at (quick-)fixing that, though the real fix is
probably to not bind the same driver to the top node as to the child
nodes and using uc_plat->label to distinguish in .probe.

Patch 2 touches the same area, and is needed for the lp5562 driver to
work with existing DT bindings.

Patch 3 introduces a helper which should allow removing some
boilerplate in most individual drivers, and 4,5 apply that in the gpio
and pwm drivers. Converting remaining drivers is trivial, but left out
for now.

Finally patch 6 is the reworked lp5562 driver. While I've changed it
to match existing DT bindings (with the goal of making it work with
our .dts that is known to work with the linux driver), most of the
logic is unchanged from Doug's original patch, so he is still listed
as author.

Doug Zobel (1):
  led: add TI LP5562 LED driver

Rasmus Villemoes (5):
  led-uclass: do not create fallback label for top-level node
  led-uclass: honour ->label field populated by driver's own .bind
  led: introduce led_bind_generic()
  led: led_gpio: use led_bind_generic() helper
  led: led_pwm: use led_bind_generic() helper

 doc/device-tree-bindings/leds/leds-lp5562.txt |  63 ++
 drivers/led/Kconfig                           |   8 +
 drivers/led/Makefile                          |   1 +
 drivers/led/led-uclass.c                      |  22 +-
 drivers/led/led_gpio.c                        |  15 +-
 drivers/led/led_lp5562.c                      | 578 ++++++++++++++++++
 drivers/led/led_pwm.c                         |  14 +-
 include/led.h                                 |   8 +
 8 files changed, 681 insertions(+), 28 deletions(-)
 create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt
 create mode 100644 drivers/led/led_lp5562.c

-- 
2.40.1.1.g1c60b9335d

[PATCH 1/6] led-uclass: do not create fallback label for top-level node

[Rasmus Villemoes]

Many existing drivers, and led-uclass itself, rely on uc_plat->label
being NULL for the device representing the top node, as opposed to the
child nodes representing individual LEDs. This means that the drivers
whose .probe methods rely on this were broken by 83c63f0d1185 ("led:
Move OF "label" property parsing to core"), and also that the top node
wrongly shows up with 'led list'. Some drivers have since been fixed
up individually, e.g.

e3aa76644c2a "led: gpio: Check device compatible string to determine the top level node"
01074697801b "led: gpio: Use NOP uclass driver for top-level node"
910b01c27c04 "drivers: led: bcm6753: do not use null label to find the top"

Binding the same driver to the top node as to the individual child
nodes is arguably wrong, and the approach of using a UCLASS_NOP driver
for the top node is probably better.

But as a temporary work-around, we can use a heuristic that only sets
the label to the fallback value derived from the node name if the node
does not have a "compatible" property - i.e., if it has been bound to
the LED driver explicitly via device_bind_driver_to_node() [similar to
what e3aa76644c2a did, but that then vanished with the next commit.]

Fixes: 83c63f0d1185 ("led: Move OF "label" property parsing to core")
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/led/led-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 68ca3c2970..5a5d07b9a7 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -72,7 +72,7 @@ static int led_post_bind(struct udevice *dev)
 	const char *default_state;
 
 	uc_plat->label = dev_read_string(dev, "label");
-	if (!uc_plat->label)
+	if (!uc_plat->label && !dev_read_string(dev, "compatible"))
 		uc_plat->label = ofnode_get_name(dev_ofnode(dev));
 
 	uc_plat->default_state = LEDST_COUNT;
-- 
2.40.1.1.g1c60b9335d

Re: [PATCH 1/6] led-uclass: do not create fallback label for top-level node

[Marek Vasut]

On 10/19/23 11:58, Rasmus Villemoes wrote:
> Many existing drivers, and led-uclass itself, rely on uc_plat->label
> being NULL for the device representing the top node, as opposed to the
> child nodes representing individual LEDs. This means that the drivers
> whose .probe methods rely on this were broken by 83c63f0d1185 ("led:
> Move OF "label" property parsing to core"), and also that the top node
> wrongly shows up with 'led list'. Some drivers have since been fixed
> up individually, e.g.
> 
> e3aa76644c2a "led: gpio: Check device compatible string to determine the top level node"
> 01074697801b "led: gpio: Use NOP uclass driver for top-level node"
> 910b01c27c04 "drivers: led: bcm6753: do not use null label to find the top"
> 
> Binding the same driver to the top node as to the individual child
> nodes is arguably wrong, and the approach of using a UCLASS_NOP driver
> for the top node is probably better.

Note that
83c63f0d1185 ("led: Move OF "label" property parsing to core")
and
01074697801b ("led: gpio: Use NOP uclass driver for top-level node")
were applied shortly after each other, so I don't see the point of the 
aforementioned rant.

I sort-of understand what you are trying to do in this patch based on 
$SUBJECT of this email, but not from this wall of text, so can you 
abbreviate the commit message ?

> But as a temporary work-around, we can use a heuristic that only sets
> the label to the fallback value derived from the node name if the node
> does not have a "compatible" property - i.e., if it has been bound to
> the LED driver explicitly via device_bind_driver_to_node() [similar to
> what e3aa76644c2a did, but that then vanished with the next commit.]
> 
> Fixes: 83c63f0d1185 ("led: Move OF "label" property parsing to core")
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/led/led-uclass.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> index 68ca3c2970..5a5d07b9a7 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -72,7 +72,7 @@ static int led_post_bind(struct udevice *dev)
>   	const char *default_state;
>   
>   	uc_plat->label = dev_read_string(dev, "label");
> -	if (!uc_plat->label)
> +	if (!uc_plat->label && !dev_read_string(dev, "compatible"))
>   		uc_plat->label = ofnode_get_name(dev_ofnode(dev));

Is there an existing driver which has a top-level DT node with "label" 
property ?

>   	uc_plat->default_state = LEDST_COUNT;

Re: [PATCH 1/6] led-uclass: do not create fallback label for top-level node

[Rasmus Villemoes]

On 19/10/2023 15.51, Marek Vasut wrote:
> On 10/19/23 11:58, Rasmus Villemoes wrote:
>> Many existing drivers, and led-uclass itself, rely on uc_plat->label
>> being NULL for the device representing the top node, as opposed to the
>> child nodes representing individual LEDs. This means that the drivers
>> whose .probe methods rely on this were broken by 83c63f0d1185 ("led:
>> Move OF "label" property parsing to core"), and also that the top node
>> wrongly shows up with 'led list'. Some drivers have since been fixed
>> up individually, e.g.
>>
>> e3aa76644c2a "led: gpio: Check device compatible string to determine
>> the top level node"
>> 01074697801b "led: gpio: Use NOP uclass driver for top-level node"
>> 910b01c27c04 "drivers: led: bcm6753: do not use null label to find the
>> top"
>>
>> Binding the same driver to the top node as to the individual child
>> nodes is arguably wrong, and the approach of using a UCLASS_NOP driver
>> for the top node is probably better.
> 
> Note that
> 83c63f0d1185 ("led: Move OF "label" property parsing to core")
> and
> 01074697801b ("led: gpio: Use NOP uclass driver for top-level node")
> were applied shortly after each other, so I don't see the point of the
> aforementioned rant.

What rant? I'm merely trying to write down what I found out while trying
to figure out why 83c63f0d1185 broke stuff, while acknowledging that the
fixes that have been applied to some drivers are probably the approach
in general.

> I sort-of understand what you are trying to do in this patch based on
> $SUBJECT of this email, but not from this wall of text, so can you
> abbreviate the commit message ?

Sure, I can try and make it shorter.

>> -    if (!uc_plat->label)
>> +    if (!uc_plat->label && !dev_read_string(dev, "compatible"))
>>           uc_plat->label = ofnode_get_name(dev_ofnode(dev));
> 
> Is there an existing driver which has a top-level DT node with "label"
> property ?

-ENOPARSE? A driver doesn't have a top-level DT node.

And regardless, this wouldn't change anything for a top-level DT node
with a "label" property, as we're still unconditionally first trying to
read a "label" property, this is merely avoiding adding a fallback value
for top-level DT nodes (using "having a "compatible" DT property as
proxy for that "is a top-level DT node"). So I really don't understand
what you are trying to ask.

Rasmus

Re: [PATCH 1/6] led-uclass: do not create fallback label for top-level node

[Marek Vasut]

On 10/23/23 10:28, Rasmus Villemoes wrote:
> On 19/10/2023 15.51, Marek Vasut wrote:
>> On 10/19/23 11:58, Rasmus Villemoes wrote:
>>> Many existing drivers, and led-uclass itself, rely on uc_plat->label
>>> being NULL for the device representing the top node, as opposed to the
>>> child nodes representing individual LEDs. This means that the drivers
>>> whose .probe methods rely on this were broken by 83c63f0d1185 ("led:
>>> Move OF "label" property parsing to core"), and also that the top node
>>> wrongly shows up with 'led list'. Some drivers have since been fixed
>>> up individually, e.g.
>>>
>>> e3aa76644c2a "led: gpio: Check device compatible string to determine
>>> the top level node"
>>> 01074697801b "led: gpio: Use NOP uclass driver for top-level node"
>>> 910b01c27c04 "drivers: led: bcm6753: do not use null label to find the
>>> top"
>>>
>>> Binding the same driver to the top node as to the individual child
>>> nodes is arguably wrong, and the approach of using a UCLASS_NOP driver
>>> for the top node is probably better.
>>
>> Note that
>> 83c63f0d1185 ("led: Move OF "label" property parsing to core")
>> and
>> 01074697801b ("led: gpio: Use NOP uclass driver for top-level node")
>> were applied shortly after each other, so I don't see the point of the
>> aforementioned rant.
> 
> What rant? I'm merely trying to write down what I found out while trying
> to figure out why 83c63f0d1185 broke stuff, while acknowledging that the
> fixes that have been applied to some drivers are probably the approach
> in general.
> 
>> I sort-of understand what you are trying to do in this patch based on
>> $SUBJECT of this email, but not from this wall of text, so can you
>> abbreviate the commit message ?
> 
> Sure, I can try and make it shorter.
> 
>>> -    if (!uc_plat->label)
>>> +    if (!uc_plat->label && !dev_read_string(dev, "compatible"))
>>>            uc_plat->label = ofnode_get_name(dev_ofnode(dev));
>>
>> Is there an existing driver which has a top-level DT node with "label"
>> property ?
> 
> -ENOPARSE? A driver doesn't have a top-level DT node.

I mean, is there a driver which does:

/ {
   led {
     label = "example";
   };
};

?

I think that is what the conditional checks here, right ?

> And regardless, this wouldn't change anything for a top-level DT node
> with a "label" property, as we're still unconditionally first trying to
> read a "label" property, this is merely avoiding adding a fallback value
> for top-level DT nodes (using "having a "compatible" DT property as
> proxy for that "is a top-level DT node"). So I really don't understand
> what you are trying to ask.

[PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind

[Rasmus Villemoes]

If the driver's own .bind method has populated uc_plat->label, don't
override that. This is necessary for an upcoming driver for ti,lp5562,
where the DT binding unfortunately says to use "chan-name" and not
"label".

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/led/led-uclass.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 5a5d07b9a7..0232fa84de 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev)
 	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
 	const char *default_state;
 
-	uc_plat->label = dev_read_string(dev, "label");
+	if (!uc_plat->label)
+		uc_plat->label = dev_read_string(dev, "label");
+
 	if (!uc_plat->label && !dev_read_string(dev, "compatible"))
 		uc_plat->label = ofnode_get_name(dev_ofnode(dev));
 
-- 
2.40.1.1.g1c60b9335d

Re: [PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind

[Marek Vasut]

On 10/19/23 11:58, Rasmus Villemoes wrote:
> If the driver's own .bind method has populated uc_plat->label, don't
> override that. This is necessary for an upcoming driver for ti,lp5562,
> where the DT binding unfortunately says to use "chan-name" and not
> "label".
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/led/led-uclass.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> index 5a5d07b9a7..0232fa84de 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev)
>   	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>   	const char *default_state;
>   
> -	uc_plat->label = dev_read_string(dev, "label");
> +	if (!uc_plat->label)
> +		uc_plat->label = dev_read_string(dev, "label");
> +

One thing I have to wonder about is, why does this controller have label 
property in the top-level node , what is that used for ?

(see Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml)

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind

[Rasmus Villemoes]

On 19/10/2023 15.54, Marek Vasut wrote:
> On 10/19/23 11:58, Rasmus Villemoes wrote:
>> If the driver's own .bind method has populated uc_plat->label, don't
>> override that. This is necessary for an upcoming driver for ti,lp5562,
>> where the DT binding unfortunately says to use "chan-name" and not
>> "label".
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>   drivers/led/led-uclass.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
>> index 5a5d07b9a7..0232fa84de 100644
>> --- a/drivers/led/led-uclass.c
>> +++ b/drivers/led/led-uclass.c
>> @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev)
>>       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>>       const char *default_state;
>>   -    uc_plat->label = dev_read_string(dev, "label");
>> +    if (!uc_plat->label)
>> +        uc_plat->label = dev_read_string(dev, "label");
>> +
> 
> One thing I have to wonder about is, why does this controller have label
> property in the top-level node , what is that used for ?
> 
> (see Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml)
> 
> Reviewed-by: Marek Vasut <marex@denx.de>

Reading the linux driver, it seems that the top-level label, if any, is
used as part of the naming for individual channels if they don't have
individual chan-name properties:


        if (pdata->led_config[chan].name) {
                led->cdev.name = pdata->led_config[chan].name;
        } else {
                snprintf(name, sizeof(name), "%s:channel%d",
                        pdata->label ? : chip->cl->name, chan);
                led->cdev.name = name;
        }

but I think the rationale in d1188adb2dabc is a bit weak, since the only
example also does have individual chan-name properties.

[Complete aside: At first I thought it was related to the multi-color
LED work that has been ongoing for many many years (I think there was an
LWN article at some point), where this could be exposed as a single
multi-color LED, as opposed to the "traditional" three/four individual
LEDs. In the former case, there would only be one sysfs entry, but with
attributes exposing the multicolor functionality. I must admit I don't
know the status of that work, when something reaches v31,
http://archive.lwn.net:8080/linux-kernel/20200722071055.GA8984@amd/t/ ,
it's hard to know if it ever lands, or if pieces of it has landed.]

Rasmus

Re: [PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind

[Marek Vasut]

On 10/23/23 10:51, Rasmus Villemoes wrote:
> On 19/10/2023 15.54, Marek Vasut wrote:
>> On 10/19/23 11:58, Rasmus Villemoes wrote:
>>> If the driver's own .bind method has populated uc_plat->label, don't
>>> override that. This is necessary for an upcoming driver for ti,lp5562,
>>> where the DT binding unfortunately says to use "chan-name" and not
>>> "label".
>>>
>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> ---
>>>    drivers/led/led-uclass.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
>>> index 5a5d07b9a7..0232fa84de 100644
>>> --- a/drivers/led/led-uclass.c
>>> +++ b/drivers/led/led-uclass.c
>>> @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev)
>>>        struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>>>        const char *default_state;
>>>    -    uc_plat->label = dev_read_string(dev, "label");
>>> +    if (!uc_plat->label)
>>> +        uc_plat->label = dev_read_string(dev, "label");
>>> +
>>
>> One thing I have to wonder about is, why does this controller have label
>> property in the top-level node , what is that used for ?
>>
>> (see Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml)
>>
>> Reviewed-by: Marek Vasut <marex@denx.de>
> 
> Reading the linux driver, it seems that the top-level label, if any, is
> used as part of the naming for individual channels if they don't have
> individual chan-name properties:
> 
> 
>          if (pdata->led_config[chan].name) {
>                  led->cdev.name = pdata->led_config[chan].name;
>          } else {
>                  snprintf(name, sizeof(name), "%s:channel%d",
>                          pdata->label ? : chip->cl->name, chan);
>                  led->cdev.name = name;
>          }
> 
> but I think the rationale in d1188adb2dabc is a bit weak, since the only
> example also does have individual chan-name properties.
> 
> [Complete aside: At first I thought it was related to the multi-color
> LED work that has been ongoing for many many years (I think there was an
> LWN article at some point), where this could be exposed as a single
> multi-color LED, as opposed to the "traditional" three/four individual
> LEDs. In the former case, there would only be one sysfs entry, but with
> attributes exposing the multicolor functionality. I must admit I don't
> know the status of that work, when something reaches v31,
> http://archive.lwn.net:8080/linux-kernel/20200722071055.GA8984@amd/t/ ,
> it's hard to know if it ever lands, or if pieces of it has landed.]

+CC Pavel

Re: [PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind

[Christian Gmeiner]

ping

Am Mo., 23. Okt. 2023 um 12:45 Uhr schrieb Marek Vasut <marex@denx.de>:
>
> On 10/23/23 10:51, Rasmus Villemoes wrote:
> > On 19/10/2023 15.54, Marek Vasut wrote:
> >> On 10/19/23 11:58, Rasmus Villemoes wrote:
> >>> If the driver's own .bind method has populated uc_plat->label, don't
> >>> override that. This is necessary for an upcoming driver for ti,lp5562,
> >>> where the DT binding unfortunately says to use "chan-name" and not
> >>> "label".
> >>>
> >>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >>> ---
> >>>    drivers/led/led-uclass.c | 4 +++-
> >>>    1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> >>> index 5a5d07b9a7..0232fa84de 100644
> >>> --- a/drivers/led/led-uclass.c
> >>> +++ b/drivers/led/led-uclass.c
> >>> @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev)
> >>>        struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> >>>        const char *default_state;
> >>>    -    uc_plat->label = dev_read_string(dev, "label");
> >>> +    if (!uc_plat->label)
> >>> +        uc_plat->label = dev_read_string(dev, "label");
> >>> +
> >>
> >> One thing I have to wonder about is, why does this controller have label
> >> property in the top-level node , what is that used for ?
> >>
> >> (see Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml)
> >>
> >> Reviewed-by: Marek Vasut <marex@denx.de>
> >
> > Reading the linux driver, it seems that the top-level label, if any, is
> > used as part of the naming for individual channels if they don't have
> > individual chan-name properties:
> >
> >
> >          if (pdata->led_config[chan].name) {
> >                  led->cdev.name = pdata->led_config[chan].name;
> >          } else {
> >                  snprintf(name, sizeof(name), "%s:channel%d",
> >                          pdata->label ? : chip->cl->name, chan);
> >                  led->cdev.name = name;
> >          }
> >
> > but I think the rationale in d1188adb2dabc is a bit weak, since the only
> > example also does have individual chan-name properties.
> >
> > [Complete aside: At first I thought it was related to the multi-color
> > LED work that has been ongoing for many many years (I think there was an
> > LWN article at some point), where this could be exposed as a single
> > multi-color LED, as opposed to the "traditional" three/four individual
> > LEDs. In the former case, there would only be one sysfs entry, but with
> > attributes exposing the multicolor functionality. I must admit I don't
> > know the status of that work, when something reaches v31,
> > http://archive.lwn.net:8080/linux-kernel/20200722071055.GA8984@amd/t/ ,
> > it's hard to know if it ever lands, or if pieces of it has landed.]
>
> +CC Pavel



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Re: [PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind

[Marek Vasut]

On 11/14/23 13:11, Christian Gmeiner wrote:
> ping
> 
> Am Mo., 23. Okt. 2023 um 12:45 Uhr schrieb Marek Vasut <marex@denx.de>:
>>
>> On 10/23/23 10:51, Rasmus Villemoes wrote:
>>> On 19/10/2023 15.54, Marek Vasut wrote:
>>>> On 10/19/23 11:58, Rasmus Villemoes wrote:
>>>>> If the driver's own .bind method has populated uc_plat->label, don't
>>>>> override that. This is necessary for an upcoming driver for ti,lp5562,
>>>>> where the DT binding unfortunately says to use "chan-name" and not
>>>>> "label".
>>>>>
>>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>>> ---
>>>>>     drivers/led/led-uclass.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
>>>>> index 5a5d07b9a7..0232fa84de 100644
>>>>> --- a/drivers/led/led-uclass.c
>>>>> +++ b/drivers/led/led-uclass.c
>>>>> @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev)
>>>>>         struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>>>>>         const char *default_state;
>>>>>     -    uc_plat->label = dev_read_string(dev, "label");
>>>>> +    if (!uc_plat->label)
>>>>> +        uc_plat->label = dev_read_string(dev, "label");
>>>>> +
>>>>
>>>> One thing I have to wonder about is, why does this controller have label
>>>> property in the top-level node , what is that used for ?
>>>>
>>>> (see Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml)
>>>>
>>>> Reviewed-by: Marek Vasut <marex@denx.de>
>>>
>>> Reading the linux driver, it seems that the top-level label, if any, is
>>> used as part of the naming for individual channels if they don't have
>>> individual chan-name properties:
>>>
>>>
>>>           if (pdata->led_config[chan].name) {
>>>                   led->cdev.name = pdata->led_config[chan].name;
>>>           } else {
>>>                   snprintf(name, sizeof(name), "%s:channel%d",
>>>                           pdata->label ? : chip->cl->name, chan);
>>>                   led->cdev.name = name;
>>>           }
>>>
>>> but I think the rationale in d1188adb2dabc is a bit weak, since the only
>>> example also does have individual chan-name properties.
>>>
>>> [Complete aside: At first I thought it was related to the multi-color
>>> LED work that has been ongoing for many many years (I think there was an
>>> LWN article at some point), where this could be exposed as a single
>>> multi-color LED, as opposed to the "traditional" three/four individual
>>> LEDs. In the former case, there would only be one sysfs entry, but with
>>> attributes exposing the multicolor functionality. I must admit I don't
>>> know the status of that work, when something reaches v31,
>>> http://archive.lwn.net:8080/linux-kernel/20200722071055.GA8984@amd/t/ ,
>>> it's hard to know if it ever lands, or if pieces of it has landed.]
>>
>> +CC Pavel

I think you want to coordinate the effort with Rasmus here .

Re: [PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2815 bytes --]

Hi!

> > > > > > diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> > > > > > index 5a5d07b9a7..0232fa84de 100644
> > > > > > --- a/drivers/led/led-uclass.c
> > > > > > +++ b/drivers/led/led-uclass.c
> > > > > > @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev)
> > > > > >         struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> > > > > >         const char *default_state;
> > > > > >     -    uc_plat->label = dev_read_string(dev, "label");
> > > > > > +    if (!uc_plat->label)
> > > > > > +        uc_plat->label = dev_read_string(dev, "label");
> > > > > > +
> > > > > 
> > > > > One thing I have to wonder about is, why does this controller have label
> > > > > property in the top-level node , what is that used for ?
> > > > > 
> > > > > (see Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml)
> > > > > 
> > > > > Reviewed-by: Marek Vasut <marex@denx.de>
> > > > 
> > > > Reading the linux driver, it seems that the top-level label, if any, is
> > > > used as part of the naming for individual channels if they don't have
> > > > individual chan-name properties:
> > > > 
> > > > 
> > > >           if (pdata->led_config[chan].name) {
> > > >                   led->cdev.name = pdata->led_config[chan].name;
> > > >           } else {
> > > >                   snprintf(name, sizeof(name), "%s:channel%d",
> > > >                           pdata->label ? : chip->cl->name, chan);
> > > >                   led->cdev.name = name;
> > > >           }
> > > > 
> > > > but I think the rationale in d1188adb2dabc is a bit weak, since the only
> > > > example also does have individual chan-name properties.
> > > > 
> > > > [Complete aside: At first I thought it was related to the multi-color
> > > > LED work that has been ongoing for many many years (I think there was an
> > > > LWN article at some point), where this could be exposed as a single
> > > > multi-color LED, as opposed to the "traditional" three/four individual
> > > > LEDs. In the former case, there would only be one sysfs entry, but with
> > > > attributes exposing the multicolor functionality. I must admit I don't
> > > > know the status of that work, when something reaches v31,
> > > > http://archive.lwn.net:8080/linux-kernel/20200722071055.GA8984@amd/t/ ,
> > > > it's hard to know if it ever lands, or if pieces of it has landed.]
> > > 
> > > +CC Pavel
> 
> I think you want to coordinate the effort with Rasmus here .

This is u-boot, not Linux, right?

Anyway cc-ing linux-leds may be better than cc-ing me directly, but
don't expect much.

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind

[Christian Gmeiner]

Am Mo., 23. Okt. 2023 um 11:30 Uhr schrieb Rasmus Villemoes
<rasmus.villemoes@prevas.dk>:
>
> On 19/10/2023 15.54, Marek Vasut wrote:
> > On 10/19/23 11:58, Rasmus Villemoes wrote:
> >> If the driver's own .bind method has populated uc_plat->label, don't
> >> override that. This is necessary for an upcoming driver for ti,lp5562,
> >> where the DT binding unfortunately says to use "chan-name" and not
> >> "label".
> >>
> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >> ---
> >>   drivers/led/led-uclass.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> >> index 5a5d07b9a7..0232fa84de 100644
> >> --- a/drivers/led/led-uclass.c
> >> +++ b/drivers/led/led-uclass.c
> >> @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev)
> >>       struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> >>       const char *default_state;
> >>   -    uc_plat->label = dev_read_string(dev, "label");
> >> +    if (!uc_plat->label)
> >> +        uc_plat->label = dev_read_string(dev, "label");
> >> +
> >
> > One thing I have to wonder about is, why does this controller have label
> > property in the top-level node , what is that used for ?
> >
> > (see Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml)
> >
> > Reviewed-by: Marek Vasut <marex@denx.de>
>
> Reading the linux driver, it seems that the top-level label, if any, is
> used as part of the naming for individual channels if they don't have
> individual chan-name properties:
>
>
>         if (pdata->led_config[chan].name) {
>                 led->cdev.name = pdata->led_config[chan].name;
>         } else {
>                 snprintf(name, sizeof(name), "%s:channel%d",
>                         pdata->label ? : chip->cl->name, chan);
>                 led->cdev.name = name;
>         }
>
> but I think the rationale in d1188adb2dabc is a bit weak, since the only
> example also does have individual chan-name properties.
>
> [Complete aside: At first I thought it was related to the multi-color
> LED work that has been ongoing for many many years (I think there was an
> LWN article at some point), where this could be exposed as a single
> multi-color LED, as opposed to the "traditional" three/four individual
> LEDs. In the former case, there would only be one sysfs entry, but with
> attributes exposing the multicolor functionality. I must admit I don't
> know the status of that work, when something reaches v31,
> http://archive.lwn.net:8080/linux-kernel/20200722071055.GA8984@amd/t/ ,
> it's hard to know if it ever lands, or if pieces of it has landed.]
>
> Rasmus
>

I have an almost similar patch sent out today as I have not seen yours
before. The end results
is the same as mine
https://patchwork.ozlabs.org/project/uboot/patch/20231023082117.39290-1-christian.gmeiner@gmail.com/

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

[PATCH 3/6] led: introduce led_bind_generic()

[Rasmus Villemoes]

All existing drivers in drivers/led/ contain a .bind method that does
exactly the same thing, with just the actual driver name
differing. Create a helper so all those individual methods can be
changed to one-liners.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/led/led-uclass.c | 18 ++++++++++++++++++
 include/led.h            |  8 ++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 0232fa84de..a4be56fc25 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -11,9 +11,27 @@
 #include <errno.h>
 #include <led.h>
 #include <dm/device-internal.h>
+#include <dm/lists.h>
 #include <dm/root.h>
 #include <dm/uclass-internal.h>
 
+int led_bind_generic(struct udevice *parent, const char *driver_name)
+{
+	struct udevice *dev;
+	ofnode node;
+	int ret;
+
+	dev_for_each_subnode(node, parent) {
+		ret = device_bind_driver_to_node(parent, driver_name,
+						 ofnode_get_name(node),
+						 node, &dev);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int led_get_by_label(const char *label, struct udevice **devp)
 {
 	struct udevice *dev;
diff --git a/include/led.h b/include/led.h
index 329041008c..a635316628 100644
--- a/include/led.h
+++ b/include/led.h
@@ -110,4 +110,12 @@ enum led_state_t led_get_state(struct udevice *dev);
  */
 int led_set_period(struct udevice *dev, int period_ms);
 
+/**
+ * led_bind_generic() - bind children of parent to given driver
+ *
+ * @parent:      Top-level LED device
+ * @driver_name: Driver for handling individual child nodes
+ */
+int led_bind_generic(struct udevice *parent, const char *driver_name);
+
 #endif
-- 
2.40.1.1.g1c60b9335d

Re: [PATCH 3/6] led: introduce led_bind_generic()

[Marek Vasut]

On 10/19/23 11:58, Rasmus Villemoes wrote:
> All existing drivers in drivers/led/ contain a .bind method that does
> exactly the same thing, with just the actual driver name
> differing. Create a helper so all those individual methods can be
> changed to one-liners.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Reviewed-by: Marek Vasut <marex@denx.de>

[PATCH 4/6] led: led_gpio: use led_bind_generic() helper

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/led/led_gpio.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/led/led_gpio.c b/drivers/led/led_gpio.c
index fbed151b5d..71421de628 100644
--- a/drivers/led/led_gpio.c
+++ b/drivers/led/led_gpio.c
@@ -11,7 +11,6 @@
 #include <log.h>
 #include <malloc.h>
 #include <asm/gpio.h>
-#include <dm/lists.h>
 
 struct led_gpio_priv {
 	struct gpio_desc gpio;
@@ -80,19 +79,7 @@ static int led_gpio_remove(struct udevice *dev)
 
 static int led_gpio_bind(struct udevice *parent)
 {
-	struct udevice *dev;
-	ofnode node;
-	int ret;
-
-	dev_for_each_subnode(node, parent) {
-		ret = device_bind_driver_to_node(parent, "gpio_led",
-						 ofnode_get_name(node),
-						 node, &dev);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
+	return led_bind_generic(parent, "gpio_led");
 }
 
 static const struct led_ops gpio_led_ops = {
-- 
2.40.1.1.g1c60b9335d

Re: [PATCH 4/6] led: led_gpio: use led_bind_generic() helper

[Marek Vasut]

On 10/19/23 11:58, Rasmus Villemoes wrote:

Commit message is missing.

[PATCH 5/6] led: led_pwm: use led_bind_generic() helper

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/led/led_pwm.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/led/led_pwm.c b/drivers/led/led_pwm.c
index 7c8eae9337..ae6de3087a 100644
--- a/drivers/led/led_pwm.c
+++ b/drivers/led/led_pwm.c
@@ -9,7 +9,6 @@
 #include <errno.h>
 #include <led.h>
 #include <malloc.h>
-#include <dm/lists.h>
 #include <pwm.h>
 
 #define LEDS_PWM_DRIVER_NAME	"led_pwm"
@@ -136,18 +135,7 @@ static int led_pwm_of_to_plat(struct udevice *dev)
 
 static int led_pwm_bind(struct udevice *parent)
 {
-	struct udevice *dev;
-	ofnode node;
-	int ret;
-
-	dev_for_each_subnode(node, parent) {
-		ret = device_bind_driver_to_node(parent, LEDS_PWM_DRIVER_NAME,
-						 ofnode_get_name(node),
-						 node, &dev);
-		if (ret)
-			return ret;
-	}
-	return 0;
+	return led_bind_generic(parent, LEDS_PWM_DRIVER_NAME);
 }
 
 static const struct led_ops led_pwm_ops = {
-- 
2.40.1.1.g1c60b9335d

Re: [PATCH 5/6] led: led_pwm: use led_bind_generic() helper

[Marek Vasut]

On 10/19/23 11:58, Rasmus Villemoes wrote:

Commit message is missing.

[PATCH 6/6] led: add TI LP5562 LED driver

[Rasmus Villemoes]

From: Doug Zobel <douglas.zobel@climate.com>

Driver for the TI LP5562 4 channel LED controller. Supports
independent on/off control of all 4 channels. Supports LED_BLINK on 3
independent channels: blue/green/red. The white channel can blink, but
shares the blue channel blink rate.

Heavily based on patch originally from Doug Zobel [1].

I have modified it so it matches the DT bindings in the linux tree,
and also follows the linux driver implementation more closely. This
should address Tom's concerns, and also matches my goal of making the
U-Boot driver work with our existing .dts which is known to work in
linux.

As our boards only have the R,G,B outputs connected, I have not
actually tested how the white channel behaves, but the R,G,B work
exactly as expected.

[1] https://lore.kernel.org/u-boot/1547150757-1561-1-git-send-email-douglas.zobel@climate.com/

Cc: Doug Zobel <douglas.zobel@climate.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 doc/device-tree-bindings/leds/leds-lp5562.txt |  63 ++
 drivers/led/Kconfig                           |   8 +
 drivers/led/Makefile                          |   1 +
 drivers/led/led_lp5562.c                      | 578 ++++++++++++++++++
 4 files changed, 650 insertions(+)
 create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt
 create mode 100644 drivers/led/led_lp5562.c

diff --git a/doc/device-tree-bindings/leds/leds-lp5562.txt b/doc/device-tree-bindings/leds/leds-lp5562.txt
new file mode 100644
index 0000000000..4e0c742959
--- /dev/null
+++ b/doc/device-tree-bindings/leds/leds-lp5562.txt
@@ -0,0 +1,63 @@
+LEDs connected to TI LP5562 controller
+
+This driver works with a TI LP5562 4-channel LED controller.
+CONFIG_LED_BLINK is supported using the controller engines.  However
+there are only 3 engines available for the 4 channels.  This means
+that the blue and white channels share the same engine.  When both
+blue and white LEDs are set to blink, they will share the same blink
+rate.  Changing the blink rate of the blue LED will affect the white
+LED and vice-versa.  Manual on/off is handled independently for all 4
+channels.
+
+Required properties:
+  - compatible : should be "ti,lp5562".
+  - #address-cells : must be 1.
+  - #size-cells : must be 0.
+  - reg : LP5562 LED controller I2C address.
+
+Optional properties:
+  - enable-gpios : Enable GPIO
+  - clock-mode : u8, configures the clock mode:
+      - 0 # automode
+      - 1 # internal
+      - 2 # external
+
+Each LED is represented as a sub-node of the ti,lp5562 device.
+
+LED sub-node required properties:
+  - reg : Zero-based channel identifier:
+    - 0 red
+    - 1 green
+    - 2 blue
+    - 3 white
+
+LED sub-node optional properties:
+  - chan-name : name of LED
+  - max-cur : LED current at max brightness in 100uA steps (0x00 - 0xFF)
+    Default : 100 (10 mA)
+
+Example:
+        leds0: lp5562@30 {
+                compatible = "ti,lp5562";
+                #address-cells = <1>;
+                #size-cells = <0>;
+                enable-gpios = <&gpio3 9 GPIO_ACTIVE_HIGH>;
+                reg = <0x30>;
+		clock-mode = /bits/8 <1>;
+
+                led@0 {
+                        reg = <0>;
+                        chan-name = "red";
+                        max-cur = /bits/ 8 <200>; /* 20mA */
+                };
+                led@1 {
+                        reg = <1>;
+                        chan-name = "green";
+                        max-cur = /bits/ 8 <200>; /* 20mA */
+                };
+                led@2 {
+                        reg = <2>;
+                        chan-name = "blue";
+                        max-cur = /bits/ 8 <200>; /* 20mA */
+                };
+        };
diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index 996b757e6d..9837960198 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -49,6 +49,14 @@ config LED_CORTINA
 	  This option enables support for LEDs connected to the Cortina
 	  Access CAxxxx SOCs.
 
+config LED_LP5562
+	bool "LED Support for LP5562"
+	depends on LED && DM_I2C
+	help
+	  This option enables support for LEDs connected to the TI LP5562
+	  4 channel I2C LED controller.  Driver fully supports blink on the
+	  B/G/R LEDs.  White LED can blink, but re-uses the period from blue.
+
 config LED_PWM
 	bool "LED PWM"
 	depends on LED && DM_PWM
diff --git a/drivers/led/Makefile b/drivers/led/Makefile
index 49ae91961d..2bcb858908 100644
--- a/drivers/led/Makefile
+++ b/drivers/led/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_LED_BCM6858) += led_bcm6858.o
 obj-$(CONFIG_LED_PWM) += led_pwm.o
 obj-$(CONFIG_$(SPL_)LED_GPIO) += led_gpio.o
 obj-$(CONFIG_LED_CORTINA) += led_cortina.o
+obj-$(CONFIG_LED_LP5562) += led_lp5562.o
diff --git a/drivers/led/led_lp5562.c b/drivers/led/led_lp5562.c
new file mode 100644
index 0000000000..87479ec551
--- /dev/null
+++ b/drivers/led/led_lp5562.c
@@ -0,0 +1,578 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Doug Zobel <douglas.zobel@climate.com>
+ *
+ * Driver for TI lp5562 4 channel LED driver.  There are only 3
+ * engines available for the 4 LEDs, so white and blue LEDs share
+ * the same engine.  This means that the blink period is shared
+ * between them.  Changing the period of blue blink will affect
+ * the white period (and vice-versa).  Blue and white On/Off
+ * states remain independent (as would PWM brightness if that's
+ * ever added to the LED core).
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <led.h>
+#include <i2c.h>
+#include <asm/gpio.h>
+#include <linux/delay.h>
+
+#define DEFAULT_CURRENT			100  /* 10 mA */
+#define MIN_BLINK_PERIOD		32   /* ms */
+#define MAX_BLINK_PERIOD		2248 /* ms */
+
+/* Register Map */
+#define REG_ENABLE			0x00
+#define REG_OP_MODE			0x01
+#define REG_B_PWM			0x02
+#define REG_G_PWM			0x03
+#define REG_R_PWM			0x04
+#define REG_B_CUR			0x05
+#define REG_G_CUR			0x06
+#define REG_R_CUR			0x07
+#define REG_CONFIG			0x08
+#define REG_ENG1_PC			0x09
+#define REG_ENG2_PC			0x0A
+#define REG_ENG3_PC			0x0B
+#define REG_STATUS			0x0C
+#define REG_RESET			0x0D
+#define REG_W_PWM			0x0E
+#define REG_W_CUR			0x0F
+#define REG_ENG1_MEM_BEGIN		0x10
+#define REG_ENG2_MEM_BEGIN		0x30
+#define REG_ENG3_MEM_BEGIN		0x50
+#define REG_LED_MAP			0x70
+
+/* LED Register Values */
+/* 0x00  ENABLE */
+#define REG_ENABLE_CHIP_ENABLE		(0x1 << 6)
+#define REG_ENABLE_ENG_EXEC_HOLD	0x0
+#define REG_ENABLE_ENG_EXEC_RUN		0x2
+#define REG_ENABLE_ENG_EXEC_MASK	0x3
+
+/* 0x01  OP MODE */
+#define REG_OP_MODE_DISABLED		0x0
+#define REG_OP_MODE_LOAD_SRAM		0x1
+#define REG_OP_MODE_RUN			0x2
+#define REG_OP_MODE_MASK		0x3
+
+/* 0x02, 0x03, 0x04, 0x0E  PWM */
+#define REG_PWM_MIN_VALUE		0
+#define REG_PWM_MAX_VALUE		0xFF
+
+/* 0x08  CONFIG */
+#define REG_CONFIG_EXT_CLK		0x0
+#define REG_CONFIG_INT_CLK		0x1
+#define REG_CONFIG_AUTO_CLK		0x2
+#define REG_CONFIG_CLK_MASK		0x3
+
+/* 0x0D  RESET */
+#define REG_RESET_RESET			0xFF
+
+/* 0x70  LED MAP */
+#define REG_LED_MAP_ENG_MASK		0x03
+#define REG_LED_MAP_W_ENG_SHIFT		6
+#define REG_LED_MAP_R_ENG_SHIFT		4
+#define REG_LED_MAP_G_ENG_SHIFT		2
+#define REG_LED_MAP_B_ENG_SHIFT		0
+
+/* Engine program related */
+#define REG_ENGINE_MEM_SIZE		0x20
+#define LED_PGRM_RAMP_INCREMENT_SHIFT	0
+#define LED_PGRM_RAMP_SIGN_SHIFT	7
+#define LED_PGRM_RAMP_STEP_SHIFT	8
+#define LED_PGRM_RAMP_PRESCALE_SHIFT	14
+
+struct lp5562_led_wrap_priv {
+	struct gpio_desc enable_gpio;
+};
+
+struct lp5562_led_priv {
+	u8 reg_pwm;
+	u8 reg_current;
+	u8 map_shift;
+	u8 enginenum;
+};
+
+/* enum values map to LED_MAP (0x70) values */
+enum lp5562_led_ctl_mode {
+	I2C = 0x0,
+#ifdef CONFIG_LED_BLINK
+	ENGINE1 = 0x1,
+	ENGINE2 = 0x2,
+	ENGINE3 = 0x3
+#endif
+};
+
+/*
+ * Update a register value
+ *  dev     - I2C udevice (parent of led)
+ *  regnum  - register number to update
+ *  value   - value to write to register
+ *  mask    - mask of bits that should be changed
+ */
+static int lp5562_led_reg_update(struct udevice *dev, int regnum,
+				 u8 value, u8 mask)
+{
+	int ret;
+
+	if (mask == 0xFF)
+		ret = dm_i2c_reg_write(dev, regnum, value);
+	else
+		ret = dm_i2c_reg_clrset(dev, regnum, mask, value);
+
+
+	/*
+	 * Data sheet says "Delay between consecutive I2C writes to
+	 * ENABLE register (00h) need to be longer than 488 μs
+	 * (typical)." and "Delay between consecutive I2C writes to
+	 * OP_MODE register need to be longer than 153 μs (typ)."
+	 *
+	 * The linux driver does usleep_range(500, 600) and
+	 * usleep_range(200, 300), respectively.
+	 */
+	switch (regnum) {
+	case REG_ENABLE:
+		udelay(600);
+		break;
+	case REG_OP_MODE:
+		udelay(300);
+		break;
+	}
+
+	return ret;
+}
+
+#ifdef CONFIG_LED_BLINK
+/*
+ * Program the lp5562 engine
+ *  dev     - I2C udevice (parent of led)
+ *  program - array of commands
+ *  size    - number of commands in program array (1-16)
+ *  engine  - engine number (1-3)
+ */
+static int lp5562_led_program_engine(struct udevice *dev, u16 *program,
+				     u8 size, u8 engine)
+{
+	int ret, cmd;
+	u8 engine_reg = REG_ENG1_MEM_BEGIN +
+			     ((engine - 1) * REG_ENGINE_MEM_SIZE);
+	u8 shift = (3 - engine) * 2;
+	__be16 prog_be[16];
+
+	if (size < 1 || size > 16 || engine < 1 || engine > 3)
+		return -EINVAL;
+
+	for (cmd = 0; cmd < size; cmd++)
+		prog_be[cmd] = cpu_to_be16(program[cmd]);
+
+	/* set engine mode to 'disabled' */
+	ret = lp5562_led_reg_update(dev, REG_OP_MODE,
+				    REG_OP_MODE_DISABLED << shift,
+				    REG_OP_MODE_MASK << shift);
+	if (ret != 0)
+		goto done;
+
+	/* set exec mode to 'hold' */
+	ret = lp5562_led_reg_update(dev, REG_ENABLE,
+				    REG_ENABLE_ENG_EXEC_HOLD << shift,
+				    REG_ENABLE_ENG_EXEC_MASK << shift);
+	if (ret != 0)
+		goto done;
+
+	/* set engine mode to 'load SRAM' */
+	ret = lp5562_led_reg_update(dev, REG_OP_MODE,
+				    REG_OP_MODE_LOAD_SRAM << shift,
+				    REG_OP_MODE_MASK << shift);
+	if (ret != 0)
+		goto done;
+
+	/* send the re-ordered program sequence */
+	ret = dm_i2c_write(dev, engine_reg, (uchar *)prog_be, sizeof(u16) * size);
+	if (ret != 0)
+		goto done;
+
+	/* set engine mode to 'run' */
+	ret = lp5562_led_reg_update(dev, REG_OP_MODE,
+				    REG_OP_MODE_RUN << shift,
+				    REG_OP_MODE_MASK << shift);
+	if (ret != 0)
+		goto done;
+
+	/* set engine exec to 'run' */
+	ret = lp5562_led_reg_update(dev, REG_ENABLE,
+				    REG_ENABLE_ENG_EXEC_RUN << shift,
+				    REG_ENABLE_ENG_EXEC_MASK << shift);
+
+done:
+	return ret;
+}
+
+/*
+ * Get the LED's current control mode (I2C or ENGINE[1-3])
+ *  dev       - led udevice (child udevice)
+ */
+static enum lp5562_led_ctl_mode lp5562_led_get_control_mode(struct udevice *dev)
+{
+	struct lp5562_led_priv *priv = dev_get_priv(dev);
+	u8 data;
+	enum lp5562_led_ctl_mode mode = I2C;
+
+	if (dm_i2c_read(dev_get_parent(dev), REG_LED_MAP, &data, 1) == 0)
+		mode = (data & (REG_LED_MAP_ENG_MASK << priv->map_shift))
+			>> priv->map_shift;
+
+	return mode;
+}
+#endif
+
+/*
+ * Set the LED's control mode to I2C or ENGINE[1-3]
+ *  dev       - led udevice (child udevice)
+ *  mode      - mode to change to
+ */
+static int lp5562_led_set_control_mode(struct udevice *dev,
+				       enum lp5562_led_ctl_mode mode)
+{
+	struct lp5562_led_priv *priv = dev_get_priv(dev);
+
+	return (lp5562_led_reg_update(dev_get_parent(dev), REG_LED_MAP,
+				      mode << priv->map_shift,
+				      REG_LED_MAP_ENG_MASK << priv->map_shift));
+}
+
+/*
+ * Return the LED's PWM value;  If LED is in BLINK state, then it is
+ * under engine control mode which doesn't use this PWM value.
+ *  dev       - led udevice (child udevice)
+ */
+static int lp5562_led_get_pwm(struct udevice *dev)
+{
+	struct lp5562_led_priv *priv = dev_get_priv(dev);
+	u8 data;
+
+	if (dm_i2c_read(dev_get_parent(dev), priv->reg_pwm, &data, 1) != 0)
+		return -EINVAL;
+
+	return data;
+}
+
+/*
+ * Set the LED's PWM value and configure it to use this (I2C mode).
+ *  dev       - led udevice (child udevice)
+ *  value     - PWM value (0 - 255)
+ */
+static int lp5562_led_set_pwm(struct udevice *dev, u8 value)
+{
+	struct lp5562_led_priv *priv = dev_get_priv(dev);
+
+	if (lp5562_led_reg_update(dev_get_parent(dev), priv->reg_pwm,
+				  value, 0xff) != 0)
+		return -EINVAL;
+
+	/* set LED to I2C register mode */
+	return lp5562_led_set_control_mode(dev, I2C);
+}
+
+/*
+ * Return the led's current state
+ *  dev     - led udevice (child udevice)
+ *
+ */
+static enum led_state_t lp5562_led_get_state(struct udevice *dev)
+{
+	enum led_state_t state = LEDST_ON;
+
+	if (lp5562_led_get_pwm(dev) == REG_PWM_MIN_VALUE)
+		state = LEDST_OFF;
+
+#ifdef CONFIG_LED_BLINK
+	if (lp5562_led_get_control_mode(dev) != I2C)
+		state = LEDST_BLINK;
+#endif
+
+	return state;
+}
+
+/*
+ * Set the led state
+ *  dev     - led udevice (child udevice)
+ *  state   - State to set the LED to
+ */
+static int lp5562_led_set_state(struct udevice *dev, enum led_state_t state)
+{
+#ifdef CONFIG_LED_BLINK
+	struct lp5562_led_priv *priv = dev_get_priv(dev);
+#endif
+
+	switch (state) {
+	case LEDST_OFF:
+		return lp5562_led_set_pwm(dev, REG_PWM_MIN_VALUE);
+	case LEDST_ON:
+		return lp5562_led_set_pwm(dev, REG_PWM_MAX_VALUE);
+#ifdef CONFIG_LED_BLINK
+	case LEDST_BLINK:
+		return lp5562_led_set_control_mode(dev, priv->enginenum);
+#endif
+	case LEDST_TOGGLE:
+		if (lp5562_led_get_state(dev) == LEDST_OFF)
+			return lp5562_led_set_state(dev, LEDST_ON);
+		else
+			return lp5562_led_set_state(dev, LEDST_OFF);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_LED_BLINK
+/*
+ * Set the blink period of an LED; note blue and white share the same
+ * engine so changing the period of one affects the other.
+ *  dev       - led udevice (child udevice)
+ *  period_ms - blink period in ms
+ */
+static int lp5562_led_set_period(struct udevice *dev, int period_ms)
+{
+	struct lp5562_led_priv *priv = dev_get_priv(dev);
+	u8 opcode = 0;
+	u16 program[7];
+	u16 wait_time;
+
+	/* Blink is implemented as an engine program.  Simple on/off
+	 * for short periods, or fade in/fade out for longer periods:
+	 *
+	 *  if (period_ms < 500):
+	 *    set PWM to 100%
+	 *    pause for period / 2
+	 *    set PWM to 0%
+	 *    pause for period / 2
+	 *    goto start
+	 *
+	 *  else
+	 *    raise PWM 0% -> 50% in 62.7 ms
+	 *    raise PWM 50% -> 100% in 62.7 ms
+	 *    pause for (period - 4 * 62.7) / 2
+	 *    lower PWM 100% -> 50% in 62.7 ms
+	 *    lower PWM 50% -> 0% in 62.7 ms
+	 *    pause for (period - 4 * 62.7) / 2
+	 *    goto start
+	 */
+
+	if (period_ms < MIN_BLINK_PERIOD)
+		period_ms = MIN_BLINK_PERIOD;
+	else if (period_ms > MAX_BLINK_PERIOD)
+		period_ms = MAX_BLINK_PERIOD;
+
+	if (period_ms < 500) {
+		/* Simple on/off blink */
+		wait_time = period_ms / 2;
+
+		/* 1st command is full brightness */
+		program[opcode++] =
+			(1 << LED_PGRM_RAMP_PRESCALE_SHIFT) |
+			REG_PWM_MAX_VALUE;
+
+		/* 2nd command is wait (period / 2) using 15.6ms steps */
+		program[opcode++] =
+			(1 << LED_PGRM_RAMP_PRESCALE_SHIFT) |
+			(((wait_time * 10) / 156) << LED_PGRM_RAMP_STEP_SHIFT) |
+			(0 << LED_PGRM_RAMP_INCREMENT_SHIFT);
+
+		/* 3rd command is 0% brightness */
+		program[opcode++] =
+			(1 << LED_PGRM_RAMP_PRESCALE_SHIFT);
+
+		/* 4th command is wait (period / 2) using 15.6ms steps */
+		program[opcode++] =
+			(1 << LED_PGRM_RAMP_PRESCALE_SHIFT) |
+			(((wait_time * 10) / 156) << LED_PGRM_RAMP_STEP_SHIFT) |
+			(0 << LED_PGRM_RAMP_INCREMENT_SHIFT);
+
+		/* 5th command: repeat */
+		program[opcode++] = 0x00;
+	} else {
+		/* fade-in / fade-out blink */
+		wait_time = ((period_ms - 251) / 2);
+
+		/* ramp up time is 256 * 0.49ms (125.4ms) done in 2 steps */
+		/* 1st command is ramp up 1/2 way */
+		program[opcode++] =
+			(0 << LED_PGRM_RAMP_PRESCALE_SHIFT) |
+			(1 << LED_PGRM_RAMP_STEP_SHIFT) |
+			(127 << LED_PGRM_RAMP_INCREMENT_SHIFT);
+
+		/* 2nd command is ramp up rest of the way */
+		program[opcode++] =
+			(0 << LED_PGRM_RAMP_PRESCALE_SHIFT) |
+			(1 << LED_PGRM_RAMP_STEP_SHIFT) |
+			(127 << LED_PGRM_RAMP_INCREMENT_SHIFT);
+
+		/* 3rd: wait ((period - 2 * ramp_time) / 2) (15.6ms steps) */
+		program[opcode++] =
+			(1 << LED_PGRM_RAMP_PRESCALE_SHIFT) |
+			(((wait_time * 10) / 156) << LED_PGRM_RAMP_STEP_SHIFT) |
+			(0 << LED_PGRM_RAMP_INCREMENT_SHIFT);
+
+		/* ramp down is same as ramp up with sign bit set */
+		/* 4th command is ramp down 1/2 way */
+		program[opcode++] =
+			(0 << LED_PGRM_RAMP_PRESCALE_SHIFT) |
+			(1 << LED_PGRM_RAMP_STEP_SHIFT) |
+			(1 << LED_PGRM_RAMP_SIGN_SHIFT) |
+			(127 << LED_PGRM_RAMP_INCREMENT_SHIFT);
+
+		/* 5th command is ramp down rest of the way */
+		program[opcode++] =
+			(0 << LED_PGRM_RAMP_PRESCALE_SHIFT) |
+			(1 << LED_PGRM_RAMP_STEP_SHIFT) |
+			(1 << LED_PGRM_RAMP_SIGN_SHIFT) |
+			(127 << LED_PGRM_RAMP_INCREMENT_SHIFT);
+
+		/* 6th: wait ((period - 2 * ramp_time) / 2) (15.6ms steps) */
+		program[opcode++] =
+			(1 << LED_PGRM_RAMP_PRESCALE_SHIFT) |
+			(((wait_time * 10) / 156) << LED_PGRM_RAMP_STEP_SHIFT) |
+			(0 << LED_PGRM_RAMP_INCREMENT_SHIFT);
+
+		/* 7th command: repeat */
+		program[opcode++] = 0x00;
+	}
+
+	return lp5562_led_program_engine(dev_get_parent(dev), program,
+					 opcode, priv->enginenum);
+}
+#endif
+
+static const struct led_ops lp5562_led_ops = {
+	.get_state = lp5562_led_get_state,
+	.set_state = lp5562_led_set_state,
+#ifdef CONFIG_LED_BLINK
+	.set_period = lp5562_led_set_period,
+#endif
+};
+
+static int lp5562_led_probe(struct udevice *dev)
+{
+	struct lp5562_led_priv *priv = dev_get_priv(dev);
+	u8 current;
+	int ret = 0;
+
+	/* Child LED nodes */
+	switch (dev_read_addr(dev)) {
+	case 0:
+		priv->reg_current = REG_R_CUR;
+		priv->reg_pwm = REG_R_PWM;
+		priv->map_shift = REG_LED_MAP_R_ENG_SHIFT;
+		priv->enginenum = 1;
+		break;
+	case 1:
+		priv->reg_current = REG_G_CUR;
+		priv->reg_pwm = REG_G_PWM;
+		priv->map_shift = REG_LED_MAP_G_ENG_SHIFT;
+		priv->enginenum = 2;
+		break;
+	case 2:
+		priv->reg_current = REG_B_CUR;
+		priv->reg_pwm = REG_B_PWM;
+		priv->map_shift = REG_LED_MAP_B_ENG_SHIFT;
+		priv->enginenum = 3; /* shared with white */
+		break;
+	case 3:
+		priv->reg_current = REG_W_CUR;
+		priv->map_shift = REG_LED_MAP_W_ENG_SHIFT;
+		priv->enginenum = 3; /* shared with blue */
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	current = dev_read_u8_default(dev, "max-cur", DEFAULT_CURRENT);
+
+	ret = lp5562_led_reg_update(dev_get_parent(dev), priv->reg_current,
+				    current, 0xff);
+
+	return ret;
+}
+
+static int lp5562_led_bind(struct udevice *dev)
+{
+	struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+
+	/*
+	 * For the child nodes, parse a "chan-name" property, since
+	 * the DT bindings for this device use that instead of
+	 * "label".
+	 */
+	uc_plat->label = dev_read_string(dev, "chan-name");
+
+	return 0;
+}
+
+U_BOOT_DRIVER(lp5562_led) = {
+	.name = "lp5562-led",
+	.id = UCLASS_LED,
+	.bind = lp5562_led_bind,
+	.probe = lp5562_led_probe,
+	.priv_auto = sizeof(struct lp5562_led_priv),
+	.ops = &lp5562_led_ops,
+};
+
+
+static int lp5562_led_wrap_probe(struct udevice *dev)
+{
+	struct lp5562_led_wrap_priv *priv = dev_get_priv(dev);
+	u8 clock_mode;
+	int ret;
+
+	/* Enable gpio if needed */
+	if (gpio_request_by_name(dev, "enabled-gpios", 0,
+				 &priv->enable_gpio, GPIOD_IS_OUT) == 0) {
+		dm_gpio_set_value(&priv->enable_gpio, 1);
+		udelay(1000);
+	}
+
+	/* Ensure all registers have default values. */
+	ret = lp5562_led_reg_update(dev, REG_RESET, REG_RESET_RESET, 0xff);
+	if (ret)
+		return ret;
+	udelay(10000);
+
+	/* Enable the chip */
+	ret = lp5562_led_reg_update(dev, REG_ENABLE, REG_ENABLE_CHIP_ENABLE, 0xff);
+	if (ret)
+		return ret;
+
+	/*
+	 * The DT bindings say 0=auto, 1=internal, 2=external, while
+	 * the register[0:1] values are 0=external, 1=internal,
+	 * 2=auto.
+	 */
+	clock_mode = dev_read_u8_default(dev, "clock-mode", 0);
+	ret = lp5562_led_reg_update(dev, REG_CONFIG, 2 - clock_mode, REG_CONFIG_CLK_MASK);
+
+	return ret;
+}
+
+static int lp5562_led_wrap_bind(struct udevice *dev)
+{
+	return led_bind_generic(dev, "lp5562-led");
+}
+
+static const struct udevice_id lp5562_led_ids[] = {
+	{ .compatible = "ti,lp5562" },
+	{ /* sentinel */ }
+};
+
+U_BOOT_DRIVER(lp5562_led_wrap) = {
+	.name = "lp5562-led-wrap",
+	.id = UCLASS_NOP,
+	.of_match = lp5562_led_ids,
+	.bind = lp5562_led_wrap_bind,
+	.probe = lp5562_led_wrap_probe,
+	.priv_auto = sizeof(struct lp5562_led_wrap_priv),
+};
-- 
2.40.1.1.g1c60b9335d

Re: [PATCH 6/6] led: add TI LP5562 LED driver

[Marek Vasut]

On 10/19/23 11:58, Rasmus Villemoes wrote:
> From: Doug Zobel <douglas.zobel@climate.com>
> 
> Driver for the TI LP5562 4 channel LED controller. Supports
> independent on/off control of all 4 channels. Supports LED_BLINK on 3
> independent channels: blue/green/red. The white channel can blink, but
> shares the blue channel blink rate.
> 
> Heavily based on patch originally from Doug Zobel [1].
> 
> I have modified it so it matches the DT bindings in the linux tree,
> and also follows the linux driver implementation more closely. This
> should address Tom's concerns, and also matches my goal of making the
> U-Boot driver work with our existing .dts which is known to work in
> linux.
> 
> As our boards only have the R,G,B outputs connected, I have not
> actually tested how the white channel behaves, but the R,G,B work
> exactly as expected.
> 
> [1] https://lore.kernel.org/u-boot/1547150757-1561-1-git-send-email-douglas.zobel@climate.com/
> 
> Cc: Doug Zobel <douglas.zobel@climate.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   doc/device-tree-bindings/leds/leds-lp5562.txt |  63 ++
>   drivers/led/Kconfig                           |   8 +
>   drivers/led/Makefile                          |   1 +
>   drivers/led/led_lp5562.c                      | 578 ++++++++++++++++++
>   4 files changed, 650 insertions(+)
>   create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt
>   create mode 100644 drivers/led/led_lp5562.c
> 
> diff --git a/doc/device-tree-bindings/leds/leds-lp5562.txt b/doc/device-tree-bindings/leds/leds-lp5562.txt
> new file mode 100644
> index 0000000000..4e0c742959
> --- /dev/null
> +++ b/doc/device-tree-bindings/leds/leds-lp5562.txt

Why not use Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml ?

[...]

> --- /dev/null
> +++ b/drivers/led/led_lp5562.c
> @@ -0,0 +1,578 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Doug Zobel <douglas.zobel@climate.com>

Why not port Linux drivers/leds/leds-lp5562.c ?

Re: [PATCH 6/6] led: add TI LP5562 LED driver

[Rasmus Villemoes]

On 19/10/2023 15.58, Marek Vasut wrote:
> On 10/19/23 11:58, Rasmus Villemoes wrote:
>> From: Doug Zobel <douglas.zobel@climate.com>
>>
>> Driver for the TI LP5562 4 channel LED controller. Supports
>> independent on/off control of all 4 channels. Supports LED_BLINK on 3
>> independent channels: blue/green/red. The white channel can blink, but
>> shares the blue channel blink rate.
>>
>> Heavily based on patch originally from Doug Zobel [1].
>>
>> I have modified it so it matches the DT bindings in the linux tree,
>> and also follows the linux driver implementation more closely. This
>> should address Tom's concerns, and also matches my goal of making the
>> U-Boot driver work with our existing .dts which is known to work in
>> linux.
>>
>> As our boards only have the R,G,B outputs connected, I have not
>> actually tested how the white channel behaves, but the R,G,B work
>> exactly as expected.
>>
>> [1]
>> https://lore.kernel.org/u-boot/1547150757-1561-1-git-send-email-douglas.zobel@climate.com/
>>
>> Cc: Doug Zobel <douglas.zobel@climate.com>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>   doc/device-tree-bindings/leds/leds-lp5562.txt |  63 ++
>>   drivers/led/Kconfig                           |   8 +
>>   drivers/led/Makefile                          |   1 +
>>   drivers/led/led_lp5562.c                      | 578 ++++++++++++++++++
>>   4 files changed, 650 insertions(+)
>>   create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt
>>   create mode 100644 drivers/led/led_lp5562.c
>>
>> diff --git a/doc/device-tree-bindings/leds/leds-lp5562.txt
>> b/doc/device-tree-bindings/leds/leds-lp5562.txt
>> new file mode 100644
>> index 0000000000..4e0c742959
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/leds/leds-lp5562.txt
> 
> Why not use Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml ?

Because I'm not adding support for all the devices covered by that
binding, nor for all the properties defined there (and some of those, I
think, do not even make sense for the lp5562 but only apply to some of
the other variants).

> [...]
> 
>> --- /dev/null
>> +++ b/drivers/led/led_lp5562.c
>> @@ -0,0 +1,578 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018 Doug Zobel <douglas.zobel@climate.com>
> 
> Why not port Linux drivers/leds/leds-lp5562.c ?

Because that would be way more work to make that fit into U-Boot's LED
framework than to take Doug's initial patch and make it build against
current U-Boot.

Rasmus

Re: [PATCH 6/6] led: add TI LP5562 LED driver

[Marek Vasut]

On 10/23/23 11:11, Rasmus Villemoes wrote:
> On 19/10/2023 15.58, Marek Vasut wrote:
>> On 10/19/23 11:58, Rasmus Villemoes wrote:
>>> From: Doug Zobel <douglas.zobel@climate.com>
>>>
>>> Driver for the TI LP5562 4 channel LED controller. Supports
>>> independent on/off control of all 4 channels. Supports LED_BLINK on 3
>>> independent channels: blue/green/red. The white channel can blink, but
>>> shares the blue channel blink rate.
>>>
>>> Heavily based on patch originally from Doug Zobel [1].
>>>
>>> I have modified it so it matches the DT bindings in the linux tree,
>>> and also follows the linux driver implementation more closely. This
>>> should address Tom's concerns, and also matches my goal of making the
>>> U-Boot driver work with our existing .dts which is known to work in
>>> linux.
>>>
>>> As our boards only have the R,G,B outputs connected, I have not
>>> actually tested how the white channel behaves, but the R,G,B work
>>> exactly as expected.
>>>
>>> [1]
>>> https://lore.kernel.org/u-boot/1547150757-1561-1-git-send-email-douglas.zobel@climate.com/
>>>
>>> Cc: Doug Zobel <douglas.zobel@climate.com>
>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> ---
>>>    doc/device-tree-bindings/leds/leds-lp5562.txt |  63 ++
>>>    drivers/led/Kconfig                           |   8 +
>>>    drivers/led/Makefile                          |   1 +
>>>    drivers/led/led_lp5562.c                      | 578 ++++++++++++++++++
>>>    4 files changed, 650 insertions(+)
>>>    create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt
>>>    create mode 100644 drivers/led/led_lp5562.c
>>>
>>> diff --git a/doc/device-tree-bindings/leds/leds-lp5562.txt
>>> b/doc/device-tree-bindings/leds/leds-lp5562.txt
>>> new file mode 100644
>>> index 0000000000..4e0c742959
>>> --- /dev/null
>>> +++ b/doc/device-tree-bindings/leds/leds-lp5562.txt
>>
>> Why not use Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml ?
> 
> Because I'm not adding support for all the devices covered by that
> binding, nor for all the properties defined there (and some of those, I
> think, do not even make sense for the lp5562 but only apply to some of
> the other variants).

Using old bindings like that will only cause divergence, please dont do 
that.

>> [...]
>>
>>> --- /dev/null
>>> +++ b/drivers/led/led_lp5562.c
>>> @@ -0,0 +1,578 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (C) 2018 Doug Zobel <douglas.zobel@climate.com>
>>
>> Why not port Linux drivers/leds/leds-lp5562.c ?
> 
> Because that would be way more work to make that fit into U-Boot's LED
> framework than to take Doug's initial patch and make it build against
> current U-Boot.

Seems the drivers are just about the same complexity, so why not port 
the new Linux one ?

Re: [PATCH 6/6] led: add TI LP5562 LED driver

[Rasmus Villemoes]

On 23/10/2023 11.39, Marek Vasut wrote:
> On 10/23/23 11:11, Rasmus Villemoes wrote:
>> On 19/10/2023 15.58, Marek Vasut wrote:
>>> On 10/19/23 11:58, Rasmus Villemoes wrote:
>>>> From: Doug Zobel <douglas.zobel@climate.com>
>>>>
>>>> Driver for the TI LP5562 4 channel LED controller. Supports
>>>> independent on/off control of all 4 channels. Supports LED_BLINK on 3
>>>> independent channels: blue/green/red. The white channel can blink, but
>>>> shares the blue channel blink rate.
>>>>
>>>> Heavily based on patch originally from Doug Zobel [1].
>>>>
>>>> I have modified it so it matches the DT bindings in the linux tree,
>>>> and also follows the linux driver implementation more closely. This
>>>> should address Tom's concerns, and also matches my goal of making the
>>>> U-Boot driver work with our existing .dts which is known to work in
>>>> linux.
>>>>
>>>> As our boards only have the R,G,B outputs connected, I have not
>>>> actually tested how the white channel behaves, but the R,G,B work
>>>> exactly as expected.
>>>>
>>>> [1]
>>>> https://lore.kernel.org/u-boot/1547150757-1561-1-git-send-email-douglas.zobel@climate.com/
>>>>
>>>> Cc: Doug Zobel <douglas.zobel@climate.com>
>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>> ---
>>>>    doc/device-tree-bindings/leds/leds-lp5562.txt |  63 ++
>>>>    drivers/led/Kconfig                           |   8 +
>>>>    drivers/led/Makefile                          |   1 +
>>>>    drivers/led/led_lp5562.c                      | 578
>>>> ++++++++++++++++++
>>>>    4 files changed, 650 insertions(+)
>>>>    create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt
>>>>    create mode 100644 drivers/led/led_lp5562.c
>>>>
>>>> diff --git a/doc/device-tree-bindings/leds/leds-lp5562.txt
>>>> b/doc/device-tree-bindings/leds/leds-lp5562.txt
>>>> new file mode 100644
>>>> index 0000000000..4e0c742959
>>>> --- /dev/null
>>>> +++ b/doc/device-tree-bindings/leds/leds-lp5562.txt
>>>
>>> Why not use Linux
>>> Documentation/devicetree/bindings/leds/leds-lp55xx.yaml ?
>>
>> Because I'm not adding support for all the devices covered by that
>> binding, nor for all the properties defined there (and some of those, I
>> think, do not even make sense for the lp5562 but only apply to some of
>> the other variants).
> 
> Using old bindings like that will only cause divergence, please dont do
> that.

I am not doing that (i.e. using old bindings). Compared to Doug's
original patch, I _have_ changed that .txt file to match the existing
linux binding and hence my existing .dts. And made all the corresponding
(mostly mechanical) changes in the driver. Which I also mentioned in the
commit log:

>>>> I have modified it so it matches the DT bindings in the linux tree,

For example reading some property values using dev_read_u8, because for
some reason the binding specifies that the values are /bits/8.

If you want I can spell out all the changes I made. It just doesn't make
sense to have that kind of info in the commit log.

>>> [...]
>>>
>>>> --- /dev/null
>>>> +++ b/drivers/led/led_lp5562.c
>>>> @@ -0,0 +1,578 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Copyright (C) 2018 Doug Zobel <douglas.zobel@climate.com>
>>>
>>> Why not port Linux drivers/leds/leds-lp5562.c ?
>>
>> Because that would be way more work to make that fit into U-Boot's LED
>> framework than to take Doug's initial patch and make it build against
>> current U-Boot.
> 
> Seems the drivers are just about the same complexity, so why not port
> the new Linux one ?

So how did you measure the complexity? LOC? If so, you may have missed
drivers/leds/leds-lp55xx-common.[ch].

They are far from the same complexity, and the linux driver simply has
tons of gunk that would have to be removed or #ifdef'ed out to compile
in U-Boot, and I'd still need to add the U-Boot-specific glue for the
struct led_ops etc. In the end I don't think much more than the register
defines could actually be reused. And since I now have a driver that
actually works I don't really want to try and see what it would take to
port the linux driver. It even includes blink support, for which there
is AFAIK no equivalent API in linux, so that would have to be
implemented separately on top - I don't need it, but left it in (and
tested it) because it was in Doug's original patch. If I removed that it
would be cut at least 1/3 of the lines.

Rasmus