Re: [PATCH] input: add synaptics_i2c driver

[Jean Delvare <khali@linux-fr.org>]

Hi Mike,

On Thu, 14 May 2009 17:20:27 +0300, Mike Rapoport wrote:
> I've addressed all the comments Jean had, and I hopefully haven't broke any I2C
> bits this time, so I dare to add Jean's Ack.
> 
> This driver supports Synaptics I2C touchpad controller on eXeda
> mobile device.

I keep seeing new bugs, sorry for missing this one during the previous
round:

> +static int __devinit synaptics_i2c_probe(struct i2c_client *client,
> +			       const struct i2c_device_id *dev_id)
> +{
> +	int ret;
> +	struct synaptics_i2c *touch;
> +
> +	touch = synaptics_i2c_touch_create(client);
> +	if (!touch)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, touch);
> +
> +	ret = synaptics_i2c_reset_config(client);
> +	if (ret)
> +		goto err_mem_free;
> +
> +	if (client->irq < 1)
> +		polling_req = 1;
> +
> +	touch->input = input_allocate_device();
> +	if (!touch->input)
> +		goto err_mem_free;

You jump without setting err to -ENOMEM. err is 0 at this point so you
will return success while the probe failed. Bad...

> +
> +	synaptics_i2c_set_input_params(touch);
> +
> +	if (!polling_req)
> +		dev_info(&touch->client->dev,
> +			  "IRQ will be used: %d\n", touch->client->irq);
> +	else
> +		dev_info(&touch->client->dev,
> +			  "Using polling at rate: %d times/sec\n", scan_rate);
> +
> +	/* Register the device in input subsystem */
> +	ret = input_register_device(touch->input);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			 "Input device register failed: %d\n", ret);
> +		goto err_input_free;
> +	}
> +	return 0;
> +
> +err_input_free:
> +	input_free_device(touch->input);
> +err_mem_free:
> +	i2c_set_clientdata(client, NULL);
> +	kfree(touch);
> +
> +	return ret;
> +}

Also note that:

> +static const struct i2c_device_id synaptics_i2c_id_table[] = {
> +	{ "synaptics_i2c", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, synaptics_i2c_id_table);

The "_i2c" in the device name is kind of redundant, as you are already
in the namespace of i2c devices. But that's up to you, it's only a
minor aesthetic issue.

All the rest looks OK.

-- 
Jean Delvare