Re: [PATCH] input: add synaptics_i2c driver

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

Hi Mike,

On Thu, 14 May 2009 12:04:02 +0300, Mike Rapoport wrote:
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> ---
>  drivers/input/mouse/Kconfig         |    9 +
>  drivers/input/mouse/Makefile        |    1 +
>  drivers/input/mouse/synaptics_i2c.c |  793 +++++++++++++++++++++++++++++++++++
>  3 files changed, 803 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/mouse/synaptics_i2c.c

I think you introduced a new bug in this version:

> (...)
> +static s32 synaptics_i2c_block_get(struct i2c_client *client,
> +				    u16 reg, u8 length, u8 *values)
> +{
> +	struct synaptics_i2c *touch = i2c_get_clientdata(client);
> +	int ret;
> +
> +	mutex_lock(&touch->mutex);
> +
> +	ret = i2c_smbus_write_byte_data(client, PAGE_SEL_REG, reg >> 8);
> +	if (!ret)
> +		ret = i2c_smbus_read_i2c_block_data(client, reg & 0xff,
> +						    length, values);
> +
> +	mutex_unlock(&touch->mutex);
> +
> +	return ret < 0 ? : 0;
> +}

Here you return 0 on success...

> +
> +static int synaptics_i2c_query(struct i2c_client *client)
> +{
> +	u8 data[7];
> +	char id[PRODUCT_ID_LENGTH + 1];
> +	int ret, retry_count, control, status, sens;
> +
> +	/* DEV_QUERY_REG0 is the Function Query area for 2D devices. */
> +	/* We're only interested in entries DEV_QUERY_REG2 */
> +	/* and following registers right now. */
> +	for (retry_count = 0; retry_count < 3; retry_count++) {
> +		ret = synaptics_i2c_block_get(client, DEV_QUERY_REG2,
> +					       sizeof(data), data);
> +		if (ret == sizeof(data))

... but here you expect the number of read values.

> +			break;
> +	}
> +
> +	if (ret == sizeof(data)) {
> +		dev_warn(&client->dev,
> +			  "Query command failed: block read failed\n");
> +	} else {

> (...)
> +static struct i2c_driver synaptics_i2c_driver = {
> +	.driver = {
> +		.name	= DRIVER_NAME,
> +		.owner	= THIS_MODULE,
> +	},
> +
> +	.probe		= synaptics_i2c_probe,
> +	.remove		= synaptics_i2c_remove,

Still missing __devexit_p() as Dmitry wrote.

> +
> +	.suspend	= synaptics_i2c_suspend,
> +	.resume		= synaptics_i2c_resume,
> +	.id_table	= synaptics_i2c_id_table,
> +};

The rest looks OK to me. After fixing the 2 errors above, you can add

Acked-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare