>From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] >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@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. Andrew folded a patch addressing this issue, submitted by [randy.dunlap@xxxxxxxxxx: 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@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-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
