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

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

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@collabora.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@kernel.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;