Re: [PATCH 1/1] Input/Touchscreen Driver: add support AD7877touchscreen driver (try #5)

From: Dmitry Torokhov
Date: Sun Mar 08 2009 - 01:39:19 EST

Hi Bryan, Michael,

On Fri, Oct 17, 2008 at 01:56:08AM +0800, Bryan Wu wrote:
> From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>
> [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@xxxxxxx>
---

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@xxxxxxxxxxxxxxxxxxxx>");
MODULE_DESCRIPTION("AD7877 touchscreen Driver");
MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/