linux-kernel - Re: [PATCH] Add TI TSC2005 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]

Message-ID: <20081208182648.GA9740@uranus.ravnborg.org>
Date:	Mon, 8 Dec 2008 19:26:48 +0100
From:	Sam Ravnborg <sam@...nborg.org>
To:	Trilok Soni <soni.trilok@...il.com>
Cc:	David Brownell <david-b@...bell.net>, dmitry.torokhov@...il.com,
	"linux-omap@...r.kernel.org Mailing List" 
	<linux-omap@...r.kernel.org>, linux-kernel@...r.kernel.org,
	spi-devel-general@...ts.sourceforge.net,
	linux-input@...r.kernel.org, lauri.leukkunen@...ia.com
Subject: Re: [PATCH] Add TI TSC2005 Touchscreen driver

> Add Texas Instruments TSC2005 chip touchscreen driver.
> 
> Signed-off-by: Trilok Soni <soni.trilok@...il.com>

Hi Trilok.
A few nitpicks below.

In general a very clean written driver with adequate comments - nice work!

	Sam


> ---
>  drivers/input/touchscreen/Kconfig   |    6 +
>  drivers/input/touchscreen/Makefile  |    1 +
>  drivers/input/touchscreen/tsc2005.c |  728 +++++++++++++++++++++++++++++++++++
>  3 files changed, 735 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/touchscreen/tsc2005.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig
> b/drivers/input/touchscreen/Kconfig
> index 3d1ab8f..77e6729 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -221,6 +221,12 @@ config TOUCHSCREEN_ATMEL_TSADCC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called atmel_tsadcc.
> 
> +config TOUCHSCREEN_TSC2005
> +	tristate "TSC2005 touchscreen support"
I would be good to see Texas Instruments spelled out here.
> +	depends on SPI_MASTER
> +	help
> +	  Say Y here for if you are using the touchscreen features of TSC2005.
And maybe with a bit text explaining where it is used or maybe where to find info.

> +#define TSC2005_VDD_LOWER_27
> +
> +#ifdef TSC2005_VDD_LOWER_27
> +#define TSC2005_HZ     (10000000)
> +#else
> +#define TSC2005_HZ     (25000000)
> +#endif

You define TSC2005_VDD_LOWER_27 and test for it two
lines later - looks strange.

> +
> +static void tsc2005_cmd(struct tsc2005 *ts, u8 cmd)
> +{
> +	u16 data = TSC2005_CMD | TSC2005_CMD_12BIT | cmd;
> +	struct spi_message msg;
> +	struct spi_transfer xfer = { 0 };
> +
> +	xfer.tx_buf = &data;
> +	xfer.rx_buf = NULL;
> +	xfer.len = 1;
> +	xfer.bits_per_word = 8;
data is a 16 bit quantity yet you specify a len of 1.
Maybe len is counted from 0?

> +
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&xfer, &msg);
> +	spi_sync(ts->spi, &msg);
> +}
> +
> +static void tsc2005_write(struct tsc2005 *ts, u8 reg, u16 value)
> +{
> +	u32 tx;
> +	struct spi_message msg;
> +	struct spi_transfer xfer = { 0 };
> +
> +	tx = (TSC2005_REG | reg | TSC2005_REG_PND0 |
> +	       TSC2005_REG_WRITE) << 16;
> +	tx |= value;
Is this endian safe? Does spi know about LSB/MSB in the value?

> +
> +	xfer.tx_buf = &tx;
> +	xfer.rx_buf = NULL;
> +	xfer.len = 4;
> +	xfer.bits_per_word = 24;
> +
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&xfer, &msg);
> +	spi_sync(ts->spi, &msg);
> +}

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