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

From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Dan Murphy <dmurphy@ti.com>, pavel@ucw.cz
Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v13 13/18] leds: lp55xx: Add multicolor framework support to lp55xx
Date: Wed, 16 Oct 2019 23:09:20 +0200	[thread overview]
Message-ID: <1a78bde8-4559-ce0d-75de-430b23dfc5f1@gmail.com> (raw)
In-Reply-To: <3d73452f-fd41-0b63-c79d-c29ed70867af@gmail.com>

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@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

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