[PATCH] leds: tlc591xx: update the maximum brightness

[PATCH] leds: tlc591xx: update the maximum brightness

[Jean-Jacques Hiblot]

The TLC chips actually offer 257 levels:
- 0: led OFF
- 1-255: Led dimmed is using a PWM. The duty cycle range from 0.4% to 99.6%
- 256: led fully ON

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/leds-tlc591xx.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
index 8eadb673dc2e..a8911ebd30e5 100644
--- a/drivers/leds/leds-tlc591xx.c
+++ b/drivers/leds/leds-tlc591xx.c
@@ -13,6 +13,7 @@
 #include <linux/slab.h>
 
 #define TLC591XX_MAX_LEDS	16
+#define TLC591XX_MAX_BRIGHTNESS	256
 
 #define TLC591XX_REG_MODE1	0x00
 #define MODE1_RESPON_ADDR_MASK	0xF0
@@ -112,11 +113,11 @@ tlc591xx_brightness_set(struct led_classdev *led_cdev,
 	struct tlc591xx_priv *priv = led->priv;
 	int err;
 
-	switch (brightness) {
+	switch ((int)brightness) {
 	case 0:
 		err = tlc591xx_set_ledout(priv, led, LEDOUT_OFF);
 		break;
-	case LED_FULL:
+	case TLC591XX_MAX_BRIGHTNESS:
 		err = tlc591xx_set_ledout(priv, led, LEDOUT_ON);
 		break;
 	default:
@@ -209,7 +210,7 @@ tlc591xx_probe(struct i2c_client *client,
 		led->priv = priv;
 		led->led_no = reg;
 		led->ldev.brightness_set_blocking = tlc591xx_brightness_set;
-		led->ldev.max_brightness = LED_FULL;
+		led->ldev.max_brightness = TLC591XX_MAX_BRIGHTNESS;
 		err = devm_led_classdev_register_ext(dev, &led->ldev,
 						     &init_data);
 		if (err < 0) {
-- 
2.17.1

Re: [PATCH] leds: tlc591xx: update the maximum brightness

[Jacek Anaszewski]

Hi Jean,

Thank you for the patch.

On 9/23/19 12:02 PM, Jean-Jacques Hiblot wrote:
> The TLC chips actually offer 257 levels:
> - 0: led OFF
> - 1-255: Led dimmed is using a PWM. The duty cycle range from 0.4% to 99.6%
> - 256: led fully ON
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  drivers/leds/leds-tlc591xx.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
> index 8eadb673dc2e..a8911ebd30e5 100644
> --- a/drivers/leds/leds-tlc591xx.c
> +++ b/drivers/leds/leds-tlc591xx.c
> @@ -13,6 +13,7 @@
>  #include <linux/slab.h>
>  
>  #define TLC591XX_MAX_LEDS	16
> +#define TLC591XX_MAX_BRIGHTNESS	256
>  
>  #define TLC591XX_REG_MODE1	0x00
>  #define MODE1_RESPON_ADDR_MASK	0xF0
> @@ -112,11 +113,11 @@ tlc591xx_brightness_set(struct led_classdev *led_cdev,
>  	struct tlc591xx_priv *priv = led->priv;
>  	int err;
>  
> -	switch (brightness) {
> +	switch ((int)brightness) {
>  	case 0:
>  		err = tlc591xx_set_ledout(priv, led, LEDOUT_OFF);
>  		break;
> -	case LED_FULL:
> +	case TLC591XX_MAX_BRIGHTNESS:
>  		err = tlc591xx_set_ledout(priv, led, LEDOUT_ON);
>  		break;
>  	default:
> @@ -209,7 +210,7 @@ tlc591xx_probe(struct i2c_client *client,
>  		led->priv = priv;
>  		led->led_no = reg;
>  		led->ldev.brightness_set_blocking = tlc591xx_brightness_set;
> -		led->ldev.max_brightness = LED_FULL;
> +		led->ldev.max_brightness = TLC591XX_MAX_BRIGHTNESS;
>  		err = devm_led_classdev_register_ext(dev, &led->ldev,
>  						     &init_data);
>  		if (err < 0) {
> 

Added tag:

Fixes: e370d010a5fe ("leds: tlc591xx: Driver for the TI 8/16 Channel i2c
LED driver")

and applied to the for-5.5 branch.

It is also a good habit to cc the author of the driver.

Cc Andrew.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: tlc591xx: update the maximum brightness

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

Hi!

> > @@ -112,11 +113,11 @@ tlc591xx_brightness_set(struct led_classdev *led_cdev,
> >  	struct tlc591xx_priv *priv = led->priv;
> >  	int err;
> >  
> > -	switch (brightness) {
> > +	switch ((int)brightness) {
> >  	case 0:

Can we get a rid of the cast here? Do we need to move away from the
enum for the brightness?

> Added tag:
> 
> Fixes: e370d010a5fe ("leds: tlc591xx: Driver for the TI 8/16 Channel i2c
> LED driver")
> 
> and applied to the for-5.5 branch.

Actually, careful with the Fixes tag. -stable people will want to
apply it, and it may not be a good idea in this case. Maximum
brightness of 256 is pretty unusual, so I'd call this "a bit risky".

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: tlc591xx: update the maximum brightness

[Jacek Anaszewski]

On 10/13/19 1:45 PM, Pavel Machek wrote:
> Hi!
> 
>>> @@ -112,11 +113,11 @@ tlc591xx_brightness_set(struct led_classdev *led_cdev,
>>>  	struct tlc591xx_priv *priv = led->priv;
>>>  	int err;
>>>  
>>> -	switch (brightness) {
>>> +	switch ((int)brightness) {
>>>  	case 0:
> 
> Can we get a rid of the cast here? Do we need to move away from the
> enum for the brightness?

I at first also wanted to ask for dropping the cast but first tried
to do it myself. Then I found out compiler (or sparse, I don't recall
exactly) complains about TLC591XX_MAX_BRIGHTNESS not being a value of
enum led_brighteess type. That's the reason for the cast Jean added,
I presume.

>> Added tag:
>>
>> Fixes: e370d010a5fe ("leds: tlc591xx: Driver for the TI 8/16 Channel i2c
>> LED driver")
>>
>> and applied to the for-5.5 branch.
> 
> Actually, careful with the Fixes tag. -stable people will want to
> apply it, and it may not be a good idea in this case. Maximum
> brightness of 256 is pretty unusual, so I'd call this "a bit risky".

I entirely disagree. Not seeing anything risky in that since
max_brightness is also initialized to this value. If userspace properly
uses the ABI, then it will be safe.

> Best regards,
> 									Pavel
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: tlc591xx: update the maximum brightness

[Jean-Jacques Hiblot]

On 13/10/2019 18:36, Jacek Anaszewski wrote:
> On 10/13/19 1:45 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> @@ -112,11 +113,11 @@ tlc591xx_brightness_set(struct led_classdev *led_cdev,
>>>>   	struct tlc591xx_priv *priv = led->priv;
>>>>   	int err;
>>>>   
>>>> -	switch (brightness) {
>>>> +	switch ((int)brightness) {
>>>>   	case 0:
>> Can we get a rid of the cast here? Do we need to move away from the
>> enum for the brightness?
> I at first also wanted to ask for dropping the cast but first tried
> to do it myself. Then I found out compiler (or sparse, I don't recall
> exactly) complains about TLC591XX_MAX_BRIGHTNESS not being a value of
> enum led_brighteess type. That's the reason for the cast Jean added,
> I presume.

Indeed that cast is to fix the warning.

JJ

>>> Added tag:
>>>
>>> Fixes: e370d010a5fe ("leds: tlc591xx: Driver for the TI 8/16 Channel i2c
>>> LED driver")
>>>
>>> and applied to the for-5.5 branch.
>> Actually, careful with the Fixes tag. -stable people will want to
>> apply it, and it may not be a good idea in this case. Maximum
>> brightness of 256 is pretty unusual, so I'd call this "a bit risky".
> I entirely disagree. Not seeing anything risky in that since
> max_brightness is also initialized to this value. If userspace properly
> uses the ABI, then it will be safe.
>
>> Best regards,
>> 									Pavel
>>