On Fri, Mar 06, 2009 at 10:07:36AM -0000, Hennerich, Michael wrote: > > > >From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] > > > >Hi Michael, > > > >On Fri, Mar 06, 2009 at 09:48:17AM -0000, Hennerich, Michael wrote: > >> > >> Hi Dmitry, > >> > >> Is there a schedule when this and my other patch get merged mainline? > >> > >> [patch 01/22] input/touchscreen driver: add support AD7877 > touchscreen > >> driver > >> > >> Do you have any concerns or is there something that should be done > >> differently? > > > >I have some concerns with regard to locking in the driver. I had a > >preliminary patch but I need to look at it again. > > > >-- > >Dmitry > > Hi Dmitry, > > I reworked the locking, it's now done differently. > I see, it is indeed much better. I have same comment about mutual exclusion between enable and disable as with 7877 driver. Could you please try the patch below and let me know if I broke the driver or not? ;) Thanks! -- Dmitry Input: ad7879 fixups Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx> --- drivers/input/touchscreen/ad7879.c | 193 +++++++++++++++++------------------- 1 files changed, 89 insertions(+), 104 deletions(-) diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c index 19287af..30b0159 100644 --- a/drivers/input/touchscreen/ad7879.c +++ b/drivers/input/touchscreen/ad7879.c @@ -1,7 +1,5 @@ /* - * File: drivers/input/touchscreen/ad7879.c - * - * Copyright (C) 2008 Michael Hennerich, Analog Devices Inc. + * Copyright (C) 2008 Michael Hennerich, Analog Devices Inc. * * Description: AD7879 based touchscreen, and GPIO driver (I2C/SPI Interface) * @@ -35,7 +33,7 @@ * Copyright (C) 2004 Texas Instruments * Copyright (C) 2005 Dirk Behme * - ad7877.c - * Copyright (C) 2006-2008 Analog Devices Inc. + * Copyright (C) 2006-2008 Analog Devices Inc. */ #include <linux/device.h> @@ -70,11 +68,11 @@ /* Control REG 1 */ #define AD7879_TMR(x) ((x & 0xFF) << 0) #define AD7879_ACQ(x) ((x & 0x3) << 8) -#define AD7879_MODE_NOC (0 << 10) /* Do not convert */ -#define AD7879_MODE_SCC (1 << 10) /* Single channel conversion */ -#define AD7879_MODE_SEQ0 (2 << 10) /* Sequence 0 in Slave Mode */ -#define AD7879_MODE_SEQ1 (3 << 10) /* Sequence 1 in Master Mode */ -#define AD7879_MODE_INT (1 << 15) /* PENIRQ disabled INT enabled */ +#define AD7879_MODE_NOC (0 << 10) /* Do not convert */ +#define AD7879_MODE_SCC (1 << 10) /* Single channel conversion */ +#define AD7879_MODE_SEQ0 (2 << 10) /* Sequence 0 in Slave Mode */ +#define AD7879_MODE_SEQ1 (3 << 10) /* Sequence 1 in Master Mode */ +#define AD7879_MODE_INT (1 << 15) /* PENIRQ disabled INT enabled */ /* Control REG 2 */ #define AD7879_FCD(x) ((x & 0x3) << 0) @@ -129,18 +127,20 @@ typedef struct i2c_client bus_device; #endif struct ad7879 { - bus_device *bus; + bus_device *bus; struct input_dev *input; struct work_struct work; struct timer_list timer; - spinlock_t lock; + + struct mutex mutex; + unsigned disabled:1; /* P: mutex */ #if defined(CONFIG_TOUCHSCREEN_AD7879_SPI) || defined(CONFIG_TOUCHSCREEN_AD7879_SPI_MODULE) struct spi_message msg; struct spi_transfer xfer[AD7879_NR_SENSE + 1]; u16 cmd; #endif - u16 conversion_data[AD7879_NR_SENSE]; + u16 conversion_data[AD7879_NR_SENSE]; char phys[32]; u8 first_conversion_delay; u8 acquisition_time; @@ -153,7 +153,6 @@ struct ad7879 { u16 cmd_crtl1; u16 cmd_crtl2; u16 cmd_crtl3; - unsigned disabled:1; /* P: lock */ unsigned gpio:1; }; @@ -163,9 +162,9 @@ static void ad7879_collect(struct ad7879 *); static void ad7879_report(struct ad7879 *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[AD7879_SEQ_XPOS] & MAX_12BIT; y = ts->conversion_data[AD7879_SEQ_YPOS] & MAX_12BIT; @@ -187,10 +186,7 @@ static void ad7879_report(struct ad7879 *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); @@ -218,7 +214,7 @@ static void ad7879_ts_event_release(struct ad7879 *ts) static void ad7879_timer(unsigned long handle) { - struct ad7879 *ts = (void *)handle; + struct ad7879 *ts = (void *)handle; ad7879_ts_event_release(ts); } @@ -240,40 +236,36 @@ static irqreturn_t ad7879_irq(int irq, void *handle) static void ad7879_disable(struct ad7879 *ts) { - unsigned long flags; + mutex_lock(&ts->mutex); - spin_lock_irqsave(&ts->lock, flags); - if (ts->disabled) { - spin_unlock_irqrestore(&ts->lock, flags); - return; - } + if (!ts->disabled) { - ts->disabled = 1; - disable_irq(ts->bus->irq); - spin_unlock_irqrestore(&ts->lock, flags); + ts->disabled = 1; + disable_irq(ts->bus->irq); - cancel_work_sync(&ts->work); + cancel_work_sync(&ts->work); - if (del_timer_sync(&ts->timer)) - ad7879_ts_event_release(ts); + if (del_timer_sync(&ts->timer)) + ad7879_ts_event_release(ts); - /* we know the chip's in lowpower mode since we always - * leave it that way after every request - */ + ad7879_write(ts->bus, AD7879_REG_CTRL2, + AD7879_PM(AD7879_PM_SHUTDOWN)); + } + + mutex_unlock(&ts->mutex); } static void ad7879_enable(struct ad7879 *ts) { - unsigned long flags; + mutex_lock(&ts->mutex); - spin_lock_irqsave(&ts->lock, flags); if (ts->disabled) { - spin_unlock_irqrestore(&ts->lock, flags); - return; + ad7879_setup(ts); + ts->disabled = 0; + enable_irq(ts->bus->irq); } - ts->disabled = 0; - enable_irq(ts->bus->irq); - spin_unlock_irqrestore(&ts->lock, flags); + + mutex_unlock(&ts->mutex); } static ssize_t ad7879_disable_show(struct device *dev, @@ -290,12 +282,11 @@ static ssize_t ad7879_disable_store(struct device *dev, { struct ad7879 *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; if (val) ad7879_disable(ts); @@ -321,22 +312,21 @@ static ssize_t ad7879_gpio_store(struct device *dev, { struct ad7879 *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->gpio = !!val; + error = ad7879_write(ts->bus, AD7879_REG_CTRL2, + ts->gpio ? + ts->cmd_crtl2 & ~AD7879_GPIO_DATA : + ts->cmd_crtl2 | AD7879_GPIO_DATA); + mutex_unlock(&ts->mutex); - ret = ad7879_write(ts->bus, AD7879_REG_CTRL2, - ts->gpio ? ts->cmd_crtl2 & ~AD7879_GPIO_DATA - : ts->cmd_crtl2 | AD7879_GPIO_DATA); - - if (ret) - return ret; - - return count; + return error ? : count; } static DEVICE_ATTR(gpio, 0664, ad7879_gpio_show, ad7879_gpio_store); @@ -351,7 +341,7 @@ static const struct attribute_group ad7879_attr_group = { .attrs = ad7879_attributes, }; -static void ad7879_setup(bus_device *bus, struct ad7879 *ts) +static void ad7879_setup(struct ad7879 *ts) { ts->cmd_crtl3 = AD7879_YPLUS_BIT | AD7879_XPLUS_BIT | @@ -371,9 +361,9 @@ static void ad7879_setup(bus_device *bus, struct ad7879 *ts) AD7879_ACQ(ts->acquisition_time) | AD7879_TMR(ts->pen_down_acc_interval); - ad7879_write(bus, AD7879_REG_CTRL2, ts->cmd_crtl2); - ad7879_write(bus, AD7879_REG_CTRL3, ts->cmd_crtl3); - ad7879_write(bus, AD7879_REG_CTRL1, ts->cmd_crtl1); + ad7879_write(ts->bus, AD7879_REG_CTRL2, ts->cmd_crtl2); + ad7879_write(ts->bus, AD7879_REG_CTRL3, ts->cmd_crtl3); + ad7879_write(ts->bus, AD7879_REG_CTRL1, ts->cmd_crtl1); } static int __devinit ad7879_construct(bus_device *bus, struct ad7879 *ts) @@ -401,7 +391,7 @@ static int __devinit ad7879_construct(bus_device *bus, struct ad7879 *ts) setup_timer(&ts->timer, ad7879_timer, (unsigned long) ts); INIT_WORK(&ts->work, ad7879_work); - spin_lock_init(&ts->lock); + mutex_init(&ts->mutex); ts->x_plate_ohms = pdata->x_plate_ohms ? : 400; ts->pressure_max = pdata->pressure_max ? : ~0; @@ -418,7 +408,7 @@ static int __devinit ad7879_construct(bus_device *bus, struct ad7879 *ts) else ts->gpio_init = AD7879_GPIO_EN | AD7879_GPIODIR; - snprintf(ts->phys, sizeof(ts->phys), "%s/inputX", dev_name(&bus->dev)); + snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&bus->dev)); input_dev->name = "AD7879 Touchscreen"; input_dev->phys = ts->phys; @@ -455,10 +445,11 @@ static int __devinit ad7879_construct(bus_device *bus, struct ad7879 *ts) goto err_free_mem; } - ad7879_setup(bus, ts); + ad7879_setup(ts); - err = request_irq(bus->irq, ad7879_irq, IRQF_TRIGGER_FALLING | - IRQF_SAMPLE_RANDOM, bus->dev.driver->name, ts); + err = request_irq(bus->irq, ad7879_irq, + IRQF_TRIGGER_FALLING | IRQF_SAMPLE_RANDOM, + bus->dev.driver->name, ts); if (err) { dev_err(&bus->dev, "irq %d busy?\n", bus->irq); @@ -474,7 +465,7 @@ static int __devinit ad7879_construct(bus_device *bus, struct ad7879 *ts) goto err_remove_attr; dev_info(&bus->dev, "Rev.%d touchscreen, irq %d\n", - revid >> 8, bus->irq); + revid >> 8, bus->irq); return 0; @@ -491,8 +482,6 @@ err_free_mem: static int __devexit ad7879_destroy(bus_device *bus, struct ad7879 *ts) { ad7879_disable(ts); - ad7879_write(ts->bus, AD7879_REG_CTRL2, - AD7879_PM(AD7879_PM_SHUTDOWN)); sysfs_remove_group(&ts->bus->dev.kobj, &ad7879_attr_group); free_irq(ts->bus->irq, ts); input_unregister_device(ts->input); @@ -507,8 +496,6 @@ static int ad7879_suspend(bus_device *bus, pm_message_t message) struct ad7879 *ts = dev_get_drvdata(&bus->dev); ad7879_disable(ts); - ad7879_write(bus, AD7879_REG_CTRL2, - AD7879_PM(AD7879_PM_SHUTDOWN)); return 0; } @@ -517,7 +504,6 @@ static int ad7879_resume(bus_device *bus) { struct ad7879 *ts = dev_get_drvdata(&bus->dev); - ad7879_setup(bus, ts); ad7879_enable(ts); return 0; @@ -531,8 +517,8 @@ static int ad7879_resume(bus_device *bus) #define MAX_SPI_FREQ_HZ 5000000 #define AD7879_CMD_MAGIC 0xE000 #define AD7879_CMD_READ (1 << 10) -#define AD7879_WRITECMD(reg) (AD7879_CMD_MAGIC | (reg & 0xF)) -#define AD7879_READCMD(reg) (AD7879_CMD_MAGIC | AD7879_CMD_READ | (reg & 0xF)) +#define AD7879_WRITECMD(reg) (AD7879_CMD_MAGIC | (reg & 0xF)) +#define AD7879_READCMD(reg) (AD7879_CMD_MAGIC | AD7879_CMD_READ | (reg & 0xF)) struct ser_req { u16 command; @@ -548,9 +534,10 @@ struct ser_req { static int ad7879_read(struct spi_device *spi, u8 reg) { - struct ser_req *req = kzalloc(sizeof *req, GFP_KERNEL); + struct ser_req *req; int status, ret; + req = kzalloc(sizeof *req, GFP_KERNEL); if (!req) return -ENOMEM; @@ -567,11 +554,8 @@ static int ad7879_read(struct spi_device *spi, u8 reg) spi_message_add_tail(&req->xfer[1], &req->msg); status = spi_sync(spi, &req->msg); + ret = status ? : req->data; - if (status == 0) - status = req->msg.status; - - ret = status ? status : req->data; kfree(req); return ret; @@ -579,9 +563,10 @@ static int ad7879_read(struct spi_device *spi, u8 reg) static int ad7879_write(struct spi_device *spi, u8 reg, u16 val) { - struct ser_req *req = kzalloc(sizeof *req, GFP_KERNEL); + struct ser_req *req; int status; + req = kzalloc(sizeof *req, GFP_KERNEL); if (!req) return -ENOMEM; @@ -600,9 +585,6 @@ static int ad7879_write(struct spi_device *spi, u8 reg, u16 val) status = spi_sync(spi, &req->msg); - if (status == 0) - status = req->msg.status; - kfree(req); return status; @@ -611,6 +593,7 @@ static int ad7879_write(struct spi_device *spi, u8 reg, u16 val) static void ad7879_collect(struct ad7879 *ts) { int status = spi_sync(ts->bus, &ts->msg); + if (status) dev_err(&ts->bus->dev, "spi_sync --> %d\n", status); } @@ -639,7 +622,7 @@ static void ad7879_setup_ts_def_msg(struct ad7879 *ts) static int __devinit ad7879_probe(struct spi_device *spi) { struct ad7879 *ts; - int ret; + int error; /* don't exceed max specified SPI CLK frequency */ if (spi->max_speed_hz > MAX_SPI_FREQ_HZ) { @@ -656,14 +639,13 @@ static int __devinit ad7879_probe(struct spi_device *spi) ad7879_setup_ts_def_msg(ts); - ret = ad7879_construct(spi, ts); - if (!ret) - return ret; - - dev_set_drvdata(&spi->dev, NULL); - kfree(ts); + error = ad7879_construct(spi, ts); + if (error) { + dev_set_drvdata(&spi->dev, NULL); + kfree(ts); + } - return ret; + return 0; } static int __devexit ad7879_remove(struct spi_device *spi) @@ -673,6 +655,7 @@ static int __devexit ad7879_remove(struct spi_device *spi) ad7879_destroy(spi, ts); dev_set_drvdata(&spi->dev, NULL); kfree(ts); + return 0; } @@ -718,16 +701,17 @@ static int ad7879_write(struct i2c_client *client, u8 reg, u16 val) static void ad7879_collect(struct ad7879 *ts) { int i; + for (i = 0; i < AD7879_NR_SENSE; i++) - ts->conversion_data[i] = - ad7879_read(ts->bus, AD7879_REG_XPLUS + i); + ts->conversion_data[i] = ad7879_read(ts->bus, + AD7879_REG_XPLUS + i); } static int __devinit ad7879_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct ad7879 *ts; - int ret; + int error; if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) { @@ -742,14 +726,13 @@ static int __devinit ad7879_probe(struct i2c_client *client, i2c_set_clientdata(client, ts); ts->bus = client; - ret = ad7879_construct(client, ts); - if (!ret) - return ret; - - i2c_set_clientdata(client, NULL); - kfree(ts); + error = ad7879_construct(client, ts); + if (error) { + i2c_set_clientdata(client, NULL); + kfree(ts); + } - return ret; + return 0; } static int __devexit ad7879_remove(struct i2c_client *client) @@ -759,8 +742,10 @@ static int __devexit ad7879_remove(struct i2c_client *client) ad7879_destroy(client, ts); i2c_set_clientdata(client, NULL); kfree(ts); + return 0; } + static const struct i2c_device_id ad7979_id[] = { { "ad7879", 0 }, { } @@ -776,7 +761,7 @@ static struct i2c_driver ad7879_driver = { .remove = __devexit_p(ad7879_remove), .suspend = ad7879_suspend, .resume = ad7879_resume, - .id_table = ad7979_id, + .id_table = ad7979_id, }; static int __init ad7879_init(void) -- 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
