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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 29 May 2010 14:32:06 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Christopher Heiny <cheiny@...aptics.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Input <linux-input@...r.kernel.org>,
	Christopher Heiny <cheiny@...aptics.com>,
	Allie Xiong <axiong@...aptics.com>,
	William Manson <wmanson@...aptics.com>,
	Joerie de Gram <j.de.gram@...il.com>
Subject: Re: [RFC PATCH 1/1] input/touchscreen: Synaptics Touchscreen Driver

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@...aptics.com>
> Signed-off-by: Allie Xiong <axiong@...aptics.com>
> Signed-off-by: Christopher Heiny <cheiny@...aptics.com>

You can add:

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

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/