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

[Hans de Goede <hansg@kernel.org>]

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: