Re: [PATCH v14 13/19] leds: lp55xx: Add multicolor framework support to lp55xx

[Jacek Anaszewski]

Dan,

On 10/23/19 2:22 PM, Dan Murphy wrote:
> Jacek
> 
> On 10/18/19 4:48 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 10/18/19 2:25 PM, Dan Murphy wrote:
>>> Add multicolor framework support for the lp55xx family.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
>>> ---
>>> Â drivers/leds/KconfigÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 1 +
>>> Â drivers/leds/leds-lp55xx-common.cÂÂÂÂÂÂÂÂ | 185 +++++++++++++++++++---
>>> Â drivers/leds/leds-lp55xx-common.hÂÂÂÂÂÂÂÂ |ÂÂ 9 ++
>>> Â include/linux/platform_data/leds-lp55xx.h |ÂÂ 7 +
>>> Â 4 files changed, 179 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index fb614a6b9afa..5706bf8d8bd1 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -377,6 +377,7 @@ config LEDS_LP50XX
>>> Â config LEDS_LP55XX_COMMON
>>> ÂÂÂÂÂ tristate "Common Driver for TI/National
>>> LP5521/5523/55231/5562/8501"
>>> ÂÂÂÂÂ depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 ||
>>> LEDS_LP8501
>>> +ÂÂÂ depends on OF
>>> ÂÂÂÂÂ select FW_LOADER
>>> ÂÂÂÂÂ select FW_LOADER_USER_HELPER
>>> ÂÂÂÂÂ help
>>> diff --git a/drivers/leds/leds-lp55xx-common.c
>>> b/drivers/leds/leds-lp55xx-common.c
>>> index 882ef39e4965..197b87ca5ca2 100644
>>> --- a/drivers/leds/leds-lp55xx-common.c
>>> +++ b/drivers/leds/leds-lp55xx-common.c
>>> @@ -131,14 +131,62 @@ static struct attribute *lp55xx_led_attrs[] = {
>>> Â };
>>> Â ATTRIBUTE_GROUPS(lp55xx_led);
>>> Â +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
>>> +static int lp55xx_map_channel(struct lp55xx_led *led, int color_id,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum led_brightness brightness)
>> If you changed the type of the first parameter to
>> struct led_mc_color_conversion* then you could make this function local
>> in LED mc class and call it in led_mc_calc_color_components() after
>> calculating brightness components.
> 
> I prefer to leave this here and if this code is ever integrated we can
> see if there is a common need for the MC class to expose a mapping API.
> 
>>
>>> +{
>>> +ÂÂÂ int i;
>>> +
>>> +ÂÂÂ for (i = 0; i < led->mc_cdev.num_leds; i++) {
>>> +ÂÂÂÂÂÂÂ if (led->color_components[i].color_id == color_id) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ led->color_components[i].brightness = brightness;
>>> +ÂÂÂÂÂÂÂÂÂÂÂ return 0;
>>> +ÂÂÂÂÂÂÂ }
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ return -EINVAL;
>>> +}
>>> +#endif
>>> +
>>> +static int lp55xx_set_mc_brightness(struct lp55xx_led *led,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct lp55xx_device_config *cfg,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum led_brightness brightness)
>>> +{
>>> +ÂÂÂ int ret = -EINVAL;
>>> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
>>> +ÂÂÂ struct led_mc_color_conversion
>>> color_components[LP55XX_MAX_GROUPED_CHAN];
>> You wouldn't need this local variable then.
> 
>>> +ÂÂÂ int i;
>>> +
>>> +ÂÂÂ if (!cfg->multicolor_brightness_fn)
>>> +ÂÂÂÂÂÂÂ return -EINVAL;
>>> +
>>> +ÂÂÂ led_mc_calc_color_components(&led->mc_cdev, brightness,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ color_components);
>> Because you could pass what you already have in the struct lp55xx_led:
>>
>> led_mc_calc_color_components(&led->mc_cdev, brightness,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ led->color_components);
> 
> Well that is not entirely accurate the led->color_components is the data
> that we have from the DT that should not be changed. Passing this into
> the MC calc function would mean that the framework would have to map the
> output to the color_id. As I indicated above for now I don't think the
> MC class should do any mapping of color_id to the output.

I proposed to use color_components because it is already there
and has required data in place. You could always copy
that data to some other local structure but it would be unnecessary
overhead.

Anyway, all what I proposed here are just nice-to-have details,
that can be covered in the future.

-- 
Best regards,
Jacek Anaszewski