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

[Marek Vasut <marex@denx.de>]

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 ?