Re: [PATCH v24 02/16] leds: multicolor: Introduce a multicolor class definition

[Dan Murphy <dmurphy@ti.com>]

Jacek

On 5/4/20 3:31 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 5/3/20 2:32 PM, Dan Murphy wrote:
>> Introduce a multicolor class that groups colored LEDs
>> within a LED node.
>>
>> The multi color class groups monochrome LEDs and allows controlling two
>> aspects of the final combined color: hue and lightness. The former is
>> controlled via the intensity file and the latter is controlled
>> via brightness file.
>>
>> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
> [...]
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -30,6 +30,17 @@ config LEDS_CLASS_FLASH
>>         for the flash related features of a LED device. It can be built
>>         as a module.
>>   +config LEDS_CLASS_MULTI_COLOR
>> +    tristate "LED MultiColor LED Class Support"
>> +    depends on LEDS_CLASS
>> +    depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
>
> I was saying about adding this dependency to the drivers based on
> LED mc class. This way it does not make any sense. Moreover it is
> erroneous:
>
> $ make menuconfig
> drivers/leds/Kconfig:33:error: recursive dependency detected!
>
> Instead you should add it to the Kconfig entries of all drivers
> that depend on LED mc class, i.e.:
>
> - config LEDS_LP50XX
> - config LEDS_LP5521
> - config LEDS_LP5523
>
> Moreover there are still some checkpatch.pl problems:
>
> ---------------------------------------------------------------
> 0003-leds-multicolor-Introduce-a-multicolor-class-definit.patch
> ---------------------------------------------------------------
> WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> #89: FILE: Documentation/leds/leds-class-multicolor.rst:1:
> +====================================

Great another requirement and issue


>
> ERROR: spaces required around that '=' (ctx:WxO)
> #294: FILE: drivers/leds/led-class-multicolor.c:62:
> +        ret =-EINVAL;
>              ^
>
> ERROR: space required before that '-' (ctx:OxV)
> #294: FILE: drivers/leds/led-class-multicolor.c:62:
> +        ret =-EINVAL;
>
Ack


> WARNING: DT binding documents should be licensed (GPL-2.0-only OR 
> BSD-2-Clause)
> #31: FILE: Documentation/devicetree/bindings/leds/leds-lp50xx.yaml:1:
> +# SPDX-License-Identifier: GPL-2.0
>
ack
> WARNING: Block comments use * on subsequent lines
> #705: FILE: drivers/leds/leds-lp50xx.c:636:
> +        /* There are only 3 LEDs per module otherwise they should be
> +           banked which also is presented as 3 LEDs*/
>
> WARNING: Block comments use a trailing */ on a separate line
> #705: FILE: drivers/leds/leds-lp50xx.c:636:
> +           banked which also is presented as 3 LEDs*/
>
Ack
>
> ---------------------------------------------------------------
> 0008-ARM-dts-n900-Add-reg-property-to-the-LP5523-channel-.patch
> ---------------------------------------------------------------
> WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?
>
> ---------------------------------------------------------------
> 0009-ARM-dts-imx6dl-yapp4-Add-reg-property-to-the-lp5562-.patch
> ---------------------------------------------------------------
> WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?
>
> ---------------------------------------------------------------
> 0010-ARM-dts-ste-href-Add-reg-property-to-the-LP5521-chan.patch
> ---------------------------------------------------------------
> WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?
>
ACK * 3

Dan