[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 at 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 at climate.com>
>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at 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 at 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