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