Re: [PATCH 1/1] input: Add support for Kionix KXTJ9 accelerometer
flat view
From: Alan Cox <hidden>
Date: 2011-05-13 18:25:48
Also in:
lkml
+ * The following table lists the maximum appropriate poll interval for each + * available output data rate.
Surely this can be static const
quoted hunk ↗ jump to hunk
+ */ +struct { + unsigned int cutoff; + u8 mask; +} kxtj9_odr_table[] = { + {
quoted hunk ↗ jump to hunk
+static int kxtj9_i2c_read(struct kxtj9_data *tj9, u8 addr, u8 *data, int len) +{ + int err; + + struct i2c_msg msgs[] = { + { + .addr = tj9->client->addr, + .flags = tj9->client->flags, + .len = 1, + .buf = &addr, + }, + { + .addr = tj9->client->addr, + .flags = tj9->client->flags | I2C_M_RD, + .len = len, + .buf = data, + }, + }; + err = i2c_transfer(tj9->client->adapter, msgs, 2); + + if (err != 2) + dev_err(&tj9->client->dev, "read transfer error\n");
Just return ret and check it with < 0 meaning error
quoted hunk ↗ jump to hunk
+static int kxtj9_i2c_write(struct kxtj9_data *tj9, u8 addr, u8 *data, int len) +{ + int err; + int i; + u8 buf[len + 1]; + + struct i2c_msg msgs[] = { + { + .addr = tj9->client->addr, + .flags = tj9->client->flags, + .len = len + 1, + .buf = buf, + }, + }; + + buf[0] = addr; + for (i = 0; i < len; i++) + buf[i + 1] = data[i];
memcpy is quite useful and the kernel one inlines nicely
quoted hunk ↗ jump to hunk
+int kxtj9_update_g_range(struct kxtj9_data *tj9, u8 new_g_range) +{ + int err; + u8 shift; + u8 buf; + u8 out; + + switch (new_g_range) { + case KXTJ9_G_2G: + shift = SHIFT_ADJ_2G; + break; + case KXTJ9_G_4G: + shift = SHIFT_ADJ_4G; + break; + case KXTJ9_G_8G: + shift = SHIFT_ADJ_8G; + break; + default: + dev_err(&tj9->client->dev, "invalid g range request\n"); + return -EINVAL;
So a user can fill the system log ? not good
quoted hunk ↗ jump to hunk
+ } + + out = (tj9->resume[RES_CTRL_REG1] & 0xE7) | new_g_range; + if (shift != tj9->pdata->shift_adj) { + if (atomic_read(&tj9->enabled)) { + buf = 0; + err = kxtj9_i2c_write(tj9, CTRL_REG1, &buf, 1); + if (err < 0) + return err; + err = kxtj9_i2c_write(tj9, CTRL_REG1, &out, 1); + if (err < 0) + return err; + }
You seem to have no locking on parallel actions and a rather meaningles atomic
quoted hunk ↗ jump to hunk
+ } + + tj9->resume[RES_CTRL_REG1] = out; + tj9->pdata->shift_adj = shift;
and unlocked too In fact the locking seems to be utterly busted everywhere in the driver.
quoted hunk ↗ jump to hunk
+static void kxtj9_device_power_off(struct kxtj9_data *tj9) +{ + int err; + u8 buf; + + buf = tj9->resume[RES_CTRL_REG1] & PC1_OFF; + err = kxtj9_i2c_write(tj9, CTRL_REG1, &buf, 1); + if (err < 0) + dev_err(&tj9->client->dev, "soft power off failed\n"); + disable_irq(tj9->irq);
What if the IRQ is shared ?
quoted hunk ↗ jump to hunk
+static irqreturn_t kxtj9_isr(int irq, void *dev) +{ + struct kxtj9_data *tj9 = dev; + + disable_irq_nosync(irq); + schedule_work(&tj9->irq_work); + + return IRQ_HANDLED; +}
Threaded irq will sort this lot out for you
quoted hunk ↗ jump to hunk
+static ssize_t kxtj9_enable_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev); + struct kxtj9_data *tj9 = i2c_get_clientdata(client); + int val = simple_strtoul(buf, NULL, 10); + if (val) + kxtj9_enable(tj9); + else + kxtj9_disable(tj9);
You seem to have no locking on this either
+ return count; +}
quoted hunk ↗ jump to hunk
+static DEVICE_ATTR(delay, S_IRUGO|S_IWUSR, kxtj9_delay_show, kxtj9_delay_store); +static DEVICE_ATTR(enable, S_IRUGO|S_IWUSR, kxtj9_enable_show, + kxtj9_enable_store); +static DEVICE_ATTR(res, S_IRUGO|S_IWUSR, kxtj9_res_show, kxtj9_res_store); +static DEVICE_ATTR(drdyenable, S_IRUGO|S_IWUSR, NULL, kxtj9_drdyenable_store); +static DEVICE_ATTR(grange, S_IRUGO|S_IWUSR, kxtj9_grange_show, + kxtj9_grange_store);
These should probably come from platform data
quoted hunk ↗ jump to hunk
+static int __devinit kxtj9_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + int err = -1; + struct kxtj9_data *tj9 = kzalloc(sizeof(*tj9), GFP_KERNEL); + if (tj9 == NULL) { + dev_err(&client->dev, + "failed to allocate memory for module data\n"); + err = -ENOMEM; + goto err0; + } + if (client->dev.platform_data == NULL) { + dev_err(&client->dev, "platform data is NULL; exiting\n"); + err = -ENODEV; + goto err0;
Leaks memory
quoted hunk ↗ jump to hunk
+ } + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { + dev_err(&client->dev, "client not i2c capable\n"); + err = -ENODEV;
Ditto
quoted hunk ↗ jump to hunk
+ memcpy(tj9->pdata, client->dev.platform_data, sizeof(*tj9->pdata)); + if (tj9->pdata->init) { + err = tj9->pdata->init(); + if (err < 0) + goto err2; + } + + tj9->irq = gpio_to_irq(tj9->pdata->gpio);
What if it's not on a gpio. Probably better any gpio and irq conversion is done by the platform callback/init code And the sysfs nodes need documentation. Overall it looks like a lot of input drivers that get posted here - Nobody has considered locking between actions and sysfs (or multiple sysfs actions in parallel) - Enable/disable is implemented in blissful ignorance of what happens if a disable occurs during a sysfs or other action Most of the rest looks like it just needs a good polish.