Hi Jean,
Jean Delvare wrote:
> Hi Mike,
>
> On Thu, 14 May 2009 12:04:02 +0300, Mike Rapoport wrote:
>> Signed-off-by: Igor Grinberg <grinberg@xxxxxxxxxxxxxx>
>> Signed-off-by: Mike Rapoport <mike@xxxxxxxxxxxxxx>
>> ---
>> 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.
I think I'd just drop the entire _query think although original driver author
really wanted to have it. It does not add anything except the head ache.
>
>> + 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.
I somehow missed all the points Dmitry had. :( So I'm going to another round
now. Sorry for the noise.
>> +
>> + .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@xxxxxxxxxxxx>
>
--
Sincerely yours,
Mike.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
