[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <dad227b8-7ff0-426c-80dd-aa3e725549b0@kernel.org>
Date: Thu, 9 Oct 2025 19:51:39 +0200
From: Hans de Goede <hansg@...nel.org>
To: Martyn Welch <martyn.welch@...labora.com>,
Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: kernel@...labora.com, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] input: goodix: Remove setting of RST pin to input
Hi,
On 9-Oct-25 3:41 PM, Martyn Welch wrote:
> The reset line is being set to input on non-ACPI devices apparently to
> save power. This isn't being done on ACPI devices as it's been found
> that some ACPI devices don't have a pull-up resistor fitted. This can
> also be the case for non-ACPI devices, resulting in:
>
> [ 941.672207] Goodix-TS 1-0014: Error reading 10 bytes from 0x814e: -110
> [ 942.696168] Goodix-TS 1-0014: Error reading 10 bytes from 0x814e: -110
> [ 945.832208] Goodix-TS 1-0014: Error reading 10 bytes from 0x814e: -110
>
> This behaviour appears to have been initialing introduced in
> ec6e1b4082d9. This doesn't seem to be based on information in either the
> GT911 or GT9271 datasheets cited as sources of information for this
> change. Thus it seems likely that it is based on functionality in the
> Android driver which it also lists. This behaviour may be viable in very
> specific instances where the hardware is well known, but seems unwise in
> the upstream kernel where such hardware requirements can't be
> guaranteed.
>
> Remove this over optimisation to improve reliability on non-ACPI
> devices.
>
> Signed-off-by: Martyn Welch <martyn.welch@...labora.com>
>
> ---
>
> Changes since v1:
> - Dropping gpiod_rst_flags and directly passing GPIOD_ASIS when
> requesting the reset pin.
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hansg@...nel.org>
Regards,
Hans
>
> drivers/input/touchscreen/goodix.c | 27 +--------------------------
> drivers/input/touchscreen/goodix.h | 1 -
> 2 files changed, 1 insertion(+), 27 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 252dcae039f8..f838f92100c2 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -796,17 +796,6 @@ int goodix_reset_no_int_sync(struct goodix_ts_data *ts)
>
> usleep_range(6000, 10000); /* T4: > 5ms */
>
> - /*
> - * Put the reset pin back in to input / high-impedance mode to save
> - * power. Only do this in the non ACPI case since some ACPI boards
> - * don't have a pull-up, so there the reset pin must stay active-high.
> - */
> - if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_GPIO) {
> - error = gpiod_direction_input(ts->gpiod_rst);
> - if (error)
> - goto error;
> - }
> -
> return 0;
>
> error:
> @@ -957,14 +946,6 @@ static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)
> return -EINVAL;
> }
>
> - /*
> - * Normally we put the reset pin in input / high-impedance mode to save
> - * power. But some x86/ACPI boards don't have a pull-up, so for the ACPI
> - * case, leave the pin as is. This results in the pin not being touched
> - * at all on x86/ACPI boards, except when needed for error-recover.
> - */
> - ts->gpiod_rst_flags = GPIOD_ASIS;
> -
> return devm_acpi_dev_add_driver_gpios(dev, gpio_mapping);
> }
> #else
> @@ -989,12 +970,6 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
> return -EINVAL;
> dev = &ts->client->dev;
>
> - /*
> - * By default we request the reset pin as input, leaving it in
> - * high-impedance when not resetting the controller to save power.
> - */
> - ts->gpiod_rst_flags = GPIOD_IN;
> -
> ts->avdd28 = devm_regulator_get(dev, "AVDD28");
> if (IS_ERR(ts->avdd28))
> return dev_err_probe(dev, PTR_ERR(ts->avdd28), "Failed to get AVDD28 regulator\n");
> @@ -1019,7 +994,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
> ts->gpiod_int = gpiod;
>
> /* Get the reset line GPIO pin number */
> - gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_RST_NAME, ts->gpiod_rst_flags);
> + gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_RST_NAME, GPIOD_ASIS);
> if (IS_ERR(gpiod))
> return dev_err_probe(dev, PTR_ERR(gpiod), "Failed to get %s GPIO\n",
> GOODIX_GPIO_RST_NAME);
> diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
> index 87797cc88b32..0d1e8a8d2cba 100644
> --- a/drivers/input/touchscreen/goodix.h
> +++ b/drivers/input/touchscreen/goodix.h
> @@ -88,7 +88,6 @@ struct goodix_ts_data {
> struct gpio_desc *gpiod_rst;
> int gpio_count;
> int gpio_int_idx;
> - enum gpiod_flags gpiod_rst_flags;
> char id[GOODIX_ID_MAX_LEN + 1];
> char cfg_name[64];
> u16 version;
Powered by blists - more mailing lists