[PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[michael.hennerich]

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

Suppress events where pressure > pressure_max.
These events come typically along with inaccurate X and Y samples.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/input/touchscreen/ad7877.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index 96c0a0d..e6917db 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -360,6 +360,13 @@ static int ad7877_rx(struct ad7877 *ts)
 		Rt /= z1;
 		Rt = (Rt + 2047) >> 12;
 
+		/*
+		 * Sample found inconsistent, pressure is beyond
+		 * the maximum. Don't report it to user space.
+		 */
+	 	if (Rt > ts->pressure_max)
+	 		return -EINVAL;
+
 		if (!timer_pending(&ts->timer))
 			input_report_key(input_dev, BTN_TOUCH, 1);
 
-- 
1.6.0.2

[PATCH 1/3] Input: touchscreen: ad7877 implement specified chip select behavior

[michael.hennerich]

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

According to the AD7877 datasheet:
Each transfer operation is 16-bit. If multiple read/write operations are
to be performed, CS must be taken high after the end of each read/write
operation before another read/write operation can be performed by
taking CS low again.

Make sure CS toggles after each transfer in the message.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/input/touchscreen/ad7877.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index 5f0221c..53f4d79 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -230,6 +230,7 @@ static int ad7877_read(struct spi_device *spi, u16 reg)
 			AD7877_READADD(reg));
 	req->xfer[0].tx_buf = &req->command;
 	req->xfer[0].len = 2;
+	req->xfer[0].cs_change = 1;
 
 	req->xfer[1].rx_buf = &req->sample;
 	req->xfer[1].len = 2;
@@ -295,20 +296,25 @@ static int ad7877_read_adc(struct spi_device *spi, unsigned command)
 
 	req->xfer[0].tx_buf = &req->reset;
 	req->xfer[0].len = 2;
+	req->xfer[0].cs_change = 1;
 
 	req->xfer[1].tx_buf = &req->ref_on;
 	req->xfer[1].len = 2;
 	req->xfer[1].delay_usecs = ts->vref_delay_usecs;
+	req->xfer[1].cs_change = 1;
 
 	req->xfer[2].tx_buf = &req->command;
 	req->xfer[2].len = 2;
 	req->xfer[2].delay_usecs = ts->vref_delay_usecs;
+	req->xfer[2].cs_change = 1;
 
 	req->xfer[3].rx_buf = &req->sample;
 	req->xfer[3].len = 2;
+	req->xfer[3].cs_change = 1;
 
 	req->xfer[4].tx_buf = &ts->cmd_crtl2;	/*REF OFF*/
 	req->xfer[4].len = 2;
+	req->xfer[4].cs_change = 1;
 
 	req->xfer[5].tx_buf = &ts->cmd_crtl1;	/*DEFAULT*/
 	req->xfer[5].len = 2;
@@ -640,17 +646,21 @@ static void ad7877_setup_ts_def_msg(struct spi_device *spi, struct ad7877 *ts)
 
 	ts->xfer[0].tx_buf = &ts->cmd_crtl1;
 	ts->xfer[0].len = 2;
+	ts->xfer[0].cs_change = 1;
 
 	spi_message_add_tail(&ts->xfer[0], m);
 
 	ts->xfer[1].tx_buf = &ts->cmd_dummy; /* Send ZERO */
 	ts->xfer[1].len = 2;
+	ts->xfer[1].cs_change = 1;
 
 	spi_message_add_tail(&ts->xfer[1], m);
 
-	for (i = 0; i < 11; i++) {
+	for (i = 0; i < AD7877_NR_SENSE; i++) {
 		ts->xfer[i + 2].rx_buf = &ts->conversion_data[AD7877_SEQ_YPOS + i];
 		ts->xfer[i + 2].len = 2;
+		if (i < (AD7877_NR_SENSE - 1))
+			ts->xfer[i + 2].cs_change = 1;
 		spi_message_add_tail(&ts->xfer[i + 2], m);
 	}
 }
-- 
1.6.0.2

[PATCH 2/3] Input: touchscreen: ad7877 implement EV_KEY:BTN_TOUCH reporting

[michael.hennerich]

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

Some input users such as Android or X require BTN_TOUCH events.
Implement EV_KEY:BTN_TOUCH and make sure that the release event
is not erroneous scheduled without a preceding valid touch.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/input/touchscreen/ad7877.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index 53f4d79..96c0a0d 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -333,7 +333,7 @@ static int ad7877_read_adc(struct spi_device *spi, unsigned command)
 	return status ? : sample;
 }
 
-static void ad7877_rx(struct ad7877 *ts)
+static int ad7877_rx(struct ad7877 *ts)
 {
 	struct input_dev *input_dev = ts->input;
 	unsigned Rt;
@@ -360,11 +360,17 @@ static void ad7877_rx(struct ad7877 *ts)
 		Rt /= z1;
 		Rt = (Rt + 2047) >> 12;
 
+		if (!timer_pending(&ts->timer))
+			input_report_key(input_dev, BTN_TOUCH, 1);
+
 		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);
+		return 0;
 	}
+
+	return -EINVAL;
 }
 
 static inline void ad7877_ts_event_release(struct ad7877 *ts)
@@ -372,6 +378,7 @@ 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_report_key(input_dev, BTN_TOUCH, 0);
 	input_sync(input_dev);
 }
 
@@ -413,11 +420,9 @@ static void ad7877_callback(void *_ts)
 	struct ad7877 *ts = _ts;
 
 	spin_lock_irq(&ts->lock);
-
-	ad7877_rx(ts);
+	if (!ad7877_rx(ts))
+		mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
 	ts->pending = 0;
-	mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
-
 	spin_unlock_irq(&ts->lock);
 }
 
@@ -728,6 +733,8 @@ static int __devinit ad7877_probe(struct spi_device *spi)
 	input_dev->phys = ts->phys;
 	input_dev->dev.parent = &spi->dev;
 
+	__set_bit(EV_KEY, input_dev->evbit);
+	__set_bit(BTN_TOUCH, input_dev->keybit);
 	__set_bit(EV_ABS, input_dev->evbit);
 	__set_bit(ABS_X, input_dev->absbit);
 	__set_bit(ABS_Y, input_dev->absbit);
-- 
1.6.0.2

[PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[michael.hennerich]

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

Suppress events where pressure > pressure_max.
These events come typically along with inaccurate X and Y samples.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/input/touchscreen/ad7877.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index 96c0a0d..e6917db 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -360,6 +360,13 @@ static int ad7877_rx(struct ad7877 *ts)
 		Rt /= z1;
 		Rt = (Rt + 2047) >> 12;
 
+		/*
+		 * Sample found inconsistent, pressure is beyond
+		 * the maximum. Don't report it to user space.
+		 */
+	 	if (Rt > ts->pressure_max)
+	 		return -EINVAL;
+
 		if (!timer_pending(&ts->timer))
 			input_report_key(input_dev, BTN_TOUCH, 1);
 
-- 
1.6.0.2

Re: [PATCH 1/3] Input: touchscreen: ad7877 implement specified chip select behavior

[Dmitry Torokhov]

On Fri, Oct 15, 2010 at 12:40:48PM +0200, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> According to the AD7877 datasheet:
> Each transfer operation is 16-bit. If multiple read/write operations are
> to be performed, CS must be taken high after the end of each read/write
> operation before another read/write operation can be performed by
> taking CS low again.
> 
> Make sure CS toggles after each transfer in the message.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>

Will apply all 3, thanks Michael.

-- 
Dmitry

Re: [Device-drivers-devel] [PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[Mike Frysinger]

On Fri, Oct 15, 2010 at 06:40,  <michael.hennerich@analog.com> wrote:
> Suppress events where pressure > pressure_max.
> These events come typically along with inaccurate X and Y samples.

were you going to commit to the blackfin tree ?

> --- a/drivers/input/touchscreen/ad7877.c
> +++ b/drivers/input/touchscreen/ad7877.c
> @@ -360,6 +360,13 @@ static int ad7877_rx(struct ad7877 *ts)
>                Rt /= z1;
>                Rt = (Rt + 2047) >> 12;
>
> +               /*
> +                * Sample found inconsistent, pressure is beyond
> +                * the maximum. Don't report it to user space.
> +                */
> +               if (Rt > ts->pressure_max)
> +                       return -EINVAL;

this has spaces in the middle of your tab indents ...
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Device-drivers-devel] [PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[Dmitry Torokhov]

On Fri, Oct 15, 2010 at 09:51:12PM -0400, Mike Frysinger wrote:
> On Fri, Oct 15, 2010 at 06:40,  <michael.hennerich@analog.com> wrote:
> > Suppress events where pressure > pressure_max.
> > These events come typically along with inaccurate X and Y samples.
> 
> were you going to commit to the blackfin tree ?
> 
> > --- a/drivers/input/touchscreen/ad7877.c
> > +++ b/drivers/input/touchscreen/ad7877.c
> > @@ -360,6 +360,13 @@ static int ad7877_rx(struct ad7877 *ts)
> >                Rt /= z1;
> >                Rt = (Rt + 2047) >> 12;
> >
> > +               /*
> > +                * Sample found inconsistent, pressure is beyond
> > +                * the maximum. Don't report it to user space.
> > +                */
> > +               if (Rt > ts->pressure_max)
> > +                       return -EINVAL;
> 
> this has spaces in the middle of your tab indents ...

I took care of that on my side...

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Device-drivers-devel] [PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[Dmitry Torokhov]

On Sun, Oct 17, 2010 at 09:08:10PM -0700, Dmitry Torokhov wrote:
> On Fri, Oct 15, 2010 at 09:51:12PM -0400, Mike Frysinger wrote:
> > On Fri, Oct 15, 2010 at 06:40,  <michael.hennerich@analog.com> wrote:
> > > Suppress events where pressure > pressure_max.
> > > These events come typically along with inaccurate X and Y samples.
> > 
> > were you going to commit to the blackfin tree ?
> > 
> > > --- a/drivers/input/touchscreen/ad7877.c
> > > +++ b/drivers/input/touchscreen/ad7877.c
> > > @@ -360,6 +360,13 @@ static int ad7877_rx(struct ad7877 *ts)
> > >                Rt /= z1;
> > >                Rt = (Rt + 2047) >> 12;
> > >
> > > +               /*
> > > +                * Sample found inconsistent, pressure is beyond
> > > +                * the maximum. Don't report it to user space.
> > > +                */
> > > +               if (Rt > ts->pressure_max)
> > > +                       return -EINVAL;
> > 
> > this has spaces in the middle of your tab indents ...
> 
> I took care of that on my side...
> 

BTW, I have a couple more small patches to the driver... Here is the
first:

Input: ad7877 - use attribute group to control visibility of attributes

Instead of manually creating one set of attributes or another set up
is_visible method in attribute group structure to control whether
aux3 or gpio3 attribute is oresented to userspace.

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

 drivers/input/touchscreen/ad7877.c |   49 +++++++++++++++++++++---------------
 1 files changed, 29 insertions(+), 20 deletions(-)


diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index b7de78e..326d733 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -206,8 +206,8 @@ struct ad7877 {
 	u16 conversion_data[AD7877_NR_SENSE] ____cacheline_aligned;
 };
 
-static int gpio3;
-module_param(gpio3, int, 0);
+static bool gpio3;
+module_param(gpio3, bool, 0);
 MODULE_PARM_DESC(gpio3, "If gpio3 is set to 1 AUX3 acts as GPIO3");
 
 /*
@@ -471,7 +471,7 @@ static void ad7877_enable(struct ad7877 *ts)
 #define SHOW(name) static ssize_t \
 name ## _show(struct device *dev, struct device_attribute *attr, char *buf) \
 { \
-	struct ad7877	*ts = dev_get_drvdata(dev); \
+	struct ad7877 *ts = dev_get_drvdata(dev); \
 	ssize_t v = ad7877_read_adc(ts->spi, \
 			AD7877_READ_CHAN(name)); \
 	if (v < 0) \
@@ -491,7 +491,7 @@ SHOW(temp2)
 static ssize_t ad7877_disable_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	struct ad7877	*ts = dev_get_drvdata(dev);
+	struct ad7877 *ts = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n", ts->disabled);
 }
@@ -521,7 +521,7 @@ 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);
+	struct ad7877 *ts = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n", ts->dac);
 }
@@ -551,7 +551,7 @@ 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);
+	struct ad7877 *ts = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n", ts->gpio3);
 }
@@ -582,7 +582,7 @@ 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);
+	struct ad7877 *ts = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n", ts->gpio4);
 }
@@ -615,16 +615,35 @@ static struct attribute *ad7877_attributes[] = {
 	&dev_attr_temp2.attr,
 	&dev_attr_aux1.attr,
 	&dev_attr_aux2.attr,
+	&dev_attr_aux3.attr,
 	&dev_attr_bat1.attr,
 	&dev_attr_bat2.attr,
 	&dev_attr_disable.attr,
 	&dev_attr_dac.attr,
+	&dev_attr_gpio3.attr,
 	&dev_attr_gpio4.attr,
 	NULL
 };
 
+static mode_t ad7877_attr_is_visible(struct kobject *kobj,
+				     struct attribute *attr, int n)
+{
+	mode_t mode = attr->mode;
+
+	if (attr == &dev_attr_aux3.attr) {
+		if (gpio3)
+			mode = 0;
+	} else if (attr == &dev_attr_gpio3.attr) {
+		if (!gpio3)
+			mode = 0;
+	}
+
+	return mode;
+}
+
 static const struct attribute_group ad7877_attr_group = {
-	.attrs = ad7877_attributes,
+	.is_visible	= ad7877_attr_is_visible,
+	.attrs		= ad7877_attributes,
 };
 
 static void ad7877_setup_ts_def_msg(struct spi_device *spi, struct ad7877 *ts)
@@ -787,20 +806,12 @@ static int __devinit ad7877_probe(struct spi_device *spi)
 	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;
+		goto err_remove_attr_group;
 
 	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);
 err_free_irq:
@@ -814,11 +825,9 @@ err_free_mem:
 
 static int __devexit ad7877_remove(struct spi_device *spi)
 {
-	struct ad7877		*ts = dev_get_drvdata(&spi->dev);
+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
 
 	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
-	device_remove_file(&spi->dev,
-			   gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
 
 	ad7877_disable(ts);
 	free_irq(ts->spi->irq, ts);
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Device-drivers-devel] [PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[Dmitry Torokhov]

On Sun, Oct 17, 2010 at 09:24:28PM -0700, Dmitry Torokhov wrote:
> On Sun, Oct 17, 2010 at 09:08:10PM -0700, Dmitry Torokhov wrote:
> > On Fri, Oct 15, 2010 at 09:51:12PM -0400, Mike Frysinger wrote:
> > > On Fri, Oct 15, 2010 at 06:40,  <michael.hennerich@analog.com> wrote:
> > > > Suppress events where pressure > pressure_max.
> > > > These events come typically along with inaccurate X and Y samples.
> > > 
> > > were you going to commit to the blackfin tree ?
> > > 
> > > > --- a/drivers/input/touchscreen/ad7877.c
> > > > +++ b/drivers/input/touchscreen/ad7877.c
> > > > @@ -360,6 +360,13 @@ static int ad7877_rx(struct ad7877 *ts)
> > > >                Rt /= z1;
> > > >                Rt = (Rt + 2047) >> 12;
> > > >
> > > > +               /*
> > > > +                * Sample found inconsistent, pressure is beyond
> > > > +                * the maximum. Don't report it to user space.
> > > > +                */
> > > > +               if (Rt > ts->pressure_max)
> > > > +                       return -EINVAL;
> > > 
> > > this has spaces in the middle of your tab indents ...
> > 
> > I took care of that on my side...
> > 
> 
> BTW, I have a couple more small patches to the driver... Here is the
> first:
> 

And here is second:

Input: ad7877 - switch to using threaded IRQ

Instead of using asynchronous SPI API and then spinning waiting for SPI
transfer to complete when disabling the device, let's use threaded IRQ
model and spi_sync().

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

 drivers/input/touchscreen/ad7877.c |   65 ++++++++++++++----------------------
 1 files changed, 25 insertions(+), 40 deletions(-)


diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index 326d733..a1952fc 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -191,13 +191,12 @@ struct ad7877 {
 	struct spi_message	msg;
 
 	struct mutex		mutex;
-	unsigned		disabled:1;	/* P: mutex */
-	unsigned		gpio3:1;	/* P: mutex */
-	unsigned		gpio4:1;	/* P: mutex */
+	bool			disabled;	/* P: mutex */
+	bool			gpio3;		/* P: mutex */
+	bool			gpio4;		/* P: mutex */
 
 	spinlock_t		lock;
 	struct timer_list	timer;		/* P: lock */
-	unsigned		pending:1;	/* P: lock */
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
@@ -333,7 +332,7 @@ static int ad7877_read_adc(struct spi_device *spi, unsigned command)
 	return status ? : sample;
 }
 
-static int ad7877_rx(struct ad7877 *ts)
+static int ad7877_process_data(struct ad7877 *ts)
 {
 	struct input_dev *input_dev = ts->input;
 	unsigned Rt;
@@ -374,6 +373,7 @@ static int ad7877_rx(struct ad7877 *ts)
 		input_report_abs(input_dev, ABS_Y, y);
 		input_report_abs(input_dev, ABS_PRESSURE, Rt);
 		input_sync(input_dev);
+
 		return 0;
 	}
 
@@ -392,64 +392,49 @@ static inline void ad7877_ts_event_release(struct ad7877 *ts)
 static void ad7877_timer(unsigned long handle)
 {
 	struct ad7877 *ts = (void *)handle;
+	unsigned long flags;
 
+	spin_lock_irqsave(&ts->lock, flags);
 	ad7877_ts_event_release(ts);
+	spin_unlock_irqrestore(&ts->lock, flags);
 }
 
 static irqreturn_t ad7877_irq(int irq, void *handle)
 {
 	struct ad7877 *ts = handle;
 	unsigned long flags;
-	int status;
+	int error;
 
-	/*
-	 * 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.
-	 */
+	error = spi_sync(ts->spi, &ts->msg);
+	if (error) {
+		dev_err(&ts->spi->dev, "spi_sync --> %d\n", error);
+		goto out;
+	}
 
 	spin_lock_irqsave(&ts->lock, flags);
-	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);
-	}
+	error = ad7877_process_data(ts);
+	if (!error)
+		mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
 	spin_unlock_irqrestore(&ts->lock, flags);
 
+out:
 	return IRQ_HANDLED;
 }
 
-static void ad7877_callback(void *_ts)
-{
-	struct ad7877 *ts = _ts;
-
-	spin_lock_irq(&ts->lock);
-	if (!ad7877_rx(ts))
-		mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
-	ts->pending = 0;
-	spin_unlock_irq(&ts->lock);
-}
-
 static void ad7877_disable(struct ad7877 *ts)
 {
 	mutex_lock(&ts->mutex);
 
 	if (!ts->disabled) {
-		ts->disabled = 1;
+		ts->disabled = true;
 		disable_irq(ts->spi->irq);
 
-		/* Wait for spi_async callback */
-		while (ts->pending)
-			msleep(1);
-
 		if (del_timer_sync(&ts->timer))
 			ad7877_ts_event_release(ts);
 	}
 
-	/* we know the chip's in lowpower mode since we always
+	/*
+	 * We know the chip's in lowpower mode since we always
 	 * leave it that way after every request
 	 */
 
@@ -461,7 +446,7 @@ static void ad7877_enable(struct ad7877 *ts)
 	mutex_lock(&ts->mutex);
 
 	if (ts->disabled) {
-		ts->disabled = 0;
+		ts->disabled = false;
 		enable_irq(ts->spi->irq);
 	}
 
@@ -672,7 +657,6 @@ static void ad7877_setup_ts_def_msg(struct spi_device *spi, struct ad7877 *ts)
 
 	spi_message_init(m);
 
-	m->complete = ad7877_callback;
 	m->context = ts;
 
 	ts->xfer[0].tx_buf = &ts->cmd_crtl1;
@@ -795,8 +779,9 @@ static int __devinit ad7877_probe(struct spi_device *spi)
 
 	/* Request AD7877 /DAV GPIO interrupt */
 
-	err = request_irq(spi->irq, ad7877_irq, IRQF_TRIGGER_FALLING,
-			spi->dev.driver->name, ts);
+	err = request_threaded_irq(spi->irq, NULL, ad7877_irq,
+				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				   spi->dev.driver->name, ts);
 	if (err) {
 		dev_dbg(&spi->dev, "irq %d busy?\n", spi->irq);
 		goto err_free_mem;
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [Device-drivers-devel] [PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[Hennerich, Michael]

Dmitry Torokhov wrote on 2010-10-18:
> On Sun, Oct 17, 2010 at 09:08:10PM -0700, Dmitry Torokhov wrote:
>> On Fri, Oct 15, 2010 at 09:51:12PM -0400, Mike Frysinger wrote:
>>> On Fri, Oct 15, 2010 at 06:40,  <michael.hennerich@analog.com>
> wrote:
>>>> Suppress events where pressure > pressure_max.
>>>> These events come typically along with inaccurate X and Y
> samples.
>>>
>>> were you going to commit to the blackfin tree ?
>>>
>>>> --- a/drivers/input/touchscreen/ad7877.c
>>>> +++ b/drivers/input/touchscreen/ad7877.c
>>>> @@ -360,6 +360,13 @@ static int ad7877_rx(struct ad7877 *ts)
>>>>                Rt /= z1;
>>>>                Rt = (Rt + 2047) >> 12;
>>>>
>>>> +               /*
>>>> +                * Sample found inconsistent, pressure is beyond
>>>> +                * the maximum. Don't report it to user space.
>>>> +                */
>>>> +               if (Rt > ts->pressure_max)
>>>> +                       return -EINVAL;
>>>
>>> this has spaces in the middle of your tab indents ...
>>
>> I took care of that on my side...
>>
>
> BTW, I have a couple more small patches to the driver... Here is the
> first:
>
> Input: ad7877 - use attribute group to control visibility of attributes
>
> Instead of manually creating one set of attributes or another set up
> is_visible method in attribute group structure to control whether
> aux3 or gpio3 attribute is oresented to userspace.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Acked-by: Michael Hennerich <michael.hennerich@analog.com>

> ---
>
>  drivers/input/touchscreen/ad7877.c |   49 +++++++++++++++++++++-------
>  -------- 1 files changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ad7877.c
> b/drivers/input/touchscreen/ad7877.c index b7de78e..326d733 100644 ---
> a/drivers/input/touchscreen/ad7877.c +++
> b/drivers/input/touchscreen/ad7877.c @@ -206,8 +206,8 @@ struct ad7877 {
>       u16 conversion_data[AD7877_NR_SENSE] ____cacheline_aligned;  };
> -static int gpio3;
> -module_param(gpio3, int, 0);
> +static bool gpio3;
> +module_param(gpio3, bool, 0);
>  MODULE_PARM_DESC(gpio3, "If gpio3 is set to 1 AUX3 acts as GPIO3");
>
>  /*
> @@ -471,7 +471,7 @@ static void ad7877_enable(struct ad7877 *ts)
> #define SHOW(name) static ssize_t \  name ## _show(struct device *dev,
> struct device_attribute *attr, char *buf) \  { \
> -     struct ad7877   *ts = dev_get_drvdata(dev); \
> +     struct ad7877 *ts = dev_get_drvdata(dev); \
>       ssize_t v = ad7877_read_adc(ts->spi, \                  AD7877_READ_CHAN(name)); \
>       if (v < 0) \ @@ -491,7 +491,7 @@ SHOW(temp2) static ssize_t
>  ad7877_disable_show(struct device *dev,                                   struct
>  device_attribute *attr, char *buf) {
> -     struct ad7877   *ts = dev_get_drvdata(dev);
> +     struct ad7877 *ts = dev_get_drvdata(dev);
>
>       return sprintf(buf, "%u\n", ts->disabled);  } @@ -521,7 +521,7 @@
> 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);
> +     struct ad7877 *ts = dev_get_drvdata(dev);
>
>       return sprintf(buf, "%u\n", ts->dac);  } @@ -551,7 +551,7 @@ 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);
> +     struct ad7877 *ts = dev_get_drvdata(dev);
>
>       return sprintf(buf, "%u\n", ts->gpio3);  } @@ -582,7 +582,7 @@
> 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);
> +     struct ad7877 *ts = dev_get_drvdata(dev);
>
>       return sprintf(buf, "%u\n", ts->gpio4);  } @@ -615,16 +615,35 @@
>  static struct attribute *ad7877_attributes[] = {     &dev_attr_temp2.attr,
>       &dev_attr_aux1.attr,    &dev_attr_aux2.attr, +  &dev_attr_aux3.attr,
>       &dev_attr_bat1.attr,    &dev_attr_bat2.attr,    &dev_attr_disable.attr,
>       &dev_attr_dac.attr, +   &dev_attr_gpio3.attr,   &dev_attr_gpio4.attr,
>       NULL };
> +static mode_t ad7877_attr_is_visible(struct kobject *kobj,
> +                                  struct attribute *attr, int n) {
> +     mode_t mode = attr->mode;
> +
> +     if (attr == &dev_attr_aux3.attr) {
> +             if (gpio3)
> +                     mode = 0;
> +     } else if (attr == &dev_attr_gpio3.attr) {
> +             if (!gpio3)
> +                     mode = 0;
> +     }
> +
> +     return mode;
> +}
> +
>  static const struct attribute_group ad7877_attr_group = {
> -     .attrs = ad7877_attributes,
> +     .is_visible     = ad7877_attr_is_visible,
> +     .attrs          = ad7877_attributes,
>  };
>
>  static void ad7877_setup_ts_def_msg(struct spi_device *spi, struct
> ad7877 *ts) @@ -787,20 +806,12 @@ static int __devinit
> ad7877_probe(struct spi_device *spi)
>       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;
> +             goto err_remove_attr_group;
>
>       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);
>  err_free_irq:
> @@ -814,11 +825,9 @@ err_free_mem:
>
>  static int __devexit ad7877_remove(struct spi_device *spi)  {
> -     struct ad7877           *ts = dev_get_drvdata(&spi->dev);
> +     struct ad7877 *ts = dev_get_drvdata(&spi->dev);
>
>       sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
> -     device_remove_file(&spi->dev,
> -                        gpio3 ? &dev_attr_gpio3 : &dev_attr_aux3);
>
>       ad7877_disable(ts);
>       free_irq(ts->spi->irq, ts);

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

RE: [Device-drivers-devel] [PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[Hennerich, Michael]

Dmitry Torokhov wrote on 2010-10-18:
> On Sun, Oct 17, 2010 at 09:24:28PM -0700, Dmitry Torokhov wrote:
>> On Sun, Oct 17, 2010 at 09:08:10PM -0700, Dmitry Torokhov wrote:
>>> On Fri, Oct 15, 2010 at 09:51:12PM -0400, Mike Frysinger wrote:
>>>> On Fri, Oct 15, 2010 at 06:40,  <michael.hennerich@analog.com>
> wrote:
>>>>> Suppress events where pressure > pressure_max.
>>>>> These events come typically along with inaccurate X and Y
> samples.
>>>>
>>>> were you going to commit to the blackfin tree ?
>>>>
>>>>> --- a/drivers/input/touchscreen/ad7877.c
>>>>> +++ b/drivers/input/touchscreen/ad7877.c
>>>>> @@ -360,6 +360,13 @@ static int ad7877_rx(struct ad7877 *ts)
>>>>>                Rt /= z1;
>>>>>                Rt = (Rt + 2047) >> 12;
>>>>>
>>>>> +               /* +                * Sample found inconsistent,
>>>>> pressure is beyond +                * the maximum. Don't report it
>>>>> to user space. +                */ +               if (Rt >
>>>>> ts->pressure_max) +                       return -EINVAL;
>>>>
>>>> this has spaces in the middle of your tab indents ...
>>>
>>> I took care of that on my side...
>>>
>>
>> BTW, I have a couple more small patches to the driver... Here is the
>> first:
>>
>
> And here is second:
>
> Input: ad7877 - switch to using threaded IRQ
>
> Instead of using asynchronous SPI API and then spinning waiting for
> SPI transfer to complete when disabling the device, let's use threaded
> IRQ model and spi_sync().

Tested on hardware - works great!

>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
>
>  drivers/input/touchscreen/ad7877.c |   65 ++++++++++++++--------------
>  -------- 1 files changed, 25 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ad7877.c
> b/drivers/input/touchscreen/ad7877.c index 326d733..a1952fc 100644 ---
> a/drivers/input/touchscreen/ad7877.c +++
> b/drivers/input/touchscreen/ad7877.c @@ -191,13 +191,12 @@ struct ad7877
> {
>       struct spi_message      msg;
>
>       struct mutex            mutex;
> -     unsigned                disabled:1;     /* P: mutex */
> -     unsigned                gpio3:1;        /* P: mutex */
> -     unsigned                gpio4:1;        /* P: mutex */
> +     bool                    disabled;       /* P: mutex */
> +     bool                    gpio3;          /* P: mutex */
> +     bool                    gpio4;          /* P: mutex */
>
>       spinlock_t              lock;
>       struct timer_list       timer;          /* P: lock */
> -     unsigned                pending:1;      /* P: lock */
>
>       /*
>        * DMA (thus cache coherency maintenance) requires the @@ -333,7
> +332,7 @@ static int ad7877_read_adc(struct spi_device *spi, unsigned
> command)
>       return status ? : sample;
>  }
> -static int ad7877_rx(struct ad7877 *ts)
> +static int ad7877_process_data(struct ad7877 *ts)
>  {    struct input_dev *input_dev = ts->input;        unsigned Rt; @@ -374,6
>  +373,7 @@ static int ad7877_rx(struct ad7877 *ts)
>               input_report_abs(input_dev, ABS_Y, y);          input_report_abs(input_dev,
>  ABS_PRESSURE, Rt);           input_sync(input_dev); +                return 0;       }
> @@ -392,64 +392,49 @@ static inline void ad7877_ts_event_release(struct
> ad7877 *ts)  static void ad7877_timer(unsigned long handle)  {
>       struct ad7877 *ts = (void *)handle;
> +     unsigned long flags;
>
> +     spin_lock_irqsave(&ts->lock, flags);
>       ad7877_ts_event_release(ts); +  spin_unlock_irqrestore(&ts->lock,
>  flags); }
>
>  static irqreturn_t ad7877_irq(int irq, void *handle)  {
>       struct ad7877 *ts = handle;
>       unsigned long flags;
> -     int status;
> +     int error;
>
> -     /* -     * 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. -        */
> +     error = spi_sync(ts->spi, &ts->msg); +  if (error) {
> +             dev_err(&ts->spi->dev, "spi_sync --> %d\n", error); +           goto out; +     }
>
>       spin_lock_irqsave(&ts->lock, flags);
> -     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);
> -     }
> +     error = ad7877_process_data(ts);
> +     if (!error)
> +             mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
>       spin_unlock_irqrestore(&ts->lock, flags);
> +out:
>       return IRQ_HANDLED;
>  }
> -static void ad7877_callback(void *_ts) -{
> -     struct ad7877 *ts = _ts;
> -
> -     spin_lock_irq(&ts->lock);
> -     if (!ad7877_rx(ts))
> -             mod_timer(&ts->timer, jiffies + TS_PEN_UP_TIMEOUT);
> -     ts->pending = 0;
> -     spin_unlock_irq(&ts->lock);
> -}
> -
>  static void ad7877_disable(struct ad7877 *ts)  {
>       mutex_lock(&ts->mutex);
>
>       if (!ts->disabled) {
> -             ts->disabled = 1;
> +             ts->disabled = true;
>               disable_irq(ts->spi->irq);
> -             /* Wait for spi_async callback */
> -             while (ts->pending)
> -                     msleep(1);
> -
>               if (del_timer_sync(&ts->timer))                         ad7877_ts_event_release(ts);    }
> -     /* we know the chip's in lowpower mode since we always
> +     /*
> +      * We know the chip's in lowpower mode since we always
>        * leave it that way after every request
>        */
> @@ -461,7 +446,7 @@ static void ad7877_enable(struct ad7877 *ts)
>       mutex_lock(&ts->mutex);
>
>       if (ts->disabled) {
> -             ts->disabled = 0;
> +             ts->disabled = false;
>               enable_irq(ts->spi->irq);
>       }
> @@ -672,7 +657,6 @@ static void ad7877_setup_ts_def_msg(struct
> spi_device *spi, struct ad7877 *ts)
>
>       spi_message_init(m);
> -     m->complete = ad7877_callback;
>       m->context = ts;
>
>       ts->xfer[0].tx_buf = &ts->cmd_crtl1; @@ -795,8 +779,9 @@ static int
> __devinit ad7877_probe(struct spi_device
> *spi)
>
>       /* Request AD7877 /DAV GPIO interrupt */
> -     err = request_irq(spi->irq, ad7877_irq, IRQF_TRIGGER_FALLING,
> -                     spi->dev.driver->name, ts);
> +     err = request_threaded_irq(spi->irq, NULL, ad7877_irq,
> +                                IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +                                spi->dev.driver->name, ts);
>       if (err) {
>               dev_dbg(&spi->dev, "irq %d busy?\n", spi->irq);
>               goto err_free_mem;

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

Re: [Device-drivers-devel] [PATCH 3/3] Input: touchscreen: ad7877 filter events where pressure is beyond the maximum

[Dmitry Torokhov]

On Mon, Oct 18, 2010 at 09:13:14AM +0100, Hennerich, Michael wrote:
> Dmitry Torokhov wrote on 2010-10-18:
> > On Sun, Oct 17, 2010 at 09:24:28PM -0700, Dmitry Torokhov wrote:
> >> On Sun, Oct 17, 2010 at 09:08:10PM -0700, Dmitry Torokhov wrote:
> >>> On Fri, Oct 15, 2010 at 09:51:12PM -0400, Mike Frysinger wrote:
> >>>> On Fri, Oct 15, 2010 at 06:40,  <michael.hennerich@analog.com>
> > wrote:
> >>>>> Suppress events where pressure > pressure_max.
> >>>>> These events come typically along with inaccurate X and Y
> > samples.
> >>>>
> >>>> were you going to commit to the blackfin tree ?
> >>>>
> >>>>> --- a/drivers/input/touchscreen/ad7877.c
> >>>>> +++ b/drivers/input/touchscreen/ad7877.c
> >>>>> @@ -360,6 +360,13 @@ static int ad7877_rx(struct ad7877 *ts)
> >>>>>                Rt /= z1;
> >>>>>                Rt = (Rt + 2047) >> 12;
> >>>>>
> >>>>> +               /* +                * Sample found inconsistent,
> >>>>> pressure is beyond +                * the maximum. Don't report it
> >>>>> to user space. +                */ +               if (Rt >
> >>>>> ts->pressure_max) +                       return -EINVAL;
> >>>>
> >>>> this has spaces in the middle of your tab indents ...
> >>>
> >>> I took care of that on my side...
> >>>
> >>
> >> BTW, I have a couple more small patches to the driver... Here is the
> >> first:
> >>
> >
> > And here is second:
> >
> > Input: ad7877 - switch to using threaded IRQ
> >
> > Instead of using asynchronous SPI API and then spinning waiting for
> > SPI transfer to complete when disabling the device, let's use threaded
> > IRQ model and spi_sync().
> 
> Tested on hardware - works great!
> 

Thanks Michael, I'll also add a tested-by tag too then.

> >
> > Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> Acked-by: Michael Hennerich <michael.hennerich@analog.com>

Thanks.

-- 
Dmitry