Re: [PATCH] TSL2550 support (I2C device driver)

On Tue, 30 Jan 2007 00:56:19 +0100
Rodolfo Giometti <giometti@xxxxxxxxxxxx> wrote:

Hello,

attached a patch to add support for Taos TSL2550 ambient light sensors
(http://www.taosinc.com/product_detail.asp?cateid=4&proid=18).


Some minor things:

+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+
+/* Address to scan */
+static unsigned short normal_i2c[] = { 0x39, I2C_CLIENT_END };
+
+/* Insmod parameters */
+I2C_CLIENT_INSMOD_1(tsl2550);
+
+static int operating_mode = 0;

The `= 0' is unneeded and undesirable.

...

+static int tsl2550_get_adc_value(struct i2c_client *client, int channel)
+{
+ u8 cmd = channel == 0 ? TSL2550_READ_ADC0 : TSL2550_READ_ADC1;
+ int timeout, ret;
+
+ /* Read ADC channel waiting at most 400ms (see data *** for further
+ * info) */
+ for (timeout = 400; timeout > 0; timeout--) {
+ i2c_smbus_write_byte(client, cmd);
+ mdelay(1);
+ ret = i2c_smbus_read_byte(client);
+ if (ret < 0)
+ return ret;
+ else if (ret & 0x0080)
+ break;
+ }
+ if (timeout == 0)
+ return -EIO;
+ return ((u8) ret) & 0x7f; /* remove the "valid" bit */
+}

eek. Is there no way to avoid the busy-wait? We cannot sleep here?

+/* --- LUX calculation ----------------------------------------------------- */
+
+#define TSL2550_MAX_LUX 1846
+
+static u8 ratio_lut[] = {
+ 100, 100, 100, 100, 100, 100, 100, 100,
+ 100, 100, 100, 100, 100, 100, 99, 99,
+ 99, 99, 99, 99, 99, 99, 99, 99,
+ 99, 99, 99, 98, 98, 98, 98, 98,
+ 98, 98, 97, 97, 97, 97, 97, 96,
+ 96, 96, 96, 95, 95, 95, 94, 94,
+ 93, 93, 93, 92, 92, 91, 91, 90,
+ 89, 89, 88, 87, 87, 86, 85, 84,
+ 83, 82, 81, 80, 79, 78, 77, 75,
+ 74, 73, 71, 69, 68, 66, 64, 62,
+ 60, 58, 56, 54, 52, 49, 47, 44,
+ 42, 41, 40, 40, 39, 39, 38, 38,
+ 37, 37, 37, 36, 36, 36, 35, 35,
+ 35, 35, 34, 34, 34, 34, 33, 33,
+ 33, 33, 32, 32, 32, 32, 32, 31,
+ 31, 31, 31, 31, 30, 30, 30, 30,
+ 30,
+};
+
+static u16 count_lut[] = {
+ 0, 1, 2, 3, 4, 5, 6, 7,
+ 8, 9, 10, 11, 12, 13, 14, 15,
+ 16, 18, 20, 22, 24, 26, 28, 30,
+ 32, 34, 36, 38, 40, 42, 44, 46,
+ 49, 53, 57, 61, 65, 69, 73, 77,
+ 81, 85, 89, 93, 97, 101, 105, 109,
+ 115, 123, 131, 139, 147, 155, 163, 171,
+ 179, 187, 195, 203, 211, 219, 227, 235,
+ 247, 263, 279, 295, 311, 327, 343, 359,
+ 375, 391, 407, 423, 439, 455, 471, 487,
+ 511, 543, 575, 607, 639, 671, 703, 735,
+ 767, 799, 831, 863, 895, 927, 959, 991,
+ 1039, 1103, 1167, 1231, 1295, 1359, 1423, 1487,
+ 1551, 1615, 1679, 1743, 1807, 1871, 1935, 1999,
+ 2095, 2223, 2351, 2479, 2607, 2735, 2863, 2991,
+ 3119, 3247, 3375, 3503, 3631, 3759, 3887, 4015
+};

These tables could perhaps be const?

+static ssize_t tsl2550_show_power_state(struct device *dev, struct device_attribute *attr, char *buf)

It's preferred to try to fit the code into an 80-col window. (But some
people disagree with this specifically for function definitions such as
this).

+static ssize_t tsl2550_store_power_state(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)

Ditto (and other places).

+static int tsl2550_detach_client(struct i2c_client *client)
+{
+ int err;
+
+ sysfs_remove_group(&client->dev.kobj, &tsl2550_attr_group);
+
+ if ((err = i2c_detach_client(client)))
+ return err;

Preferred coding style is

err = i2c_detach_client(client);
if (err)
return err;

(multiple places)


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/