[PATCH] input: goodix: Remove setting of RST pin to input

[PATCH] input: goodix: Remove setting of RST pin to input

[Martyn Welch]

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@collabora.com>

---
 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:
-- 
2.39.5

Re: [PATCH] input: goodix: Remove setting of RST pin to input

[Hans de Goede]

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@collabora.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: