Re: [PATCH 1/1] input: Add support for Kionix KXTJ9 accelerometer

[Alan Cox]

> + * The following table lists the maximum appropriate poll interval for each
> + * available output data rate.

Surely this can be static const

> + */
> +struct {
> +	unsigned int cutoff;
> +	u8 mask;
> +} kxtj9_odr_table[] = {
> +	{

> +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


> +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


> +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

> +	}
> +
> +	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

> +	}
> +
> +	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.



> +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 ?


> +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



> +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;
> +}

> +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


> +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

> +	}
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev, "client not i2c capable\n");
> +		err = -ENODEV;

Ditto


> +	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.
--
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/