All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] Input/Touchscreen Driver: add support AD7877 touchscreen driver (try #5)
@ 2008-10-16 17:56 Bryan Wu
  2009-03-08  6:38 ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Bryan Wu @ 2008-10-16 17:56 UTC (permalink / raw)
  To: dmitry.torokhov, akpm
  Cc: linux-input, linux-kernel, Michael Hennerich, Bryan Wu

From: Michael Hennerich <michael.hennerich@analog.com>

[try #5]
 - Fix indention
 - Lock spi_async()
 - Remove useless header comments
 - Use strict_strtoul
 - Use EDGE triggered interrupts ?\226?\128?\147 this simplifies some of the IRQ handling.
 - Fix error cleanup path
 - Remove duplicated code
 - SPI access functions parameter use struct spi_device

[try #4] Changelog (based on LKML feedback by Dmitry Torokhov)
 - Fix req use after free.
 - Fix sysfs interaction (we migh re-eanble irgs upon a sysfs read).
 - Fix DAC bogus data read case

[try #3] Changlog (Add feedback from Dmitry Torokhov):
 - Change handling of spi_sync / spi_async return value handling
 - Remove depreciated dev->power.power_state
 - Fix error return path in ad7877_probe
 - delete pending kernel timer
 - Some minor cleanup (indention, use dev_err etc.)

[try #2] Changelog:
 - move locking inside ad7877_enable and ad7877_disable
 - use setup_timer
 - use input_dev->dev.parent
 - fix unregister device
 - kill EV_KEY since it's not used
 - fix indention style

Apply patch from Philip Douglass:
Make ad7877 ts driver not dependent on Blackfin specific cs_change_per_word_option

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/input/touchscreen/Kconfig  |   13 +
 drivers/input/touchscreen/Makefile |    1 +
 drivers/input/touchscreen/ad7877.c |  871 ++++++++++++++++++++++++++++++++++++
 3 files changed, 885 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/touchscreen/ad7877.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 6e1e8c6..2fc51d0 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -29,6 +29,19 @@ config TOUCHSCREEN_ADS7846
 	  To compile this driver as a module, choose M here: the
 	  module will be called ads7846.
 
+config TOUCHSCREEN_AD7877
+	tristate "AD7877 based touchscreens"
+	depends on SPI_MASTER
+	help
+	  Say Y here if you have a touchscreen interface using the
+	  AD7877 controller, and your board-specific initialization
+	  code includes that in its table of SPI devices.
+
+	  If unsure, say N (but it's safe to say "Y").
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7877.
+
 config TOUCHSCREEN_BITSY
 	tristate "Compaq iPAQ H3600 (Bitsy) touchscreen"
 	depends on SA1100_BITSY
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 15cf290..89a3660 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -6,6 +6,7 @@
 
 wm97xx-ts-y := wm97xx-core.o
 
+obj-$(CONFIG_TOUCHSCREEN_AD7877)	+= ad7877.o
 obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
 obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC)	+= atmel_tsadcc.o
 obj-$(CONFIG_TOUCHSCREEN_BITSY)		+= h3600_ts_input.o
diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
new file mode 100644
index 0000000..aaf88c9
--- /dev/null
+++ b/drivers/input/touchscreen/ad7877.c
@@ -0,0 +1,871 @@
+/*
+ * File:        drivers/input/touchscreen/ad7877.c
+ *
+ * Based on: 	ads7846.c
+ *
+ *		Copyright (C) 2006-2008 Michael Hennerich, Analog Devices Inc.
+ *
+ * Description:	AD7877 based touchscreen, sensor (ADCs), DAC and GPIO driver
+ *
+ * Bugs:        Enter bugs at http://blackfin.uclinux.org/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see the file COPYING, or write
+ * to the Free Software Foundation, Inc.,
+ * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * History:
+ * Copyright (c) 2005 David Brownell
+ * Copyright (c) 2006 Nokia Corporation
+ * Various changes: Imre Deak <imre.deak@nokia.com>
+ *
+ * Using code from:
+ *  - corgi_ts.c
+ *	Copyright (C) 2004-2005 Richard Purdie
+ *  - omap_ts.[hc], ads7846.h, ts_osk.c
+ *	Copyright (C) 2002 MontaVista Software
+ *	Copyright (C) 2004 Texas Instruments
+ *	Copyright (C) 2005 Dirk Behme
+ */
+
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/ad7877.h>
+#include <asm/irq.h>
+
+#define	TS_PEN_UP_TIMEOUT	msecs_to_jiffies(50)
+
+#define MAX_SPI_FREQ_HZ			20000000
+#define	MAX_12BIT			((1<<12)-1)
+
+#define AD7877_REG_ZEROS			0
+#define AD7877_REG_CTRL1			1
+#define AD7877_REG_CTRL2			2
+#define AD7877_REG_ALERT			3
+#define AD7877_REG_AUX1HIGH			4
+#define AD7877_REG_AUX1LOW			5
+#define AD7877_REG_BAT1HIGH			6
+#define AD7877_REG_BAT1LOW			7
+#define AD7877_REG_BAT2HIGH			8
+#define AD7877_REG_BAT2LOW			9
+#define AD7877_REG_TEMP1HIGH			10
+#define AD7877_REG_TEMP1LOW			11
+#define AD7877_REG_SEQ0				12
+#define AD7877_REG_SEQ1				13
+#define AD7877_REG_DAC				14
+#define AD7877_REG_NONE1			15
+#define AD7877_REG_EXTWRITE			15
+#define AD7877_REG_XPLUS			16
+#define AD7877_REG_YPLUS			17
+#define AD7877_REG_Z2				18
+#define AD7877_REG_aux1				19
+#define AD7877_REG_aux2				20
+#define AD7877_REG_aux3				21
+#define AD7877_REG_bat1				22
+#define AD7877_REG_bat2				23
+#define AD7877_REG_temp1			24
+#define AD7877_REG_temp2			25
+#define AD7877_REG_Z1				26
+#define AD7877_REG_GPIOCTRL1			27
+#define AD7877_REG_GPIOCTRL2			28
+#define AD7877_REG_GPIODATA			29
+#define AD7877_REG_NONE2			30
+#define AD7877_REG_NONE3			31
+
+#define AD7877_SEQ_YPLUS_BIT			(1<<11)
+#define AD7877_SEQ_XPLUS_BIT			(1<<10)
+#define AD7877_SEQ_Z2_BIT			(1<<9)
+#define AD7877_SEQ_AUX1_BIT			(1<<8)
+#define AD7877_SEQ_AUX2_BIT			(1<<7)
+#define AD7877_SEQ_AUX3_BIT			(1<<6)
+#define AD7877_SEQ_BAT1_BIT			(1<<5)
+#define AD7877_SEQ_BAT2_BIT			(1<<4)
+#define AD7877_SEQ_TEMP1_BIT			(1<<3)
+#define AD7877_SEQ_TEMP2_BIT			(1<<2)
+#define AD7877_SEQ_Z1_BIT			(1<<1)
+
+enum {
+	AD7877_SEQ_YPOS  = 0,
+	AD7877_SEQ_XPOS  = 1,
+	AD7877_SEQ_Z2    = 2,
+	AD7877_SEQ_AUX1  = 3,
+	AD7877_SEQ_AUX2  = 4,
+	AD7877_SEQ_AUX3  = 5,
+	AD7877_SEQ_BAT1  = 6,
+	AD7877_SEQ_BAT2  = 7,
+	AD7877_SEQ_TEMP1 = 8,
+	AD7877_SEQ_TEMP2 = 9,
+	AD7877_SEQ_Z1    = 10,
+	AD7877_NR_SENSE  = 11,
+};
+
+/* DAC Register Default RANGE 0 to Vcc, Volatge Mode, DAC On */
+#define AD7877_DAC_CONF			0x1
+
+/* If gpio3 is set AUX3/GPIO3 acts as GPIO Output */
+#define AD7877_EXTW_GPIO_3_CONF		0x1C4
+#define AD7877_EXTW_GPIO_DATA		0x200
+
+/* Control REG 2 */
+#define AD7877_TMR(x)			((x & 0x3) << 0)
+#define AD7877_REF(x)			((x & 0x1) << 2)
+#define AD7877_POL(x)			((x & 0x1) << 3)
+#define AD7877_FCD(x)			((x & 0x3) << 4)
+#define AD7877_PM(x)			((x & 0x3) << 6)
+#define AD7877_ACQ(x)			((x & 0x3) << 8)
+#define AD7877_AVG(x)			((x & 0x3) << 10)
+
+/* Control REG 1 */
+#define	AD7877_SER			(1 << 11)	/* non-differential */
+#define	AD7877_DFR			(0 << 11)	/* differential */
+
+#define AD7877_MODE_NOC  (0)	/* Do not convert */
+#define AD7877_MODE_SCC  (1)	/* Single channel conversion */
+#define AD7877_MODE_SEQ0 (2)	/* Sequence 0 in Slave Mode */
+#define AD7877_MODE_SEQ1 (3)	/* Sequence 1 in Master Mode */
+
+#define AD7877_CHANADD(x)		((x&0xF)<<7)
+#define AD7877_READADD(x)		((x)<<2)
+#define AD7877_WRITEADD(x)		((x)<<12)
+
+#define AD7877_READ_CHAN(x) (AD7877_WRITEADD(AD7877_REG_CTRL1) | AD7877_SER | \
+		AD7877_MODE_SCC | AD7877_CHANADD(AD7877_REG_ ## x) | \
+		AD7877_READADD(AD7877_REG_ ## x))
+
+#define AD7877_MM_SEQUENCE (AD7877_SEQ_YPLUS_BIT | AD7877_SEQ_XPLUS_BIT | \
+		AD7877_SEQ_Z2_BIT | AD7877_SEQ_Z1_BIT)
+
+/*
+ * Non-touchscreen sensors only use single-ended conversions.
+ */
+
+struct ser_req {
+	u16			reset;
+	u16			ref_on;
+	u16			command;
+	u16			sample;
+	struct spi_message	msg;
+	struct spi_transfer	xfer[6];
+};
+
+struct ad7877 {
+	struct input_dev	*input;
+	char			phys[32];
+
+	struct spi_device	*spi;
+	u16			model;
+	u16			vref_delay_usecs;
+	u16			x_plate_ohms;
+	u16			pressure_max;
+
+	u16			cmd_crtl1;
+	u16			cmd_crtl2;
+	u16			cmd_dummy;
+	u16			dac;
+
+	u8			stopacq_polarity;
+	u8			first_conversion_delay;
+	u8			acquisition_time;
+	u8			averaging;
+	u8			pen_down_acc_interval;
+
+	u16 			conversion_data[AD7877_NR_SENSE];
+
+	struct spi_transfer	xfer[AD7877_NR_SENSE + 2];
+	struct spi_message	msg;
+
+	spinlock_t		lock;
+	struct timer_list	timer;		/* P: lock */
+	unsigned		pendown:1;	/* P: lock */
+	unsigned		pending:1;	/* P: lock */
+	unsigned		disabled:1;
+	unsigned		gpio3:1;
+	unsigned		gpio4:1;
+};
+
+static int gpio3;
+
+/*
+ * ad7877_read/write are only used for initial setup and for sysfs controls.
+ * The main traffic is done using spi_async() in the interrupt handler.
+ */
+
+static int ad7877_read(struct spi_device *spi, u16 reg)
+{
+	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
+	int			status, ret;
+
+	if (!req)
+		return -ENOMEM;
+
+	spi_message_init(&req->msg);
+
+	req->command = (u16) (AD7877_WRITEADD(AD7877_REG_CTRL1) |
+			AD7877_READADD(reg));
+	req->xfer[0].tx_buf = &req->command;
+	req->xfer[0].len = 2;
+
+	req->xfer[1].rx_buf = &req->sample;
+	req->xfer[1].len = 2;
+
+	spi_message_add_tail(&req->xfer[0], &req->msg);
+	spi_message_add_tail(&req->xfer[1], &req->msg);
+
+	status = spi_sync(spi, &req->msg);
+
+	if (status == 0)
+		status = req->msg.status;
+
+	ret = status ? status : req->sample;
+	kfree(req);
+
+	return ret;
+}
+
+static int ad7877_write(struct spi_device *spi, u16 reg, u16 val)
+{
+	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
+	int			status;
+
+	if (!req)
+		return -ENOMEM;
+
+	spi_message_init(&req->msg);
+
+	req->command = (u16) (AD7877_WRITEADD(reg) | (val & MAX_12BIT));
+	req->xfer[0].tx_buf = &req->command;
+	req->xfer[0].len = 2;
+
+	spi_message_add_tail(&req->xfer[0], &req->msg);
+
+	status = spi_sync(spi, &req->msg);
+
+	if (status == 0)
+		status = req->msg.status;
+
+	kfree(req);
+
+	return status;
+}
+
+static int ad7877_read_adc(struct spi_device *spi, unsigned command)
+{
+	struct ad7877 		*ts = dev_get_drvdata(&spi->dev);
+	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
+	int			status;
+	int			sample;
+	int			i;
+
+	if (!req)
+		return -ENOMEM;
+
+	spi_message_init(&req->msg);
+
+	/* activate reference, so it has time to settle; */
+	req->ref_on = AD7877_WRITEADD(AD7877_REG_CTRL2) |
+			 AD7877_POL(ts->stopacq_polarity) |
+			 AD7877_AVG(0) | AD7877_PM(2) | AD7877_TMR(0) |
+			 AD7877_ACQ(ts->acquisition_time) | AD7877_FCD(0);
+
+	req->reset = AD7877_WRITEADD(AD7877_REG_CTRL1) | AD7877_MODE_NOC;
+
+	req->command = (u16) command;
+
+	req->xfer[0].tx_buf = &req->reset;
+	req->xfer[0].len = 2;
+
+	req->xfer[1].tx_buf = &req->ref_on;
+	req->xfer[1].len = 2;
+	req->xfer[1].delay_usecs = ts->vref_delay_usecs;
+
+	req->xfer[2].tx_buf = &req->command;
+	req->xfer[2].len = 2;
+	req->xfer[2].delay_usecs = ts->vref_delay_usecs;
+
+	req->xfer[3].rx_buf = &req->sample;
+	req->xfer[3].len = 2;
+
+	req->xfer[4].tx_buf = &ts->cmd_crtl2;	/*REF OFF*/
+	req->xfer[4].len = 2;
+
+	req->xfer[5].tx_buf = &ts->cmd_crtl1;	/*DEFAULT*/
+	req->xfer[5].len = 2;
+
+	/* group all the transfers together, so we can't interfere with
+	 * reading touchscreen state; disable penirq while sampling
+	 */
+	for (i = 0; i < 6; i++)
+		spi_message_add_tail(&req->xfer[i], &req->msg);
+
+	status = spi_sync(spi, &req->msg);
+
+	if (status == 0)
+		status = req->msg.status;
+
+	sample = req->sample;
+
+	kfree(req);
+	return status ? status : sample;
+}
+
+static void ad7877_rx(struct ad7877 *ts)
+{
+	struct input_dev	*input_dev = ts->input;
+	unsigned		Rt;
+	u16			x, y, z1, z2;
+
+	x = ts->conversion_data[AD7877_SEQ_XPOS] & MAX_12BIT;
+	y = ts->conversion_data[AD7877_SEQ_YPOS] & MAX_12BIT;
+	z1 = ts->conversion_data[AD7877_SEQ_Z1] & MAX_12BIT;
+	z2 = ts->conversion_data[AD7877_SEQ_Z2] & MAX_12BIT;
+
+	/*
+	 * The samples processed here are already preprocessed by the AD7877.
+	 * The preprocessing function consists of an averaging filter.
+	 * The combination of 'first conversion delay' and averaging provides a robust solution,
+	 * discarding the spurious noise in the signal and keeping only the data of interest.
+	 * The size of the averaging filter is programmable. (dev.platform_data, see linux/spi/ad7877.h)
+	 * Other user-programmable conversion controls include variable acquisition time,
+	 * and first conversion delay. Up to 16 averages can be taken per conversion.
+	 */
+
+	if (likely(x && z1)) {
+		/* compute touch pressure resistance using equation #1 */
+		Rt = (z2 - z1) * x * ts->x_plate_ohms;
+		Rt /= z1;
+		Rt = (Rt + 2047) >> 12;
+	} else
+		Rt = 0;
+
+	if (Rt) {
+		input_report_abs(input_dev, ABS_X, x);
+		input_report_abs(input_dev, ABS_Y, y);
+		input_report_abs(input_dev, ABS_PRESSURE, Rt);
+		input_sync(input_dev);
+	}
+}
+
+static inline void ad7877_ts_event_release(struct ad7877 *ts)
+{
+	struct input_dev *input_dev = ts->input;
+
+	input_report_abs(input_dev, ABS_PRESSURE, 0);
+	input_sync(input_dev);
+}
+
+static void ad7877_timer(unsigned long handle)
+{
+	struct ad7877	*ts = (void *)handle;
+
+	ad7877_ts_event_release(ts);
+}
+
+static irqreturn_t ad7877_irq(int irq, void *handle)
+{
+	struct ad7877 *ts = handle;
+	unsigned long flags;
+	int status;
+
+	/* The repeated conversion sequencer controlled by TMR kicked off too fast.
+	 * We ignore the last and process the sample sequence currently in the queue
+	 * It can't be older than 9.4ms, and we need to avoid that ts->msg
+	 * doesn't get issued twice while in work.
+	 */
+
+	if (ts->pending)
+		return IRQ_HANDLED;
+
+	spin_lock_irqsave(&ts->lock, flags);
+	ts->pending = 1;
+	spin_unlock_irqrestore(&ts->lock, flags);
+
+	status = spi_async(ts->spi, &ts->msg);
+
+	if (status)
+		dev_err(&ts->spi->dev, "spi_sync --> %d\n", status);
+
+	return IRQ_HANDLED;
+}
+
+static void ad7877_callback(void *_ts)
+{
+	struct ad7877 *ts = _ts;
+	unsigned long flags;
+
+	ad7877_rx(ts);
+
+	spin_lock_irqsave(&ts->lock, flags);
+	ts->pending = 0;
+	mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
+	spin_unlock_irqrestore(&ts->lock, flags);
+}
+
+static void ad7877_disable(struct ad7877 *ts)
+{
+	unsigned long flags;
+
+	if (ts->disabled)
+		return;
+
+	spin_lock_irqsave(&ts->lock, flags);
+	ts->disabled = 1;
+	disable_irq(ts->spi->irq);
+	spin_unlock_irqrestore(&ts->lock, flags);
+
+	while (ts->pending) /* Wait for spi_async callback */
+		msleep(1);
+
+	if (del_timer_sync(&ts->timer))
+		ad7877_ts_event_release(ts);
+
+	/* we know the chip's in lowpower mode since we always
+	 * leave it that way after every request
+	 */
+}
+
+static void ad7877_enable(struct ad7877 *ts)
+{
+	unsigned long flags;
+
+	if (!ts->disabled)
+		return;
+
+	spin_lock_irqsave(&ts->lock, flags);
+	ts->disabled = 0;
+	enable_irq(ts->spi->irq);
+	spin_unlock_irqrestore(&ts->lock, flags);
+}
+
+#define SHOW(name) static ssize_t \
+name ## _show(struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+	struct ad7877	*ts = dev_get_drvdata(dev); \
+	ssize_t v = ad7877_read_adc(ts->spi, \
+			AD7877_READ_CHAN(name)); \
+	if (v < 0) \
+		return v; \
+	return sprintf(buf, "%u\n", (unsigned) v); \
+} \
+static DEVICE_ATTR(name, S_IRUGO, name ## _show, NULL);
+
+SHOW(aux1)
+SHOW(aux2)
+SHOW(aux3)
+SHOW(bat1)
+SHOW(bat2)
+SHOW(temp1)
+SHOW(temp2)
+
+static ssize_t ad7877_disable_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct ad7877	*ts = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", ts->disabled);
+}
+
+static ssize_t ad7877_disable_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct ad7877 *ts = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	ret = strict_strtoul(buf, 10, &val);
+
+	if (ret)
+		return ret;
+
+	if (val)
+		ad7877_disable(ts);
+	else
+		ad7877_enable(ts);
+
+	return count;
+}
+
+static DEVICE_ATTR(disable, 0664, ad7877_disable_show, ad7877_disable_store);
+
+static ssize_t ad7877_dac_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct ad7877	*ts = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", ts->dac);
+}
+
+static ssize_t ad7877_dac_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct ad7877 *ts = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	ret = strict_strtoul(buf, 10, &val);
+
+	if (ret)
+		return ret;
+
+	ts->dac = val & 0xFF;
+
+	ad7877_write(ts->spi, AD7877_REG_DAC, (ts->dac << 4) | AD7877_DAC_CONF);
+
+	return count;
+}
+
+static DEVICE_ATTR(dac, 0664, ad7877_dac_show, ad7877_dac_store);
+
+static ssize_t ad7877_gpio3_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct ad7877	*ts = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", ts->gpio3);
+}
+
+static ssize_t ad7877_gpio3_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct ad7877 *ts = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	ret = strict_strtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	ts->gpio3 = !!val;
+
+	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA |
+		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
+
+	return count;
+}
+
+static DEVICE_ATTR(gpio3, 0664, ad7877_gpio3_show, ad7877_gpio3_store);
+
+static ssize_t ad7877_gpio4_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct ad7877	*ts = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", ts->gpio4);
+}
+
+static ssize_t ad7877_gpio4_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct ad7877 *ts = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	ret = strict_strtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	ts->gpio4 = !!val;
+
+	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA |
+		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
+
+	return count;
+}
+
+static DEVICE_ATTR(gpio4, 0664, ad7877_gpio4_show, ad7877_gpio4_store);
+
+static struct attribute *ad7877_attributes[] = {
+	&dev_attr_temp1.attr,
+	&dev_attr_temp2.attr,
+	&dev_attr_aux1.attr,
+	&dev_attr_aux2.attr,
+	&dev_attr_bat1.attr,
+	&dev_attr_bat2.attr,
+	&dev_attr_disable.attr,
+	&dev_attr_dac.attr,
+	&dev_attr_gpio4.attr,
+	NULL
+};
+
+static const struct attribute_group ad7877_attr_group = {
+	.attrs = ad7877_attributes,
+};
+
+static void ad7877_setup_ts_def_msg(struct spi_device *spi, struct ad7877 *ts)
+{
+	struct spi_message *m;
+	int i;
+
+	ts->cmd_crtl2 = AD7877_WRITEADD(AD7877_REG_CTRL2) |
+			AD7877_POL(ts->stopacq_polarity) |
+			AD7877_AVG(ts->averaging) | AD7877_PM(1) |
+			AD7877_TMR(ts->pen_down_acc_interval) |
+			AD7877_ACQ(ts->acquisition_time) |
+			AD7877_FCD(ts->first_conversion_delay);
+
+	ad7877_write(spi, AD7877_REG_CTRL2, ts->cmd_crtl2);
+
+	ts->cmd_crtl1 = AD7877_WRITEADD(AD7877_REG_CTRL1) |
+			AD7877_READADD(AD7877_REG_XPLUS-1) |
+			AD7877_MODE_SEQ1 | AD7877_DFR;
+
+	ad7877_write(spi, AD7877_REG_CTRL1, ts->cmd_crtl1);
+
+	ts->cmd_dummy = 0;
+
+	m = &ts->msg;
+
+	spi_message_init(m);
+
+	m->complete = ad7877_callback;
+	m->context = ts;
+
+	ts->xfer[0].tx_buf = &ts->cmd_crtl1;
+	ts->xfer[0].len = 2;
+
+	spi_message_add_tail(&ts->xfer[0], m);
+
+	ts->xfer[1].tx_buf = &ts->cmd_dummy; /* Send ZERO */
+	ts->xfer[1].len = 2;
+
+	spi_message_add_tail(&ts->xfer[1], m);
+
+	for (i = 0; i < 11; i++) {
+		ts->xfer[i + 2].rx_buf = &ts->conversion_data[AD7877_SEQ_YPOS + i];
+		ts->xfer[i + 2].len = 2;
+		spi_message_add_tail(&ts->xfer[i + 2], m);
+	}
+}
+
+static int __devinit ad7877_probe(struct spi_device *spi)
+{
+	struct ad7877			*ts;
+	struct input_dev		*input_dev;
+	struct ad7877_platform_data	*pdata = spi->dev.platform_data;
+	int				err;
+	u16				verify;
+
+	if (!spi->irq) {
+		dev_dbg(&spi->dev, "no IRQ?\n");
+		return -ENODEV;
+	}
+
+	if (!pdata) {
+		dev_dbg(&spi->dev, "no platform data?\n");
+		return -ENODEV;
+	}
+
+	/* don't exceed max specified SPI CLK frequency */
+	if (spi->max_speed_hz > MAX_SPI_FREQ_HZ) {
+		dev_dbg(&spi->dev, "SPI CLK %d Hz?\n",spi->max_speed_hz);
+		return -EINVAL;
+	}
+
+	ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		kfree(ts);
+		return -ENOMEM;
+	}
+
+	dev_set_drvdata(&spi->dev, ts);
+	ts->spi = spi;
+	ts->input = input_dev;
+
+	setup_timer(&ts->timer, ad7877_timer, (unsigned long) ts);
+	spin_lock_init(&ts->lock);
+
+	ts->model = pdata->model ? : 7877;
+	ts->vref_delay_usecs = pdata->vref_delay_usecs ? : 100;
+	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
+	ts->pressure_max = pdata->pressure_max ? : ~0;
+
+	ts->stopacq_polarity = pdata->stopacq_polarity;
+	ts->first_conversion_delay = pdata->first_conversion_delay;
+	ts->acquisition_time = pdata->acquisition_time;
+	ts->averaging = pdata->averaging;
+	ts->pen_down_acc_interval = pdata->pen_down_acc_interval;
+
+	snprintf(ts->phys, sizeof(ts->phys), "%s/inputX", spi->dev.bus_id);
+
+	input_dev->name = "AD7877 Touchscreen";
+	input_dev->phys = ts->phys;
+	input_dev->dev.parent = &spi->dev;
+
+	__set_bit(EV_ABS, input_dev->evbit);
+	__set_bit(ABS_X, input_dev->absbit);
+	__set_bit(ABS_Y, input_dev->absbit);
+	__set_bit(ABS_PRESSURE, input_dev->absbit);
+
+	input_set_abs_params(input_dev, ABS_X,
+			pdata->x_min ? : 0,
+			pdata->x_max ? : MAX_12BIT,
+			0, 0);
+	input_set_abs_params(input_dev, ABS_Y,
+			pdata->y_min ? : 0,
+			pdata->y_max ? : MAX_12BIT,
+			0, 0);
+	input_set_abs_params(input_dev, ABS_PRESSURE,
+			pdata->pressure_min, pdata->pressure_max, 0, 0);
+
+	ad7877_write(spi, AD7877_REG_SEQ1, AD7877_MM_SEQUENCE);
+
+	verify = ad7877_read(spi, AD7877_REG_SEQ1);
+
+	if (verify != AD7877_MM_SEQUENCE){
+		dev_err(&spi->dev, "%s: Failed to probe %s\n", spi->dev.bus_id,
+			 input_dev->name);
+		err = -ENODEV;
+		goto err_free_mem;
+	}
+
+	if (gpio3)
+		ad7877_write(spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_3_CONF);
+
+	ad7877_setup_ts_def_msg(spi, ts);
+
+	/* Request AD7877 /DAV GPIO interrupt */
+
+	err = request_irq(spi->irq, ad7877_irq, IRQF_TRIGGER_FALLING |
+			IRQF_SAMPLE_RANDOM, spi->dev.driver->name, ts);
+	if (err) {
+		dev_dbg(&spi->dev, "irq %d busy?\n", spi->irq);
+		goto err_free_mem;
+	}
+
+	err = sysfs_create_group(&spi->dev.kobj, &ad7877_attr_group);
+
+	if (gpio3)
+		err |= device_create_file(&spi->dev, &dev_attr_gpio3);
+	else
+		err |= device_create_file(&spi->dev, &dev_attr_aux3);
+
+	if (err)
+		goto err_free_irq;
+
+	err = input_register_device(input_dev);
+	if (err)
+		goto err_remove_attr;
+
+	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
+
+	return 0;
+
+err_remove_attr:
+	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
+
+	if (gpio3)
+		device_remove_file(&spi->dev, &dev_attr_gpio3);
+	else
+		device_remove_file(&spi->dev, &dev_attr_aux3);
+err_free_irq:
+	free_irq(spi->irq, ts);
+err_free_mem:
+	input_free_device(input_dev);
+	kfree(ts);
+	dev_set_drvdata(&spi->dev, NULL);
+	return err;
+}
+
+static int __devexit ad7877_remove(struct spi_device *spi)
+{
+	struct ad7877		*ts = dev_get_drvdata(&spi->dev);
+
+	ad7877_disable(ts);
+
+	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
+
+	if (gpio3)
+		device_remove_file(&spi->dev, &dev_attr_gpio3);
+	else
+		device_remove_file(&spi->dev, &dev_attr_aux3);
+
+	free_irq(ts->spi->irq, ts);
+
+	input_unregister_device(ts->input);
+
+	kfree(ts);
+
+	dev_dbg(&spi->dev, "unregistered touchscreen\n");
+	dev_set_drvdata(&spi->dev, NULL);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int ad7877_suspend(struct spi_device *spi, pm_message_t message)
+{
+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
+
+	ad7877_disable(ts);
+
+	return 0;
+}
+
+static int ad7877_resume(struct spi_device *spi)
+{
+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
+
+	ad7877_enable(ts);
+
+	return 0;
+}
+#else
+#define ad7877_suspend NULL
+#define ad7877_resume  NULL
+#endif
+
+static struct spi_driver ad7877_driver = {
+	.driver = {
+		.name	= "ad7877",
+		.bus	= &spi_bus_type,
+		.owner	= THIS_MODULE,
+	},
+	.probe		= ad7877_probe,
+	.remove		= __devexit_p(ad7877_remove),
+	.suspend	= ad7877_suspend,
+	.resume		= ad7877_resume,
+};
+
+static int __init ad7877_init(void)
+{
+	return spi_register_driver(&ad7877_driver);
+}
+module_init(ad7877_init);
+
+static void __exit ad7877_exit(void)
+{
+	spi_unregister_driver(&ad7877_driver);
+}
+module_exit(ad7877_exit);
+
+module_param(gpio3, int, 0);
+MODULE_PARM_DESC(gpio3,
+	"If gpio3 is set to 1 AUX3 acts as GPIO3");
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("AD7877 touchscreen Driver");
+MODULE_LICENSE("GPL");
-- 
1.5.6


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] Input/Touchscreen Driver: add support AD7877 touchscreen driver (try #5)
  2008-10-16 17:56 [PATCH 1/1] Input/Touchscreen Driver: add support AD7877 touchscreen driver (try #5) Bryan Wu
@ 2009-03-08  6:38 ` Dmitry Torokhov
  2009-03-09 13:20     ` Hennerich, Michael
  2009-03-09 14:45     ` Hennerich, Michael
  0 siblings, 2 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2009-03-08  6:38 UTC (permalink / raw)
  To: Bryan Wu, Michael Hennerich; +Cc: akpm, linux-input, linux-kernel

Hi Bryan, Michael,

On Fri, Oct 17, 2008 at 01:56:08AM +0800, Bryan Wu wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> [try #5]
>  - Fix indention
>  - Lock spi_async()
>  - Remove useless header comments
>  - Use strict_strtoul
>  - Use EDGE triggered interrupts ?\226?\128?\147 this simplifies some of the IRQ handling.
>  - Fix error cleanup path
>  - Remove duplicated code
>  - SPI access functions parameter use struct spi_device
> 

I looked at the latest version of the driver in Andrew's queue and I
think it needs the following changes:

- you can't have parts of bitfield updated from IRQ context and part from
  process context unless every field is protected by the same spinlock.

- You need more full mutual exclusion between enable and disable so you
  don't inadvertingly kill the timer if you disable and enable from
  different processes.

- I think you need serialization in various store() methods so that
  consistent values are written into chip's registers.

- There were coule of inverted logic errors in disabling chip code.

- Kay is trying to get rid of bus_id in devices, you need to use
  dev_name() instead.

Could you please try the patch below and let me know if the driver still
works so I can commit it. I also did some formatting changes, I hope you
don't mind. Thanks!

-- 
Dmitry

Input: ad7877 fixups

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/touchscreen/ad7877.c |  226 ++++++++++++++++--------------------
 1 files changed, 99 insertions(+), 127 deletions(-)


diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index 6615bcd..6702364 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -1,7 +1,7 @@
 /*
  * File:        drivers/input/touchscreen/ad7877.c
  *
- * Based on: 	ads7846.c
+ * Based on:	ads7846.c
  *
  *		Copyright (C) 2006-2008 Michael Hennerich, Analog Devices Inc.
  *
@@ -185,21 +185,24 @@ struct ad7877 {
 	u8			averaging;
 	u8			pen_down_acc_interval;
 
-	u16 			conversion_data[AD7877_NR_SENSE];
+	u16			conversion_data[AD7877_NR_SENSE];
 
 	struct spi_transfer	xfer[AD7877_NR_SENSE + 2];
 	struct spi_message	msg;
 
+	struct mutex		mutex;
+	unsigned		disabled:1;	/* P: mutex */
+	unsigned		gpio3:1;	/* P: mutex */
+	unsigned		gpio4:1;	/* P: mutex */
+
 	spinlock_t		lock;
 	struct timer_list	timer;		/* P: lock */
-	unsigned		pendown:1;	/* P: lock */
 	unsigned		pending:1;	/* P: lock */
-	unsigned		disabled:1;
-	unsigned		gpio3:1;
-	unsigned		gpio4:1;
 };
 
 static int gpio3;
+module_param(gpio3, int, 0);
+MODULE_PARM_DESC(gpio3, "If gpio3 is set to 1 AUX3 acts as GPIO3");
 
 /*
  * ad7877_read/write are only used for initial setup and for sysfs controls.
@@ -208,9 +211,10 @@ static int gpio3;
 
 static int ad7877_read(struct spi_device *spi, u16 reg)
 {
-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
-	int			status, ret;
+	struct ser_req *req;
+	int status, ret;
 
+	req = kzalloc(sizeof *req, GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
@@ -228,11 +232,8 @@ static int ad7877_read(struct spi_device *spi, u16 reg)
 	spi_message_add_tail(&req->xfer[1], &req->msg);
 
 	status = spi_sync(spi, &req->msg);
+	ret = status ? : req->sample;
 
-	if (status == 0)
-		status = req->msg.status;
-
-	ret = status ? status : req->sample;
 	kfree(req);
 
 	return ret;
@@ -240,9 +241,10 @@ static int ad7877_read(struct spi_device *spi, u16 reg)
 
 static int ad7877_write(struct spi_device *spi, u16 reg, u16 val)
 {
-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
-	int			status;
+	struct ser_req *req;
+	int status;
 
+	req = kzalloc(sizeof *req, GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
@@ -256,9 +258,6 @@ static int ad7877_write(struct spi_device *spi, u16 reg, u16 val)
 
 	status = spi_sync(spi, &req->msg);
 
-	if (status == 0)
-		status = req->msg.status;
-
 	kfree(req);
 
 	return status;
@@ -266,12 +265,13 @@ static int ad7877_write(struct spi_device *spi, u16 reg, u16 val)
 
 static int ad7877_read_adc(struct spi_device *spi, unsigned command)
 {
-	struct ad7877 		*ts = dev_get_drvdata(&spi->dev);
-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
-	int			status;
-	int			sample;
-	int			i;
+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
+	struct ser_req *req;
+	int status;
+	int sample;
+	int i;
 
+	req = kzalloc(sizeof *req, GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
@@ -314,21 +314,18 @@ static int ad7877_read_adc(struct spi_device *spi, unsigned command)
 		spi_message_add_tail(&req->xfer[i], &req->msg);
 
 	status = spi_sync(spi, &req->msg);
-
-	if (status == 0)
-		status = req->msg.status;
-
 	sample = req->sample;
 
 	kfree(req);
-	return status ? status : sample;
+
+	return status ? : sample;
 }
 
 static void ad7877_rx(struct ad7877 *ts)
 {
-	struct input_dev	*input_dev = ts->input;
-	unsigned		Rt;
-	u16			x, y, z1, z2;
+	struct input_dev *input_dev = ts->input;
+	unsigned Rt;
+	u16 x, y, z1, z2;
 
 	x = ts->conversion_data[AD7877_SEQ_XPOS] & MAX_12BIT;
 	y = ts->conversion_data[AD7877_SEQ_YPOS] & MAX_12BIT;
@@ -350,10 +347,7 @@ static void ad7877_rx(struct ad7877 *ts)
 		Rt = (z2 - z1) * x * ts->x_plate_ohms;
 		Rt /= z1;
 		Rt = (Rt + 2047) >> 12;
-	} else
-		Rt = 0;
 
-	if (Rt) {
 		input_report_abs(input_dev, ABS_X, x);
 		input_report_abs(input_dev, ABS_Y, y);
 		input_report_abs(input_dev, ABS_PRESSURE, Rt);
@@ -371,7 +365,7 @@ static inline void ad7877_ts_event_release(struct ad7877 *ts)
 
 static void ad7877_timer(unsigned long handle)
 {
-	struct ad7877	*ts = (void *)handle;
+	struct ad7877 *ts = (void *)handle;
 
 	ad7877_ts_event_release(ts);
 }
@@ -382,78 +376,72 @@ static irqreturn_t ad7877_irq(int irq, void *handle)
 	unsigned long flags;
 	int status;
 
-	/* The repeated conversion sequencer controlled by TMR kicked off too fast.
-	 * We ignore the last and process the sample sequence currently in the queue
-	 * It can't be older than 9.4ms, and we need to avoid that ts->msg
-	 * doesn't get issued twice while in work.
+	/*
+	 * The repeated conversion sequencer controlled by TMR kicked off
+	 * too fast. We ignore the last and process the sample sequence
+	 * currently in the queue. It can't be older than 9.4ms, and we
+	 * need to avoid that ts->msg doesn't get issued twice while in work.
 	 */
 
 	spin_lock_irqsave(&ts->lock, flags);
-	if (ts->pending) {
-		spin_unlock_irqrestore(&ts->lock, flags);
-		return IRQ_HANDLED;
+	if (!ts->pending) {
+		ts->pending = 1;
+
+		status = spi_async(ts->spi, &ts->msg);
+		if (status)
+			dev_err(&ts->spi->dev, "spi_sync --> %d\n", status);
 	}
-	ts->pending = 1;
 	spin_unlock_irqrestore(&ts->lock, flags);
 
-	status = spi_async(ts->spi, &ts->msg);
-
-	if (status)
-		dev_err(&ts->spi->dev, "spi_sync --> %d\n", status);
-
 	return IRQ_HANDLED;
 }
 
 static void ad7877_callback(void *_ts)
 {
 	struct ad7877 *ts = _ts;
-	unsigned long flags;
 
-	ad7877_rx(ts);
+	spin_lock_irq(&ts->lock);
 
-	spin_lock_irqsave(&ts->lock, flags);
+	ad7877_rx(ts);
 	ts->pending = 0;
 	mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
-	spin_unlock_irqrestore(&ts->lock, flags);
+
+	spin_unlock_irq(&ts->lock);
 }
 
 static void ad7877_disable(struct ad7877 *ts)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&ts->lock, flags);
-	if (ts->disabled) {
-		spin_unlock_irqrestore(&ts->lock, flags);
-		return;
-	}
+	mutex_lock(&ts->mutex);
 
-	ts->disabled = 1;
-	disable_irq(ts->spi->irq);
-	spin_unlock_irqrestore(&ts->lock, flags);
+	if (!ts->disabled) {
+		ts->disabled = 1;
+		disable_irq(ts->spi->irq);
 
-	while (ts->pending) /* Wait for spi_async callback */
-		msleep(1);
+		/* Wait for spi_async callback */
+		while (ts->pending)
+			msleep(1);
 
-	if (del_timer_sync(&ts->timer))
-		ad7877_ts_event_release(ts);
+		if (del_timer_sync(&ts->timer))
+			ad7877_ts_event_release(ts);
+	}
 
 	/* we know the chip's in lowpower mode since we always
 	 * leave it that way after every request
 	 */
+
+	mutex_unlock(&ts->mutex);
 }
 
 static void ad7877_enable(struct ad7877 *ts)
 {
-	unsigned long flags;
+	mutex_lock(&ts->mutex);
 
-	spin_lock_irqsave(&ts->lock, flags);
 	if (ts->disabled) {
-		spin_unlock_irqrestore(&ts->lock, flags);
-		return;
+		ts->disabled = 0;
+		enable_irq(ts->spi->irq);
 	}
-	ts->disabled = 0;
-	enable_irq(ts->spi->irq);
-	spin_unlock_irqrestore(&ts->lock, flags);
+
+	mutex_unlock(&ts->mutex);
 }
 
 #define SHOW(name) static ssize_t \
@@ -490,12 +478,11 @@ static ssize_t ad7877_disable_store(struct device *dev,
 {
 	struct ad7877 *ts = dev_get_drvdata(dev);
 	unsigned long val;
-	int ret;
+	int error;
 
-	ret = strict_strtoul(buf, 10, &val);
-
-	if (ret)
-		return ret;
+	error = strict_strtoul(buf, 10, &val);
+	if (error)
+		return error;
 
 	if (val)
 		ad7877_disable(ts);
@@ -521,16 +508,16 @@ static ssize_t ad7877_dac_store(struct device *dev,
 {
 	struct ad7877 *ts = dev_get_drvdata(dev);
 	unsigned long val;
-	int ret;
-
-	ret = strict_strtoul(buf, 10, &val);
+	int error;
 
-	if (ret)
-		return ret;
+	error = strict_strtoul(buf, 10, &val);
+	if (error)
+		return error;
 
+	mutex_lock(&ts->mutex);
 	ts->dac = val & 0xFF;
-
 	ad7877_write(ts->spi, AD7877_REG_DAC, (ts->dac << 4) | AD7877_DAC_CONF);
+	mutex_unlock(&ts->mutex);
 
 	return count;
 }
@@ -551,16 +538,17 @@ static ssize_t ad7877_gpio3_store(struct device *dev,
 {
 	struct ad7877 *ts = dev_get_drvdata(dev);
 	unsigned long val;
-	int ret;
+	int error;
 
-	ret = strict_strtoul(buf, 10, &val);
-	if (ret)
-		return ret;
+	error = strict_strtoul(buf, 10, &val);
+	if (error)
+		return error;
 
+	mutex_lock(&ts->mutex);
 	ts->gpio3 = !!val;
-
 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA |
 		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
+	mutex_unlock(&ts->mutex);
 
 	return count;
 }
@@ -581,16 +569,17 @@ static ssize_t ad7877_gpio4_store(struct device *dev,
 {
 	struct ad7877 *ts = dev_get_drvdata(dev);
 	unsigned long val;
-	int ret;
+	int error;
 
-	ret = strict_strtoul(buf, 10, &val);
-	if (ret)
-		return ret;
+	error = strict_strtoul(buf, 10, &val);
+	if (error)
+		return error;
 
+	mutex_lock(&ts->mutex);
 	ts->gpio4 = !!val;
-
 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA |
-		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
+		     (ts->gpio4 << 4) | (ts->gpio3 << 5));
+	mutex_unlock(&ts->mutex);
 
 	return count;
 }
@@ -685,14 +674,10 @@ static int __devinit ad7877_probe(struct spi_device *spi)
 	}
 
 	ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
-	if (!ts)
-		return -ENOMEM;
-
-
 	input_dev = input_allocate_device();
-	if (!input_dev) {
-		kfree(ts);
-		return -ENOMEM;
+	if (!ts || !input_dev) {
+		err = -ENOMEM;
+		goto err_free_mem;
 	}
 
 	dev_set_drvdata(&spi->dev, ts);
@@ -700,6 +685,7 @@ static int __devinit ad7877_probe(struct spi_device *spi)
 	ts->input = input_dev;
 
 	setup_timer(&ts->timer, ad7877_timer, (unsigned long) ts);
+	mutex_init(&ts->mutex);
 	spin_lock_init(&ts->lock);
 
 	ts->model = pdata->model ? : 7877;
@@ -713,7 +699,7 @@ static int __devinit ad7877_probe(struct spi_device *spi)
 	ts->averaging = pdata->averaging;
 	ts->pen_down_acc_interval = pdata->pen_down_acc_interval;
 
-	snprintf(ts->phys, sizeof(ts->phys), "%s/inputX", spi->dev.bus_id);
+	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi->dev));
 
 	input_dev->name = "AD7877 Touchscreen";
 	input_dev->phys = ts->phys;
@@ -761,30 +747,25 @@ static int __devinit ad7877_probe(struct spi_device *spi)
 	}
 
 	err = sysfs_create_group(&spi->dev.kobj, &ad7877_attr_group);
-
-	if (gpio3)
-		err |= device_create_file(&spi->dev, &dev_attr_gpio3);
-	else
-		err |= device_create_file(&spi->dev, &dev_attr_aux3);
-
 	if (err)
 		goto err_free_irq;
 
+	err = device_create_file(&spi->dev,
+				 gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
+	if (err)
+		goto err_remove_attr_group;
+
 	err = input_register_device(input_dev);
 	if (err)
 		goto err_remove_attr;
 
-	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
-
 	return 0;
 
 err_remove_attr:
+	device_remove_file(&spi->dev,
+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
+err_remove_attr_group:
 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
-
-	if (gpio3)
-		device_remove_file(&spi->dev, &dev_attr_gpio3);
-	else
-		device_remove_file(&spi->dev, &dev_attr_aux3);
 err_free_irq:
 	free_irq(spi->irq, ts);
 err_free_mem:
@@ -798,19 +779,14 @@ static int __devexit ad7877_remove(struct spi_device *spi)
 {
 	struct ad7877		*ts = dev_get_drvdata(&spi->dev);
 
-	ad7877_disable(ts);
-
 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
+	device_remove_file(&spi->dev,
+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
 
-	if (gpio3)
-		device_remove_file(&spi->dev, &dev_attr_gpio3);
-	else
-		device_remove_file(&spi->dev, &dev_attr_aux3);
-
+	ad7877_disable(ts);
 	free_irq(ts->spi->irq, ts);
 
 	input_unregister_device(ts->input);
-
 	kfree(ts);
 
 	dev_dbg(&spi->dev, "unregistered touchscreen\n");
@@ -866,10 +842,6 @@ static void __exit ad7877_exit(void)
 }
 module_exit(ad7877_exit);
 
-module_param(gpio3, int, 0);
-MODULE_PARM_DESC(gpio3,
-	"If gpio3 is set to 1 AUX3 acts as GPIO3");
-
 MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
 MODULE_DESCRIPTION("AD7877 touchscreen Driver");
 MODULE_LICENSE("GPL");

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* RE: [PATCH 1/1] Input/Touchscreen Driver: add support AD7877touchscreen driver (try #5)
  2009-03-08  6:38 ` Dmitry Torokhov
@ 2009-03-09 13:20     ` Hennerich, Michael
  2009-03-09 14:45     ` Hennerich, Michael
  1 sibling, 0 replies; 6+ messages in thread
From: Hennerich, Michael @ 2009-03-09 13:20 UTC (permalink / raw)
  To: Dmitry Torokhov, Bryan Wu; +Cc: akpm, linux-input, linux-kernel

>From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>Subject: Re: [PATCH 1/1] Input/Touchscreen Driver: add support
>AD7877touchscreen driver (try #5)
>
>Hi Bryan, Michael,
>
>On Fri, Oct 17, 2008 at 01:56:08AM +0800, Bryan Wu wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> [try #5]
>>  - Fix indention
>>  - Lock spi_async()
>>  - Remove useless header comments
>>  - Use strict_strtoul
>>  - Use EDGE triggered interrupts ?\226?\128?\147 this simplifies some
of
>the IRQ handling.
>>  - Fix error cleanup path
>>  - Remove duplicated code
>>  - SPI access functions parameter use struct spi_device
>>
>
>I looked at the latest version of the driver in Andrew's queue and I
>think it needs the following changes:
>
>- you can't have parts of bitfield updated from IRQ context and part
from
>  process context unless every field is protected by the same spinlock.
>
>- You need more full mutual exclusion between enable and disable so you
>  don't inadvertingly kill the timer if you disable and enable from
>  different processes.
>
>- I think you need serialization in various store() methods so that
>  consistent values are written into chip's registers.
>
>- There were coule of inverted logic errors in disabling chip code.
>
>- Kay is trying to get rid of bus_id in devices, you need to use
>  dev_name() instead.

Andrew folded a patch addressing this issue, submitted by
[randy.dunlap@oracle.com: touchscreen/ad787x: don't use bus_id] into his

[patch 03/22] input: AD7879 Touchscreen driver patch.

You must have missed this one.

>
>Could you please try the patch below and let me know if the driver
still
>works so I can commit it. I also did some formatting changes, I hope
you
>don't mind. Thanks!

Thanks a lot for your great efforts!

But your patch doesn't apply on Andrews folded patch send to you last
Wednesday. I also tried it without the "don't use bus_id" patch.

Can you please send me your modified files?

Best regards,
Michael


>
>--
>Dmitry
>
>Input: ad7877 fixups
>
>Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>---
>
> drivers/input/touchscreen/ad7877.c |  226
++++++++++++++++----------------
>----
> 1 files changed, 99 insertions(+), 127 deletions(-)
>
>
>diff --git a/drivers/input/touchscreen/ad7877.c
>b/drivers/input/touchscreen/ad7877.c
>index 6615bcd..6702364 100644
>--- a/drivers/input/touchscreen/ad7877.c
>+++ b/drivers/input/touchscreen/ad7877.c
>@@ -1,7 +1,7 @@
> /*
>  * File:        drivers/input/touchscreen/ad7877.c
>  *
>- * Based on: 	ads7846.c
>+ * Based on:	ads7846.c
>  *
>  *		Copyright (C) 2006-2008 Michael Hennerich, Analog
Devices Inc.
>  *
>@@ -185,21 +185,24 @@ struct ad7877 {
> 	u8			averaging;
> 	u8			pen_down_acc_interval;
>
>-	u16 			conversion_data[AD7877_NR_SENSE];
>+	u16			conversion_data[AD7877_NR_SENSE];
>
> 	struct spi_transfer	xfer[AD7877_NR_SENSE + 2];
> 	struct spi_message	msg;
>
>+	struct mutex		mutex;
>+	unsigned		disabled:1;	/* P: mutex */
>+	unsigned		gpio3:1;	/* P: mutex */
>+	unsigned		gpio4:1;	/* P: mutex */
>+
> 	spinlock_t		lock;
> 	struct timer_list	timer;		/* P: lock */
>-	unsigned		pendown:1;	/* P: lock */
> 	unsigned		pending:1;	/* P: lock */
>-	unsigned		disabled:1;
>-	unsigned		gpio3:1;
>-	unsigned		gpio4:1;
> };
>
> static int gpio3;
>+module_param(gpio3, int, 0);
>+MODULE_PARM_DESC(gpio3, "If gpio3 is set to 1 AUX3 acts as GPIO3");
>
> /*
>  * ad7877_read/write are only used for initial setup and for sysfs
>controls.
>@@ -208,9 +211,10 @@ static int gpio3;
>
> static int ad7877_read(struct spi_device *spi, u16 reg)
> {
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status, ret;
>+	struct ser_req *req;
>+	int status, ret;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -228,11 +232,8 @@ static int ad7877_read(struct spi_device *spi, u16
>reg)
> 	spi_message_add_tail(&req->xfer[1], &req->msg);
>
> 	status = spi_sync(spi, &req->msg);
>+	ret = status ? : req->sample;
>
>-	if (status == 0)
>-		status = req->msg.status;
>-
>-	ret = status ? status : req->sample;
> 	kfree(req);
>
> 	return ret;
>@@ -240,9 +241,10 @@ static int ad7877_read(struct spi_device *spi, u16
>reg)
>
> static int ad7877_write(struct spi_device *spi, u16 reg, u16 val)
> {
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status;
>+	struct ser_req *req;
>+	int status;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -256,9 +258,6 @@ static int ad7877_write(struct spi_device *spi, u16
>reg, u16 val)
>
> 	status = spi_sync(spi, &req->msg);
>
>-	if (status == 0)
>-		status = req->msg.status;
>-
> 	kfree(req);
>
> 	return status;
>@@ -266,12 +265,13 @@ static int ad7877_write(struct spi_device *spi,
u16
>reg, u16 val)
>
> static int ad7877_read_adc(struct spi_device *spi, unsigned command)
> {
>-	struct ad7877 		*ts = dev_get_drvdata(&spi->dev);
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status;
>-	int			sample;
>-	int			i;
>+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
>+	struct ser_req *req;
>+	int status;
>+	int sample;
>+	int i;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -314,21 +314,18 @@ static int ad7877_read_adc(struct spi_device
*spi,
>unsigned command)
> 		spi_message_add_tail(&req->xfer[i], &req->msg);
>
> 	status = spi_sync(spi, &req->msg);
>-
>-	if (status == 0)
>-		status = req->msg.status;
>-
> 	sample = req->sample;
>
> 	kfree(req);
>-	return status ? status : sample;
>+
>+	return status ? : sample;
> }
>
> static void ad7877_rx(struct ad7877 *ts)
> {
>-	struct input_dev	*input_dev = ts->input;
>-	unsigned		Rt;
>-	u16			x, y, z1, z2;
>+	struct input_dev *input_dev = ts->input;
>+	unsigned Rt;
>+	u16 x, y, z1, z2;
>
> 	x = ts->conversion_data[AD7877_SEQ_XPOS] & MAX_12BIT;
> 	y = ts->conversion_data[AD7877_SEQ_YPOS] & MAX_12BIT;
>@@ -350,10 +347,7 @@ static void ad7877_rx(struct ad7877 *ts)
> 		Rt = (z2 - z1) * x * ts->x_plate_ohms;
> 		Rt /= z1;
> 		Rt = (Rt + 2047) >> 12;
>-	} else
>-		Rt = 0;
>
>-	if (Rt) {
> 		input_report_abs(input_dev, ABS_X, x);
> 		input_report_abs(input_dev, ABS_Y, y);
> 		input_report_abs(input_dev, ABS_PRESSURE, Rt);
>@@ -371,7 +365,7 @@ static inline void ad7877_ts_event_release(struct
>ad7877 *ts)
>
> static void ad7877_timer(unsigned long handle)
> {
>-	struct ad7877	*ts = (void *)handle;
>+	struct ad7877 *ts = (void *)handle;
>
> 	ad7877_ts_event_release(ts);
> }
>@@ -382,78 +376,72 @@ static irqreturn_t ad7877_irq(int irq, void
*handle)
> 	unsigned long flags;
> 	int status;
>
>-	/* The repeated conversion sequencer controlled by TMR kicked
off too
>fast.
>-	 * We ignore the last and process the sample sequence currently
in
>the queue
>-	 * It can't be older than 9.4ms, and we need to avoid that
ts->msg
>-	 * doesn't get issued twice while in work.
>+	/*
>+	 * The repeated conversion sequencer controlled by TMR kicked
off
>+	 * too fast. We ignore the last and process the sample sequence
>+	 * currently in the queue. It can't be older than 9.4ms, and we
>+	 * need to avoid that ts->msg doesn't get issued twice while in
work.
> 	 */
>
> 	spin_lock_irqsave(&ts->lock, flags);
>-	if (ts->pending) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return IRQ_HANDLED;
>+	if (!ts->pending) {
>+		ts->pending = 1;
>+
>+		status = spi_async(ts->spi, &ts->msg);
>+		if (status)
>+			dev_err(&ts->spi->dev, "spi_sync --> %d\n",
status);
> 	}
>-	ts->pending = 1;
> 	spin_unlock_irqrestore(&ts->lock, flags);
>
>-	status = spi_async(ts->spi, &ts->msg);
>-
>-	if (status)
>-		dev_err(&ts->spi->dev, "spi_sync --> %d\n", status);
>-
> 	return IRQ_HANDLED;
> }
>
> static void ad7877_callback(void *_ts)
> {
> 	struct ad7877 *ts = _ts;
>-	unsigned long flags;
>
>-	ad7877_rx(ts);
>+	spin_lock_irq(&ts->lock);
>
>-	spin_lock_irqsave(&ts->lock, flags);
>+	ad7877_rx(ts);
> 	ts->pending = 0;
> 	mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+
>+	spin_unlock_irq(&ts->lock);
> }
>
> static void ad7877_disable(struct ad7877 *ts)
> {
>-	unsigned long flags;
>-
>-	spin_lock_irqsave(&ts->lock, flags);
>-	if (ts->disabled) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return;
>-	}
>+	mutex_lock(&ts->mutex);
>
>-	ts->disabled = 1;
>-	disable_irq(ts->spi->irq);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+	if (!ts->disabled) {
>+		ts->disabled = 1;
>+		disable_irq(ts->spi->irq);
>
>-	while (ts->pending) /* Wait for spi_async callback */
>-		msleep(1);
>+		/* Wait for spi_async callback */
>+		while (ts->pending)
>+			msleep(1);
>
>-	if (del_timer_sync(&ts->timer))
>-		ad7877_ts_event_release(ts);
>+		if (del_timer_sync(&ts->timer))
>+			ad7877_ts_event_release(ts);
>+	}
>
> 	/* we know the chip's in lowpower mode since we always
> 	 * leave it that way after every request
> 	 */
>+
>+	mutex_unlock(&ts->mutex);
> }
>
> static void ad7877_enable(struct ad7877 *ts)
> {
>-	unsigned long flags;
>+	mutex_lock(&ts->mutex);
>
>-	spin_lock_irqsave(&ts->lock, flags);
> 	if (ts->disabled) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return;
>+		ts->disabled = 0;
>+		enable_irq(ts->spi->irq);
> 	}
>-	ts->disabled = 0;
>-	enable_irq(ts->spi->irq);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+
>+	mutex_unlock(&ts->mutex);
> }
>
> #define SHOW(name) static ssize_t \
>@@ -490,12 +478,11 @@ static ssize_t ad7877_disable_store(struct device
>*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
> 	if (val)
> 		ad7877_disable(ts);
>@@ -521,16 +508,16 @@ static ssize_t ad7877_dac_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>-
>-	ret = strict_strtoul(buf, 10, &val);
>+	int error;
>
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->dac = val & 0xFF;
>-
> 	ad7877_write(ts->spi, AD7877_REG_DAC, (ts->dac << 4) |
>AD7877_DAC_CONF);
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -551,16 +538,17 @@ static ssize_t ad7877_gpio3_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->gpio3 = !!val;
>-
> 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA
|
> 		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -581,16 +569,17 @@ static ssize_t ad7877_gpio4_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->gpio4 = !!val;
>-
> 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA
|
>-		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+		     (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -685,14 +674,10 @@ static int __devinit ad7877_probe(struct
spi_device
>*spi)
> 	}
>
> 	ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
>-	if (!ts)
>-		return -ENOMEM;
>-
>-
> 	input_dev = input_allocate_device();
>-	if (!input_dev) {
>-		kfree(ts);
>-		return -ENOMEM;
>+	if (!ts || !input_dev) {
>+		err = -ENOMEM;
>+		goto err_free_mem;
> 	}
>
> 	dev_set_drvdata(&spi->dev, ts);
>@@ -700,6 +685,7 @@ static int __devinit ad7877_probe(struct spi_device
>*spi)
> 	ts->input = input_dev;
>
> 	setup_timer(&ts->timer, ad7877_timer, (unsigned long) ts);
>+	mutex_init(&ts->mutex);
> 	spin_lock_init(&ts->lock);
>
> 	ts->model = pdata->model ? : 7877;
>@@ -713,7 +699,7 @@ static int __devinit ad7877_probe(struct spi_device
>*spi)
> 	ts->averaging = pdata->averaging;
> 	ts->pen_down_acc_interval = pdata->pen_down_acc_interval;
>
>-	snprintf(ts->phys, sizeof(ts->phys), "%s/inputX",
spi->dev.bus_id);
>+	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi-
>>dev));
>
> 	input_dev->name = "AD7877 Touchscreen";
> 	input_dev->phys = ts->phys;
>@@ -761,30 +747,25 @@ static int __devinit ad7877_probe(struct
spi_device
>*spi)
> 	}
>
> 	err = sysfs_create_group(&spi->dev.kobj, &ad7877_attr_group);
>-
>-	if (gpio3)
>-		err |= device_create_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		err |= device_create_file(&spi->dev, &dev_attr_aux3);
>-
> 	if (err)
> 		goto err_free_irq;
>
>+	err = device_create_file(&spi->dev,
>+				 gpio3 ? &dev_attr_gpio3 :
&dev_attr_aux3);
>+	if (err)
>+		goto err_remove_attr_group;
>+
> 	err = input_register_device(input_dev);
> 	if (err)
> 		goto err_remove_attr;
>
>-	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
>-
> 	return 0;
>
> err_remove_attr:
>+	device_remove_file(&spi->dev,
>+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
>+err_remove_attr_group:
> 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
>-
>-	if (gpio3)
>-		device_remove_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		device_remove_file(&spi->dev, &dev_attr_aux3);
> err_free_irq:
> 	free_irq(spi->irq, ts);
> err_free_mem:
>@@ -798,19 +779,14 @@ static int __devexit ad7877_remove(struct
spi_device
>*spi)
> {
> 	struct ad7877		*ts = dev_get_drvdata(&spi->dev);
>
>-	ad7877_disable(ts);
>-
> 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
>+	device_remove_file(&spi->dev,
>+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
>
>-	if (gpio3)
>-		device_remove_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		device_remove_file(&spi->dev, &dev_attr_aux3);
>-
>+	ad7877_disable(ts);
> 	free_irq(ts->spi->irq, ts);
>
> 	input_unregister_device(ts->input);
>-
> 	kfree(ts);
>
> 	dev_dbg(&spi->dev, "unregistered touchscreen\n");
>@@ -866,10 +842,6 @@ static void __exit ad7877_exit(void)
> }
> module_exit(ad7877_exit);
>
>-module_param(gpio3, int, 0);
>-MODULE_PARM_DESC(gpio3,
>-	"If gpio3 is set to 1 AUX3 acts as GPIO3");
>-
> MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> MODULE_DESCRIPTION("AD7877 touchscreen Driver");
> MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH 1/1] Input/Touchscreen Driver: add support AD7877touchscreen driver (try #5)
@ 2009-03-09 13:20     ` Hennerich, Michael
  0 siblings, 0 replies; 6+ messages in thread
From: Hennerich, Michael @ 2009-03-09 13:20 UTC (permalink / raw)
  To: Dmitry Torokhov, Bryan Wu; +Cc: akpm, linux-input, linux-kernel

>From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>Subject: Re: [PATCH 1/1] Input/Touchscreen Driver: add support
>AD7877touchscreen driver (try #5)
>
>Hi Bryan, Michael,
>
>On Fri, Oct 17, 2008 at 01:56:08AM +0800, Bryan Wu wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> [try #5]
>>  - Fix indention
>>  - Lock spi_async()
>>  - Remove useless header comments
>>  - Use strict_strtoul
>>  - Use EDGE triggered interrupts ?\226?\128?\147 this simplifies some
of
>the IRQ handling.
>>  - Fix error cleanup path
>>  - Remove duplicated code
>>  - SPI access functions parameter use struct spi_device
>>
>
>I looked at the latest version of the driver in Andrew's queue and I
>think it needs the following changes:
>
>- you can't have parts of bitfield updated from IRQ context and part
from
>  process context unless every field is protected by the same spinlock.
>
>- You need more full mutual exclusion between enable and disable so you
>  don't inadvertingly kill the timer if you disable and enable from
>  different processes.
>
>- I think you need serialization in various store() methods so that
>  consistent values are written into chip's registers.
>
>- There were coule of inverted logic errors in disabling chip code.
>
>- Kay is trying to get rid of bus_id in devices, you need to use
>  dev_name() instead.

Andrew folded a patch addressing this issue, submitted by
[randy.dunlap@oracle.com: touchscreen/ad787x: don't use bus_id] into his

[patch 03/22] input: AD7879 Touchscreen driver patch.

You must have missed this one.

>
>Could you please try the patch below and let me know if the driver
still
>works so I can commit it. I also did some formatting changes, I hope
you
>don't mind. Thanks!

Thanks a lot for your great efforts!

But your patch doesn't apply on Andrews folded patch send to you last
Wednesday. I also tried it without the "don't use bus_id" patch.

Can you please send me your modified files?

Best regards,
Michael


>
>--
>Dmitry
>
>Input: ad7877 fixups
>
>Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>---
>
> drivers/input/touchscreen/ad7877.c |  226
++++++++++++++++----------------
>----
> 1 files changed, 99 insertions(+), 127 deletions(-)
>
>
>diff --git a/drivers/input/touchscreen/ad7877.c
>b/drivers/input/touchscreen/ad7877.c
>index 6615bcd..6702364 100644
>--- a/drivers/input/touchscreen/ad7877.c
>+++ b/drivers/input/touchscreen/ad7877.c
>@@ -1,7 +1,7 @@
> /*
>  * File:        drivers/input/touchscreen/ad7877.c
>  *
>- * Based on: 	ads7846.c
>+ * Based on:	ads7846.c
>  *
>  *		Copyright (C) 2006-2008 Michael Hennerich, Analog
Devices Inc.
>  *
>@@ -185,21 +185,24 @@ struct ad7877 {
> 	u8			averaging;
> 	u8			pen_down_acc_interval;
>
>-	u16 			conversion_data[AD7877_NR_SENSE];
>+	u16			conversion_data[AD7877_NR_SENSE];
>
> 	struct spi_transfer	xfer[AD7877_NR_SENSE + 2];
> 	struct spi_message	msg;
>
>+	struct mutex		mutex;
>+	unsigned		disabled:1;	/* P: mutex */
>+	unsigned		gpio3:1;	/* P: mutex */
>+	unsigned		gpio4:1;	/* P: mutex */
>+
> 	spinlock_t		lock;
> 	struct timer_list	timer;		/* P: lock */
>-	unsigned		pendown:1;	/* P: lock */
> 	unsigned		pending:1;	/* P: lock */
>-	unsigned		disabled:1;
>-	unsigned		gpio3:1;
>-	unsigned		gpio4:1;
> };
>
> static int gpio3;
>+module_param(gpio3, int, 0);
>+MODULE_PARM_DESC(gpio3, "If gpio3 is set to 1 AUX3 acts as GPIO3");
>
> /*
>  * ad7877_read/write are only used for initial setup and for sysfs
>controls.
>@@ -208,9 +211,10 @@ static int gpio3;
>
> static int ad7877_read(struct spi_device *spi, u16 reg)
> {
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status, ret;
>+	struct ser_req *req;
>+	int status, ret;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -228,11 +232,8 @@ static int ad7877_read(struct spi_device *spi, u16
>reg)
> 	spi_message_add_tail(&req->xfer[1], &req->msg);
>
> 	status = spi_sync(spi, &req->msg);
>+	ret = status ? : req->sample;
>
>-	if (status == 0)
>-		status = req->msg.status;
>-
>-	ret = status ? status : req->sample;
> 	kfree(req);
>
> 	return ret;
>@@ -240,9 +241,10 @@ static int ad7877_read(struct spi_device *spi, u16
>reg)
>
> static int ad7877_write(struct spi_device *spi, u16 reg, u16 val)
> {
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status;
>+	struct ser_req *req;
>+	int status;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -256,9 +258,6 @@ static int ad7877_write(struct spi_device *spi, u16
>reg, u16 val)
>
> 	status = spi_sync(spi, &req->msg);
>
>-	if (status == 0)
>-		status = req->msg.status;
>-
> 	kfree(req);
>
> 	return status;
>@@ -266,12 +265,13 @@ static int ad7877_write(struct spi_device *spi,
u16
>reg, u16 val)
>
> static int ad7877_read_adc(struct spi_device *spi, unsigned command)
> {
>-	struct ad7877 		*ts = dev_get_drvdata(&spi->dev);
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status;
>-	int			sample;
>-	int			i;
>+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
>+	struct ser_req *req;
>+	int status;
>+	int sample;
>+	int i;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -314,21 +314,18 @@ static int ad7877_read_adc(struct spi_device
*spi,
>unsigned command)
> 		spi_message_add_tail(&req->xfer[i], &req->msg);
>
> 	status = spi_sync(spi, &req->msg);
>-
>-	if (status == 0)
>-		status = req->msg.status;
>-
> 	sample = req->sample;
>
> 	kfree(req);
>-	return status ? status : sample;
>+
>+	return status ? : sample;
> }
>
> static void ad7877_rx(struct ad7877 *ts)
> {
>-	struct input_dev	*input_dev = ts->input;
>-	unsigned		Rt;
>-	u16			x, y, z1, z2;
>+	struct input_dev *input_dev = ts->input;
>+	unsigned Rt;
>+	u16 x, y, z1, z2;
>
> 	x = ts->conversion_data[AD7877_SEQ_XPOS] & MAX_12BIT;
> 	y = ts->conversion_data[AD7877_SEQ_YPOS] & MAX_12BIT;
>@@ -350,10 +347,7 @@ static void ad7877_rx(struct ad7877 *ts)
> 		Rt = (z2 - z1) * x * ts->x_plate_ohms;
> 		Rt /= z1;
> 		Rt = (Rt + 2047) >> 12;
>-	} else
>-		Rt = 0;
>
>-	if (Rt) {
> 		input_report_abs(input_dev, ABS_X, x);
> 		input_report_abs(input_dev, ABS_Y, y);
> 		input_report_abs(input_dev, ABS_PRESSURE, Rt);
>@@ -371,7 +365,7 @@ static inline void ad7877_ts_event_release(struct
>ad7877 *ts)
>
> static void ad7877_timer(unsigned long handle)
> {
>-	struct ad7877	*ts = (void *)handle;
>+	struct ad7877 *ts = (void *)handle;
>
> 	ad7877_ts_event_release(ts);
> }
>@@ -382,78 +376,72 @@ static irqreturn_t ad7877_irq(int irq, void
*handle)
> 	unsigned long flags;
> 	int status;
>
>-	/* The repeated conversion sequencer controlled by TMR kicked
off too
>fast.
>-	 * We ignore the last and process the sample sequence currently
in
>the queue
>-	 * It can't be older than 9.4ms, and we need to avoid that
ts->msg
>-	 * doesn't get issued twice while in work.
>+	/*
>+	 * The repeated conversion sequencer controlled by TMR kicked
off
>+	 * too fast. We ignore the last and process the sample sequence
>+	 * currently in the queue. It can't be older than 9.4ms, and we
>+	 * need to avoid that ts->msg doesn't get issued twice while in
work.
> 	 */
>
> 	spin_lock_irqsave(&ts->lock, flags);
>-	if (ts->pending) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return IRQ_HANDLED;
>+	if (!ts->pending) {
>+		ts->pending = 1;
>+
>+		status = spi_async(ts->spi, &ts->msg);
>+		if (status)
>+			dev_err(&ts->spi->dev, "spi_sync --> %d\n",
status);
> 	}
>-	ts->pending = 1;
> 	spin_unlock_irqrestore(&ts->lock, flags);
>
>-	status = spi_async(ts->spi, &ts->msg);
>-
>-	if (status)
>-		dev_err(&ts->spi->dev, "spi_sync --> %d\n", status);
>-
> 	return IRQ_HANDLED;
> }
>
> static void ad7877_callback(void *_ts)
> {
> 	struct ad7877 *ts = _ts;
>-	unsigned long flags;
>
>-	ad7877_rx(ts);
>+	spin_lock_irq(&ts->lock);
>
>-	spin_lock_irqsave(&ts->lock, flags);
>+	ad7877_rx(ts);
> 	ts->pending = 0;
> 	mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+
>+	spin_unlock_irq(&ts->lock);
> }
>
> static void ad7877_disable(struct ad7877 *ts)
> {
>-	unsigned long flags;
>-
>-	spin_lock_irqsave(&ts->lock, flags);
>-	if (ts->disabled) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return;
>-	}
>+	mutex_lock(&ts->mutex);
>
>-	ts->disabled = 1;
>-	disable_irq(ts->spi->irq);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+	if (!ts->disabled) {
>+		ts->disabled = 1;
>+		disable_irq(ts->spi->irq);
>
>-	while (ts->pending) /* Wait for spi_async callback */
>-		msleep(1);
>+		/* Wait for spi_async callback */
>+		while (ts->pending)
>+			msleep(1);
>
>-	if (del_timer_sync(&ts->timer))
>-		ad7877_ts_event_release(ts);
>+		if (del_timer_sync(&ts->timer))
>+			ad7877_ts_event_release(ts);
>+	}
>
> 	/* we know the chip's in lowpower mode since we always
> 	 * leave it that way after every request
> 	 */
>+
>+	mutex_unlock(&ts->mutex);
> }
>
> static void ad7877_enable(struct ad7877 *ts)
> {
>-	unsigned long flags;
>+	mutex_lock(&ts->mutex);
>
>-	spin_lock_irqsave(&ts->lock, flags);
> 	if (ts->disabled) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return;
>+		ts->disabled = 0;
>+		enable_irq(ts->spi->irq);
> 	}
>-	ts->disabled = 0;
>-	enable_irq(ts->spi->irq);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+
>+	mutex_unlock(&ts->mutex);
> }
>
> #define SHOW(name) static ssize_t \
>@@ -490,12 +478,11 @@ static ssize_t ad7877_disable_store(struct device
>*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
> 	if (val)
> 		ad7877_disable(ts);
>@@ -521,16 +508,16 @@ static ssize_t ad7877_dac_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>-
>-	ret = strict_strtoul(buf, 10, &val);
>+	int error;
>
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->dac = val & 0xFF;
>-
> 	ad7877_write(ts->spi, AD7877_REG_DAC, (ts->dac << 4) |
>AD7877_DAC_CONF);
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -551,16 +538,17 @@ static ssize_t ad7877_gpio3_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->gpio3 = !!val;
>-
> 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA
|
> 		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -581,16 +569,17 @@ static ssize_t ad7877_gpio4_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->gpio4 = !!val;
>-
> 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA
|
>-		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+		     (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -685,14 +674,10 @@ static int __devinit ad7877_probe(struct
spi_device
>*spi)
> 	}
>
> 	ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
>-	if (!ts)
>-		return -ENOMEM;
>-
>-
> 	input_dev = input_allocate_device();
>-	if (!input_dev) {
>-		kfree(ts);
>-		return -ENOMEM;
>+	if (!ts || !input_dev) {
>+		err = -ENOMEM;
>+		goto err_free_mem;
> 	}
>
> 	dev_set_drvdata(&spi->dev, ts);
>@@ -700,6 +685,7 @@ static int __devinit ad7877_probe(struct spi_device
>*spi)
> 	ts->input = input_dev;
>
> 	setup_timer(&ts->timer, ad7877_timer, (unsigned long) ts);
>+	mutex_init(&ts->mutex);
> 	spin_lock_init(&ts->lock);
>
> 	ts->model = pdata->model ? : 7877;
>@@ -713,7 +699,7 @@ static int __devinit ad7877_probe(struct spi_device
>*spi)
> 	ts->averaging = pdata->averaging;
> 	ts->pen_down_acc_interval = pdata->pen_down_acc_interval;
>
>-	snprintf(ts->phys, sizeof(ts->phys), "%s/inputX",
spi->dev.bus_id);
>+	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi-
>>dev));
>
> 	input_dev->name = "AD7877 Touchscreen";
> 	input_dev->phys = ts->phys;
>@@ -761,30 +747,25 @@ static int __devinit ad7877_probe(struct
spi_device
>*spi)
> 	}
>
> 	err = sysfs_create_group(&spi->dev.kobj, &ad7877_attr_group);
>-
>-	if (gpio3)
>-		err |= device_create_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		err |= device_create_file(&spi->dev, &dev_attr_aux3);
>-
> 	if (err)
> 		goto err_free_irq;
>
>+	err = device_create_file(&spi->dev,
>+				 gpio3 ? &dev_attr_gpio3 :
&dev_attr_aux3);
>+	if (err)
>+		goto err_remove_attr_group;
>+
> 	err = input_register_device(input_dev);
> 	if (err)
> 		goto err_remove_attr;
>
>-	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
>-
> 	return 0;
>
> err_remove_attr:
>+	device_remove_file(&spi->dev,
>+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
>+err_remove_attr_group:
> 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
>-
>-	if (gpio3)
>-		device_remove_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		device_remove_file(&spi->dev, &dev_attr_aux3);
> err_free_irq:
> 	free_irq(spi->irq, ts);
> err_free_mem:
>@@ -798,19 +779,14 @@ static int __devexit ad7877_remove(struct
spi_device
>*spi)
> {
> 	struct ad7877		*ts = dev_get_drvdata(&spi->dev);
>
>-	ad7877_disable(ts);
>-
> 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
>+	device_remove_file(&spi->dev,
>+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
>
>-	if (gpio3)
>-		device_remove_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		device_remove_file(&spi->dev, &dev_attr_aux3);
>-
>+	ad7877_disable(ts);
> 	free_irq(ts->spi->irq, ts);
>
> 	input_unregister_device(ts->input);
>-
> 	kfree(ts);
>
> 	dev_dbg(&spi->dev, "unregistered touchscreen\n");
>@@ -866,10 +842,6 @@ static void __exit ad7877_exit(void)
> }
> module_exit(ad7877_exit);
>
>-module_param(gpio3, int, 0);
>-MODULE_PARM_DESC(gpio3,
>-	"If gpio3 is set to 1 AUX3 acts as GPIO3");
>-
> MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> MODULE_DESCRIPTION("AD7877 touchscreen Driver");
> MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH 1/1] Input/Touchscreen Driver: add support AD7877touchscreen driver (try #5)
  2009-03-08  6:38 ` Dmitry Torokhov
@ 2009-03-09 14:45     ` Hennerich, Michael
  2009-03-09 14:45     ` Hennerich, Michael
  1 sibling, 0 replies; 6+ messages in thread
From: Hennerich, Michael @ 2009-03-09 14:45 UTC (permalink / raw)
  To: Dmitry Torokhov, Bryan Wu; +Cc: akpm, linux-input, linux-kernel

Hi Dmitry,

Please ignore my last email.
Your patch applied cleanly without the [randy.dunlap@oracle.com:
touchscreen/ad787x: don't use bus_id] patch.

The other troubles I had were caused by my stupid mail program.

>Could you please try the patch below and let me know if the driver
still
>works so I can commit it. I also did some formatting changes, I hope
you
>don't mind. Thanks!

Thanks a lot. And yes it works perfectly.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>

Best regards,
Michael

>-----Original Message-----
>From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>Sent: Sunday, March 08, 2009 7:39 AM
>To: Bryan Wu; Hennerich, Michael
>Cc: akpm@linux-foundation.org; linux-input@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: Re: [PATCH 1/1] Input/Touchscreen Driver: add support
>AD7877touchscreen driver (try #5)
>
>Hi Bryan, Michael,
>
>On Fri, Oct 17, 2008 at 01:56:08AM +0800, Bryan Wu wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> [try #5]
>>  - Fix indention
>>  - Lock spi_async()
>>  - Remove useless header comments
>>  - Use strict_strtoul
>>  - Use EDGE triggered interrupts ?\226?\128?\147 this simplifies some
of
>the IRQ handling.
>>  - Fix error cleanup path
>>  - Remove duplicated code
>>  - SPI access functions parameter use struct spi_device
>>
>
>I looked at the latest version of the driver in Andrew's queue and I
>think it needs the following changes:
>
>- you can't have parts of bitfield updated from IRQ context and part
from
>  process context unless every field is protected by the same spinlock.
>
>- You need more full mutual exclusion between enable and disable so you
>  don't inadvertingly kill the timer if you disable and enable from
>  different processes.
>
>- I think you need serialization in various store() methods so that
>  consistent values are written into chip's registers.
>
>- There were coule of inverted logic errors in disabling chip code.
>
>- Kay is trying to get rid of bus_id in devices, you need to use
>  dev_name() instead.
>
>Could you please try the patch below and let me know if the driver
still
>works so I can commit it. I also did some formatting changes, I hope
you
>don't mind. Thanks!
>
>--
>Dmitry
>
>Input: ad7877 fixups
>
>Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>---
>
> drivers/input/touchscreen/ad7877.c |  226
++++++++++++++++----------------
>----
> 1 files changed, 99 insertions(+), 127 deletions(-)
>
>
>diff --git a/drivers/input/touchscreen/ad7877.c
>b/drivers/input/touchscreen/ad7877.c
>index 6615bcd..6702364 100644
>--- a/drivers/input/touchscreen/ad7877.c
>+++ b/drivers/input/touchscreen/ad7877.c
>@@ -1,7 +1,7 @@
> /*
>  * File:        drivers/input/touchscreen/ad7877.c
>  *
>- * Based on: 	ads7846.c
>+ * Based on:	ads7846.c
>  *
>  *		Copyright (C) 2006-2008 Michael Hennerich, Analog
Devices Inc.
>  *
>@@ -185,21 +185,24 @@ struct ad7877 {
> 	u8			averaging;
> 	u8			pen_down_acc_interval;
>
>-	u16 			conversion_data[AD7877_NR_SENSE];
>+	u16			conversion_data[AD7877_NR_SENSE];
>
> 	struct spi_transfer	xfer[AD7877_NR_SENSE + 2];
> 	struct spi_message	msg;
>
>+	struct mutex		mutex;
>+	unsigned		disabled:1;	/* P: mutex */
>+	unsigned		gpio3:1;	/* P: mutex */
>+	unsigned		gpio4:1;	/* P: mutex */
>+
> 	spinlock_t		lock;
> 	struct timer_list	timer;		/* P: lock */
>-	unsigned		pendown:1;	/* P: lock */
> 	unsigned		pending:1;	/* P: lock */
>-	unsigned		disabled:1;
>-	unsigned		gpio3:1;
>-	unsigned		gpio4:1;
> };
>
> static int gpio3;
>+module_param(gpio3, int, 0);
>+MODULE_PARM_DESC(gpio3, "If gpio3 is set to 1 AUX3 acts as GPIO3");
>
> /*
>  * ad7877_read/write are only used for initial setup and for sysfs
>controls.
>@@ -208,9 +211,10 @@ static int gpio3;
>
> static int ad7877_read(struct spi_device *spi, u16 reg)
> {
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status, ret;
>+	struct ser_req *req;
>+	int status, ret;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -228,11 +232,8 @@ static int ad7877_read(struct spi_device *spi, u16
>reg)
> 	spi_message_add_tail(&req->xfer[1], &req->msg);
>
> 	status = spi_sync(spi, &req->msg);
>+	ret = status ? : req->sample;
>
>-	if (status == 0)
>-		status = req->msg.status;
>-
>-	ret = status ? status : req->sample;
> 	kfree(req);
>
> 	return ret;
>@@ -240,9 +241,10 @@ static int ad7877_read(struct spi_device *spi, u16
>reg)
>
> static int ad7877_write(struct spi_device *spi, u16 reg, u16 val)
> {
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status;
>+	struct ser_req *req;
>+	int status;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -256,9 +258,6 @@ static int ad7877_write(struct spi_device *spi, u16
>reg, u16 val)
>
> 	status = spi_sync(spi, &req->msg);
>
>-	if (status == 0)
>-		status = req->msg.status;
>-
> 	kfree(req);
>
> 	return status;
>@@ -266,12 +265,13 @@ static int ad7877_write(struct spi_device *spi,
u16
>reg, u16 val)
>
> static int ad7877_read_adc(struct spi_device *spi, unsigned command)
> {
>-	struct ad7877 		*ts = dev_get_drvdata(&spi->dev);
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status;
>-	int			sample;
>-	int			i;
>+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
>+	struct ser_req *req;
>+	int status;
>+	int sample;
>+	int i;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -314,21 +314,18 @@ static int ad7877_read_adc(struct spi_device
*spi,
>unsigned command)
> 		spi_message_add_tail(&req->xfer[i], &req->msg);
>
> 	status = spi_sync(spi, &req->msg);
>-
>-	if (status == 0)
>-		status = req->msg.status;
>-
> 	sample = req->sample;
>
> 	kfree(req);
>-	return status ? status : sample;
>+
>+	return status ? : sample;
> }
>
> static void ad7877_rx(struct ad7877 *ts)
> {
>-	struct input_dev	*input_dev = ts->input;
>-	unsigned		Rt;
>-	u16			x, y, z1, z2;
>+	struct input_dev *input_dev = ts->input;
>+	unsigned Rt;
>+	u16 x, y, z1, z2;
>
> 	x = ts->conversion_data[AD7877_SEQ_XPOS] & MAX_12BIT;
> 	y = ts->conversion_data[AD7877_SEQ_YPOS] & MAX_12BIT;
>@@ -350,10 +347,7 @@ static void ad7877_rx(struct ad7877 *ts)
> 		Rt = (z2 - z1) * x * ts->x_plate_ohms;
> 		Rt /= z1;
> 		Rt = (Rt + 2047) >> 12;
>-	} else
>-		Rt = 0;
>
>-	if (Rt) {
> 		input_report_abs(input_dev, ABS_X, x);
> 		input_report_abs(input_dev, ABS_Y, y);
> 		input_report_abs(input_dev, ABS_PRESSURE, Rt);
>@@ -371,7 +365,7 @@ static inline void ad7877_ts_event_release(struct
>ad7877 *ts)
>
> static void ad7877_timer(unsigned long handle)
> {
>-	struct ad7877	*ts = (void *)handle;
>+	struct ad7877 *ts = (void *)handle;
>
> 	ad7877_ts_event_release(ts);
> }
>@@ -382,78 +376,72 @@ static irqreturn_t ad7877_irq(int irq, void
*handle)
> 	unsigned long flags;
> 	int status;
>
>-	/* The repeated conversion sequencer controlled by TMR kicked
off too
>fast.
>-	 * We ignore the last and process the sample sequence currently
in
>the queue
>-	 * It can't be older than 9.4ms, and we need to avoid that
ts->msg
>-	 * doesn't get issued twice while in work.
>+	/*
>+	 * The repeated conversion sequencer controlled by TMR kicked
off
>+	 * too fast. We ignore the last and process the sample sequence
>+	 * currently in the queue. It can't be older than 9.4ms, and we
>+	 * need to avoid that ts->msg doesn't get issued twice while in
work.
> 	 */
>
> 	spin_lock_irqsave(&ts->lock, flags);
>-	if (ts->pending) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return IRQ_HANDLED;
>+	if (!ts->pending) {
>+		ts->pending = 1;
>+
>+		status = spi_async(ts->spi, &ts->msg);
>+		if (status)
>+			dev_err(&ts->spi->dev, "spi_sync --> %d\n",
status);
> 	}
>-	ts->pending = 1;
> 	spin_unlock_irqrestore(&ts->lock, flags);
>
>-	status = spi_async(ts->spi, &ts->msg);
>-
>-	if (status)
>-		dev_err(&ts->spi->dev, "spi_sync --> %d\n", status);
>-
> 	return IRQ_HANDLED;
> }
>
> static void ad7877_callback(void *_ts)
> {
> 	struct ad7877 *ts = _ts;
>-	unsigned long flags;
>
>-	ad7877_rx(ts);
>+	spin_lock_irq(&ts->lock);
>
>-	spin_lock_irqsave(&ts->lock, flags);
>+	ad7877_rx(ts);
> 	ts->pending = 0;
> 	mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+
>+	spin_unlock_irq(&ts->lock);
> }
>
> static void ad7877_disable(struct ad7877 *ts)
> {
>-	unsigned long flags;
>-
>-	spin_lock_irqsave(&ts->lock, flags);
>-	if (ts->disabled) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return;
>-	}
>+	mutex_lock(&ts->mutex);
>
>-	ts->disabled = 1;
>-	disable_irq(ts->spi->irq);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+	if (!ts->disabled) {
>+		ts->disabled = 1;
>+		disable_irq(ts->spi->irq);
>
>-	while (ts->pending) /* Wait for spi_async callback */
>-		msleep(1);
>+		/* Wait for spi_async callback */
>+		while (ts->pending)
>+			msleep(1);
>
>-	if (del_timer_sync(&ts->timer))
>-		ad7877_ts_event_release(ts);
>+		if (del_timer_sync(&ts->timer))
>+			ad7877_ts_event_release(ts);
>+	}
>
> 	/* we know the chip's in lowpower mode since we always
> 	 * leave it that way after every request
> 	 */
>+
>+	mutex_unlock(&ts->mutex);
> }
>
> static void ad7877_enable(struct ad7877 *ts)
> {
>-	unsigned long flags;
>+	mutex_lock(&ts->mutex);
>
>-	spin_lock_irqsave(&ts->lock, flags);
> 	if (ts->disabled) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return;
>+		ts->disabled = 0;
>+		enable_irq(ts->spi->irq);
> 	}
>-	ts->disabled = 0;
>-	enable_irq(ts->spi->irq);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+
>+	mutex_unlock(&ts->mutex);
> }
>
> #define SHOW(name) static ssize_t \
>@@ -490,12 +478,11 @@ static ssize_t ad7877_disable_store(struct device
>*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
> 	if (val)
> 		ad7877_disable(ts);
>@@ -521,16 +508,16 @@ static ssize_t ad7877_dac_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>-
>-	ret = strict_strtoul(buf, 10, &val);
>+	int error;
>
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->dac = val & 0xFF;
>-
> 	ad7877_write(ts->spi, AD7877_REG_DAC, (ts->dac << 4) |
>AD7877_DAC_CONF);
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -551,16 +538,17 @@ static ssize_t ad7877_gpio3_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->gpio3 = !!val;
>-
> 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA
|
> 		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -581,16 +569,17 @@ static ssize_t ad7877_gpio4_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->gpio4 = !!val;
>-
> 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA
|
>-		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+		     (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -685,14 +674,10 @@ static int __devinit ad7877_probe(struct
spi_device
>*spi)
> 	}
>
> 	ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
>-	if (!ts)
>-		return -ENOMEM;
>-
>-
> 	input_dev = input_allocate_device();
>-	if (!input_dev) {
>-		kfree(ts);
>-		return -ENOMEM;
>+	if (!ts || !input_dev) {
>+		err = -ENOMEM;
>+		goto err_free_mem;
> 	}
>
> 	dev_set_drvdata(&spi->dev, ts);
>@@ -700,6 +685,7 @@ static int __devinit ad7877_probe(struct spi_device
>*spi)
> 	ts->input = input_dev;
>
> 	setup_timer(&ts->timer, ad7877_timer, (unsigned long) ts);
>+	mutex_init(&ts->mutex);
> 	spin_lock_init(&ts->lock);
>
> 	ts->model = pdata->model ? : 7877;
>@@ -713,7 +699,7 @@ static int __devinit ad7877_probe(struct spi_device
>*spi)
> 	ts->averaging = pdata->averaging;
> 	ts->pen_down_acc_interval = pdata->pen_down_acc_interval;
>
>-	snprintf(ts->phys, sizeof(ts->phys), "%s/inputX",
spi->dev.bus_id);
>+	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi-
>>dev));
>
> 	input_dev->name = "AD7877 Touchscreen";
> 	input_dev->phys = ts->phys;
>@@ -761,30 +747,25 @@ static int __devinit ad7877_probe(struct
spi_device
>*spi)
> 	}
>
> 	err = sysfs_create_group(&spi->dev.kobj, &ad7877_attr_group);
>-
>-	if (gpio3)
>-		err |= device_create_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		err |= device_create_file(&spi->dev, &dev_attr_aux3);
>-
> 	if (err)
> 		goto err_free_irq;
>
>+	err = device_create_file(&spi->dev,
>+				 gpio3 ? &dev_attr_gpio3 :
&dev_attr_aux3);
>+	if (err)
>+		goto err_remove_attr_group;
>+
> 	err = input_register_device(input_dev);
> 	if (err)
> 		goto err_remove_attr;
>
>-	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
>-
> 	return 0;
>
> err_remove_attr:
>+	device_remove_file(&spi->dev,
>+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
>+err_remove_attr_group:
> 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
>-
>-	if (gpio3)
>-		device_remove_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		device_remove_file(&spi->dev, &dev_attr_aux3);
> err_free_irq:
> 	free_irq(spi->irq, ts);
> err_free_mem:
>@@ -798,19 +779,14 @@ static int __devexit ad7877_remove(struct
spi_device
>*spi)
> {
> 	struct ad7877		*ts = dev_get_drvdata(&spi->dev);
>
>-	ad7877_disable(ts);
>-
> 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
>+	device_remove_file(&spi->dev,
>+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
>
>-	if (gpio3)
>-		device_remove_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		device_remove_file(&spi->dev, &dev_attr_aux3);
>-
>+	ad7877_disable(ts);
> 	free_irq(ts->spi->irq, ts);
>
> 	input_unregister_device(ts->input);
>-
> 	kfree(ts);
>
> 	dev_dbg(&spi->dev, "unregistered touchscreen\n");
>@@ -866,10 +842,6 @@ static void __exit ad7877_exit(void)
> }
> module_exit(ad7877_exit);
>
>-module_param(gpio3, int, 0);
>-MODULE_PARM_DESC(gpio3,
>-	"If gpio3 is set to 1 AUX3 acts as GPIO3");
>-
> MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> MODULE_DESCRIPTION("AD7877 touchscreen Driver");
> MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH 1/1] Input/Touchscreen Driver: add support AD7877touchscreen driver (try #5)
@ 2009-03-09 14:45     ` Hennerich, Michael
  0 siblings, 0 replies; 6+ messages in thread
From: Hennerich, Michael @ 2009-03-09 14:45 UTC (permalink / raw)
  To: Dmitry Torokhov, Bryan Wu; +Cc: akpm, linux-input, linux-kernel

Hi Dmitry,

Please ignore my last email.
Your patch applied cleanly without the [randy.dunlap@oracle.com:
touchscreen/ad787x: don't use bus_id] patch.

The other troubles I had were caused by my stupid mail program.

>Could you please try the patch below and let me know if the driver
still
>works so I can commit it. I also did some formatting changes, I hope
you
>don't mind. Thanks!

Thanks a lot. And yes it works perfectly.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>

Best regards,
Michael

>-----Original Message-----
>From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>Sent: Sunday, March 08, 2009 7:39 AM
>To: Bryan Wu; Hennerich, Michael
>Cc: akpm@linux-foundation.org; linux-input@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: Re: [PATCH 1/1] Input/Touchscreen Driver: add support
>AD7877touchscreen driver (try #5)
>
>Hi Bryan, Michael,
>
>On Fri, Oct 17, 2008 at 01:56:08AM +0800, Bryan Wu wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> [try #5]
>>  - Fix indention
>>  - Lock spi_async()
>>  - Remove useless header comments
>>  - Use strict_strtoul
>>  - Use EDGE triggered interrupts ?\226?\128?\147 this simplifies some
of
>the IRQ handling.
>>  - Fix error cleanup path
>>  - Remove duplicated code
>>  - SPI access functions parameter use struct spi_device
>>
>
>I looked at the latest version of the driver in Andrew's queue and I
>think it needs the following changes:
>
>- you can't have parts of bitfield updated from IRQ context and part
from
>  process context unless every field is protected by the same spinlock.
>
>- You need more full mutual exclusion between enable and disable so you
>  don't inadvertingly kill the timer if you disable and enable from
>  different processes.
>
>- I think you need serialization in various store() methods so that
>  consistent values are written into chip's registers.
>
>- There were coule of inverted logic errors in disabling chip code.
>
>- Kay is trying to get rid of bus_id in devices, you need to use
>  dev_name() instead.
>
>Could you please try the patch below and let me know if the driver
still
>works so I can commit it. I also did some formatting changes, I hope
you
>don't mind. Thanks!
>
>--
>Dmitry
>
>Input: ad7877 fixups
>
>Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>---
>
> drivers/input/touchscreen/ad7877.c |  226
++++++++++++++++----------------
>----
> 1 files changed, 99 insertions(+), 127 deletions(-)
>
>
>diff --git a/drivers/input/touchscreen/ad7877.c
>b/drivers/input/touchscreen/ad7877.c
>index 6615bcd..6702364 100644
>--- a/drivers/input/touchscreen/ad7877.c
>+++ b/drivers/input/touchscreen/ad7877.c
>@@ -1,7 +1,7 @@
> /*
>  * File:        drivers/input/touchscreen/ad7877.c
>  *
>- * Based on: 	ads7846.c
>+ * Based on:	ads7846.c
>  *
>  *		Copyright (C) 2006-2008 Michael Hennerich, Analog
Devices Inc.
>  *
>@@ -185,21 +185,24 @@ struct ad7877 {
> 	u8			averaging;
> 	u8			pen_down_acc_interval;
>
>-	u16 			conversion_data[AD7877_NR_SENSE];
>+	u16			conversion_data[AD7877_NR_SENSE];
>
> 	struct spi_transfer	xfer[AD7877_NR_SENSE + 2];
> 	struct spi_message	msg;
>
>+	struct mutex		mutex;
>+	unsigned		disabled:1;	/* P: mutex */
>+	unsigned		gpio3:1;	/* P: mutex */
>+	unsigned		gpio4:1;	/* P: mutex */
>+
> 	spinlock_t		lock;
> 	struct timer_list	timer;		/* P: lock */
>-	unsigned		pendown:1;	/* P: lock */
> 	unsigned		pending:1;	/* P: lock */
>-	unsigned		disabled:1;
>-	unsigned		gpio3:1;
>-	unsigned		gpio4:1;
> };
>
> static int gpio3;
>+module_param(gpio3, int, 0);
>+MODULE_PARM_DESC(gpio3, "If gpio3 is set to 1 AUX3 acts as GPIO3");
>
> /*
>  * ad7877_read/write are only used for initial setup and for sysfs
>controls.
>@@ -208,9 +211,10 @@ static int gpio3;
>
> static int ad7877_read(struct spi_device *spi, u16 reg)
> {
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status, ret;
>+	struct ser_req *req;
>+	int status, ret;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -228,11 +232,8 @@ static int ad7877_read(struct spi_device *spi, u16
>reg)
> 	spi_message_add_tail(&req->xfer[1], &req->msg);
>
> 	status = spi_sync(spi, &req->msg);
>+	ret = status ? : req->sample;
>
>-	if (status == 0)
>-		status = req->msg.status;
>-
>-	ret = status ? status : req->sample;
> 	kfree(req);
>
> 	return ret;
>@@ -240,9 +241,10 @@ static int ad7877_read(struct spi_device *spi, u16
>reg)
>
> static int ad7877_write(struct spi_device *spi, u16 reg, u16 val)
> {
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status;
>+	struct ser_req *req;
>+	int status;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -256,9 +258,6 @@ static int ad7877_write(struct spi_device *spi, u16
>reg, u16 val)
>
> 	status = spi_sync(spi, &req->msg);
>
>-	if (status == 0)
>-		status = req->msg.status;
>-
> 	kfree(req);
>
> 	return status;
>@@ -266,12 +265,13 @@ static int ad7877_write(struct spi_device *spi,
u16
>reg, u16 val)
>
> static int ad7877_read_adc(struct spi_device *spi, unsigned command)
> {
>-	struct ad7877 		*ts = dev_get_drvdata(&spi->dev);
>-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
>-	int			status;
>-	int			sample;
>-	int			i;
>+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
>+	struct ser_req *req;
>+	int status;
>+	int sample;
>+	int i;
>
>+	req = kzalloc(sizeof *req, GFP_KERNEL);
> 	if (!req)
> 		return -ENOMEM;
>
>@@ -314,21 +314,18 @@ static int ad7877_read_adc(struct spi_device
*spi,
>unsigned command)
> 		spi_message_add_tail(&req->xfer[i], &req->msg);
>
> 	status = spi_sync(spi, &req->msg);
>-
>-	if (status == 0)
>-		status = req->msg.status;
>-
> 	sample = req->sample;
>
> 	kfree(req);
>-	return status ? status : sample;
>+
>+	return status ? : sample;
> }
>
> static void ad7877_rx(struct ad7877 *ts)
> {
>-	struct input_dev	*input_dev = ts->input;
>-	unsigned		Rt;
>-	u16			x, y, z1, z2;
>+	struct input_dev *input_dev = ts->input;
>+	unsigned Rt;
>+	u16 x, y, z1, z2;
>
> 	x = ts->conversion_data[AD7877_SEQ_XPOS] & MAX_12BIT;
> 	y = ts->conversion_data[AD7877_SEQ_YPOS] & MAX_12BIT;
>@@ -350,10 +347,7 @@ static void ad7877_rx(struct ad7877 *ts)
> 		Rt = (z2 - z1) * x * ts->x_plate_ohms;
> 		Rt /= z1;
> 		Rt = (Rt + 2047) >> 12;
>-	} else
>-		Rt = 0;
>
>-	if (Rt) {
> 		input_report_abs(input_dev, ABS_X, x);
> 		input_report_abs(input_dev, ABS_Y, y);
> 		input_report_abs(input_dev, ABS_PRESSURE, Rt);
>@@ -371,7 +365,7 @@ static inline void ad7877_ts_event_release(struct
>ad7877 *ts)
>
> static void ad7877_timer(unsigned long handle)
> {
>-	struct ad7877	*ts = (void *)handle;
>+	struct ad7877 *ts = (void *)handle;
>
> 	ad7877_ts_event_release(ts);
> }
>@@ -382,78 +376,72 @@ static irqreturn_t ad7877_irq(int irq, void
*handle)
> 	unsigned long flags;
> 	int status;
>
>-	/* The repeated conversion sequencer controlled by TMR kicked
off too
>fast.
>-	 * We ignore the last and process the sample sequence currently
in
>the queue
>-	 * It can't be older than 9.4ms, and we need to avoid that
ts->msg
>-	 * doesn't get issued twice while in work.
>+	/*
>+	 * The repeated conversion sequencer controlled by TMR kicked
off
>+	 * too fast. We ignore the last and process the sample sequence
>+	 * currently in the queue. It can't be older than 9.4ms, and we
>+	 * need to avoid that ts->msg doesn't get issued twice while in
work.
> 	 */
>
> 	spin_lock_irqsave(&ts->lock, flags);
>-	if (ts->pending) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return IRQ_HANDLED;
>+	if (!ts->pending) {
>+		ts->pending = 1;
>+
>+		status = spi_async(ts->spi, &ts->msg);
>+		if (status)
>+			dev_err(&ts->spi->dev, "spi_sync --> %d\n",
status);
> 	}
>-	ts->pending = 1;
> 	spin_unlock_irqrestore(&ts->lock, flags);
>
>-	status = spi_async(ts->spi, &ts->msg);
>-
>-	if (status)
>-		dev_err(&ts->spi->dev, "spi_sync --> %d\n", status);
>-
> 	return IRQ_HANDLED;
> }
>
> static void ad7877_callback(void *_ts)
> {
> 	struct ad7877 *ts = _ts;
>-	unsigned long flags;
>
>-	ad7877_rx(ts);
>+	spin_lock_irq(&ts->lock);
>
>-	spin_lock_irqsave(&ts->lock, flags);
>+	ad7877_rx(ts);
> 	ts->pending = 0;
> 	mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+
>+	spin_unlock_irq(&ts->lock);
> }
>
> static void ad7877_disable(struct ad7877 *ts)
> {
>-	unsigned long flags;
>-
>-	spin_lock_irqsave(&ts->lock, flags);
>-	if (ts->disabled) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return;
>-	}
>+	mutex_lock(&ts->mutex);
>
>-	ts->disabled = 1;
>-	disable_irq(ts->spi->irq);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+	if (!ts->disabled) {
>+		ts->disabled = 1;
>+		disable_irq(ts->spi->irq);
>
>-	while (ts->pending) /* Wait for spi_async callback */
>-		msleep(1);
>+		/* Wait for spi_async callback */
>+		while (ts->pending)
>+			msleep(1);
>
>-	if (del_timer_sync(&ts->timer))
>-		ad7877_ts_event_release(ts);
>+		if (del_timer_sync(&ts->timer))
>+			ad7877_ts_event_release(ts);
>+	}
>
> 	/* we know the chip's in lowpower mode since we always
> 	 * leave it that way after every request
> 	 */
>+
>+	mutex_unlock(&ts->mutex);
> }
>
> static void ad7877_enable(struct ad7877 *ts)
> {
>-	unsigned long flags;
>+	mutex_lock(&ts->mutex);
>
>-	spin_lock_irqsave(&ts->lock, flags);
> 	if (ts->disabled) {
>-		spin_unlock_irqrestore(&ts->lock, flags);
>-		return;
>+		ts->disabled = 0;
>+		enable_irq(ts->spi->irq);
> 	}
>-	ts->disabled = 0;
>-	enable_irq(ts->spi->irq);
>-	spin_unlock_irqrestore(&ts->lock, flags);
>+
>+	mutex_unlock(&ts->mutex);
> }
>
> #define SHOW(name) static ssize_t \
>@@ -490,12 +478,11 @@ static ssize_t ad7877_disable_store(struct device
>*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
> 	if (val)
> 		ad7877_disable(ts);
>@@ -521,16 +508,16 @@ static ssize_t ad7877_dac_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>-
>-	ret = strict_strtoul(buf, 10, &val);
>+	int error;
>
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->dac = val & 0xFF;
>-
> 	ad7877_write(ts->spi, AD7877_REG_DAC, (ts->dac << 4) |
>AD7877_DAC_CONF);
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -551,16 +538,17 @@ static ssize_t ad7877_gpio3_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->gpio3 = !!val;
>-
> 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA
|
> 		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -581,16 +569,17 @@ static ssize_t ad7877_gpio4_store(struct device
*dev,
> {
> 	struct ad7877 *ts = dev_get_drvdata(dev);
> 	unsigned long val;
>-	int ret;
>+	int error;
>
>-	ret = strict_strtoul(buf, 10, &val);
>-	if (ret)
>-		return ret;
>+	error = strict_strtoul(buf, 10, &val);
>+	if (error)
>+		return error;
>
>+	mutex_lock(&ts->mutex);
> 	ts->gpio4 = !!val;
>-
> 	ad7877_write(ts->spi, AD7877_REG_EXTWRITE, AD7877_EXTW_GPIO_DATA
|
>-		 (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+		     (ts->gpio4 << 4) | (ts->gpio3 << 5));
>+	mutex_unlock(&ts->mutex);
>
> 	return count;
> }
>@@ -685,14 +674,10 @@ static int __devinit ad7877_probe(struct
spi_device
>*spi)
> 	}
>
> 	ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
>-	if (!ts)
>-		return -ENOMEM;
>-
>-
> 	input_dev = input_allocate_device();
>-	if (!input_dev) {
>-		kfree(ts);
>-		return -ENOMEM;
>+	if (!ts || !input_dev) {
>+		err = -ENOMEM;
>+		goto err_free_mem;
> 	}
>
> 	dev_set_drvdata(&spi->dev, ts);
>@@ -700,6 +685,7 @@ static int __devinit ad7877_probe(struct spi_device
>*spi)
> 	ts->input = input_dev;
>
> 	setup_timer(&ts->timer, ad7877_timer, (unsigned long) ts);
>+	mutex_init(&ts->mutex);
> 	spin_lock_init(&ts->lock);
>
> 	ts->model = pdata->model ? : 7877;
>@@ -713,7 +699,7 @@ static int __devinit ad7877_probe(struct spi_device
>*spi)
> 	ts->averaging = pdata->averaging;
> 	ts->pen_down_acc_interval = pdata->pen_down_acc_interval;
>
>-	snprintf(ts->phys, sizeof(ts->phys), "%s/inputX",
spi->dev.bus_id);
>+	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi-
>>dev));
>
> 	input_dev->name = "AD7877 Touchscreen";
> 	input_dev->phys = ts->phys;
>@@ -761,30 +747,25 @@ static int __devinit ad7877_probe(struct
spi_device
>*spi)
> 	}
>
> 	err = sysfs_create_group(&spi->dev.kobj, &ad7877_attr_group);
>-
>-	if (gpio3)
>-		err |= device_create_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		err |= device_create_file(&spi->dev, &dev_attr_aux3);
>-
> 	if (err)
> 		goto err_free_irq;
>
>+	err = device_create_file(&spi->dev,
>+				 gpio3 ? &dev_attr_gpio3 :
&dev_attr_aux3);
>+	if (err)
>+		goto err_remove_attr_group;
>+
> 	err = input_register_device(input_dev);
> 	if (err)
> 		goto err_remove_attr;
>
>-	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
>-
> 	return 0;
>
> err_remove_attr:
>+	device_remove_file(&spi->dev,
>+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
>+err_remove_attr_group:
> 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
>-
>-	if (gpio3)
>-		device_remove_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		device_remove_file(&spi->dev, &dev_attr_aux3);
> err_free_irq:
> 	free_irq(spi->irq, ts);
> err_free_mem:
>@@ -798,19 +779,14 @@ static int __devexit ad7877_remove(struct
spi_device
>*spi)
> {
> 	struct ad7877		*ts = dev_get_drvdata(&spi->dev);
>
>-	ad7877_disable(ts);
>-
> 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
>+	device_remove_file(&spi->dev,
>+			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
>
>-	if (gpio3)
>-		device_remove_file(&spi->dev, &dev_attr_gpio3);
>-	else
>-		device_remove_file(&spi->dev, &dev_attr_aux3);
>-
>+	ad7877_disable(ts);
> 	free_irq(ts->spi->irq, ts);
>
> 	input_unregister_device(ts->input);
>-
> 	kfree(ts);
>
> 	dev_dbg(&spi->dev, "unregistered touchscreen\n");
>@@ -866,10 +842,6 @@ static void __exit ad7877_exit(void)
> }
> module_exit(ad7877_exit);
>
>-module_param(gpio3, int, 0);
>-MODULE_PARM_DESC(gpio3,
>-	"If gpio3 is set to 1 AUX3 acts as GPIO3");
>-
> MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> MODULE_DESCRIPTION("AD7877 touchscreen Driver");
> MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2009-03-09 14:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-16 17:56 [PATCH 1/1] Input/Touchscreen Driver: add support AD7877 touchscreen driver (try #5) Bryan Wu
2009-03-08  6:38 ` Dmitry Torokhov
2009-03-09 13:20   ` [PATCH 1/1] Input/Touchscreen Driver: add support AD7877touchscreen " Hennerich, Michael
2009-03-09 13:20     ` Hennerich, Michael
2009-03-09 14:45   ` Hennerich, Michael
2009-03-09 14:45     ` Hennerich, Michael

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.