Re: + input-add-tsc2007-based-touchscreen-driver.patch added to -mm tree

[Jean Delvare <khali@linux-fr.org>]

On Mon, 8 Dec 2008 14:35:59 +0900, Kwangwoo Lee wrote:
> From d5258e21e38e38e4128112844f24621ffb1c1e86 Mon Sep 17 00:00:00 2001
> From: Kwangwoo Lee <kwangwoo.lee@gmail.com>
> Date: Mon, 8 Dec 2008 13:42:14 +0900
> Subject: [PATCH] Add tsc2007 based touchscreen driver.
> 
> corrected some parts following in helpful comments:
> Remove unnecessary headers and variables.
> Use i2c_smbus_read_word_data() to prevent race condition.
> Fix i2c_check_functionality() argument correctly.
> 
> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@gmail.com>

Looks much better, thanks. Just one thing:

> +static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
> +{
> +	s32 data;
> +	u16 val;
> +
> +	data = i2c_smbus_read_word_data(tsc->client, cmd);
> +	if (data < 0) {
> +		dev_err(&tsc->client->dev, "i2c io error: %d\n", data);
> +		return data;
> +	}
> +
> +	/* The protocol and raw data format from i2c interface:
> +	 * S Addr Wr [A] Comm [A] S Addr Rd [A] [DataLow] A [DataHigh] NA P
> +	 * Where DataLow has [D11-D4], DataHigh has [D3-D0 << 4 | Dummy 4bit].
> +	 */
> +
> +	val = (u16) (data & 0xFFFF);

These masking and cast are unnecessary. i2c_smbus_read_word_data()
returns a 16-bit value already.

> +	val = be16_to_cpu(val) >> 4;

I don't think this is correct. The SMBus read word protocol says that
the LSB goes first on the wire. If your chip sends the MSB first (and
in fast almost all I2C/SMBus chips I know, do that) then you need to
swap the bytes in the driver, but this must be done independently of
the CPU endianness! This means that you should use swab16() rather than
be16_to_cpu().

So I believe that what you really want is:

#include <linux/swab.h>

	val = swab16(data) >> 4;

> +
> +	dev_dbg(&tsc->client->dev, "data: 0x%x, val: 0x%x\n", data, val);
> +
> +	return val;
> +}

The rest looks OK.

-- 
Jean Delvare