linux-kernel - Re: [PATCH 03/31] hwmon: pmbus: changing LED_* from enum led

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220125020355.GA355320@roeck-us.net>
Date:   Mon, 24 Jan 2022 18:03:55 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Luiz Sampaio <sampaio.ime@...il.com>
Cc:     Jean Delvare <jdelvare@...e.com>, linux-kernel@...r.kernel.org,
        linux-hwmon@...r.kernel.org
Subject: Re: [PATCH 03/31] hwmon: pmbus: changing LED_* from enum
 led_brightness to actual value

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