And one more issue regarding LEDS_CLASS_MULTI_COLOR config.
On 10/16/19 10:44 PM, Jacek Anaszewski wrote:
> Dan,
>
> Some variable naming related nitpicking below.
>
> On 10/16/19 5:59 PM, Dan Murphy wrote:
>> Add multicolor framework support for the lp55xx family.
>>
>> Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
>> ---
>> drivers/leds/Kconfig | 2 +
>> drivers/leds/leds-lp55xx-common.c | 175 +++++++++++++++++++---
>> drivers/leds/leds-lp55xx-common.h | 9 ++
>> include/linux/platform_data/leds-lp55xx.h | 7 +
>> 4 files changed, 170 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index fb614a6b9afa..a121a2855c06 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -377,8 +377,10 @@ 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
>> + select LEDS_CLASS_MULTI_COLOR
Why so? This is unnecessary.
>> help
>> This option supports common operations for LP5521/5523/55231/5562/8501
>> devices.
>> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
>> index 824d1d73dde1..026ebc2f8e18 100644
>> --- a/drivers/leds/leds-lp55xx-common.c
>> +++ b/drivers/leds/leds-lp55xx-common.c
>> @@ -131,14 +131,54 @@ static struct attribute *lp55xx_led_attrs[] = {
>> };
>> ATTRIBUTE_GROUPS(lp55xx_led);
>>
>> +static int lp55xx_map_channel(struct lp55xx_led *led, int color_id,
>> + enum led_brightness brightness)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < led->mc_cdev.num_leds; i++) {
>> + if (led->color_component[i].color_id == color_id) {
>
> I'd use plural "color_components" for the property name.
>
>> + led->color_component[i].brightness = brightness;
>> + return 0;
>> + }
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> static int lp55xx_set_brightness(struct led_classdev *cdev,
>> enum led_brightness brightness)
>> {
>> + struct led_mc_color_conversion color_component[LP55XX_MAX_GROUPED_CHAN];
>> struct lp55xx_led *led = cdev_to_lp55xx_led(cdev);
>> struct lp55xx_device_config *cfg = led->chip->cfg;
>> + int ret;
>> + int i;
>>
>> - led->brightness = (u8)brightness;
>> - return cfg->brightness_fn(led);
>> + if (led->mc_cdev.num_leds > 1) {
>> + if (!cfg->multicolor_brightness_fn)
>> + return -EINVAL;
>> +
>> + led_mc_calc_color_components(&led->mc_cdev, brightness,
>> + color_component);
>
> Similarly here - you calculate all components so it is weird to pass
> variable of singular color_component form.
>
>> +
>> + for (i = 0; i < led->mc_cdev.num_leds; i++) {
>> + ret = lp55xx_map_channel(led,
>> + color_component[i].color_id,
>> + color_component[i].brightness);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + ret = cfg->multicolor_brightness_fn(led);
>> + if (ret)
>> + return ret;
Please wrap what you have under "if: case into a function, e.g.:
static int lp55xx_set_mc_brightness()
{
int ret = -EINVAL;
#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
if (!cfg->multicolor_brightness_fn)
....
#endif
return ret;
}
And then have here:
if (led->mc_cdev.num_leds > 1)
ret = lp55xx_set_mc_brightness();
You won't need inline empty led_mc_calc_color_components() then.
>> + } else {
>> + led->brightness = (u8)brightness;
>> + ret = cfg->brightness_fn(led);
>> + }
>> +
>> + return ret;
> [...]
>
--
Best regards,
Jacek Anaszewski
