The current/old gpio framework used doesn't properly listen to
ACTIVE_LOW and ACTIVE_HIGH flags. The newer gpio framework takes into
account these flags when setting gpio values.
Also use gpiod_set_value_cansleep since wake and reset pins can be
provided by bus based io expanders.
Signed-off-by: Franklin S Cooper Jr <[email protected]>
---
.../bindings/input/touchscreen/edt-ft5x06.txt | 4 +-
drivers/input/touchscreen/edt-ft5x06.c | 115 +++++++--------------
include/linux/input/edt-ft5x06.h | 4 +-
3 files changed, 43 insertions(+), 80 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index 76db967..9330d4d 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -50,6 +50,6 @@ Example:
pinctrl-0 = <&edt_ft5x06_pins>;
interrupt-parent = <&gpio2>;
interrupts = <5 0>;
- reset-gpios = <&gpio2 6 1>;
- wake-gpios = <&gpio4 9 0>;
+ reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
+ wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
};
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 394b1de..6b128b3 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -91,9 +91,9 @@ struct edt_ft5x06_ts_data {
u16 num_x;
u16 num_y;
- int reset_pin;
- int irq_pin;
- int wake_pin;
+ struct gpio_desc *reset_pin;
+ struct gpio_desc *wake_pin;
+ struct gpio_desc *irq_pin;
#if defined(CONFIG_DEBUG_FS)
struct dentry *debug_dir;
@@ -755,36 +755,14 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
static int edt_ft5x06_ts_reset(struct i2c_client *client,
struct edt_ft5x06_ts_data *tsdata)
{
- int error;
-
- if (gpio_is_valid(tsdata->wake_pin)) {
- error = devm_gpio_request_one(&client->dev,
- tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
- "edt-ft5x06 wake");
- if (error) {
- dev_err(&client->dev,
- "Failed to request GPIO %d as wake pin, error %d\n",
- tsdata->wake_pin, error);
- return error;
- }
-
+ if (tsdata->wake_pin) {
msleep(5);
- gpio_set_value(tsdata->wake_pin, 1);
+ gpiod_set_value_cansleep(tsdata->wake_pin, 1);
}
- if (gpio_is_valid(tsdata->reset_pin)) {
- /* this pulls reset down, enabling the low active reset */
- error = devm_gpio_request_one(&client->dev,
- tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
- "edt-ft5x06 reset");
- if (error) {
- dev_err(&client->dev,
- "Failed to request GPIO %d as reset pin, error %d\n",
- tsdata->reset_pin, error);
- return error;
- }
+ if (tsdata->reset_pin) {
msleep(5);
- gpio_set_value(tsdata->reset_pin, 1);
+ gpiod_set_value_cansleep(tsdata->reset_pin, 1);
msleep(300);
}
@@ -931,30 +909,6 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata)
}
}
-#ifdef CONFIG_OF
-static int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
- struct edt_ft5x06_ts_data *tsdata)
-{
- struct device_node *np = dev->of_node;
-
- /*
- * irq_pin is not needed for DT setup.
- * irq is associated via 'interrupts' property in DT
- */
- tsdata->irq_pin = -EINVAL;
- tsdata->reset_pin = of_get_named_gpio(np, "reset-gpios", 0);
- tsdata->wake_pin = of_get_named_gpio(np, "wake-gpios", 0);
-
- return 0;
-}
-#else
-static inline int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
- struct edt_ft5x06_ts_data *tsdata)
-{
- return -ENODEV;
-}
-#endif
-
static int edt_ft5x06_ts_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -964,6 +918,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
struct input_dev *input;
int error;
char fw_version[EDT_NAME_LEN];
+ struct gpio_desc *gpio;
dev_dbg(&client->dev, "probing for EDT FT5x06 I2C\n");
@@ -973,34 +928,41 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
return -ENOMEM;
}
- if (!pdata) {
- error = edt_ft5x06_i2c_ts_probe_dt(&client->dev, tsdata);
- if (error) {
- dev_err(&client->dev,
- "DT probe failed and no platform data present\n");
- return error;
- }
- } else {
- tsdata->reset_pin = pdata->reset_pin;
- tsdata->irq_pin = pdata->irq_pin;
- tsdata->wake_pin = -EINVAL;
+ gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(gpio)) {
+ error = PTR_ERR(gpio);
+ dev_err(&client->dev,
+ "Failed to request GPIO reset pin, error %d\n", error);
+ return error;
}
+ tsdata->reset_pin = gpio;
- error = edt_ft5x06_ts_reset(client, tsdata);
- if (error)
+ gpio = devm_gpiod_get_optional(&client->dev, "wake", GPIOD_OUT_LOW);
+
+ if (IS_ERR(gpio)) {
+ error = PTR_ERR(gpio);
+ dev_err(&client->dev,
+ "Failed to request GPIO wake pin, error %d\n", error);
return error;
+ }
- if (gpio_is_valid(tsdata->irq_pin)) {
- error = devm_gpio_request_one(&client->dev, tsdata->irq_pin,
- GPIOF_IN, "edt-ft5x06 irq");
- if (error) {
- dev_err(&client->dev,
- "Failed to request GPIO %d, error %d\n",
- tsdata->irq_pin, error);
- return error;
- }
+ tsdata->wake_pin = gpio;
+
+ gpio = devm_gpiod_get_optional(&client->dev, "irq", GPIOD_IN);
+
+ if (IS_ERR(gpio)) {
+ error = PTR_ERR(gpio);
+ dev_err(&client->dev,
+ "Failed to request GPIO irq pin, error %d\n", error);
+ return error;
}
+ tsdata->irq_pin = gpio;
+
+ error = edt_ft5x06_ts_reset(client, tsdata);
+ if (error)
+ return error;
+
input = devm_input_allocate_device(&client->dev);
if (!input) {
dev_err(&client->dev, "failed to allocate input device.\n");
@@ -1074,7 +1036,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
dev_dbg(&client->dev,
"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
- client->irq, tsdata->wake_pin, tsdata->reset_pin);
+ client->irq, desc_to_gpio(tsdata->wake_pin),
+ desc_to_gpio(tsdata->reset_pin));
return 0;
diff --git a/include/linux/input/edt-ft5x06.h b/include/linux/input/edt-ft5x06.h
index 8a1e0d1..058473a 100644
--- a/include/linux/input/edt-ft5x06.h
+++ b/include/linux/input/edt-ft5x06.h
@@ -10,8 +10,8 @@
*/
struct edt_ft5x06_platform_data {
- int irq_pin;
- int reset_pin;
+ int irq_gpio;
+ int reset_gpio;
/* startup defaults for operational parameters */
bool use_parameters;
--
2.1.4
On Fri, Aug 21, 2015 at 02:08:32PM -0500, Franklin S Cooper Jr wrote:
> The current/old gpio framework used doesn't properly listen to
> ACTIVE_LOW and ACTIVE_HIGH flags. The newer gpio framework takes into
> account these flags when setting gpio values.
>
> Also use gpiod_set_value_cansleep since wake and reset pins can be
> provided by bus based io expanders.
>
> Signed-off-by: Franklin S Cooper Jr <[email protected]>
> ---
> .../bindings/input/touchscreen/edt-ft5x06.txt | 4 +-
> drivers/input/touchscreen/edt-ft5x06.c | 115 +++++++--------------
> include/linux/input/edt-ft5x06.h | 4 +-
> 3 files changed, 43 insertions(+), 80 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> index 76db967..9330d4d 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> @@ -50,6 +50,6 @@ Example:
> pinctrl-0 = <&edt_ft5x06_pins>;
> interrupt-parent = <&gpio2>;
> interrupts = <5 0>;
> - reset-gpios = <&gpio2 6 1>;
> - wake-gpios = <&gpio4 9 0>;
> + reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
> + wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
> };
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 394b1de..6b128b3 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -91,9 +91,9 @@ struct edt_ft5x06_ts_data {
> u16 num_x;
> u16 num_y;
>
> - int reset_pin;
> - int irq_pin;
> - int wake_pin;
> + struct gpio_desc *reset_pin;
> + struct gpio_desc *wake_pin;
> + struct gpio_desc *irq_pin;
>
> #if defined(CONFIG_DEBUG_FS)
> struct dentry *debug_dir;
> @@ -755,36 +755,14 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
> static int edt_ft5x06_ts_reset(struct i2c_client *client,
> struct edt_ft5x06_ts_data *tsdata)
> {
> - int error;
> -
> - if (gpio_is_valid(tsdata->wake_pin)) {
> - error = devm_gpio_request_one(&client->dev,
> - tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
> - "edt-ft5x06 wake");
> - if (error) {
> - dev_err(&client->dev,
> - "Failed to request GPIO %d as wake pin, error %d\n",
> - tsdata->wake_pin, error);
> - return error;
> - }
> -
> + if (tsdata->wake_pin) {
> msleep(5);
> - gpio_set_value(tsdata->wake_pin, 1);
> + gpiod_set_value_cansleep(tsdata->wake_pin, 1);
> }
> - if (gpio_is_valid(tsdata->reset_pin)) {
> - /* this pulls reset down, enabling the low active reset */
> - error = devm_gpio_request_one(&client->dev,
> - tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
> - "edt-ft5x06 reset");
> - if (error) {
> - dev_err(&client->dev,
> - "Failed to request GPIO %d as reset pin, error %d\n",
> - tsdata->reset_pin, error);
> - return error;
> - }
>
> + if (tsdata->reset_pin) {
> msleep(5);
> - gpio_set_value(tsdata->reset_pin, 1);
> + gpiod_set_value_cansleep(tsdata->reset_pin, 1);
So this leaves the reset pin active. How exactly was this tested?
Thanks.
--
Dmitry
On 08/24/2015 03:16 PM, Franklin S Cooper Jr. wrote:
>
> On 08/24/2015 03:01 PM, Dmitry Torokhov wrote:
>> On Mon, Aug 24, 2015 at 02:48:36PM -0500, Franklin S Cooper Jr. wrote:
>>> On 08/24/2015 02:41 PM, Dmitry Torokhov wrote:
>>>> On Fri, Aug 21, 2015 at 02:08:32PM -0500, Franklin S Cooper Jr wrote:
>>>>> The current/old gpio framework used doesn't properly listen to
>>>>> ACTIVE_LOW and ACTIVE_HIGH flags. The newer gpio framework takes into
>>>>> account these flags when setting gpio values.
>>>>>
>>>>> Also use gpiod_set_value_cansleep since wake and reset pins can be
>>>>> provided by bus based io expanders.
>>>>>
>>>>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
>>>>> ---
>>>>> .../bindings/input/touchscreen/edt-ft5x06.txt | 4 +-
>>>>> drivers/input/touchscreen/edt-ft5x06.c | 115 +++++++--------------
>>>>> include/linux/input/edt-ft5x06.h | 4 +-
>>>>> 3 files changed, 43 insertions(+), 80 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>> index 76db967..9330d4d 100644
>>>>> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>> @@ -50,6 +50,6 @@ Example:
>>>>> pinctrl-0 = <&edt_ft5x06_pins>;
>>>>> interrupt-parent = <&gpio2>;
>>>>> interrupts = <5 0>;
>>>>> - reset-gpios = <&gpio2 6 1>;
>>>>> - wake-gpios = <&gpio4 9 0>;
>>>>> + reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
>>>>> + wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
>>>>> };
>>>>> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
>>>>> index 394b1de..6b128b3 100644
>>>>> --- a/drivers/input/touchscreen/edt-ft5x06.c
>>>>> +++ b/drivers/input/touchscreen/edt-ft5x06.c
>>>>> @@ -91,9 +91,9 @@ struct edt_ft5x06_ts_data {
>>>>> u16 num_x;
>>>>> u16 num_y;
>>>>>
>>>>> - int reset_pin;
>>>>> - int irq_pin;
>>>>> - int wake_pin;
>>>>> + struct gpio_desc *reset_pin;
>>>>> + struct gpio_desc *wake_pin;
>>>>> + struct gpio_desc *irq_pin;
>>>>>
>>>>> #if defined(CONFIG_DEBUG_FS)
>>>>> struct dentry *debug_dir;
>>>>> @@ -755,36 +755,14 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
>>>>> static int edt_ft5x06_ts_reset(struct i2c_client *client,
>>>>> struct edt_ft5x06_ts_data *tsdata)
>>>>> {
>>>>> - int error;
>>>>> -
>>>>> - if (gpio_is_valid(tsdata->wake_pin)) {
>>>>> - error = devm_gpio_request_one(&client->dev,
>>>>> - tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
>>>>> - "edt-ft5x06 wake");
>>>>> - if (error) {
>>>>> - dev_err(&client->dev,
>>>>> - "Failed to request GPIO %d as wake pin, error %d\n",
>>>>> - tsdata->wake_pin, error);
>>>>> - return error;
>>>>> - }
>>>>> -
>>>>> + if (tsdata->wake_pin) {
>>>>> msleep(5);
>>>>> - gpio_set_value(tsdata->wake_pin, 1);
>>>>> + gpiod_set_value_cansleep(tsdata->wake_pin, 1);
>>>>> }
>>>>> - if (gpio_is_valid(tsdata->reset_pin)) {
>>>>> - /* this pulls reset down, enabling the low active reset */
>>>>> - error = devm_gpio_request_one(&client->dev,
>>>>> - tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
>>>>> - "edt-ft5x06 reset");
>>>>> - if (error) {
>>>>> - dev_err(&client->dev,
>>>>> - "Failed to request GPIO %d as reset pin, error %d\n",
>>>>> - tsdata->reset_pin, error);
>>>>> - return error;
>>>>> - }
>>>>>
>>>>> + if (tsdata->reset_pin) {
>>>>> msleep(5);
>>>>> - gpio_set_value(tsdata->reset_pin, 1);
>>>>> + gpiod_set_value_cansleep(tsdata->reset_pin, 1);
>>>> So this leaves the reset pin active. How exactly was this tested?
>>> Normally if the output gpio connected to the reset pin is ACTIVE_HIGH then this will take the tsc out of reset since
>>> the reset pin is active low. However, I have a board that has an inverter between the gpio and reset pin. So if I leave
>>> the gpio as ACTIVE_HIGH then the inverter would cause the reset pin to go low which will keep it in reset. So instead
>>> I set the gpio to ACTIVE_LOW which gives me the expected result.
>> I do not really care about particular board. Assuming that polarity of
>> the GPIO in DTS is specified correctly the effect of:
>>
>> gpiod_set_value_cansleep(tsdata->reset_pin, 1);
>>
>> is reset pin being _active_, i.e. the chip is staying in reset state.
> Setting the reset pin to 1/high will take the tsc out of reset. Setting the pin to 0/low will put the tsc into reset mode.
>> By the way, both reset and wake pins are active low according to the
>> data sheet I found.
> Your right. Reset and wake are both active low. However, that part of the code is trying to get the tsc out of reset which is
> why its setting the value to 1. Setting the pin to 1 was being done even before my patch.
>
> What has changed was removing the below line which puts the tsc in reset.
>
> error = devm_gpio_request_one(&client->dev, tsdata->reset_pin, GPIOF_OUT_INIT_LOW,"edt-ft5x06 reset");
>
> However, the above line isn't needed since when I request the gpio I already configured it as an output
> with a default value of 0.
>
Sorry accidentally hit reply in this thread instead of reply all. Adding back everyone removed from CC list.
>> Thanks.
>>
On Mon, Aug 24, 2015 at 03:23:43PM -0500, Franklin S Cooper Jr. wrote:
>
>
> On 08/24/2015 03:16 PM, Franklin S Cooper Jr. wrote:
> >
> > On 08/24/2015 03:01 PM, Dmitry Torokhov wrote:
> >> On Mon, Aug 24, 2015 at 02:48:36PM -0500, Franklin S Cooper Jr. wrote:
> >>> On 08/24/2015 02:41 PM, Dmitry Torokhov wrote:
> >>>> On Fri, Aug 21, 2015 at 02:08:32PM -0500, Franklin S Cooper Jr wrote:
> >>>>> The current/old gpio framework used doesn't properly listen to
> >>>>> ACTIVE_LOW and ACTIVE_HIGH flags. The newer gpio framework takes into
> >>>>> account these flags when setting gpio values.
> >>>>>
> >>>>> Also use gpiod_set_value_cansleep since wake and reset pins can be
> >>>>> provided by bus based io expanders.
> >>>>>
> >>>>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
> >>>>> ---
> >>>>> .../bindings/input/touchscreen/edt-ft5x06.txt | 4 +-
> >>>>> drivers/input/touchscreen/edt-ft5x06.c | 115 +++++++--------------
> >>>>> include/linux/input/edt-ft5x06.h | 4 +-
> >>>>> 3 files changed, 43 insertions(+), 80 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> >>>>> index 76db967..9330d4d 100644
> >>>>> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> >>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> >>>>> @@ -50,6 +50,6 @@ Example:
> >>>>> pinctrl-0 = <&edt_ft5x06_pins>;
> >>>>> interrupt-parent = <&gpio2>;
> >>>>> interrupts = <5 0>;
> >>>>> - reset-gpios = <&gpio2 6 1>;
> >>>>> - wake-gpios = <&gpio4 9 0>;
> >>>>> + reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
> >>>>> + wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
> >>>>> };
> >>>>> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> >>>>> index 394b1de..6b128b3 100644
> >>>>> --- a/drivers/input/touchscreen/edt-ft5x06.c
> >>>>> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> >>>>> @@ -91,9 +91,9 @@ struct edt_ft5x06_ts_data {
> >>>>> u16 num_x;
> >>>>> u16 num_y;
> >>>>>
> >>>>> - int reset_pin;
> >>>>> - int irq_pin;
> >>>>> - int wake_pin;
> >>>>> + struct gpio_desc *reset_pin;
> >>>>> + struct gpio_desc *wake_pin;
> >>>>> + struct gpio_desc *irq_pin;
> >>>>>
> >>>>> #if defined(CONFIG_DEBUG_FS)
> >>>>> struct dentry *debug_dir;
> >>>>> @@ -755,36 +755,14 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
> >>>>> static int edt_ft5x06_ts_reset(struct i2c_client *client,
> >>>>> struct edt_ft5x06_ts_data *tsdata)
> >>>>> {
> >>>>> - int error;
> >>>>> -
> >>>>> - if (gpio_is_valid(tsdata->wake_pin)) {
> >>>>> - error = devm_gpio_request_one(&client->dev,
> >>>>> - tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
> >>>>> - "edt-ft5x06 wake");
> >>>>> - if (error) {
> >>>>> - dev_err(&client->dev,
> >>>>> - "Failed to request GPIO %d as wake pin, error %d\n",
> >>>>> - tsdata->wake_pin, error);
> >>>>> - return error;
> >>>>> - }
> >>>>> -
> >>>>> + if (tsdata->wake_pin) {
> >>>>> msleep(5);
> >>>>> - gpio_set_value(tsdata->wake_pin, 1);
> >>>>> + gpiod_set_value_cansleep(tsdata->wake_pin, 1);
> >>>>> }
> >>>>> - if (gpio_is_valid(tsdata->reset_pin)) {
> >>>>> - /* this pulls reset down, enabling the low active reset */
> >>>>> - error = devm_gpio_request_one(&client->dev,
> >>>>> - tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
> >>>>> - "edt-ft5x06 reset");
> >>>>> - if (error) {
> >>>>> - dev_err(&client->dev,
> >>>>> - "Failed to request GPIO %d as reset pin, error %d\n",
> >>>>> - tsdata->reset_pin, error);
> >>>>> - return error;
> >>>>> - }
> >>>>>
> >>>>> + if (tsdata->reset_pin) {
> >>>>> msleep(5);
> >>>>> - gpio_set_value(tsdata->reset_pin, 1);
> >>>>> + gpiod_set_value_cansleep(tsdata->reset_pin, 1);
> >>>> So this leaves the reset pin active. How exactly was this tested?
> >>> Normally if the output gpio connected to the reset pin is ACTIVE_HIGH then this will take the tsc out of reset since
> >>> the reset pin is active low. However, I have a board that has an inverter between the gpio and reset pin. So if I leave
> >>> the gpio as ACTIVE_HIGH then the inverter would cause the reset pin to go low which will keep it in reset. So instead
> >>> I set the gpio to ACTIVE_LOW which gives me the expected result.
> >> I do not really care about particular board. Assuming that polarity of
> >> the GPIO in DTS is specified correctly the effect of:
> >>
> >> gpiod_set_value_cansleep(tsdata->reset_pin, 1);
> >>
> >> is reset pin being _active_, i.e. the chip is staying in reset state.
> > Setting the reset pin to 1/high will take the tsc out of reset. Setting the pin to 0/low will put the tsc into reset mode.
It seems you are not quite getting gpiod API. Consider:
For gpios described as GPIO_ACTIVE_LOW:
gpiod_set_value_cansleep(gpio, 1) => GPIO is LOW
gpiod_set_value_cansleep(gpio, 0) => GPIO is HIGH
For gpios described as GPIO_ACTIVE_HIGH:
gpiod_set_value_cansleep(gpio, 1) => GPIO is HIGH
gpiod_set_value_cansleep(gpio, 0) => GPIO is LOW
IOW with gpiod API 1 means logical active and can be either HIGH or LOW,
depending on GPIO definition in DTS.
> >> By the way, both reset and wake pins are active low according to the
> >> data sheet I found.
> > Your right. Reset and wake are both active low. However, that part of the code is trying to get the tsc out of reset which is
> > why its setting the value to 1. Setting the pin to 1 was being done even before my patch.
> >
> > What has changed was removing the below line which puts the tsc in reset.
> >
> > error = devm_gpio_request_one(&client->dev, tsdata->reset_pin, GPIOF_OUT_INIT_LOW,"edt-ft5x06 reset");
> >
> > However, the above line isn't needed since when I request the gpio I already configured it as an output
> > with a default value of 0.
So the old sequence resulted in GPIO going XXX->0->1 which brought the
chip into reset and out of it. With the sequence you have, assuming
that the GPIO is described as GPIO_ACTIVE_LOW (to match data sheet),
we'll be getting XXX->1->0:
devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW) =>
GPIO is *HIGH*
gpiod_set_value_cansleep(gpio, 1) => GPIO is *LOW*
> >
> Sorry accidentally hit reply in this thread instead of reply all. Adding back everyone removed from CC list.
Thanks.
--
Dmitry
On 08/24/2015 03:56 PM, Dmitry Torokhov wrote:
> On Mon, Aug 24, 2015 at 03:23:43PM -0500, Franklin S Cooper Jr. wrote:
>>
>> On 08/24/2015 03:16 PM, Franklin S Cooper Jr. wrote:
>>> On 08/24/2015 03:01 PM, Dmitry Torokhov wrote:
>>>> On Mon, Aug 24, 2015 at 02:48:36PM -0500, Franklin S Cooper Jr. wrote:
>>>>> On 08/24/2015 02:41 PM, Dmitry Torokhov wrote:
>>>>>> On Fri, Aug 21, 2015 at 02:08:32PM -0500, Franklin S Cooper Jr wrote:
>>>>>>> The current/old gpio framework used doesn't properly listen to
>>>>>>> ACTIVE_LOW and ACTIVE_HIGH flags. The newer gpio framework takes into
>>>>>>> account these flags when setting gpio values.
>>>>>>>
>>>>>>> Also use gpiod_set_value_cansleep since wake and reset pins can be
>>>>>>> provided by bus based io expanders.
>>>>>>>
>>>>>>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
>>>>>>> ---
>>>>>>> .../bindings/input/touchscreen/edt-ft5x06.txt | 4 +-
>>>>>>> drivers/input/touchscreen/edt-ft5x06.c | 115 +++++++--------------
>>>>>>> include/linux/input/edt-ft5x06.h | 4 +-
>>>>>>> 3 files changed, 43 insertions(+), 80 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>>>> index 76db967..9330d4d 100644
>>>>>>> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>>>> @@ -50,6 +50,6 @@ Example:
>>>>>>> pinctrl-0 = <&edt_ft5x06_pins>;
>>>>>>> interrupt-parent = <&gpio2>;
>>>>>>> interrupts = <5 0>;
>>>>>>> - reset-gpios = <&gpio2 6 1>;
>>>>>>> - wake-gpios = <&gpio4 9 0>;
>>>>>>> + reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
>>>>>>> + wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
>>>>>>> };
>>>>>>> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
>>>>>>> index 394b1de..6b128b3 100644
>>>>>>> --- a/drivers/input/touchscreen/edt-ft5x06.c
>>>>>>> +++ b/drivers/input/touchscreen/edt-ft5x06.c
>>>>>>> @@ -91,9 +91,9 @@ struct edt_ft5x06_ts_data {
>>>>>>> u16 num_x;
>>>>>>> u16 num_y;
>>>>>>>
>>>>>>> - int reset_pin;
>>>>>>> - int irq_pin;
>>>>>>> - int wake_pin;
>>>>>>> + struct gpio_desc *reset_pin;
>>>>>>> + struct gpio_desc *wake_pin;
>>>>>>> + struct gpio_desc *irq_pin;
>>>>>>>
>>>>>>> #if defined(CONFIG_DEBUG_FS)
>>>>>>> struct dentry *debug_dir;
>>>>>>> @@ -755,36 +755,14 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
>>>>>>> static int edt_ft5x06_ts_reset(struct i2c_client *client,
>>>>>>> struct edt_ft5x06_ts_data *tsdata)
>>>>>>> {
>>>>>>> - int error;
>>>>>>> -
>>>>>>> - if (gpio_is_valid(tsdata->wake_pin)) {
>>>>>>> - error = devm_gpio_request_one(&client->dev,
>>>>>>> - tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
>>>>>>> - "edt-ft5x06 wake");
>>>>>>> - if (error) {
>>>>>>> - dev_err(&client->dev,
>>>>>>> - "Failed to request GPIO %d as wake pin, error %d\n",
>>>>>>> - tsdata->wake_pin, error);
>>>>>>> - return error;
>>>>>>> - }
>>>>>>> -
>>>>>>> + if (tsdata->wake_pin) {
>>>>>>> msleep(5);
>>>>>>> - gpio_set_value(tsdata->wake_pin, 1);
>>>>>>> + gpiod_set_value_cansleep(tsdata->wake_pin, 1);
>>>>>>> }
>>>>>>> - if (gpio_is_valid(tsdata->reset_pin)) {
>>>>>>> - /* this pulls reset down, enabling the low active reset */
>>>>>>> - error = devm_gpio_request_one(&client->dev,
>>>>>>> - tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
>>>>>>> - "edt-ft5x06 reset");
>>>>>>> - if (error) {
>>>>>>> - dev_err(&client->dev,
>>>>>>> - "Failed to request GPIO %d as reset pin, error %d\n",
>>>>>>> - tsdata->reset_pin, error);
>>>>>>> - return error;
>>>>>>> - }
>>>>>>>
>>>>>>> + if (tsdata->reset_pin) {
>>>>>>> msleep(5);
>>>>>>> - gpio_set_value(tsdata->reset_pin, 1);
>>>>>>> + gpiod_set_value_cansleep(tsdata->reset_pin, 1);
>>>>>> So this leaves the reset pin active. How exactly was this tested?
>>>>> Normally if the output gpio connected to the reset pin is ACTIVE_HIGH then this will take the tsc out of reset since
>>>>> the reset pin is active low. However, I have a board that has an inverter between the gpio and reset pin. So if I leave
>>>>> the gpio as ACTIVE_HIGH then the inverter would cause the reset pin to go low which will keep it in reset. So instead
>>>>> I set the gpio to ACTIVE_LOW which gives me the expected result.
>>>> I do not really care about particular board. Assuming that polarity of
>>>> the GPIO in DTS is specified correctly the effect of:
>>>>
>>>> gpiod_set_value_cansleep(tsdata->reset_pin, 1);
>>>>
>>>> is reset pin being _active_, i.e. the chip is staying in reset state.
>>> Setting the reset pin to 1/high will take the tsc out of reset. Setting the pin to 0/low will put the tsc into reset mode.
> It seems you are not quite getting gpiod API. Consider:
>
> For gpios described as GPIO_ACTIVE_LOW:
>
> gpiod_set_value_cansleep(gpio, 1) => GPIO is LOW
> gpiod_set_value_cansleep(gpio, 0) => GPIO is HIGH
>
> For gpios described as GPIO_ACTIVE_HIGH:
>
> gpiod_set_value_cansleep(gpio, 1) => GPIO is HIGH
> gpiod_set_value_cansleep(gpio, 0) => GPIO is LOW
>
> IOW with gpiod API 1 means logical active and can be either HIGH or LOW,
> depending on GPIO definition in DTS.
>
>>>> By the way, both reset and wake pins are active low according to the
>>>> data sheet I found.
>>> Your right. Reset and wake are both active low. However, that part of the code is trying to get the tsc out of reset which is
>>> why its setting the value to 1. Setting the pin to 1 was being done even before my patch.
>>>
>>> What has changed was removing the below line which puts the tsc in reset.
>>>
>>> error = devm_gpio_request_one(&client->dev, tsdata->reset_pin, GPIOF_OUT_INIT_LOW,"edt-ft5x06 reset");
>>>
>>> However, the above line isn't needed since when I request the gpio I already configured it as an output
>>> with a default value of 0.
> So the old sequence resulted in GPIO going XXX->0->1 which brought the
> chip into reset and out of it. With the sequence you have, assuming
> that the GPIO is described as GPIO_ACTIVE_LOW (to match data sheet),
I'm not setting the GPIO_ACTIVE_LOW to match the datasheet. I'm setting it to ACTIVE_LOW because
of the inverter that is between the gpio and reset pin makes it "appear" as if the gpio is ACTIVE_LOW.
> we'll be getting XXX->1->0:
>
> devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW) =>
> GPIO is *HIGH*
>
> gpiod_set_value_cansleep(gpio, 1) => GPIO is *LOW*
So everything you stated is exactly what I wanted and expected when you take into account the inverter will invert the gpio signal before
it reaches the reset pin.
So with the old sequence with the gpio being set to ACTIVE_HIGH/flags being ignored this is the result on my board with the inverter ic
between the gpio and tsc reset pin.
devm_gpio_request_one(&client->dev, tsdata->wake_pin, GPIOF_OUT_INIT_LOW, "edt-ft5x06 wake");
devm_gpio_request_one -> 0(gpio) -> 1 (inverter ic) -> reset pin sees a high signal which takes it out of reset. Not what I wanted.
gpio_set_value(tsdata->reset_pin, 1);
gpio_set_value -> 1(gpio)-> 0 (inverter ic) -> reset pin sees a low signal with puts it into reset. Now what I wanted. Causes probe to fail
Which is the opposite of what I want.
So my solution was to set my gpio to ACTIVE_LOW which the new gpio framework supports.
devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW)
devm_gpiod_get_optional -> 0 -> 1(ACTIVE_LOW flag detected)-> 1(gpio)-> 0(inverter ic) -> reset pin set to low. Put tsc into reset.
gpiod_set_value_cansleep(gpio, 1)
gpiod_set_value_cansleep -> 1 -> 0(ACTIVE LOW flag detected) -> 0(gpio) -> 1 (inverter ic) -> reset pin sees a high signal. Take tsc out of reset
So the tsc driver is assuming that the reset pin is ACTIVE_LOW. I'm not changing the assumption. What
I'm changing is the behaviour of the gpio pin connected to the tsc reset pin.
So assuming that the gpio connected to tsc reset pin is ACTIVE_HIGH then the behaviour doesn't change at all especially since the previous gpio framework
ignores the ACTIVE_HIGH/ACTIVE_LOW flags.
>
>> Sorry accidentally hit reply in this thread instead of reply all. Adding back everyone removed from CC list.
> Thanks.
>
On Mon, Aug 24, 2015 at 05:16:15PM -0500, Franklin S Cooper Jr. wrote:
>
>
> On 08/24/2015 03:56 PM, Dmitry Torokhov wrote:
> > On Mon, Aug 24, 2015 at 03:23:43PM -0500, Franklin S Cooper Jr. wrote:
> >>
> >> On 08/24/2015 03:16 PM, Franklin S Cooper Jr. wrote:
> >>> On 08/24/2015 03:01 PM, Dmitry Torokhov wrote:
> >>>> On Mon, Aug 24, 2015 at 02:48:36PM -0500, Franklin S Cooper Jr. wrote:
> >>>>> On 08/24/2015 02:41 PM, Dmitry Torokhov wrote:
> >>>>>> On Fri, Aug 21, 2015 at 02:08:32PM -0500, Franklin S Cooper Jr wrote:
> >>>>>>> The current/old gpio framework used doesn't properly listen to
> >>>>>>> ACTIVE_LOW and ACTIVE_HIGH flags. The newer gpio framework takes into
> >>>>>>> account these flags when setting gpio values.
> >>>>>>>
> >>>>>>> Also use gpiod_set_value_cansleep since wake and reset pins can be
> >>>>>>> provided by bus based io expanders.
> >>>>>>>
> >>>>>>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
> >>>>>>> ---
> >>>>>>> .../bindings/input/touchscreen/edt-ft5x06.txt | 4 +-
> >>>>>>> drivers/input/touchscreen/edt-ft5x06.c | 115 +++++++--------------
> >>>>>>> include/linux/input/edt-ft5x06.h | 4 +-
> >>>>>>> 3 files changed, 43 insertions(+), 80 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> >>>>>>> index 76db967..9330d4d 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> >>>>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> >>>>>>> @@ -50,6 +50,6 @@ Example:
> >>>>>>> pinctrl-0 = <&edt_ft5x06_pins>;
> >>>>>>> interrupt-parent = <&gpio2>;
> >>>>>>> interrupts = <5 0>;
> >>>>>>> - reset-gpios = <&gpio2 6 1>;
> >>>>>>> - wake-gpios = <&gpio4 9 0>;
> >>>>>>> + reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
> >>>>>>> + wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
> >>>>>>> };
> >>>>>>> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> >>>>>>> index 394b1de..6b128b3 100644
> >>>>>>> --- a/drivers/input/touchscreen/edt-ft5x06.c
> >>>>>>> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> >>>>>>> @@ -91,9 +91,9 @@ struct edt_ft5x06_ts_data {
> >>>>>>> u16 num_x;
> >>>>>>> u16 num_y;
> >>>>>>>
> >>>>>>> - int reset_pin;
> >>>>>>> - int irq_pin;
> >>>>>>> - int wake_pin;
> >>>>>>> + struct gpio_desc *reset_pin;
> >>>>>>> + struct gpio_desc *wake_pin;
> >>>>>>> + struct gpio_desc *irq_pin;
> >>>>>>>
> >>>>>>> #if defined(CONFIG_DEBUG_FS)
> >>>>>>> struct dentry *debug_dir;
> >>>>>>> @@ -755,36 +755,14 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
> >>>>>>> static int edt_ft5x06_ts_reset(struct i2c_client *client,
> >>>>>>> struct edt_ft5x06_ts_data *tsdata)
> >>>>>>> {
> >>>>>>> - int error;
> >>>>>>> -
> >>>>>>> - if (gpio_is_valid(tsdata->wake_pin)) {
> >>>>>>> - error = devm_gpio_request_one(&client->dev,
> >>>>>>> - tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
> >>>>>>> - "edt-ft5x06 wake");
> >>>>>>> - if (error) {
> >>>>>>> - dev_err(&client->dev,
> >>>>>>> - "Failed to request GPIO %d as wake pin, error %d\n",
> >>>>>>> - tsdata->wake_pin, error);
> >>>>>>> - return error;
> >>>>>>> - }
> >>>>>>> -
> >>>>>>> + if (tsdata->wake_pin) {
> >>>>>>> msleep(5);
> >>>>>>> - gpio_set_value(tsdata->wake_pin, 1);
> >>>>>>> + gpiod_set_value_cansleep(tsdata->wake_pin, 1);
> >>>>>>> }
> >>>>>>> - if (gpio_is_valid(tsdata->reset_pin)) {
> >>>>>>> - /* this pulls reset down, enabling the low active reset */
> >>>>>>> - error = devm_gpio_request_one(&client->dev,
> >>>>>>> - tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
> >>>>>>> - "edt-ft5x06 reset");
> >>>>>>> - if (error) {
> >>>>>>> - dev_err(&client->dev,
> >>>>>>> - "Failed to request GPIO %d as reset pin, error %d\n",
> >>>>>>> - tsdata->reset_pin, error);
> >>>>>>> - return error;
> >>>>>>> - }
> >>>>>>>
> >>>>>>> + if (tsdata->reset_pin) {
> >>>>>>> msleep(5);
> >>>>>>> - gpio_set_value(tsdata->reset_pin, 1);
> >>>>>>> + gpiod_set_value_cansleep(tsdata->reset_pin, 1);
> >>>>>> So this leaves the reset pin active. How exactly was this tested?
> >>>>> Normally if the output gpio connected to the reset pin is ACTIVE_HIGH then this will take the tsc out of reset since
> >>>>> the reset pin is active low. However, I have a board that has an inverter between the gpio and reset pin. So if I leave
> >>>>> the gpio as ACTIVE_HIGH then the inverter would cause the reset pin to go low which will keep it in reset. So instead
> >>>>> I set the gpio to ACTIVE_LOW which gives me the expected result.
> >>>> I do not really care about particular board. Assuming that polarity of
> >>>> the GPIO in DTS is specified correctly the effect of:
> >>>>
> >>>> gpiod_set_value_cansleep(tsdata->reset_pin, 1);
> >>>>
> >>>> is reset pin being _active_, i.e. the chip is staying in reset state.
> >>> Setting the reset pin to 1/high will take the tsc out of reset. Setting the pin to 0/low will put the tsc into reset mode.
> > It seems you are not quite getting gpiod API. Consider:
> >
> > For gpios described as GPIO_ACTIVE_LOW:
> >
> > gpiod_set_value_cansleep(gpio, 1) => GPIO is LOW
> > gpiod_set_value_cansleep(gpio, 0) => GPIO is HIGH
> >
> > For gpios described as GPIO_ACTIVE_HIGH:
> >
> > gpiod_set_value_cansleep(gpio, 1) => GPIO is HIGH
> > gpiod_set_value_cansleep(gpio, 0) => GPIO is LOW
> >
> > IOW with gpiod API 1 means logical active and can be either HIGH or LOW,
> > depending on GPIO definition in DTS.
> >
> >>>> By the way, both reset and wake pins are active low according to the
> >>>> data sheet I found.
> >>> Your right. Reset and wake are both active low. However, that part of the code is trying to get the tsc out of reset which is
> >>> why its setting the value to 1. Setting the pin to 1 was being done even before my patch.
> >>>
> >>> What has changed was removing the below line which puts the tsc in reset.
> >>>
> >>> error = devm_gpio_request_one(&client->dev, tsdata->reset_pin, GPIOF_OUT_INIT_LOW,"edt-ft5x06 reset");
> >>>
> >>> However, the above line isn't needed since when I request the gpio I already configured it as an output
> >>> with a default value of 0.
> > So the old sequence resulted in GPIO going XXX->0->1 which brought the
> > chip into reset and out of it. With the sequence you have, assuming
> > that the GPIO is described as GPIO_ACTIVE_LOW (to match data sheet),
> I'm not setting the GPIO_ACTIVE_LOW to match the datasheet. I'm setting it to ACTIVE_LOW because
> of the inverter that is between the gpio and reset pin makes it "appear" as if the gpio is ACTIVE_LOW.
If the "normal" behavior (according to the data sheet) is active low and
you have an invertor, then your DTS should be GPIO_ACTIVE_HIGH. And
people with hardware that does not have invertor will use
GPIO_ACTIVE_LOW. And the driver should work with both settings.
> > we'll be getting XXX->1->0:
> >
> > devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW) =>
> > GPIO is *HIGH*
> >
> > gpiod_set_value_cansleep(gpio, 1) => GPIO is *LOW*
> So everything you stated is exactly what I wanted and expected when you take into account the inverter will invert the gpio signal before
> it reaches the reset pin.
I do not want to take into account invertor, that is what DTS polarity
flag for. Regular setup for this touchscreen - GPIO_ACTIVE_LOW, setup
with invertor - GPIO_ACTIVE_HIGH. From the driver perspective you should
be leaving the power on/reset function with reset and wake GPIOs
*inactive* (as in gpiod_set_value_cansleep(gpio, 0)).
Thanks.
--
Dmitry
On 08/24/2015 07:17 PM, Dmitry Torokhov wrote:
> On Mon, Aug 24, 2015 at 05:16:15PM -0500, Franklin S Cooper Jr. wrote:
>>
>> On 08/24/2015 03:56 PM, Dmitry Torokhov wrote:
>>> On Mon, Aug 24, 2015 at 03:23:43PM -0500, Franklin S Cooper Jr. wrote:
>>>> On 08/24/2015 03:16 PM, Franklin S Cooper Jr. wrote:
>>>>> On 08/24/2015 03:01 PM, Dmitry Torokhov wrote:
>>>>>> On Mon, Aug 24, 2015 at 02:48:36PM -0500, Franklin S Cooper Jr. wrote:
>>>>>>> On 08/24/2015 02:41 PM, Dmitry Torokhov wrote:
>>>>>>>> On Fri, Aug 21, 2015 at 02:08:32PM -0500, Franklin S Cooper Jr wrote:
>>>>>>>>> The current/old gpio framework used doesn't properly listen to
>>>>>>>>> ACTIVE_LOW and ACTIVE_HIGH flags. The newer gpio framework takes into
>>>>>>>>> account these flags when setting gpio values.
>>>>>>>>>
>>>>>>>>> Also use gpiod_set_value_cansleep since wake and reset pins can be
>>>>>>>>> provided by bus based io expanders.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
>>>>>>>>> ---
>>>>>>>>> .../bindings/input/touchscreen/edt-ft5x06.txt | 4 +-
>>>>>>>>> drivers/input/touchscreen/edt-ft5x06.c | 115 +++++++--------------
>>>>>>>>> include/linux/input/edt-ft5x06.h | 4 +-
>>>>>>>>> 3 files changed, 43 insertions(+), 80 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>>>>>> index 76db967..9330d4d 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>>>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>>>>>> @@ -50,6 +50,6 @@ Example:
>>>>>>>>> pinctrl-0 = <&edt_ft5x06_pins>;
>>>>>>>>> interrupt-parent = <&gpio2>;
>>>>>>>>> interrupts = <5 0>;
>>>>>>>>> - reset-gpios = <&gpio2 6 1>;
>>>>>>>>> - wake-gpios = <&gpio4 9 0>;
>>>>>>>>> + reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
>>>>>>>>> + wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
>>>>>>>>> };
>>>>>>>>> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
>>>>>>>>> index 394b1de..6b128b3 100644
>>>>>>>>> --- a/drivers/input/touchscreen/edt-ft5x06.c
>>>>>>>>> +++ b/drivers/input/touchscreen/edt-ft5x06.c
>>>>>>>>> @@ -91,9 +91,9 @@ struct edt_ft5x06_ts_data {
>>>>>>>>> u16 num_x;
>>>>>>>>> u16 num_y;
>>>>>>>>>
>>>>>>>>> - int reset_pin;
>>>>>>>>> - int irq_pin;
>>>>>>>>> - int wake_pin;
>>>>>>>>> + struct gpio_desc *reset_pin;
>>>>>>>>> + struct gpio_desc *wake_pin;
>>>>>>>>> + struct gpio_desc *irq_pin;
>>>>>>>>>
>>>>>>>>> #if defined(CONFIG_DEBUG_FS)
>>>>>>>>> struct dentry *debug_dir;
>>>>>>>>> @@ -755,36 +755,14 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
>>>>>>>>> static int edt_ft5x06_ts_reset(struct i2c_client *client,
>>>>>>>>> struct edt_ft5x06_ts_data *tsdata)
>>>>>>>>> {
>>>>>>>>> - int error;
>>>>>>>>> -
>>>>>>>>> - if (gpio_is_valid(tsdata->wake_pin)) {
>>>>>>>>> - error = devm_gpio_request_one(&client->dev,
>>>>>>>>> - tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
>>>>>>>>> - "edt-ft5x06 wake");
>>>>>>>>> - if (error) {
>>>>>>>>> - dev_err(&client->dev,
>>>>>>>>> - "Failed to request GPIO %d as wake pin, error %d\n",
>>>>>>>>> - tsdata->wake_pin, error);
>>>>>>>>> - return error;
>>>>>>>>> - }
>>>>>>>>> -
>>>>>>>>> + if (tsdata->wake_pin) {
>>>>>>>>> msleep(5);
>>>>>>>>> - gpio_set_value(tsdata->wake_pin, 1);
>>>>>>>>> + gpiod_set_value_cansleep(tsdata->wake_pin, 1);
>>>>>>>>> }
>>>>>>>>> - if (gpio_is_valid(tsdata->reset_pin)) {
>>>>>>>>> - /* this pulls reset down, enabling the low active reset */
>>>>>>>>> - error = devm_gpio_request_one(&client->dev,
>>>>>>>>> - tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
>>>>>>>>> - "edt-ft5x06 reset");
>>>>>>>>> - if (error) {
>>>>>>>>> - dev_err(&client->dev,
>>>>>>>>> - "Failed to request GPIO %d as reset pin, error %d\n",
>>>>>>>>> - tsdata->reset_pin, error);
>>>>>>>>> - return error;
>>>>>>>>> - }
>>>>>>>>>
>>>>>>>>> + if (tsdata->reset_pin) {
>>>>>>>>> msleep(5);
>>>>>>>>> - gpio_set_value(tsdata->reset_pin, 1);
>>>>>>>>> + gpiod_set_value_cansleep(tsdata->reset_pin, 1);
>>>>>>>> So this leaves the reset pin active. How exactly was this tested?
>>>>>>> Normally if the output gpio connected to the reset pin is ACTIVE_HIGH then this will take the tsc out of reset since
>>>>>>> the reset pin is active low. However, I have a board that has an inverter between the gpio and reset pin. So if I leave
>>>>>>> the gpio as ACTIVE_HIGH then the inverter would cause the reset pin to go low which will keep it in reset. So instead
>>>>>>> I set the gpio to ACTIVE_LOW which gives me the expected result.
>>>>>> I do not really care about particular board. Assuming that polarity of
>>>>>> the GPIO in DTS is specified correctly the effect of:
>>>>>>
>>>>>> gpiod_set_value_cansleep(tsdata->reset_pin, 1);
>>>>>>
>>>>>> is reset pin being _active_, i.e. the chip is staying in reset state.
>>>>> Setting the reset pin to 1/high will take the tsc out of reset. Setting the pin to 0/low will put the tsc into reset mode.
>>> It seems you are not quite getting gpiod API. Consider:
>>>
>>> For gpios described as GPIO_ACTIVE_LOW:
>>>
>>> gpiod_set_value_cansleep(gpio, 1) => GPIO is LOW
>>> gpiod_set_value_cansleep(gpio, 0) => GPIO is HIGH
>>>
>>> For gpios described as GPIO_ACTIVE_HIGH:
>>>
>>> gpiod_set_value_cansleep(gpio, 1) => GPIO is HIGH
>>> gpiod_set_value_cansleep(gpio, 0) => GPIO is LOW
>>>
>>> IOW with gpiod API 1 means logical active and can be either HIGH or LOW,
>>> depending on GPIO definition in DTS.
>>>
>>>>>> By the way, both reset and wake pins are active low according to the
>>>>>> data sheet I found.
>>>>> Your right. Reset and wake are both active low. However, that part of the code is trying to get the tsc out of reset which is
>>>>> why its setting the value to 1. Setting the pin to 1 was being done even before my patch.
>>>>>
>>>>> What has changed was removing the below line which puts the tsc in reset.
>>>>>
>>>>> error = devm_gpio_request_one(&client->dev, tsdata->reset_pin, GPIOF_OUT_INIT_LOW,"edt-ft5x06 reset");
>>>>>
>>>>> However, the above line isn't needed since when I request the gpio I already configured it as an output
>>>>> with a default value of 0.
>>> So the old sequence resulted in GPIO going XXX->0->1 which brought the
>>> chip into reset and out of it. With the sequence you have, assuming
>>> that the GPIO is described as GPIO_ACTIVE_LOW (to match data sheet),
>> I'm not setting the GPIO_ACTIVE_LOW to match the datasheet. I'm setting it to ACTIVE_LOW because
>> of the inverter that is between the gpio and reset pin makes it "appear" as if the gpio is ACTIVE_LOW.
> If the "normal" behavior (according to the data sheet) is active low and
> you have an invertor, then your DTS should be GPIO_ACTIVE_HIGH. And
> people with hardware that does not have invertor will use
> GPIO_ACTIVE_LOW. And the driver should work with both settings.
>
>>> we'll be getting XXX->1->0:
>>>
>>> devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW) =>
>>> GPIO is *HIGH*
>>>
>>> gpiod_set_value_cansleep(gpio, 1) => GPIO is *LOW*
>> So everything you stated is exactly what I wanted and expected when you take into account the inverter will invert the gpio signal before
>> it reaches the reset pin.
> I do not want to take into account invertor, that is what DTS polarity
> flag for. Regular setup for this touchscreen - GPIO_ACTIVE_LOW, setup
> with invertor - GPIO_ACTIVE_HIGH. From the driver perspective you should
> be leaving the power on/reset function with reset and wake GPIOs
> *inactive* (as in gpiod_set_value_cansleep(gpio, 0)).
Ok. I was talking to someone about this and I believe I understand what your saying. So
essentially the set_value call before my patch was based on voltage while the new gpio
is based on logic.
So the solution would be to change gpiod_set_value_cansleep to 0. Then my hardware inverted GPIO
signal will be set to GPIO_ACTIVE_HIGH in dt. I can then tweak all the DTS files currently using this
driver in mainline to reflect this change.
>
> Thanks.
>