TSL2550 driver bugfix

Hi all,

I've tested TSL2550 driver and I've found a bug: when light is off, returned value from tsl2550_calculate_lux function is -1 when it should be 0 (sensor correctly read that light was off).

I think the bug is that a zero c0 value (approximated value of ch0) is misinterpreted as an error.
I'm attaching you a patch that fixes the bug.

Regards,
Michele Jr De Candia

Quoted 10 lines · Michele De Candia (VT)

Hi all,

I've tested TSL2550 driver and I've found a bug: when light is off, returned value from tsl2550_calculate_lux function is -1 when it should be 0 (sensor correctly read that light was off).

I think the bug is that a zero c0 value (approximated value of ch0) is misinterpreted as an error.
I'm attaching you a patch that fixes the bug.

Quoted 3 lines

Regards,
Michele Jr De Candia

Sounds reasonable, but I think a stray line got away in your patch (see below) Not to mention, if the c1 <= c0 check is still valid, should you not also confirm that c1 == 0 as well? Perhaps reverse the ordering?

if (c1 <= c0)

	if(c0) {
		r = c1* 128 / c0;
		lux = ((c0 - c1) * ratio_lut[r]) / 256;
	} else
		lux = 0;
else
	return -1;

Signed-off-by Michele Jr De Candia <[email protected]>

drivers/i2c/chips/tsl2550.c +10 -4

index 1a9cc13..6bad072 100644
@@ -189,10 +189,16 @@ static int tsl2550_calculate_lux(u8 ch0, u8 ch1)
 	u8 r = 128;
 
 	/* Avoid division by 0 and count 1 cannot be greater than count 0 */
-	if (c0 && (c1 <= c0))
-		r = c1 * 128 / c0;
-	else
-		return -1;
+	if (c0)	{
+		if (c1 <= c0)
+			r = c1 * 128 / c0;
+		else
+			return -1;
+			
+		/* Calculate LUX */
+		lux = ((c0 - c1) * ratio_lut[r]) / 256;
+	}
+	else lux = 0;
 
 

This last line should have been removed I think?
/* Calculate LUX */

	lux = ((c0 - c1) * ratio_lut[r]) / 256;

---
Jonathan Cameron

Quoted 12 lines · Jonathan Cameron

Quoted 10 lines · Michele De Candia (VT)

Hi all,

I've tested TSL2550 driver and I've found a bug: when light is off, returned value from tsl2550_calculate_lux function is -1 when it should be 0 (sensor correctly read that light was off).

I think the bug is that a zero c0 value (approximated value of ch0) is misinterpreted as an error.
I'm attaching you a patch that fixes the bug.

Quoted 8 lines

Quoted 3 lines

Regards,
Michele Jr De Candia

Sounds reasonable, but I think a stray line got away in your patch (see below)
Not to mention, if the c1 <= c0 check is still valid, should you not also confirm that c1 == 0 as well? Perhaps reverse the ordering?

That's right. A new patch can be found in attachment.

Quoted 37 lines

if (c1 <= c0)

    if(c0) {
        r = c1* 128 / c0;
        lux = ((c0 - c1) * ratio_lut[r]) / 256;
    } else
        lux = 0;
else
    return -1;

Signed-off-by Michele Jr De Candia <[email protected]>

drivers/i2c/chips/tsl2550.c +10 -4

index 1a9cc13..6bad072 100644
@@ -189,10 +189,16 @@ static int tsl2550_calculate_lux(u8 ch0, u8 ch1)
     u8 r = 128;
 
     /* Avoid division by 0 and count 1 cannot be greater than count 0 */
-    if (c0 && (c1 <= c0))
-        r = c1 * 128 / c0;
-    else
-        return -1;
+    if (c0)    {
+        if (c1 <= c0)
+            r = c1 * 128 / c0;
+        else
+            return -1;
+           
+        /* Calculate LUX */
+        lux = ((c0 - c1) * ratio_lut[r]) / 256;
+    }
+    else lux = 0;
 
 

This last line should have been removed I think?

Quoted 7 lines

     /* Calculate LUX */
    lux = ((c0 - c1) * ratio_lut[r]) / 256;

---
Jonathan Cameron

Than

What do you think about 'Bugfix for missing Lux returning in dark environment' ?

Quoted 79 lines · Jonathan Cameron

Looks good to me. (perhaps a bit more detail in the commit message of the patch?)

Acked-by Jonathan Cameron <jic23-KWPb1pKIrIJaa/[email protected]>

Quoted 72 lines

Quoted 14 lines · Jonathan Cameron

Quoted 11 lines · Michele De Candia (VT)

Hi all,

I've tested TSL2550 driver and I've found a bug: when light is off, returned value from tsl2550_calculate_lux function is -1 when it should
be 0 (sensor correctly read that light was off).

I think the bug is that a zero c0 value (approximated value of ch0) is misinterpreted as an error.
I'm attaching you a patch that fixes the bug.

Quoted 9 lines

Quoted 3 lines

Regards,
Michele Jr De Candia

Sounds reasonable, but I think a stray line got away in your patch (see below)
Not to mention, if the c1 <= c0 check is still valid, should you not also
confirm that c1 == 0 as well? Perhaps reverse the ordering?

That's right. A new patch can be found in attachment.

Quoted 35 lines

if (c1 <= c0)

    if(c0) {
        r = c1* 128 / c0;
        lux = ((c0 - c1) * ratio_lut[r]) / 256;
    } else
        lux = 0;
else
    return -1;

Signed-off-by Michele Jr De Candia <[email protected]>

drivers/i2c/chips/tsl2550.c +0 -0

index 1a9cc13..6bad072 100644
@@ -189,10 +189,16 @@ static int tsl2550_calculate_lux(u8 ch0, u8 ch1)
     u8 r = 128;
 
     /* Avoid division by 0 and count 1 cannot be greater than count

0 */

drivers/i2c/chips/tsl2550.c +9 -4

-    if (c0 && (c1 <= c0))
-        r = c1 * 128 / c0;
-    else
-        return -1;
+    if (c0)    {
+        if (c1 <= c0)
+            r = c1 * 128 / c0;
+        else
+            return -1;
+           +        /* Calculate LUX */
+        lux = ((c0 - c1) * ratio_lut[r]) / 256;
+    }
+    else lux = 0;

Quoted 8 lines

This last line should have been removed I think?

     /* Calculate LUX */
    lux = ((c0 - c1) * ratio_lut[r]) / 256;

---
Jonathan Cameron

Than

Signed-off-by Michele Jr De Candia <[email protected]>

drivers/i2c/chips/tsl2550.c +7 -2

index 1a9cc13..c4e1104 100644
@@ -189,14 +189,17 @@ static int tsl2550_calculate_lux(u8 ch0, u8 ch1)
 	u8 r = 128;
 
 	/* Avoid division by 0 and count 1 cannot be greater than count 0 */
-	if (c0 && (c1 <= c0))
-		r = c1 * 128 / c0;
+	if (c1 <= c0)
+		if (c0) {
+			r = c1 * 128 / c0;
+
+			/* Calculate LUX */
+			lux = ((c0 - c1) * ratio_lut[r]) / 256;
+		} else

For another, I do hit the case c1 > c0 from times to times, in particular when I move the sensor in dark areas. Do you hit this as well? This might be specific to the serial evaluation module I'm using, it is very slow and this might cause c0 and c1 to be sampled at significantly different times (say 200 ms apart.) If the light conditions change meanwhile, this can explain why c1 > c0.

I am not
sure what to do in this case. We can return -EAGAIN and let user-space retry. But we could also restart the computation automatically in this case. Of course if the problem only affects the evaluation module then we don't really care.

Quoted 7 lines · Jean Delvare

For another, I do hit the case c1 > c0 from times to times, in particular when I move the sensor in dark areas. Do you hit this as well? This might be specific to the serial evaluation module I'm using, it is very slow and this might cause c0 and c1 to be sampled at significantly different times (say 200 ms apart.) If the light conditions change meanwhile, this can explain why c1 > c0.

Scratch this theory. Reading the TSL2550 datasheet again, I understand that sampling happens continuously and is unrelated to I2C reads. Sampling of each channel takes 400 ms in standard mode, so c0 and c1 samplings are _always_ 400 ms apart, regardless of how fast the I2C interface is. So you should be able do see the problem too.

This also means that the code in tsl2550_get_adc_value() is more complex than it needs to be. The only case where it makes sense to wait 400 ms to get a reading is right after power-up. 400 ms after power-up, c0 will always have a valid value, and after 800 ms c1 will always have a valid value as well. So I propose to simplify tsl2550_get_adc_value() and just return -EAGAIN if there is no valid reading. In practice I doubt we'll ever hit it.

Quoted 6 lines

I am not
sure what to do in this case. We can return -EAGAIN and let user-space retry. But we could also restart the computation automatically in this case. Of course if the problem only affects the evaluation module then we don't really care.

Thinking some more about this: if we want to retry then we will have to wait at least 400 ms to read c0 again, and if it's not enough 400 more ms to read c1 again. And even then, there's no guarantee the new readings will be OK if the light conditions are still changing. This is an intrinsic weakness of the TSL2550 that both channels are sampled sequentially. I doubt we want to let the user wait for the lux value for several seconds, so returning -EAGAIN seems OK to me.

But I am not using the light sensor a lot myself, so my practical experience is rather limited, thus I would welcome comments and opinions on this.

Hi Jean,

Quoted 32 lines · Jean Delvare

Quoted 7 lines · Jean Delvare

For another, I do hit the case c1 > c0 from times to times, in particular when I move the sensor in dark areas. Do you hit this as well? This might be specific to the serial evaluation module I'm using, it is very slow and this might cause c0 and c1 to be sampled at significantly different times (say 200 ms apart.) If the light conditions change meanwhile, this can explain why c1 > c0.

Scratch this theory. Reading the TSL2550 datasheet again, I understand that sampling happens continuously and is unrelated to I2C reads. Sampling of each channel takes 400 ms in standard mode, so c0 and c1 samplings are _always_ 400 ms apart, regardless of how fast the I2C interface is. So you should be able do see the problem too.

This also means that the code in tsl2550_get_adc_value() is more complex than it needs to be. The only case where it makes sense to wait 400 ms to get a reading is right after power-up. 400 ms after power-up, c0 will always have a valid value, and after 800 ms c1 will always have a valid value as well. So I propose to simplify tsl2550_get_adc_value() and just return -EAGAIN if there is no valid reading. In practice I doubt we'll ever hit it.

Quoted 6 lines

I am not
sure what to do in this case. We can return -EAGAIN and let user-space retry. But we could also restart the computation automatically in this case. Of course if the problem only affects the evaluation module then we don't really care.

Quoted 8 lines

Thinking some more about this: if we want to retry then we will have to wait at least 400 ms to read c0 again, and if it's not enough 400 more ms to read c1 again. And even then, there's no guarantee the new readings will be OK if the light conditions are still changing. This is an intrinsic weakness of the TSL2550 that both channels are sampled sequentially. I doubt we want to let the user wait for the lux value for several seconds, so returning -EAGAIN seems OK to me.

I think that returning -EAGAIN should be fine in this case, giving to the user application the responsibility to check the returned value.

Quoted 5 lines

But I am not using the light sensor a lot myself, so my practical experience is rather limited, thus I would welcome comments and opinions on this.

From my experience, the case of c1 > c0 happens when light conditions
are still changing and, in this case, returning -EAGAIN it's the correct way. This behaviour is due to the sequentially read of c1 and c0
channel: we can do nothing to improve it through the driver.

I suggest, like you said, just to replace the -1 return value with -EAGAIN.

If you are in accord with this, I can pass you a new patch.

Signed-off-by Michele Jr De Candia <[email protected]>

drivers/i2c/chips/tsl2550.c +8 -3

index 1a9cc13..561d7bf 100644
@@ -27,7 +27,7 @@
 #include <linux/delay.h>
 
 #define TSL2550_DRV_NAME	"tsl2550"
-#define DRIVER_VERSION		"1.1.1"
+#define DRIVER_VERSION		"1.1.2"
 
 /*
  * Defines

@@ -189,13 +189,16 @@ static int tsl2550_calculate_lux(u8 ch0, u8 ch1)
 	u8 r = 128;
 
 	/* Avoid division by 0 and count 1 cannot be greater than count 0 */
-	if (c0 && (c1 <= c0))
-		r = c1 * 128 / c0;
+	if (c1 <= c0)
+		if (c0) {
+			r = c1 * 128 / c0;
+
+			/* Calculate LUX */
+			lux = ((c0 - c1) * ratio_lut[r]) / 256;
+		} else

Quoted 9 lines

Quoted 1 line

If you are in accord with this, I can pass you a new patch.

Yes I agree, please send an updated patch and I'll apply it.

In the meantime I've improved the performance of the i2c-taos-evm driver I'm using for the TSL5220, I'll post it later today, once I am fully done with testing. I'm not sure if there are other users of this driver though.

Quoted 2 lines

Thanks,

You can find the patch in attachment. Maybe I'll submit you more patches about this driver but they are not ready yet (extended mode support and spurious values filtering).