[PATCH 7/7] leds-lm3530: enhanced arithmetic operation

[PATCH 7/7] leds-lm3530: enhanced arithmetic operation

[Kim, Milo]

Use shift operation rather than 'divide-by-2'.

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>

diff --git a/drivers/leds/leds-lm3530.c b/drivers/leds/leds-lm3530.c
index 51c1f6c..e0b1ba8 100644
--- a/drivers/leds/leds-lm3530.c
+++ b/drivers/leds/leds-lm3530.c
@@ -249,12 +249,12 @@ static void lm3530_brightness_set(struct led_classdev *led_cdev,
 
 		/* set the brightness in brightness control register*/
 		err = i2c_smbus_write_byte_data(drvdata->client,
-				LM3530_BRT_CTRL_REG, brt_val / 2);
+				LM3530_BRT_CTRL_REG, brt_val >> 1);
 		if (err)
 			dev_err(&drvdata->client->dev,
 				"Unable to set brightness: %d\n", err);
 		else
-			drvdata->brightness = brt_val / 2;
+			drvdata->brightness = brt_val >> 1;
 
 		if (brt_val == 0 && !pdata->is_vin_always_on) {
 			err = regulator_disable(drvdata->regulator);
-- 
1.7.4.1


Best Regards,
Milo (Woogyom) Kim
Texas Instruments Incorporated

Re: [PATCH 7/7] leds-lm3530: enhanced arithmetic operation

[Lars-Peter Clausen]

On 01/20/2012 02:52 AM, Kim, Milo wrote:
> Use shift operation rather than 'divide-by-2'.

The compiler will already take of this for you.

But why is the divide by two necessary anyway? This sort of looks like
max_brightness is not set properly and the code compensates for it by
ignoring the lsb.

> 
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> 
> diff --git a/drivers/leds/leds-lm3530.c b/drivers/leds/leds-lm3530.c
> index 51c1f6c..e0b1ba8 100644
> --- a/drivers/leds/leds-lm3530.c
> +++ b/drivers/leds/leds-lm3530.c
> @@ -249,12 +249,12 @@ static void lm3530_brightness_set(struct led_classdev *led_cdev,
>  
>  		/* set the brightness in brightness control register*/
>  		err = i2c_smbus_write_byte_data(drvdata->client,
> -				LM3530_BRT_CTRL_REG, brt_val / 2);
> +				LM3530_BRT_CTRL_REG, brt_val >> 1);
>  		if (err)
>  			dev_err(&drvdata->client->dev,
>  				"Unable to set brightness: %d\n", err);
>  		else
> -			drvdata->brightness = brt_val / 2;
> +			drvdata->brightness = brt_val >> 1;
>  
>  		if (brt_val == 0 && !pdata->is_vin_always_on) {
>  			err = regulator_disable(drvdata->regulator);

RE: [PATCH 7/7] leds-lm3530: enhanced arithmetic operation

[Kim, Milo]

The reason why dividing by two is only 7-bits of brightness value in the LM3530 register.

Bit7 | bit6 | bit5 | bit4 | bit3 | bit2 | bit1 | bit0
------------------------------------------------------
 N/A |        LED Brightness Data (Bits[6:0])
------------------------------------------------------

So maximum value is limited to 127.

And we can think the alternative is '(brt_val & 0x7F)' - taking off the MSB to make it 7bits.
But the result is unexpected because value 128 and 255 has same brightness.
Brightness should be changed step by step, so appropriate method is dividing by two.

For the bit arithmetic and division, is it safe to predict same results ?
As you told, gcc makes same operation for that.(The objdump shows the same assembly code.)
But I think that using bit arithmetic is good habit.

Thanks,
Milo -

-----Original Message-----
From: Lars-Peter Clausen [mailto:lars@metafoo.de] 
Sent: Friday, January 20, 2012 5:32 PM
To: Kim, Milo
Cc: Linus Walleij; shreshthakumar.sahu@stericsson.com; linux-kernel@vger.kernel.org; rpurdie@rpsys.net
Subject: Re: [PATCH 7/7] leds-lm3530: enhanced arithmetic operation

On 01/20/2012 02:52 AM, Kim, Milo wrote:
> Use shift operation rather than 'divide-by-2'.

The compiler will already take of this for you.

But why is the divide by two necessary anyway? This sort of looks like
max_brightness is not set properly and the code compensates for it by
ignoring the lsb.

> 
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> 
> diff --git a/drivers/leds/leds-lm3530.c b/drivers/leds/leds-lm3530.c
> index 51c1f6c..e0b1ba8 100644
> --- a/drivers/leds/leds-lm3530.c
> +++ b/drivers/leds/leds-lm3530.c
> @@ -249,12 +249,12 @@ static void lm3530_brightness_set(struct led_classdev *led_cdev,
>  
>  		/* set the brightness in brightness control register*/
>  		err = i2c_smbus_write_byte_data(drvdata->client,
> -				LM3530_BRT_CTRL_REG, brt_val / 2);
> +				LM3530_BRT_CTRL_REG, brt_val >> 1);
>  		if (err)
>  			dev_err(&drvdata->client->dev,
>  				"Unable to set brightness: %d\n", err);
>  		else
> -			drvdata->brightness = brt_val / 2;
> +			drvdata->brightness = brt_val >> 1;
>  
>  		if (brt_val == 0 && !pdata->is_vin_always_on) {
>  			err = regulator_disable(drvdata->regulator);

Re: [PATCH 7/7] leds-lm3530: enhanced arithmetic operation

[Lars-Peter Clausen]

On 01/20/2012 12:20 PM, Kim, Milo wrote:
> The reason why dividing by two is only 7-bits of brightness value in the LM3530 register.
> 
> Bit7 | bit6 | bit5 | bit4 | bit3 | bit2 | bit1 | bit0
> ------------------------------------------------------
>  N/A |        LED Brightness Data (Bits[6:0])
> ------------------------------------------------------
> 
> So maximum value is limited to 127.

So the max_brightness property of your led should be set to 127.

> 
> And we can think the alternative is '(brt_val & 0x7F)' - taking off the MSB to make it 7bits.
> But the result is unexpected because value 128 and 255 has same brightness.
> Brightness should be changed step by step, so appropriate method is dividing by two.
> 
> For the bit arithmetic and division, is it safe to predict same results ?
> As you told, gcc makes same operation for that.(The objdump shows the same assembly code.)
> But I think that using bit arithmetic is good habit.

In my opinion it is not, because it has different semantics. Use division by
two when you want to divide by two, use right shift if you for example want
to extract a value from a bit-field.

> 
> Thanks,
> Milo -
> 
> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars@metafoo.de] 
> Sent: Friday, January 20, 2012 5:32 PM
> To: Kim, Milo
> Cc: Linus Walleij; shreshthakumar.sahu@stericsson.com; linux-kernel@vger.kernel.org; rpurdie@rpsys.net
> Subject: Re: [PATCH 7/7] leds-lm3530: enhanced arithmetic operation
> 
> On 01/20/2012 02:52 AM, Kim, Milo wrote:
>> Use shift operation rather than 'divide-by-2'.
> 
> The compiler will already take of this for you.
> 
> But why is the divide by two necessary anyway? This sort of looks like
> max_brightness is not set properly and the code compensates for it by
> ignoring the lsb.
> 
>>
>> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
>>
>> diff --git a/drivers/leds/leds-lm3530.c b/drivers/leds/leds-lm3530.c
>> index 51c1f6c..e0b1ba8 100644
>> --- a/drivers/leds/leds-lm3530.c
>> +++ b/drivers/leds/leds-lm3530.c
>> @@ -249,12 +249,12 @@ static void lm3530_brightness_set(struct led_classdev *led_cdev,
>>  
>>  		/* set the brightness in brightness control register*/
>>  		err = i2c_smbus_write_byte_data(drvdata->client,
>> -				LM3530_BRT_CTRL_REG, brt_val / 2);
>> +				LM3530_BRT_CTRL_REG, brt_val >> 1);
>>  		if (err)
>>  			dev_err(&drvdata->client->dev,
>>  				"Unable to set brightness: %d\n", err);
>>  		else
>> -			drvdata->brightness = brt_val / 2;
>> +			drvdata->brightness = brt_val >> 1;
>>  
>>  		if (brt_val == 0 && !pdata->is_vin_always_on) {
>>  			err = regulator_disable(drvdata->regulator);
> 
> 

RE: [PATCH 7/7] leds-lm3530: enhanced arithmetic operation

[Kim, Milo]

>> The reason why dividing by two is only 7-bits of brightness value in the LM3530 register.
>> 
>> Bit7 | bit6 | bit5 | bit4 | bit3 | bit2 | bit1 | bit0
>> ------------------------------------------------------
>>  N/A |        LED Brightness Data (Bits[6:0])
>> ------------------------------------------------------
>> 
>> So maximum value is limited to 127.

> So the max_brightness property of your led should be set to 127.

That's good point. Thanks a lot !
Additional patch needs for this.

Best Regards,
Milo -