[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0596944a-4c34-467c-ba3f-e2bbb10081f1@kernel.org>
Date: Tue, 7 Oct 2025 16:06:08 +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] input: goodix: Remove setting of RST pin to input
Hi Martyn,
On 7-Oct-25 12:23, 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 initially 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 known to have a pull-up fitted, 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>
I have no objection against this simplification of the code, but the reset
pin will still be set to input mode when requested after this change,
see the use of gpiod_rst_flags in the driver.
For v2 it would be best to drop gpiod_rst_flags and directly pass
GPIOD_ASIS when requesting the reset pin.
Note for ACPI we want GPIOD_ASIS because there the driver only resets
the chip if it is non-responsive since typically it has already been
reset by the BIOS.
For non ACPI goodix_reset() will immediately call:
gpiod_direction_output(ts->gpiod_rst, 0)
followed by a msleep(20) so there using GPIOD_ASIS does not matter
as the direction + value will get set immediately after requesting it.
Regards,
Hans
>
> ---
> drivers/input/touchscreen/goodix.c | 11 -----------
> 1 file changed, 11 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 252dcae039f8..e7ef744011ad 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:
Powered by blists - more mailing lists