Skip to contentSkip to conversation

Current ad7879 patches

PATCH In review Dmitry Torokhov 3 participants 10 messages Jun 25, 2010 – Jun 30, 2010

Subsystems (3)
lore
Share & tools
lore

Dmitry Torokhov

author maintainer
#0 lore raw

cover Current ad7879 patches

Mike, Michael,

I was looking through outstanding patches to ad7879 and here they are. Since I made some changes to the patches you wrote and there also some new code and I do not have the hardware I'd appreciate if you could try them out so they can be applied.

Thank you.

Signature · 21 lines
Dmitry

Dmitry Torokhov (2):
      Input: ad7879 - add open and close methods
      Input: ad7879 - use threaded IRQ

Michael Hennerich (1):
      Input: ad7879 - use i2c_smbus_read_i2c_block_data() to lower overhead

Mike Frysinger (1):
      Input: ad7879 - split bus logic out


 drivers/input/touchscreen/ad7879-i2c.c |  143 ++++++++
 drivers/input/touchscreen/ad7879-spi.c |  190 ++++++++++
 drivers/input/touchscreen/ad7879.c     |  606 +++++++++-----------------------
 drivers/input/touchscreen/ad7879.h     |   30 ++
 4 files changed, 533 insertions(+), 436 deletions(-)
 create mode 100644 drivers/input/touchscreen/ad7879-i2c.c
 create mode 100644 drivers/input/touchscreen/ad7879-spi.c
 create mode 100644 drivers/input/touchscreen/ad7879.h
author maintainer
#1 lore raw

PATCH 1/4 Input: ad7879 - use threaded IRQ

---

 drivers/input/touchscreen/ad7879.c |   36 +++++++++---------------------------
 1 files changed, 9 insertions(+), 27 deletions(-)
drivers/input/touchscreen/ad7879.c +9 -27
index 4b32fb4..f947457 100644
@@ -44,7 +44,6 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/slab.h>
-#include <linux/workqueue.h>
 #include <linux/spi/spi.h>
 #include <linux/i2c.h>
 #include <linux/gpio.h>
@@ -131,13 +130,12 @@ typedef struct i2c_client	bus_device;
 struct ad7879 {
 	bus_device		*bus;
 	struct input_dev	*input;
-	struct work_struct	work;
 	struct timer_list	timer;
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip	gc;
 #endif
 	struct mutex		mutex;
-	unsigned		disabled:1;	/* P: mutex */
+	bool			disabled;	/* P: mutex */
 
 #if defined(CONFIG_TOUCHSCREEN_AD7879_SPI) || defined(CONFIG_TOUCHSCREEN_AD7879_SPI_MODULE)
 	struct spi_message	msg;
@@ -196,16 +194,6 @@ static void ad7879_report(struct ad7879 *ts)
 	}
 }
 
-static void ad7879_work(struct work_struct *work)
-{
-	struct ad7879 *ts = container_of(work, struct ad7879, work);
-
-	/* use keventd context to read the result registers */
-	ad7879_collect(ts);
-	ad7879_report(ts);
-	mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
-}
-
 static void ad7879_ts_event_release(struct ad7879 *ts)
 {
 	struct input_dev *input_dev = ts->input;
@@ -225,13 +213,10 @@ static irqreturn_t ad7879_irq(int irq, void *handle)
 {
 	struct ad7879 *ts = handle;
 
-	/* 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
-	 */
+	ad7879_collect(ts);
+	ad7879_report(ts);
 
-	if (!work_pending(&ts->work))
-		schedule_work(&ts->work);
+	mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
 
 	return IRQ_HANDLED;
 }
@@ -249,11 +234,9 @@ static void ad7879_disable(struct ad7879 *ts)
 
 	if (!ts->disabled) {
 
-		ts->disabled = 1;
+		ts->disabled = true;
 		disable_irq(ts->bus->irq);
 
-		cancel_work_sync(&ts->work);
-
 		if (del_timer_sync(&ts->timer))
 			ad7879_ts_event_release(ts);
 
@@ -270,7 +253,7 @@ static void ad7879_enable(struct ad7879 *ts)
 
 	if (ts->disabled) {
 		ad7879_setup(ts);
-		ts->disabled = 0;
+		ts->disabled = false;
 		enable_irq(ts->bus->irq);
 	}
 
@@ -458,7 +441,6 @@ static int __devinit ad7879_construct(bus_device *bus, struct ad7879 *ts)
 	ts->input = input_dev;
 
 	setup_timer(&ts->timer, ad7879_timer, (unsigned long) ts);
-	INIT_WORK(&ts->work, ad7879_work);
 	mutex_init(&ts->mutex);
 
 	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
@@ -526,9 +508,9 @@ static int __devinit ad7879_construct(bus_device *bus, struct ad7879 *ts)
 
 	ad7879_setup(ts);
 
-	err = request_irq(bus->irq, ad7879_irq,
-			  IRQF_TRIGGER_FALLING, bus->dev.driver->name, ts);
-
+	err = request_threaded_irq(bus->irq, NULL, ad7879_irq,
+				   IRQF_TRIGGER_FALLING,
+				   bus->dev.driver->name, ts);
 	if (err) {
 		dev_err(&bus->dev, "irq %d busy?\n", bus->irq);
 		goto err_free_mem;
author maintainer
#2 lore raw

PATCH 2/4 Input: ad7879 - split bus logic out

In reply to Dmitry Torokhov

From: Mike Frysinger <[email protected]>

The ad7879 driver is using the old bus method of only supporting one at a time (I2C or SPI). So refactor it like the other input drivers that support multiple busses simultaneously.

---

 drivers/input/touchscreen/ad7879-i2c.c |  140 +++++++++
 drivers/input/touchscreen/ad7879-spi.c |  190 ++++++++++++
 drivers/input/touchscreen/ad7879.c     |  495 +++++++-------------------------
 drivers/input/touchscreen/ad7879.h     |   30 ++
 4 files changed, 461 insertions(+), 394 deletions(-)
 create mode 100644 drivers/input/touchscreen/ad7879-i2c.c
 create mode 100644 drivers/input/touchscreen/ad7879-spi.c
 create mode 100644 drivers/input/touchscreen/ad7879.h
drivers/input/touchscreen/ad7879-i2c.c new file +140 -0
new file mode 100644
index 0000000..41b32cb
@@ -0,0 +1,140 @@
+/*
+ * AD7879-1/AD7889-1 touchscreen (I2C bus)
+ *
+ * Copyright (C) 2008-2010 Michael Hennerich, Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#include <linux/input.h>	/* BUS_I2C */
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/types.h>
+
+#include "ad7879.h"
+
+#define AD7879_DEVID		0x79	/* AD7879-1/AD7889-1 */
+
+#ifdef CONFIG_PM
+static int ad7879_i2c_suspend(struct i2c_client *client, pm_message_t message)
+{
+	struct ad7879 *ts = i2c_get_clientdata(client);
+
+	ad7879_disable(ts);
+
+	return 0;
+}
+
+static int ad7879_i2c_resume(struct i2c_client *client)
+{
+	struct ad7879 *ts = i2c_get_clientdata(client);
+
+	ad7879_enable(ts);
+
+	return 0;
+}
+#else
+# define ad7879_i2c_suspend NULL
+# define ad7879_i2c_resume  NULL
+#endif
+
+/* All registers are word-sized.
+ * AD7879 uses a high-byte first convention.
+ */
+static int ad7879_i2c_read(struct device *dev, u8 reg)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return swab16(i2c_smbus_read_word_data(client, reg));
+}
+
+static int ad7879_i2c_multi_read(struct device *dev,
+				 u8 first_reg, u8 count, u16 *buf)
+{
+	u8 idx;
+
+	for (idx = 0; idx < count; ++idx)
+		buf[idx] = ad7879_i2c_read(dev, first_reg + idx);
+
+	return 0;
+}
+
+static int ad7879_i2c_write(struct device *dev, u8 reg, u16 val)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return i2c_smbus_write_word_data(client, reg, swab16(val));
+}
+
+static const struct ad7879_bus_ops ad7879_i2c_bus_ops = {
+	.bustype	= BUS_I2C,
+	.read		= ad7879_i2c_read,
+	.multi_read	= ad7879_i2c_multi_read,
+	.write		= ad7879_i2c_write,
+};
+
+static int __devinit ad7879_i2c_probe(struct i2c_client *client,
+				      const struct i2c_device_id *id)
+{
+	struct ad7879 *ts;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_WORD_DATA)) {
+		dev_err(&client->dev, "SMBUS Word Data not Supported\n");
+		return -EIO;
+	}
+
+	ts = ad7879_probe(&client->dev, client->irq, AD7879_DEVID,
+			  &ad7879_i2c_bus_ops);
+	if (IS_ERR(ts))
+		return PTR_ERR(ts);
+
+	i2c_set_clientdata(client, ts);
+
+	return 0;
+}
+
+static int __devexit ad7879_i2c_remove(struct i2c_client *client)
+{
+	struct ad7879 *ts = i2c_get_clientdata(client);
+
+	ad7879_remove(ts);
+
+	return 0;
+}
+
+static const struct i2c_device_id ad7879_id[] = {
+	{ "ad7879", 0 },
+	{ "ad7889", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ad7879_id);
+
+static struct i2c_driver ad7879_i2c_driver = {
+	.driver = {
+		.name	= "ad7879",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= ad7879_i2c_probe,
+	.remove		= __devexit_p(ad7879_i2c_remove),
+	.suspend	= ad7879_i2c_suspend,
+	.resume		= ad7879_i2c_resume,
+	.id_table	= ad7879_id,
+};
+
+static int __init ad7879_i2c_init(void)
+{
+	return i2c_add_driver(&ad7879_i2c_driver);
+}
+module_init(ad7879_i2c_init);
+
+static void __exit ad7879_i2c_exit(void)
+{
+	i2c_del_driver(&ad7879_i2c_driver);
+}
+module_exit(ad7879_i2c_exit);
+
+MODULE_AUTHOR("Michael Hennerich <[email protected]>");
+MODULE_DESCRIPTION("AD7879(-1) touchscreen I2C bus driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("i2c:ad7879");
drivers/input/touchscreen/ad7879-spi.c new file +190 -0

190 lines not on this page · raw message

drivers/input/touchscreen/ad7879.c +101 -394

698 lines not on this page · raw message

drivers/input/touchscreen/ad7879.h new file +30 -0
new file mode 100644
index 0000000..169f155
@@ -0,0 +1,30 @@
+/*
+ * AD7879/AD7889 touchscreen (bus interfaces)
+ *
+ * Copyright (C) 2008-2010 Michael Hennerich, Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#ifndef _AD7879_H_
+#define _AD7879_H_
+
+#include <linux/types.h>
+
+struct ad7879;
+struct device;
+
+struct ad7879_bus_ops {
+	u16 bustype;
+	int (*read)(struct device *dev, u8 reg);
+	int (*multi_read)(struct device *dev, u8 first_reg, u8 count, u16 *buf);
+	int (*write)(struct device *dev, u8 reg, u16 val);
+};
+
+void ad7879_disable(struct ad7879 *);
+void ad7879_enable(struct ad7879 *);
+struct ad7879 *ad7879_probe(struct device *dev, u8 devid, unsigned irq,
+			    const struct ad7879_bus_ops *bops);
+void ad7879_remove(struct ad7879 *);
+
+#endif
#3 lore raw

Re: [PATCH 2/4] Input: ad7879 - split bus logic out

Quoted 11 lines · On Friday, June 25, 2010 04:22:24 Dmitry Torokhov wrote:

From: Mike Frysinger <[email protected]>

The ad7879 driver is using the old bus method of only supporting one at a time (I2C or SPI). So refactor it like the other input drivers that support multiple busses simultaneously.

---

Quoted 9 lines
 drivers/input/touchscreen/ad7879-i2c.c |  140 +++++++++
 drivers/input/touchscreen/ad7879-spi.c |  190 ++++++++++++
 drivers/input/touchscreen/ad7879.c     |  495
+++++++------------------------- drivers/input/touchscreen/ad7879.h     | 
 30 ++
 4 files changed, 461 insertions(+), 394 deletions(-)
 create mode 100644 drivers/input/touchscreen/ad7879-i2c.c
 create mode 100644 drivers/input/touchscreen/ad7879-spi.c
 create mode 100644 drivers/input/touchscreen/ad7879.h

i think you forgot the Kconfig and Makefile changes. without those, the driver cannot be configured/compiled correctly.

i also see you changed the code from passing a struct device pointer to a struct ad7879 pointer. i went with the struct device because it generated smaller code due to pushing down of the dev get/set drvdata. the i2c struct sucks because it doesnt stick its dev member at the start of the structure. -mike

author maintainer
#4 lore raw

PATCH 3/4 Input: ad7879 - add open and close methods

In reply to Dmitry Torokhov

---

 drivers/input/touchscreen/ad7879-i2c.c |    4 +
 drivers/input/touchscreen/ad7879-spi.c |    4 +
 drivers/input/touchscreen/ad7879.c     |  105 +++++++++++++++++++++++---------
 drivers/input/touchscreen/ad7879.h     |    4 +
 4 files changed, 81 insertions(+), 36 deletions(-)
drivers/input/touchscreen/ad7879-i2c.c +2 -2
index 41b32cb..f75f753 100644
@@ -20,7 +20,7 @@ static int ad7879_i2c_suspend(struct i2c_client *client, pm_message_t message)
 {
 	struct ad7879 *ts = i2c_get_clientdata(client);
 
-	ad7879_disable(ts);
+	ad7879_suspend(ts);
 
 	return 0;
 }
@@ -29,7 +29,7 @@ static int ad7879_i2c_resume(struct i2c_client *client)
 {
 	struct ad7879 *ts = i2c_get_clientdata(client);
 
-	ad7879_enable(ts);
+	ad7879_resume(ts);
 
 	return 0;
 }
drivers/input/touchscreen/ad7879-spi.c +2 -2
index cd12dac..6e32f3a 100644
@@ -25,7 +25,7 @@ static int ad7879_spi_suspend(struct spi_device *spi, pm_message_t message)
 {
 	struct ad7879 *ts = spi_get_drvdata(spi);
 
-	ad7879_disable(ts);
+	ad7879_suspend(ts);
 
 	return 0;
 }
@@ -34,7 +34,7 @@ static int ad7879_spi_resume(struct spi_device *spi)
 {
 	struct ad7879 *ts = spi_get_drvdata(spi);
 
-	ad7879_enable(ts);
+	ad7879_resume(ts);
 
 	return 0;
 }
drivers/input/touchscreen/ad7879.c +75 -30

168 lines not on this page · raw message

drivers/input/touchscreen/ad7879.h +2 -2
index 169f155..6b45a27 100644
@@ -21,8 +21,8 @@ struct ad7879_bus_ops {
 	int (*write)(struct device *dev, u8 reg, u16 val);
 };
 
-void ad7879_disable(struct ad7879 *);
-void ad7879_enable(struct ad7879 *);
+void ad7879_suspend(struct ad7879 *);
+void ad7879_resume(struct ad7879 *);
 struct ad7879 *ad7879_probe(struct device *dev, u8 devid, unsigned irq,
 			    const struct ad7879_bus_ops *bops);
 void ad7879_remove(struct ad7879 *);
author maintainer
#5 lore raw

PATCH 4/4 Input: ad7879 - use i2c_smbus_read_i2c_block_data() to lower overhead

In reply to Dmitry Torokhov

From: Michael Hennerich <[email protected]>

Avoid additional addressing overhead incurred by word_data transfers.

---

 drivers/input/touchscreen/ad7879-i2c.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/input/touchscreen/ad7879-i2c.c +4 -1
index f75f753..79e3857 100644
@@ -51,10 +51,13 @@ static int ad7879_i2c_read(struct device *dev, u8 reg)
 static int ad7879_i2c_multi_read(struct device *dev,
 				 u8 first_reg, u8 count, u16 *buf)
 {
+	struct i2c_client *client = to_i2c_client(dev);
 	u8 idx;
 
+	i2c_smbus_read_i2c_block_data(client, first_reg, count * 2, (u8 *)buf);
+
 	for (idx = 0; idx < count; ++idx)
-		buf[idx] = ad7879_i2c_read(dev, first_reg + idx);
+		buf[idx] = swab16(buf[idx]);
 
 	return 0;
 }
#6 lore raw
In reply to Dmitry Torokhov

Dmitry Torokhov wrote on 2010-06-25:

Quoted 9 lines

Mike, Michael,

I was looking through outstanding patches to ad7879 and here they are. Since I made some changes to the patches you wrote and there also some new code and I do not have the hardware I'd appreciate if you could try them out so they can be applied.

Thank you.

Hi Dmitry,

I tested your ad7879 patch set -
A few issues see patch below.

-Fix ad7879_probe() parameter passing.
-Add missing input_set_drvdata()
-Avoid unbalanced irq enable warning: disable_irq() after request.
-Fix sysfs disable hook logic: the file is called disable so don't treat it as enable.

 drivers/input/touchscreen/ad7879-i2c.c |    2 +-
 drivers/input/touchscreen/ad7879-spi.c |    2 +-
 drivers/input/touchscreen/ad7879.c     |    6 +++++-
 3 files changed, 7 insertions(+), 3 deletions(-)
drivers/input/touchscreen/ad7879-i2c.c +1 -1
index 79e3857..d82a38e 100644
@@ -87,7 +87,7 @@ static int __devinit ad7879_i2c_probe(struct i2c_client *client,
                return -EIO;
        }
 
-       ts = ad7879_probe(&client->dev, client->irq, AD7879_DEVID,
+       ts = ad7879_probe(&client->dev, AD7879_DEVID, client->irq,
                          &ad7879_i2c_bus_ops);
        if (IS_ERR(ts))
                return PTR_ERR(ts);
drivers/input/touchscreen/ad7879-spi.c +1 -1
index 6e32f3a..9fa8e5d 100644
@@ -141,7 +141,7 @@ static int __devinit ad7879_spi_probe(struct spi_device *spi)
                return -EINVAL;
        }
 
-       ts = ad7879_probe(&spi->dev, spi->irq, AD7879_DEVID, &ad7879_spi_bus_ops);
+       ts = ad7879_probe(&spi->dev, AD7879_DEVID, spi->irq, &ad7879_spi_bus_ops);
        if (IS_ERR(ts))
                return PTR_ERR(ts);
 
drivers/input/touchscreen/ad7879.c +5 -1
index 4e2c1b7..1ac0713 100644
@@ -315,7 +315,7 @@ static ssize_t ad7879_disable_store(struct device *dev,
        if (error)
                return error;
 
-       ad7879_toggle(ts, val);
+       ad7879_toggle(ts, !val);
 
        return count;
 }
@@ -557,6 +557,8 @@ struct ad7879 *ad7879_probe(struct device *dev, u8 devid, unsigned int irq,
                goto err_free_mem;
        }
 
+       disable_irq(ts->irq);   /* request_irq returns with irqs enabled */
+
        err = sysfs_create_group(&dev->kobj, &ad7879_attr_group);
        if (err)
                goto err_free_irq;
@@ -565,6 +567,8 @@ struct ad7879 *ad7879_probe(struct device *dev, u8 devid, unsigned int irq,
        if (err)
                goto err_remove_attr;
 
+       input_set_drvdata(input_dev, ts);
+
        err = input_register_device(input_dev);
        if (err)
                goto err_remove_gpio;

Greetings,
Michael

Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

 drivers/input/touchscreen/ad7879-i2c.c |    2 +-
 drivers/input/touchscreen/ad7879-spi.c |    2 +-
 drivers/input/touchscreen/ad7879.c     |    6 +++++-
 3 files changed, 7 insertions(+), 3 deletions(-)
drivers/input/touchscreen/ad7879-i2c.c +1 -1
index 79e3857..d82a38e 100644
@@ -87,7 +87,7 @@ static int __devinit ad7879_i2c_probe(struct i2c_client *client,
 		return -EIO;
 	}
 
-	ts = ad7879_probe(&client->dev, client->irq, AD7879_DEVID,
+	ts = ad7879_probe(&client->dev, AD7879_DEVID, client->irq,
 			  &ad7879_i2c_bus_ops);
 	if (IS_ERR(ts))
 		return PTR_ERR(ts);
drivers/input/touchscreen/ad7879-spi.c +1 -1
index 6e32f3a..9fa8e5d 100644
@@ -141,7 +141,7 @@ static int __devinit ad7879_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	ts = ad7879_probe(&spi->dev, spi->irq, AD7879_DEVID, &ad7879_spi_bus_ops);
+	ts = ad7879_probe(&spi->dev, AD7879_DEVID, spi->irq, &ad7879_spi_bus_ops);
 	if (IS_ERR(ts))
 		return PTR_ERR(ts);
 
drivers/input/touchscreen/ad7879.c +5 -1
index 4e2c1b7..1ac0713 100644
@@ -315,7 +315,7 @@ static ssize_t ad7879_disable_store(struct device *dev,
 	if (error)
 		return error;
 
-	ad7879_toggle(ts, val);
+	ad7879_toggle(ts, !val);
 
 	return count;
 }
@@ -557,6 +557,8 @@ struct ad7879 *ad7879_probe(struct device *dev, u8 devid, unsigned int irq,
 		goto err_free_mem;
 	}
 
+	disable_irq(ts->irq);	/* request_irq returns with irqs enabled */
+
 	err = sysfs_create_group(&dev->kobj, &ad7879_attr_group);
 	if (err)
 		goto err_free_irq;
@@ -565,6 +567,8 @@ struct ad7879 *ad7879_probe(struct device *dev, u8 devid, unsigned int irq,
 	if (err)
 		goto err_remove_attr;
 
+	input_set_drvdata(input_dev, ts);
+
 	err = input_register_device(input_dev);
 	if (err)
 		goto err_remove_gpio;
author maintainer
#7 lore raw
Quoted 17 lines · Michael

Dmitry Torokhov wrote on 2010-06-25:

Quoted 9 lines

Mike, Michael,

I was looking through outstanding patches to ad7879 and here they are. Since I made some changes to the patches you wrote and there also some new code and I do not have the hardware I'd appreciate if you could try them out so they can be applied.

Thank you.

Hi Dmitry,

I tested your ad7879 patch set -
A few issues see patch below.

Review

Quoted 4 lines
(quoted diff) +0 -4
-Fix ad7879_probe() parameter passing.
-Add missing input_set_drvdata()
-Avoid unbalanced irq enable warning: disable_irq() after request.
-Fix sysfs disable hook logic: the file is called disable so don't treat it as enable.

Thanks for catching all the mistakes Michael. Regardgin disable_irq() - do you think it would be even better to call __ad7879_disable() there to make sure the chip is in low power mode? And I guess we need to add matching enable_irq() in error handlig path and in ad7879_remove() in case we reload the driver...

BTW, I will be applying the other 2 patches you sent.

Signature · 1 line
Dmitry
#8 lore raw

Dmitry Torokhov wrote on 2010-06-30:

Quoted 17 lines · Michael
Quoted 16 lines

Dmitry Torokhov wrote on 2010-06-25:

Quoted 9 lines

Mike, Michael,

I was looking through outstanding patches to ad7879 and here they are. Since I made some changes to the patches you wrote and there also some new code and I do not have the hardware I'd appreciate if you could try them out so they can be applied.

Thank you.

Hi Dmitry,

I tested your ad7879 patch set -
A few issues see patch below.

Quoted 9 lines
Quoted 5 lines
(quoted diff) +0 -2
-Fix ad7879_probe() parameter passing. -Add missing input_set_drvdata()
-Avoid unbalanced irq enable warning: disable_irq() after request. -Fix

sysfs disable hook logic: the file is called disable so don't treat it as enable.

Thanks for catching all the mistakes Michael. Regardgin disable_irq()
- do you think it would be even better to call __ad7879_disable() there to make sure the chip is in low power mode?

Right - __ad7879_disable() is a good option here.

Quoted 3 lines

And I guess we need
to add matching enable_irq() in error handlig path and in ad7879_remove() in case we reload the driver...

I don't see a problem here. free_irq() will take care and unconditional disable the irq for us. Both the error return as well as the module remove path will call free_irq().

Quoted 2 lines

BTW, I will be applying the other 2 patches you sent.

Thanks

Greetings,
Michael

Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

author maintainer
#9 lore raw
Quoted 30 lines · Michael

Dmitry Torokhov wrote on 2010-06-30:

Quoted 17 lines · Michael
Quoted 16 lines

Dmitry Torokhov wrote on 2010-06-25:

Quoted 9 lines

Mike, Michael,

I was looking through outstanding patches to ad7879 and here they are. Since I made some changes to the patches you wrote and there also some new code and I do not have the hardware I'd appreciate if you could try them out so they can be applied.

Thank you.

Hi Dmitry,

I tested your ad7879 patch set -
A few issues see patch below.

Quoted 9 lines
Quoted 5 lines
(quoted diff) +0 -2
-Fix ad7879_probe() parameter passing. -Add missing input_set_drvdata()
-Avoid unbalanced irq enable warning: disable_irq() after request. -Fix

sysfs disable hook logic: the file is called disable so don't treat it as enable.

Thanks for catching all the mistakes Michael. Regardgin disable_irq()
- do you think it would be even better to call __ad7879_disable() there to make sure the chip is in low power mode?

Right - __ad7879_disable() is a good option here.

Quoted 7 lines
Quoted 3 lines

And I guess we need
to add matching enable_irq() in error handlig path and in ad7879_remove() in case we reload the driver...

I don't see a problem here. free_irq() will take care and unconditional disable the irq for us. Both the error return as well as the module remove path will call free_irq().

free_irq() will disable the irq in the chip and will set desc->action to NULL but will not alter desc->depth as far as I can see.

I guess we can rely on the fact that request_irq resets desc->depth for non-shared IRQs...

Signature · 1 line
Dmitry