Re: [RFC PATCH 1/1] input/touchscreen: Synaptics Touchscreen Driver

[Jean Delvare]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/