Re: [PATCH 03/31] hwmon: pmbus: changing LED_* from enum led_brightness to actual value

From: Guenter Roeck <linux@roeck-us.net>
To: Luiz Sampaio <sampaio.ime@gmail.com>
Cc: Jean Delvare <jdelvare@suse.com>,
	linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org
Subject: Re: [PATCH 03/31] hwmon: pmbus: changing LED_* from enum led_brightness to actual value
Date: Mon, 24 Jan 2022 18:03:55 -0800	[thread overview]
Message-ID: <20220125020355.GA355320@roeck-us.net> (raw)
In-Reply-To: <20220121165436.30956-4-sampaio.ime@gmail.com>

On Fri, Jan 21, 2022 at 01:54:08PM -0300, Luiz Sampaio wrote:
> The enum led_brightness, which contains the declaration of LED_OFF,
> LED_ON, LED_HALF and LED_FULL is obsolete, as the led class now supports
> max_brightness.

There is no Signed-off-by:, so this is not not a valid/acceptable patch.

> ---
>  drivers/hwmon/pmbus/ibm-cffps.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
> index e3294a1a54bb..cc20a70c723b 100644
> --- a/drivers/hwmon/pmbus/ibm-cffps.c
> +++ b/drivers/hwmon/pmbus/ibm-cffps.c
> @@ -382,10 +382,10 @@ static int ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
>  	u8 next_led_state;
>  	struct ibm_cffps *psu = container_of(led_cdev, struct ibm_cffps, led);
>  
> -	if (brightness == LED_OFF) {
> +	if (brightness == 0) {
>  		next_led_state = CFFPS_LED_OFF;
>  	} else {
> -		brightness = LED_FULL;
> +		brightness = 255;

If the LED subsystem no longer likes defines, we should have local constants
instead and not just use magic numbers. At the same time, this LED only supports
two states, off or on. There will have to be an explanation why the full range of
0..255 is still used and not just 0/1.

>  
>  		if (psu->led_state != CFFPS_LED_BLINK)
>  			next_led_state = CFFPS_LED_ON;
> @@ -426,7 +426,7 @@ static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
>  		return rc;
>  
>  	psu->led_state = CFFPS_LED_BLINK;
> -	led_cdev->brightness = LED_FULL;
> +	led_cdev->brightness = 255;
>  	*delay_on = CFFPS_BLINK_RATE_MS;
>  	*delay_off = CFFPS_BLINK_RATE_MS;
>  
> @@ -442,7 +442,7 @@ static void ibm_cffps_create_led_class(struct ibm_cffps *psu)
>  	snprintf(psu->led_name, sizeof(psu->led_name), "%s-%02x", client->name,
>  		 client->addr);
>  	psu->led.name = psu->led_name;
> -	psu->led.max_brightness = LED_FULL;
> +	psu->led.max_brightness = 255;
>  	psu->led.brightness_set_blocking = ibm_cffps_led_brightness_set;
>  	psu->led.blink_set = ibm_cffps_led_blink_set;
>  