Skip to contentSkip to conversation

backlight: Fix the logic to set bits in adp8860_bl and adp8870_bl drivers

PATCH In review Axel Lin 3 participants 4 messages Jan 13, 2012 – Jan 16, 2012

Subsystems (3)
Framebuffer layer Backlight class/subsystem ADP8860 backlight driver (ADP8860/ADP8861/ADP8863)
lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/1 backlight: Fix the logic to set bits in adp8860_bl and adp8870_bl drivers

We need to write new value if the bit mask fields of new value is not equal to old value. It does not make sense to write new value only when all the bit_mask bits are zero.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/video/backlight/adp8860_bl.c |    2 +-
 drivers/video/backlight/adp8870_bl.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/video/backlight/adp8860_bl.c +1 -1
index 66bc74d..378276c 100644
@@ -146,7 +146,7 @@ static int adp8860_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask
 
 	ret = adp8860_read(client, reg, &reg_val);
 
-	if (!ret && ((reg_val & bit_mask) == 0)) {
+	if (!ret && ((reg_val & bit_mask) != bit_mask)) {
 		reg_val |= bit_mask;
 		ret = adp8860_write(client, reg, reg_val);
 	}
drivers/video/backlight/adp8870_bl.c +1 -1
index 6c68a68..6735059 100644
@@ -160,7 +160,7 @@ static int adp8870_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask
 
 	ret = adp8870_read(client, reg, &reg_val);
 
-	if (!ret && ((reg_val & bit_mask) == 0)) {
+	if (!ret && ((reg_val & bit_mask) != bit_mask)) {
 		reg_val |= bit_mask;
 		ret = adp8870_write(client, reg, reg_val);
 	}
Signature · 1 line
1.7.5.4
#1 lore raw
Quoted 3 lines · On Fri, 13 Jan 2012 16:15:21 +0800 Axel Lin <[email protected]> wrote:

We need to write new value if the bit mask fields of new value is not equal to old value. It does not make sense to write new value only when all the bit_mask bits are zero.

When fixing bugs, please always describe the user-visible effects of the bug.

Quoted 18 lines
drivers/video/backlight/adp8860_bl.c +1 -1
index 66bc74d..378276c 100644
@@ -146,7 +146,7 @@ static int adp8860_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask
 
 	ret = adp8860_read(client, reg, &reg_val);
 
-	if (!ret && ((reg_val & bit_mask) == 0)) {
+	if (!ret && ((reg_val & bit_mask) != bit_mask)) {
 		reg_val |= bit_mask;
 		ret = adp8860_write(client, reg, reg_val);
 	}
drivers/video/backlight/adp8870_bl.c +0 -0
index 6c68a68..6735059 100644

drivers/video/backlight/adp8870_bl.c static int adp8870_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask · line 166

Quoted 8 lines
drivers/video/backlight/adp8870_bl.c +1 -1
@@ -160,7 +160,7 @@ static int adp8870_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask
 
 	ret = adp8870_read(client, reg, &reg_val);
 
-	if (!ret && ((reg_val & bit_mask) == 0)) {
+	if (!ret && ((reg_val & bit_mask) != bit_mask)) {
 		reg_val |= bit_mask;
 		ret = adp8870_write(client, reg, reg_val);
 	}

This patch is applicable to 3.2.x, but I don't know whether to route it to -stable becasue you didn't tell me what effect the patch has :(

author
#2 lore raw
Quoted 21 lines
Quoted 21 lines
drivers/video/backlight/adp8860_bl.c +1 -1
index 66bc74d..378276c 100644
@@ -146,7 +146,7 @@ static int adp8860_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask
 
       ret = adp8860_read(client, reg, &reg_val);
 
-     if (!ret && ((reg_val & bit_mask) == 0)) {
+     if (!ret && ((reg_val & bit_mask) != bit_mask)) {
               reg_val |= bit_mask;
               ret = adp8860_write(client, reg, reg_val);
       }
drivers/video/backlight/adp8870_bl.c +0 -0
index 6c68a68..6735059 100644
@@ -160,7 +160,7 @@ static int adp8870_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask
 
       ret = adp8870_read(client, reg, &reg_val);
Quoted 9 lines

drivers/video/backlight/adp8870_bl.c static int adp8870_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask · line 166

Quoted 5 lines
drivers/video/backlight/adp8870_bl.c +1 -1
-     if (!ret && ((reg_val & bit_mask) == 0)) {
+     if (!ret && ((reg_val & bit_mask) != bit_mask)) {
               reg_val |= bit_mask;
               ret = adp8870_write(client, reg, reg_val);
       }

This patch is applicable to 3.2.x, but I don't know whether to route it to -stable becasue you didn't tell me what effect the patch has :(

It's because I don't have this hardware.
I just found the bug while reading the code.

I was thinking I should CC stable, but since the code is there for a long time and I don't know any complains of the bug. So I didn't CC stable.

Michael, do you see any user-visible effects of the bug?

Thanks,
Axel

maintainer
#3 lore raw

Axel Lin wrote on 2012-01-14:

Quoted 29 lines
Quoted 19 lines
Quoted 19 lines
(quoted diff) +0 -0

b/drivers/video/backlight/adp8860_bl.c index 66bc74d..378276c 100644 --- a/drivers/video/backlight/adp8860_bl.c +++ b/drivers/video/backlight/adp8860_bl.c @@ -146,7 +146,7 @@ static int adp8860_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask

      ret = adp8860_read(client, reg, &reg_val);

- if (!ret && ((reg_val & bit_mask) == 0)) { + if (!ret &&
((reg_val & bit_mask) != bit_mask)) { reg_val |=
bit_mask; ret = adp8860_write(client, reg, reg_val);
} diff --git a/drivers/video/backlight/adp8870_bl.c
b/drivers/video/backlight/adp8870_bl.c index 6c68a68..6735059 100644 --- a/drivers/video/backlight/adp8870_bl.c +++ b/drivers/video/backlight/adp8870_bl.c @@ -160,7 +160,7 @@ static int adp8870_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask

      ret = adp8870_read(client, reg, &reg_val);
Quoted 9 lines

drivers/video/backlight/adp8870_bl.c static int adp8870_set_bits(struct i2c_client *client, int reg, uint8_t bit_mask · line 166

Quoted 5 lines
drivers/video/backlight/adp8870_bl.c +1 -1
-     if (!ret && ((reg_val & bit_mask) == 0)) {
+     if (!ret && ((reg_val & bit_mask) != bit_mask)) {
              reg_val |= bit_mask;
              ret = adp8870_write(client, reg, reg_val);
      }

This patch is applicable to 3.2.x, but I don't know whether to route it to -stable becasue you didn't tell me what effect the patch has :(

Quoted 8 lines

It's because I don't have this hardware.
I just found the bug while reading the code.

I was thinking I should CC stable, but since the code is there for a long time
and I don't know any complains of the bug. So I didn't CC stable.

Michael, do you see any user-visible effects of the bug?

No - This fixes a bug. The code seemed to be working before, only by accident.

Greetings,
Michael

--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif