Re: [PATCH v13 13/18] leds: lp55xx: Add multicolor framework support to lp55xx

[Jacek Anaszewski]

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
>  	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;
> +	} else {
> +		led->brightness = (u8)brightness;
> +		ret = cfg->brightness_fn(led);
> +	}
> +
> +	return ret;
[...]

-- 
Best regards,
Jacek Anaszewski