[PATCH] input: zforce_ts: add DT support for reset GPIO polarity

[PATCH] input: zforce_ts: add DT support for reset GPIO polarity

[Dirk Behme]

From: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>

According to

Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt

the RST GPIO is supposed to provide a polarity flag parameter

gpios = <&GPIO_BANK GPIO_NUMBER GPIO_POLARITY>

with GPIO_POLARITY

reset active low  = 1 (GPIO_ACTIVE_LOW)
reset active high = 0 (GPIO_ACTIVE_HIGH)

Example for GPIO_ACTIVE_LOW (1) reset GPIO:

	zforce_ts@50 { /* Neonode zForce I2C */
		compatible = "neonode,zforce-ts";
		...
		gpios = <&gpio6 14 GPIO_ACTIVE_HIGH>, /* INT */
			<&gpio1 29 GPIO_ACTIVE_LOW>; /* RST */
		...
	};

Add the missing polarity flag evaluation to the driver.

Signed-off-by: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>
Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 drivers/input/touchscreen/zforce_ts.c   | 27 +++++++++++++++++++++++----
 include/linux/platform_data/zforce_ts.h |  3 +++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
index 19880c7..125311d 100644
--- a/drivers/input/touchscreen/zforce_ts.c
+++ b/drivers/input/touchscreen/zforce_ts.c
@@ -162,6 +162,20 @@ static int zforce_command(struct zforce_ts *ts, u8 cmd)
 	return 0;
 }
 
+static void zforce_reset_assert(struct zforce_ts *ts)
+{
+	const struct zforce_ts_platdata *pdata = ts->pdata;
+
+	gpio_set_value(pdata->gpio_rst, pdata->reset_active_low ? 0 : 1);
+}
+
+static void zforce_reset_deassert(struct zforce_ts *ts)
+{
+	const struct zforce_ts_platdata *pdata = ts->pdata;
+
+	gpio_set_value(pdata->gpio_rst, pdata->reset_active_low ? 1 : 0);
+}
+
 static int zforce_send_wait(struct zforce_ts *ts, const char *buf, int len)
 {
 	struct i2c_client *client = ts->client;
@@ -691,7 +705,7 @@ static void zforce_reset(void *data)
 {
 	struct zforce_ts *ts = data;
 
-	gpio_set_value(ts->pdata->gpio_rst, 0);
+	zforce_reset_assert(ts);
 
 	udelay(10);
 
@@ -703,6 +717,7 @@ static struct zforce_ts_platdata *zforce_parse_dt(struct device *dev)
 {
 	struct zforce_ts_platdata *pdata;
 	struct device_node *np = dev->of_node;
+	enum of_gpio_flags flags;
 
 	if (!np)
 		return ERR_PTR(-ENOENT);
@@ -719,12 +734,14 @@ static struct zforce_ts_platdata *zforce_parse_dt(struct device *dev)
 		return ERR_PTR(-EINVAL);
 	}
 
-	pdata->gpio_rst = of_get_gpio(np, 1);
+	pdata->gpio_rst = of_get_gpio_flags(np, 1, &flags);
 	if (!gpio_is_valid(pdata->gpio_rst)) {
 		dev_err(dev, "failed to get reset gpio\n");
 		return ERR_PTR(-EINVAL);
 	}
 
+	pdata->reset_active_low = flags & OF_GPIO_ACTIVE_LOW;
+
 	if (of_property_read_u32(np, "x-size", &pdata->x_max)) {
 		dev_err(dev, "failed to get x-size property\n");
 		return ERR_PTR(-EINVAL);
@@ -765,7 +782,9 @@ static int zforce_probe(struct i2c_client *client,
 	}
 
 	ret = devm_gpio_request_one(&client->dev, pdata->gpio_rst,
-				    GPIOF_OUT_INIT_LOW, "zforce_ts_rst");
+				    pdata->reset_active_low ?
+				    GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				    "zforce_ts_rst");
 	if (ret) {
 		dev_err(&client->dev, "request of gpio %d failed, %d\n",
 			pdata->gpio_rst, ret);
@@ -864,7 +883,7 @@ static int zforce_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, ts);
 
 	/* let the controller boot */
-	gpio_set_value(pdata->gpio_rst, 1);
+	zforce_reset_deassert(ts);
 
 	ts->command_waiting = NOTIFICATION_BOOTCOMPLETE;
 	if (wait_for_completion_timeout(&ts->command_done, WAIT_TIMEOUT) == 0)
diff --git a/include/linux/platform_data/zforce_ts.h b/include/linux/platform_data/zforce_ts.h
index 0472ab2..44cd90f 100644
--- a/include/linux/platform_data/zforce_ts.h
+++ b/include/linux/platform_data/zforce_ts.h
@@ -15,9 +15,12 @@
 #ifndef _LINUX_INPUT_ZFORCE_TS_H
 #define _LINUX_INPUT_ZFORCE_TS_H
 
+#include <linux/of_gpio.h>
+
 struct zforce_ts_platdata {
 	int gpio_int;
 	int gpio_rst;
+	enum of_gpio_flags reset_active_low;
 
 	unsigned int x_max;
 	unsigned int y_max;
-- 
2.3.4

Re: [PATCH] input: zforce_ts: add DT support for reset GPIO polarity

[Dmitry Torokhov]

On Mon, Jun 15, 2015 at 07:54:25AM +0200, Dirk Behme wrote:
> From: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>
> 
> According to
> 
> Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> 
> the RST GPIO is supposed to provide a polarity flag parameter
> 
> gpios = <&GPIO_BANK GPIO_NUMBER GPIO_POLARITY>
> 
> with GPIO_POLARITY
> 
> reset active low  = 1 (GPIO_ACTIVE_LOW)
> reset active high = 0 (GPIO_ACTIVE_HIGH)
> 
> Example for GPIO_ACTIVE_LOW (1) reset GPIO:
> 
> 	zforce_ts@50 { /* Neonode zForce I2C */
> 		compatible = "neonode,zforce-ts";
> 		...
> 		gpios = <&gpio6 14 GPIO_ACTIVE_HIGH>, /* INT */
> 			<&gpio1 29 GPIO_ACTIVE_LOW>; /* RST */
> 		...
> 	};
> 
> Add the missing polarity flag evaluation to the driver.
> 
> Signed-off-by: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>
> Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>  drivers/input/touchscreen/zforce_ts.c   | 27 +++++++++++++++++++++++----
>  include/linux/platform_data/zforce_ts.h |  3 +++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
> index 19880c7..125311d 100644
> --- a/drivers/input/touchscreen/zforce_ts.c
> +++ b/drivers/input/touchscreen/zforce_ts.c
> @@ -162,6 +162,20 @@ static int zforce_command(struct zforce_ts *ts, u8 cmd)
>  	return 0;
>  }
>  
> +static void zforce_reset_assert(struct zforce_ts *ts)
> +{
> +	const struct zforce_ts_platdata *pdata = ts->pdata;
> +
> +	gpio_set_value(pdata->gpio_rst, pdata->reset_active_low ? 0 : 1);

Instead of doing this I'd rather we converted the driver to use gpiod
that handles polarity automatically.

Thanks.

-- 
Dmitry

Re: [PATCH] input: zforce_ts: add DT support for reset GPIO polarity

[Dirk Behme]

On 17.06.2015 01:57, Dmitry Torokhov wrote:
> On Mon, Jun 15, 2015 at 07:54:25AM +0200, Dirk Behme wrote:
>> From: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>
>>
>> According to
>>
>> Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
>>
>> the RST GPIO is supposed to provide a polarity flag parameter
>>
>> gpios = <&GPIO_BANK GPIO_NUMBER GPIO_POLARITY>
>>
>> with GPIO_POLARITY
>>
>> reset active low  = 1 (GPIO_ACTIVE_LOW)
>> reset active high = 0 (GPIO_ACTIVE_HIGH)
>>
>> Example for GPIO_ACTIVE_LOW (1) reset GPIO:
>>
>> 	zforce_ts@50 { /* Neonode zForce I2C */
>> 		compatible = "neonode,zforce-ts";
>> 		...
>> 		gpios = <&gpio6 14 GPIO_ACTIVE_HIGH>, /* INT */
>> 			<&gpio1 29 GPIO_ACTIVE_LOW>; /* RST */
>> 		...
>> 	};
>>
>> Add the missing polarity flag evaluation to the driver.
>>
>> Signed-off-by: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>
>> Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>>   drivers/input/touchscreen/zforce_ts.c   | 27 +++++++++++++++++++++++----
>>   include/linux/platform_data/zforce_ts.h |  3 +++
>>   2 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
>> index 19880c7..125311d 100644
>> --- a/drivers/input/touchscreen/zforce_ts.c
>> +++ b/drivers/input/touchscreen/zforce_ts.c
>> @@ -162,6 +162,20 @@ static int zforce_command(struct zforce_ts *ts, u8 cmd)
>>   	return 0;
>>   }
>>
>> +static void zforce_reset_assert(struct zforce_ts *ts)
>> +{
>> +	const struct zforce_ts_platdata *pdata = ts->pdata;
>> +
>> +	gpio_set_value(pdata->gpio_rst, pdata->reset_active_low ? 0 : 1);
>
> Instead of doing this I'd rather we converted the driver to use gpiod
> that handles polarity automatically.


Thanks, we'll look into that.

Just to understand correctly: Converting this driver to gpiod will be an 
additional patch on top of this patch as it doesn't change any 
functionality provided by this patch, but does some clean up. Correct?


Best regards

Dirk

Re: [PATCH] input: zforce_ts: add DT support for reset GPIO polarity

[Dmitry Torokhov]

On Wed, Jun 24, 2015 at 07:18:38AM +0200, Dirk Behme wrote:
> On 17.06.2015 01:57, Dmitry Torokhov wrote:
> >On Mon, Jun 15, 2015 at 07:54:25AM +0200, Dirk Behme wrote:
> >>From: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>
> >>
> >>According to
> >>
> >>Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> >>
> >>the RST GPIO is supposed to provide a polarity flag parameter
> >>
> >>gpios = <&GPIO_BANK GPIO_NUMBER GPIO_POLARITY>
> >>
> >>with GPIO_POLARITY
> >>
> >>reset active low  = 1 (GPIO_ACTIVE_LOW)
> >>reset active high = 0 (GPIO_ACTIVE_HIGH)
> >>
> >>Example for GPIO_ACTIVE_LOW (1) reset GPIO:
> >>
> >>	zforce_ts@50 { /* Neonode zForce I2C */
> >>		compatible = "neonode,zforce-ts";
> >>		...
> >>		gpios = <&gpio6 14 GPIO_ACTIVE_HIGH>, /* INT */
> >>			<&gpio1 29 GPIO_ACTIVE_LOW>; /* RST */
> >>		...
> >>	};
> >>
> >>Add the missing polarity flag evaluation to the driver.
> >>
> >>Signed-off-by: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>
> >>Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> >>Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> >>---
> >>  drivers/input/touchscreen/zforce_ts.c   | 27 +++++++++++++++++++++++----
> >>  include/linux/platform_data/zforce_ts.h |  3 +++
> >>  2 files changed, 26 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
> >>index 19880c7..125311d 100644
> >>--- a/drivers/input/touchscreen/zforce_ts.c
> >>+++ b/drivers/input/touchscreen/zforce_ts.c
> >>@@ -162,6 +162,20 @@ static int zforce_command(struct zforce_ts *ts, u8 cmd)
> >>  	return 0;
> >>  }
> >>
> >>+static void zforce_reset_assert(struct zforce_ts *ts)
> >>+{
> >>+	const struct zforce_ts_platdata *pdata = ts->pdata;
> >>+
> >>+	gpio_set_value(pdata->gpio_rst, pdata->reset_active_low ? 0 : 1);
> >
> >Instead of doing this I'd rather we converted the driver to use gpiod
> >that handles polarity automatically.
> 
> 
> Thanks, we'll look into that.
> 
> Just to understand correctly: Converting this driver to gpiod will
> be an additional patch on top of this patch as it doesn't change any
> functionality provided by this patch, but does some clean up.
> Correct?

No, gpiod understands annotations on gpio descriptions and converts the
logical "active"/"inactive" value into appropriate output depending on
the polarity specified for gpio. So zforce_reset_assert() woudl only
need to do

	gpiod_set_value[_cansleep](ts->gpio_rst, 1);


Thanks.

-- 
Dmitry

Re: [PATCH] input: zforce_ts: add DT support for reset GPIO polarity

[Dirk Behme]

Hi Dmitry,

On 30.06.2015 02:08, Dmitry Torokhov wrote:
> On Wed, Jun 24, 2015 at 07:18:38AM +0200, Dirk Behme wrote:
>> On 17.06.2015 01:57, Dmitry Torokhov wrote:
>>> On Mon, Jun 15, 2015 at 07:54:25AM +0200, Dirk Behme wrote:
>>>> From: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>
>>>>
>>>> According to
>>>>
>>>> Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
>>>>
>>>> the RST GPIO is supposed to provide a polarity flag parameter
>>>>
>>>> gpios = <&GPIO_BANK GPIO_NUMBER GPIO_POLARITY>
>>>>
>>>> with GPIO_POLARITY
>>>>
>>>> reset active low  = 1 (GPIO_ACTIVE_LOW)
>>>> reset active high = 0 (GPIO_ACTIVE_HIGH)
>>>>
>>>> Example for GPIO_ACTIVE_LOW (1) reset GPIO:
>>>>
>>>> 	zforce_ts@50 { /* Neonode zForce I2C */
>>>> 		compatible = "neonode,zforce-ts";
>>>> 		...
>>>> 		gpios = <&gpio6 14 GPIO_ACTIVE_HIGH>, /* INT */
>>>> 			<&gpio1 29 GPIO_ACTIVE_LOW>; /* RST */
>>>> 		...
>>>> 	};
>>>>
>>>> Add the missing polarity flag evaluation to the driver.
>>>>
>>>> Signed-off-by: Knut Wohlrab <Knut.Wohlrab@de.bosch.com>
>>>> Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>> ---
>>>>   drivers/input/touchscreen/zforce_ts.c   | 27 +++++++++++++++++++++++----
>>>>   include/linux/platform_data/zforce_ts.h |  3 +++
>>>>   2 files changed, 26 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
>>>> index 19880c7..125311d 100644
>>>> --- a/drivers/input/touchscreen/zforce_ts.c
>>>> +++ b/drivers/input/touchscreen/zforce_ts.c
>>>> @@ -162,6 +162,20 @@ static int zforce_command(struct zforce_ts *ts, u8 cmd)
>>>>   	return 0;
>>>>   }
>>>>
>>>> +static void zforce_reset_assert(struct zforce_ts *ts)
>>>> +{
>>>> +	const struct zforce_ts_platdata *pdata = ts->pdata;
>>>> +
>>>> +	gpio_set_value(pdata->gpio_rst, pdata->reset_active_low ? 0 : 1);
>>>
>>> Instead of doing this I'd rather we converted the driver to use gpiod
>>> that handles polarity automatically.
>>
>>
>> Thanks, we'll look into that.
>>
>> Just to understand correctly: Converting this driver to gpiod will
>> be an additional patch on top of this patch as it doesn't change any
>> functionality provided by this patch, but does some clean up.
>> Correct?
>
> No, gpiod understands annotations on gpio descriptions and converts the
> logical "active"/"inactive" value into appropriate output depending on
> the polarity specified for gpio. So zforce_reset_assert() woudl only
> need to do
>
> 	gpiod_set_value[_cansleep](ts->gpio_rst, 1);


Yes, this is understood. But my question has been a different one ;) Let 
me rephrase it:

Based on the existing driver this patch adds support for the (missing) 
GPIO polarity on the reset pin. Based on the existing GPIO framework 
used by the driver. Not talking about gpiod for the moment, I hope this 
is fine (?).

Then, you've asked to convert the driver to gpiod. That's fine, too. And 
will make the driver cleaner. And yes, it will handle the polarity, too.

With this, my proposal is to have two patches:


1) the existing one adding the polarity based on the existing GPIO 
framework (handling the polarity 'manually')

2) converting the driver to gpiod, handling the polarity implicitly, then.


I'd propose to have 2 patches as described above as it would separate 
the additional functionality (patch #1) and the gpiod framework change 
(patch #2).


So my question is about the structuring of the patches, not about 
details of gpiod_xxx().

Best regards

Dirk