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 <dmur...@ti.com>
> ---
> 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
