Hi Christopher,
On Fri, 28 May 2010 17:29:40 -0700, Christopher Heiny wrote:
> Initial driver for Synaptics touchscreens using RMI4 protocol.
>
> Signed-off-by: William Manson <WManson@xxxxxxxxxxxxx>
> Signed-off-by: Allie Xiong <axiong@xxxxxxxxxxxxx>
> Signed-off-by: Christopher Heiny <cheiny@xxxxxxxxxxxxx>
You can add:
Acked-by: Jean Delvare <khali@xxxxxxxxxxxx>
for the i2c parts. I still have a few comments you might be interested
in, maybe for a future incremental patch:
> (...)
> --- /dev/null
> +++ b/drivers/input/touchscreen/rmi_i2c_gta01.c
> @@ -0,0 +1,117 @@
> (...)
> +static void
> +rmi_i2c_release(struct device *dev)
> +{
> + struct platform_device *pd = container_of(dev,
> + struct platform_device, dev);
You could use to_platform_device(dev) instead, it's more readable.
> (...)
> --- /dev/null
> +++ b/drivers/input/touchscreen/rmi_phys_i2c.c
> (...)
> +/* TODO: for multiple device support will need a per-device mutex */
This comment would be better placed below, where page_mutex is declared.
> +#define DRIVER_NAME "rmi4-i2c"
> +
> +/* TODO: for multiple device support will need a per-device device name */
This comment is confusing... you would need different names if you were
supporting different device _types_ in the same driver. But you
definitely do _not_ need different names to support several devices of
the same type in a given system.
> +#define DEVICE_NAME "rmi4-i2c"
The use of dashes in i2c device names is strongly discouraged.
Including "i2c" in these names is discouraged as well, as it is
redundant. "rmi4_ts" would be a better name IMHO.
> (...)
> +static int
> +rmi_i2c_probe(struct i2c_client *client, const struct i2c_device_id *dev_id)
> +{
> + struct instance_data *id;
> + int retval = 0;
> + int i;
> + bool found = false;
> +
> + struct rmi_i2c_data *rmii2cdata;
> + struct rmi_i2c_platformdata *platformdata;
> +
> + pr_debug("Probing i2c RMI device\n");
> +
> + /* Allocate and initialize the instance data for this client */
> + id = kzalloc(sizeof(*id) * 2, GFP_KERNEL);
I still don't get the * 2.
> (...)
> + /* cast to our struct rmi_i2c_data so we know
> + the fields (see rmi_ic2.h) */
> + rmii2cdata = ((struct rmi_i2c_data *)(client->dev.platform_data));
Explicit cast still not needed, you can just write:
rmii2cdata = client->dev.platform_data;
--
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
