Re: [PATCH] Input: edt-ft5x06 - Switch to newer gpio framework
From: Dmitry Torokhov
Date: Mon Aug 24 2015 - 16:56:34 EST
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 <fcooper@xxxxxx>
> >>>>> ---
> >>>>> .../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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/