lkml.org 
[lkml]   [2012]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: [PATCH 7/7] leds-lm3530: enhanced arithmetic operation
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);




\
 
 \ /
  Last update: 2012-01-20 12:23    [from the cache]
©2003-2011 Jasper Spaans