i2c/tsl2550: Use combined SMBus transactions

PATCH In review Jean Delvare 3 participants 3 messages Jul 15, 2009 – Aug 5, 2009 1 message not in the archive I2C subsystem host drivers

lore

Share & tools

lore

Jean Delvare

author

2009-07-15 20:28 UTC #0 lore raw

PATCH 1/1 i2c/tsl2550: Use combined SMBus transactions

Make the I/O faster, mainly by using combined SMBus transactions when possible. While the TSL2550 datasheet doesn't say the device supports them, they seem to work just fine in practice, and a combined transaction is faster than two simple transactions in many cases and always more reliable.

A side effect is to suppress the delays between SMBus writes and reads. The datasheet doesn't say they are needed and things work just fine for me without them.

I also couldn't see any reason for the delay between reading the two channels. Nor for the loop to get a reading in the first place. The 400 ms delay between samples only matters at chip power-up, after that the chip always hold the previously sampled value so we never get to wait.

All these changes make reading the lux value much faster and cheaper.

Signed-off-by Jean Delvare <[email protected]>

Cc Michele De Candia <[email protected]>

Cc Rodolfo Giometti <[email protected]>

---
Michele, Rodolfo, can you please test and confirm the driver still works OK for you? Thanks.

 drivers/i2c/chips/tsl2550.c |   42 ++++++++++--------------------------------
 1 file changed, 10 insertions(+), 32 deletions(-)

drivers/i2c/chips/tsl2550.c +10 ~~-32~~

@@ -24,10 +24,9 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/mutex.h>
-#include <linux/delay.h>
 
 #define TSL2550_DRV_NAME	"tsl2550"
-#define DRIVER_VERSION		"1.1.2"
+#define DRIVER_VERSION		"1.2"
 
 /*
  * Defines

@@ -96,32 +95,13 @@ static int tsl2550_set_power_state(struc
 
 static int tsl2550_get_adc_value(struct i2c_client *client, u8 cmd)
 {
-	unsigned long end;
-	int loop = 0, ret = 0;
+	int ret;
 
-	/*
-	 * Read ADC channel waiting at most 400ms (see data sheet for further
-	 * info).
-	 * To avoid long busy wait we spin for few milliseconds then
-	 * start sleeping.
-	 */
-	end = jiffies + msecs_to_jiffies(400);
-	while (time_before(jiffies, end)) {
-		i2c_smbus_write_byte(client, cmd);
-
-		if (loop++ < 5)
-			mdelay(1);
-		else
-			msleep(1);
-
-		ret = i2c_smbus_read_byte(client);
-		if (ret < 0)
-			return ret;
-		else if (ret & 0x0080)
-			break;
-	}
+	ret = i2c_smbus_read_byte_data(client, cmd);
+	if (ret < 0)
+		return ret;
 	if (!(ret & 0x80))
-		return -EIO;
+		return -EAGAIN;
 	return ret & 0x7f;	/* remove the "valid" bit */
 }

@@ -285,8 +265,6 @@ static ssize_t __tsl2550_show_lux(struct
 		return ret;
 	ch0 = ret;
 
-	mdelay(1);
-
 	ret = tsl2550_get_adc_value(client, TSL2550_READ_ADC1);
 	if (ret < 0)
 		return ret;

@@ -345,11 +323,10 @@ static int tsl2550_init_client(struct i2
 	 * Probe the chip. To do so we try to power up the device and then to
 	 * read back the 0x03 code
 	 */
-	err = i2c_smbus_write_byte(client, TSL2550_POWER_UP);
+	err = i2c_smbus_read_byte_data(client, TSL2550_POWER_UP);
 	if (err < 0)
 		return err;
-	mdelay(1);
-	if (i2c_smbus_read_byte(client) != TSL2550_POWER_UP)
+	if (err != TSL2550_POWER_UP)
 		return -ENODEV;
 	data->power_state = 1;

@@ -374,7 +351,8 @@ static int __devinit tsl2550_probe(struc
 	struct tsl2550_data *data;
 	int *opmode, err = 0;
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) {
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WRITE_BYTE
+					    | I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
 		err = -EIO;
 		goto exit;
 	}

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

Rodolfo Giometti

Acked-by

2009-07-19 09:52 UTC #2 lore raw

Quoted 16 lines · Jean Delvare

Make the I/O faster, mainly by using combined SMBus transactions when possible. While the TSL2550 datasheet doesn't say the device supports them, they seem to work just fine in practice, and a combined transaction is faster than two simple transactions in many cases and always more reliable.
A side effect is to suppress the delays between SMBus writes and reads. The datasheet doesn't say they are needed and things work just fine for me without them.
I also couldn't see any reason for the delay between reading the two channels. Nor for the loop to get a reading in the first place. The 400 ms delay between samples only matters at chip power-up, after that the chip always hold the previously sampled value so we never get to wait.

Quoted 8 lines

All these changes make reading the lux value much faster and cheaper.
Signed-off-by Jean Delvare <[email protected]>
Cc Michele De Candia <[email protected]>
Cc Rodolfo Giometti <[email protected]>
---
Michele, Rodolfo, can you please test and confirm the driver still works OK for you? Thanks.

I'm sorry but I cannot do it right now (and in the near future) since my hardware is out of duty, but if Michele and you have tested this new implementation then the modification it's ok for me! :)

Acked-by Rodolfo Giometti <[email protected]>

Ciao,

Rodolfo

Signature · 4 lines

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/[email protected]
Linux Device Driver                          [email protected]
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

De Candia Michele

2009-08-05 09:47 UTC #3 lore raw

R: [PATCH] i2c/tsl2550: Use combined SMBus transactions

In reply to a message not in the archive

Hi Jean,
Sorry for my delay in answering; I've just tested your patch and it works fine on my hardware. A lux read is 4 times faster then previous implementation, great job!

Regards,
Michele

-----Messaggio originale-----
Da: [email protected] [mailto:[email protected]] Per conto di Jean Delvare
Inviato: mercoledì 15 luglio 2009 22.28
A: Linux I2C
Cc: De Candia Michele; Rodolfo Giometti
Oggetto: [PATCH] i2c/tsl2550: Use combined SMBus transactions