[PATCH 1/3] Input: ads7846 - introduce .gpio_pendown to get pendown state

[PATCH 1/3] Input: ads7846 - introduce .gpio_pendown to get pendown state

[Eric Miao]

The GPIO connected to ADS7846 nPENIRQ signal is usually used to get
the pendown state as well. Introduce a .gpio_pendown, and use this
to decide the pendown state if .get_pendown_state is NULL.

Signed-off-by: Eric Miao <eric.miao@marvell.com>
---
 drivers/input/touchscreen/ads7846.c |   73 ++++++++++++++++++++++++++++-------
 include/linux/spi/ads7846.h         |    4 ++
 2 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
index ce6f48c..4fd449a 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -24,6 +24,7 @@
 #include <linux/input.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/gpio.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/ads7846.h>
 #include <asm/irq.h>
@@ -116,6 +117,7 @@ struct ads7846 {
 	void			*filter_data;
 	void			(*filter_cleanup)(void *data);
 	int			(*get_pendown_state)(void);
+	int			gpio_pendown;
 };

 /* leave chip selected when we're done, for quicker re-select? */
@@ -491,6 +493,17 @@ static struct attribute_group ads784x_attr_group = {

 /*--------------------------------------------------------------------------*/

+static int get_pendown_state(struct ads7846 *ts)
+{
+	if (ts->get_pendown_state)
+		return ts->get_pendown_state();
+
+	if (ts->gpio_pendown != -1)
+		return !gpio_get_value(ts->gpio_pendown);
+
+	return 0;
+}
+
 /*
  * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
  * to retrieve touchscreen status.
@@ -550,7 +563,7 @@ static void ads7846_rx(void *ads)
 	 */
 	if (ts->penirq_recheck_delay_usecs) {
 		udelay(ts->penirq_recheck_delay_usecs);
-		if (!ts->get_pendown_state())
+		if (!get_pendown_state(ts))
 			Rt = 0;
 	}

@@ -677,7 +690,7 @@ static enum hrtimer_restart ads7846_timer(struct
hrtimer *handle)

 	spin_lock_irq(&ts->lock);

-	if (unlikely(!ts->get_pendown_state() ||
+	if (unlikely(!get_pendown_state(ts) ||
 		     device_suspended(&ts->spi->dev))) {
 		if (ts->pendown) {
 			struct input_dev *input = ts->input;
@@ -716,7 +729,7 @@ static irqreturn_t ads7846_irq(int irq, void *handle)
 	unsigned long flags;

 	spin_lock_irqsave(&ts->lock, flags);
-	if (likely(ts->get_pendown_state())) {
+	if (likely(get_pendown_state(ts))) {
 		if (!ts->irq_disabled) {
 			/* The ARM do_simple_IRQ() dispatcher doesn't act
 			 * like the other dispatchers:  it will report IRQs
@@ -806,6 +819,38 @@ static int ads7846_resume(struct spi_device *spi)
 	return 0;
 }

+static int __devinit setup_pendown(struct spi_device *spi, struct ads7846 *ts)
+{
+	struct ads7846_platform_data *pdata = spi->dev.platform_data;
+	int err;
+
+	ts->gpio_pendown = -1;
+
+	/* REVISIT when the irq can be triggered active-low, or if for some
+	 * reason the touchscreen isn't hooked up, we don't need to access
+	 * the pendown state.
+	 */
+	if (pdata->get_pendown_state == NULL && pdata->gpio_pendown == -1) {
+		dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
+		return -EINVAL;
+	}
+
+	if (pdata->get_pendown_state) {
+		ts->get_pendown_state = pdata->get_pendown_state;
+		return 0;
+	}
+
+	err = gpio_request(pdata->gpio_pendown, "ads7846_pendown");
+	if (err) {
+		dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
+				pdata->gpio_pendown);
+		return err;
+	}
+
+	ts->gpio_pendown = pdata->gpio_pendown;
+	return 0;
+}
+
 static int __devinit ads7846_probe(struct spi_device *spi)
 {
 	struct ads7846			*ts;
@@ -833,15 +878,6 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		return -EINVAL;
 	}

-	/* REVISIT when the irq can be triggered active-low, or if for some
-	 * reason the touchscreen isn't hooked up, we don't need to access
-	 * the pendown state.
-	 */
-	if (pdata->get_pendown_state == NULL) {
-		dev_dbg(&spi->dev, "no get_pendown_state function?\n");
-		return -EINVAL;
-	}
-
 	/* We'd set TX wordsize 8 bits and RX wordsize to 13 bits ... except
 	 * that even if the hardware can do that, the SPI controller driver
 	 * may not.  So we stick to very-portable 8 bit words, both RX and TX.
@@ -893,7 +929,10 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		ts->filter_data = ts;
 	} else
 		ts->filter = ads7846_no_filter;
-	ts->get_pendown_state = pdata->get_pendown_state;
+
+	err = setup_pendown(spi, ts);
+	if (err)
+		goto err_cleanup_filter;

 	if (pdata->penirq_recheck_delay_usecs)
 		ts->penirq_recheck_delay_usecs =
@@ -1085,7 +1124,7 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 			spi->dev.driver->name, ts)) {
 		dev_dbg(&spi->dev, "irq %d busy?\n", spi->irq);
 		err = -EBUSY;
-		goto err_cleanup_filter;
+		goto err_free_gpio;
 	}

 	err = ads784x_hwmon_register(spi, ts);
@@ -1116,6 +1155,9 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 	ads784x_hwmon_unregister(spi, ts);
  err_free_irq:
 	free_irq(spi->irq, ts);
+ err_free_gpio:
+	if (ts->gpio_pendown != -1)
+		gpio_free(ts->gpio_pendown);
  err_cleanup_filter:
 	if (ts->filter_cleanup)
 		ts->filter_cleanup(ts->filter_data);
@@ -1140,6 +1182,9 @@ static int __devexit ads7846_remove(struct
spi_device *spi)
 	/* suspend left the IRQ disabled */
 	enable_irq(ts->spi->irq);

+	if (ts->gpio_pendown != -1)
+		gpio_free(ts->gpio_pendown);
+
 	if (ts->filter_cleanup)
 		ts->filter_cleanup(ts->filter_data);

diff --git a/include/linux/spi/ads7846.h b/include/linux/spi/ads7846.h
index daf7440..5850605 100644
--- a/include/linux/spi/ads7846.h
+++ b/include/linux/spi/ads7846.h
@@ -43,6 +43,10 @@ struct ads7846_platform_data {
 	u16	debounce_tol;		/* tolerance used for filtering */
 	u16	debounce_rep;		/* additional consecutive good readings
 					 * required after the first two */
+	int	gpio_pendown;		/* the GPIO usually connected to PENIRQ
+					 * and used to decide pendown state
+					 * (-1) to use (*get_pendown_state) */
+
 	int	(*get_pendown_state)(void);
 	int	(*filter_init)	(struct ads7846_platform_data *pdata,
 				 void **filter_data);
-- 
1.5.4.3

Re: [PATCH 1/3] Input: ads7846 - introduce .gpio_pendown to get pendown state

[Russell King - ARM Linux]

On Thu, Sep 04, 2008 at 05:21:05PM +0800, Eric Miao wrote:
> The GPIO connected to ADS7846 nPENIRQ signal is usually used to get
> the pendown state as well. Introduce a .gpio_pendown, and use this
> to decide the pendown state if .get_pendown_state is NULL.

> +static int get_pendown_state(struct ads7846 *ts)
> +{
> +	if (ts->get_pendown_state)
> +		return ts->get_pendown_state();
> +
> +	if (ts->gpio_pendown != -1)
> +		return !gpio_get_value(ts->gpio_pendown);
> +
> +	return 0;
> +}
> +


> +	int	gpio_pendown;		/* the GPIO usually connected to PENIRQ
> +					 * and used to decide pendown state
> +					 * (-1) to use (*get_pendown_state) */
> +

The comment doesn't match the code.  get_pendown_state() is used if it's
non-NULL, otherwise, the GPIO if it's not -1.

I'm getting a little worried about these tests for valid gpio numbers in
drivers - some check for non-zero, this one checks for not -1.  I get the
feeling that this is storing up problems for later.

Maybe using gpio_is_valid() would be a good idea?

But... a related question: do we need to do the check here?  Surely either
a pendown function is going to be supplied, or if not we're going to use
a GPIO, in which case the GPIO better be valid.  Maybe something to check
at driver initialisation time only?

Re: [PATCH 1/3] Input: ads7846 - introduce .gpio_pendown to get pendown state

[Eric Miao]

On Thu, Sep 4, 2008 at 6:00 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Sep 04, 2008 at 05:21:05PM +0800, Eric Miao wrote:
>> The GPIO connected to ADS7846 nPENIRQ signal is usually used to get
>> the pendown state as well. Introduce a .gpio_pendown, and use this
>> to decide the pendown state if .get_pendown_state is NULL.
>
>> +static int get_pendown_state(struct ads7846 *ts)
>> +{
>> +     if (ts->get_pendown_state)
>> +             return ts->get_pendown_state();
>> +
>> +     if (ts->gpio_pendown != -1)
>> +             return !gpio_get_value(ts->gpio_pendown);
>> +
>> +     return 0;
>> +}
>> +
>
>
>> +     int     gpio_pendown;           /* the GPIO usually connected to PENIRQ
>> +                                      * and used to decide pendown state
>> +                                      * (-1) to use (*get_pendown_state) */
>> +
>
> The comment doesn't match the code.  get_pendown_state() is used if it's
> non-NULL, otherwise, the GPIO if it's not -1.
>
> I'm getting a little worried about these tests for valid gpio numbers in
> drivers - some check for non-zero, this one checks for not -1.  I get the
> feeling that this is storing up problems for later.
>
> Maybe using gpio_is_valid() would be a good idea?
>

Indeed, I'll fix the other SPI GPIO CS patch as well.

> But... a related question: do we need to do the check here?  Surely either
> a pendown function is going to be supplied, or if not we're going to use
> a GPIO, in which case the GPIO better be valid.  Maybe something to check
> at driver initialisation time only?
>

There is a warning If none is provided, and probe() will fail.
So actually get_pendown_state() will never reach the last
"return 0" statement.

-- 
Cheers
- eric

Re: [PATCH 1/3] Input: ads7846 - introduce .gpio_pendown to get pendown state

[Eric Miao]

On Thu, Sep 4, 2008 at 6:38 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Thu, Sep 4, 2008 at 6:00 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Thu, Sep 04, 2008 at 05:21:05PM +0800, Eric Miao wrote:
>>> The GPIO connected to ADS7846 nPENIRQ signal is usually used to get
>>> the pendown state as well. Introduce a .gpio_pendown, and use this
>>> to decide the pendown state if .get_pendown_state is NULL.
>>
>>> +static int get_pendown_state(struct ads7846 *ts)
>>> +{
>>> +     if (ts->get_pendown_state)
>>> +             return ts->get_pendown_state();
>>> +
>>> +     if (ts->gpio_pendown != -1)
>>> +             return !gpio_get_value(ts->gpio_pendown);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>
>>
>>> +     int     gpio_pendown;           /* the GPIO usually connected to PENIRQ
>>> +                                      * and used to decide pendown state
>>> +                                      * (-1) to use (*get_pendown_state) */
>>> +
>>
>> The comment doesn't match the code.  get_pendown_state() is used if it's
>> non-NULL, otherwise, the GPIO if it's not -1.
>>
>> I'm getting a little worried about these tests for valid gpio numbers in
>> drivers - some check for non-zero, this one checks for not -1.  I get the
>> feeling that this is storing up problems for later.
>>
>> Maybe using gpio_is_valid() would be a good idea?
>>
>
> Indeed, I'll fix the other SPI GPIO CS patch as well.
>
>> But... a related question: do we need to do the check here?  Surely either
>> a pendown function is going to be supplied, or if not we're going to use
>> a GPIO, in which case the GPIO better be valid.  Maybe something to check
>> at driver initialisation time only?
>>
>
> There is a warning If none is provided, and probe() will fail.
> So actually get_pendown_state() will never reach the last
> "return 0" statement.
>

So maybe I can simplify that code a bit more :)

Re: [PATCH 1/3] Input: ads7846 - introduce .gpio_pendown to get pendown state

[Eric Miao]

Signed-off-by: Eric Miao <eric.miao@marvell.com>
---

This one should be cleaner.

 drivers/input/touchscreen/ads7846.c |   68 +++++++++++++++++++++++++++-------
 include/linux/spi/ads7846.h         |    3 ++
 2 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
index ce6f48c..8583c76 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -24,6 +24,7 @@
 #include <linux/input.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/gpio.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/ads7846.h>
 #include <asm/irq.h>
@@ -116,6 +117,7 @@ struct ads7846 {
 	void			*filter_data;
 	void			(*filter_cleanup)(void *data);
 	int			(*get_pendown_state)(void);
+	int			gpio_pendown;
 };

 /* leave chip selected when we're done, for quicker re-select? */
@@ -491,6 +493,14 @@ static struct attribute_group ads784x_attr_group = {

 /*--------------------------------------------------------------------------*/

+static int get_pendown_state(struct ads7846 *ts)
+{
+	if (ts->get_pendown_state)
+		return ts->get_pendown_state();
+
+	return !gpio_get_value(ts->gpio_pendown);
+}
+
 /*
  * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
  * to retrieve touchscreen status.
@@ -550,7 +560,7 @@ static void ads7846_rx(void *ads)
 	 */
 	if (ts->penirq_recheck_delay_usecs) {
 		udelay(ts->penirq_recheck_delay_usecs);
-		if (!ts->get_pendown_state())
+		if (!get_pendown_state(ts))
 			Rt = 0;
 	}

@@ -677,7 +687,7 @@ static enum hrtimer_restart ads7846_timer(struct
hrtimer *handle)

 	spin_lock_irq(&ts->lock);

-	if (unlikely(!ts->get_pendown_state() ||
+	if (unlikely(!get_pendown_state(ts) ||
 		     device_suspended(&ts->spi->dev))) {
 		if (ts->pendown) {
 			struct input_dev *input = ts->input;
@@ -716,7 +726,7 @@ static irqreturn_t ads7846_irq(int irq, void *handle)
 	unsigned long flags;

 	spin_lock_irqsave(&ts->lock, flags);
-	if (likely(ts->get_pendown_state())) {
+	if (likely(get_pendown_state(ts))) {
 		if (!ts->irq_disabled) {
 			/* The ARM do_simple_IRQ() dispatcher doesn't act
 			 * like the other dispatchers:  it will report IRQs
@@ -806,6 +816,36 @@ static int ads7846_resume(struct spi_device *spi)
 	return 0;
 }

+static int __devinit setup_pendown(struct spi_device *spi, struct ads7846 *ts)
+{
+	struct ads7846_platform_data *pdata = spi->dev.platform_data;
+	int err;
+
+	/* REVISIT when the irq can be triggered active-low, or if for some
+	 * reason the touchscreen isn't hooked up, we don't need to access
+	 * the pendown state.
+	 */
+	if (!pdata->get_pendown_state && !gpio_is_valid(pdata->gpio_pendown)) {
+		dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
+		return -EINVAL;
+	}
+
+	if (pdata->get_pendown_state) {
+		ts->get_pendown_state = pdata->get_pendown_state;
+		return 0;
+	}
+
+	err = gpio_request(pdata->gpio_pendown, "ads7846_pendown");
+	if (err) {
+		dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
+				pdata->gpio_pendown);
+		return err;
+	}
+
+	ts->gpio_pendown = pdata->gpio_pendown;
+	return 0;
+}
+
 static int __devinit ads7846_probe(struct spi_device *spi)
 {
 	struct ads7846			*ts;
@@ -833,15 +873,6 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		return -EINVAL;
 	}

-	/* REVISIT when the irq can be triggered active-low, or if for some
-	 * reason the touchscreen isn't hooked up, we don't need to access
-	 * the pendown state.
-	 */
-	if (pdata->get_pendown_state == NULL) {
-		dev_dbg(&spi->dev, "no get_pendown_state function?\n");
-		return -EINVAL;
-	}
-
 	/* We'd set TX wordsize 8 bits and RX wordsize to 13 bits ... except
 	 * that even if the hardware can do that, the SPI controller driver
 	 * may not.  So we stick to very-portable 8 bit words, both RX and TX.
@@ -893,7 +924,10 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		ts->filter_data = ts;
 	} else
 		ts->filter = ads7846_no_filter;
-	ts->get_pendown_state = pdata->get_pendown_state;
+
+	err = setup_pendown(spi, ts);
+	if (err)
+		goto err_cleanup_filter;

 	if (pdata->penirq_recheck_delay_usecs)
 		ts->penirq_recheck_delay_usecs =
@@ -1085,7 +1119,7 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 			spi->dev.driver->name, ts)) {
 		dev_dbg(&spi->dev, "irq %d busy?\n", spi->irq);
 		err = -EBUSY;
-		goto err_cleanup_filter;
+		goto err_free_gpio;
 	}

 	err = ads784x_hwmon_register(spi, ts);
@@ -1116,6 +1150,9 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 	ads784x_hwmon_unregister(spi, ts);
  err_free_irq:
 	free_irq(spi->irq, ts);
+ err_free_gpio:
+	if (ts->gpio_pendown != -1)
+		gpio_free(ts->gpio_pendown);
  err_cleanup_filter:
 	if (ts->filter_cleanup)
 		ts->filter_cleanup(ts->filter_data);
@@ -1140,6 +1177,9 @@ static int __devexit ads7846_remove(struct
spi_device *spi)
 	/* suspend left the IRQ disabled */
 	enable_irq(ts->spi->irq);

+	if (ts->gpio_pendown != -1)
+		gpio_free(ts->gpio_pendown);
+
 	if (ts->filter_cleanup)
 		ts->filter_cleanup(ts->filter_data);

diff --git a/include/linux/spi/ads7846.h b/include/linux/spi/ads7846.h
index daf7440..05eab2f 100644
--- a/include/linux/spi/ads7846.h
+++ b/include/linux/spi/ads7846.h
@@ -43,6 +43,9 @@ struct ads7846_platform_data {
 	u16	debounce_tol;		/* tolerance used for filtering */
 	u16	debounce_rep;		/* additional consecutive good readings
 					 * required after the first two */
+	int	gpio_pendown;		/* the GPIO used to decide the pendown
+					 * state if get_pendown_state == NULL
+					 */
 	int	(*get_pendown_state)(void);
 	int	(*filter_init)	(struct ads7846_platform_data *pdata,
 				 void **filter_data);
-- 
1.5.4.3

Re: [PATCH 1/3] Input: ads7846 - introduce .gpio_pendown to get pendown state

[Dmitry Torokhov]

On Thu, Sep 04, 2008 at 06:50:04PM +0800, Eric Miao wrote:
> Signed-off-by: Eric Miao <eric.miao@marvell.com>
> ---
> 
> This one should be cleaner.
> 

Applied to next. Thank you Eric.

-- 
Dmitry