[PATCH 0/2] input: touch: goodix: Extend reset pull-up fix to DT platforms

[PATCH 0/2] input: touch: goodix: Extend reset pull-up fix to DT platforms

[Esben Haabendal]

This extends the fix for platforms without external pull-up on reset pin
to device-tree platforms.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Esben Haabendal (2):
      dt-bindings: input: touchscreen: goodix: Add no-reset-pull-up property
      Input: goodix - Allow DT specification of missing reset pull-up

 .../devicetree/bindings/input/touchscreen/goodix.yaml          |  4 ++++
 drivers/input/touchscreen/goodix.c                             | 10 +++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)
---
base-commit: 9c32cda43eb78f78c73aee4aa344b777714e259b
change-id: 20250422-goodix-no-reset-pull-up-d2e4a9a3e07a

Best regards,
-- 
Esben Haabendal <esben@geanix.com>

[PATCH 1/2] dt-bindings: input: touchscreen: goodix: Add no-reset-pull-up property

[Esben Haabendal]

This should be added for boards where there is no pull-up on the reset pin,
as the driver will otherwise switch the reset signal to high-impedance to
save power, which obviously not safe without pull-up.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
index eb4992f708b70fef93bd4b59b9565123f7c6ad5d..7e5c4b98f2cb1ef61798252ea5c573068a46d4aa 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
@@ -45,6 +45,10 @@ properties:
   reset-gpios:
     maxItems: 1
 
+  no-reset-pull-up:
+    type: boolean
+    description: There is no pull-up on reset pin
+
   AVDD28-supply:
     description: Analog power supply regulator on AVDD28 pin
 

-- 
2.49.0

Re: [PATCH 1/2] dt-bindings: input: touchscreen: goodix: Add no-reset-pull-up property

[Krzysztof Kozlowski]

On Tue, Apr 22, 2025 at 05:15:02PM GMT, Esben Haabendal wrote:
> This should be added for boards where there is no pull-up on the reset pin,
> as the driver will otherwise switch the reset signal to high-impedance to
> save power, which obviously not safe without pull-up.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> index eb4992f708b70fef93bd4b59b9565123f7c6ad5d..7e5c4b98f2cb1ef61798252ea5c573068a46d4aa 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> @@ -45,6 +45,10 @@ properties:
>    reset-gpios:
>      maxItems: 1
>  
> +  no-reset-pull-up:

Is this common property? Where is it defined? Otherwise missing vendor
prefix.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: input: touchscreen: goodix: Add no-reset-pull-up property

[Esben Haabendal]

On Monday, April 28th, 2025 at 09:48, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Apr 22, 2025 at 05:15:02PM GMT, Esben Haabendal wrote:
> 
> > This should be added for boards where there is no pull-up on the reset pin,
> > as the driver will otherwise switch the reset signal to high-impedance to
> > save power, which obviously not safe without pull-up.
> > 
> > Signed-off-by: Esben Haabendal esben@geanix.com
> > ---
> > Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 4 ++++
> > 1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> > index eb4992f708b70fef93bd4b59b9565123f7c6ad5d..7e5c4b98f2cb1ef61798252ea5c573068a46d4aa 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> > @@ -45,6 +45,10 @@ properties:
> > reset-gpios:
> > maxItems: 1
> > 
> > + no-reset-pull-up:
> 
> Is this common property? Where is it defined? Otherwise missing vendor
> prefix.

Good question. When is something a common property?

The idea of marking something as not having a pull-up on the reset pin could be considered a common thing I guess.
But for now, I am defining it for the goodix driver only, as I am only aware of these devices needing to handle it in a special way.

Should I rename it to goodix,no-reset-pull-up?

/Esben

Re: [PATCH 1/2] dt-bindings: input: touchscreen: goodix: Add no-reset-pull-up property

[Dmitry Torokhov]

On Mon, Apr 28, 2025 at 07:58:55AM +0000, Esben Haabendal wrote:
> On Monday, April 28th, 2025 at 09:48, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Tue, Apr 22, 2025 at 05:15:02PM GMT, Esben Haabendal wrote:
> > 
> > > This should be added for boards where there is no pull-up on the reset pin,
> > > as the driver will otherwise switch the reset signal to high-impedance to
> > > save power, which obviously not safe without pull-up.
> > > 
> > > Signed-off-by: Esben Haabendal esben@geanix.com
> > > ---
> > > Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> > > index eb4992f708b70fef93bd4b59b9565123f7c6ad5d..7e5c4b98f2cb1ef61798252ea5c573068a46d4aa 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> > > @@ -45,6 +45,10 @@ properties:
> > > reset-gpios:
> > > maxItems: 1
> > > 
> > > + no-reset-pull-up:
> > 
> > Is this common property? Where is it defined? Otherwise missing vendor
> > prefix.
> 
> Good question. When is something a common property?
> 
> The idea of marking something as not having a pull-up on the reset pin could be considered a common thing I guess.
> But for now, I am defining it for the goodix driver only, as I am only aware of these devices needing to handle it in a special way.
> 
> Should I rename it to goodix,no-reset-pull-up?

We already have GPIO_PULL_UP/GPIO_PULL_DOWN flags available in GPIO
bindings. So maybe the correct way is to have the driver rely on them
and only leave the reset line in high-impedance mode if GPIO tells it
that there is a pull-up?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] dt-bindings: input: touchscreen: goodix: Add no-reset-pull-up property

[Esben Haabendal]

"Dmitry Torokhov" <dmitry.torokhov@gmail.com> writes:

> On Mon, Apr 28, 2025 at 07:58:55AM +0000, Esben Haabendal wrote:
>> On Monday, April 28th, 2025 at 09:48, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> > On Tue, Apr 22, 2025 at 05:15:02PM GMT, Esben Haabendal wrote:
>> >
>> > > This should be added for boards where there is no pull-up on the reset pin,
>> > > as the driver will otherwise switch the reset signal to high-impedance to
>> > > save power, which obviously not safe without pull-up.
>> > >
>> > > Signed-off-by: Esben Haabendal esben@geanix.com
>> > > ---
>> > > Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 4 ++++
>> > > 1 file changed, 4 insertions(+)
>> > >
>> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> > > index eb4992f708b70fef93bd4b59b9565123f7c6ad5d..7e5c4b98f2cb1ef61798252ea5c573068a46d4aa 100644
>> > > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> > > @@ -45,6 +45,10 @@ properties:
>> > > reset-gpios:
>> > > maxItems: 1
>> > >
>> > > + no-reset-pull-up:
>> >
>> > Is this common property? Where is it defined? Otherwise missing vendor
>> > prefix.
>>
>> Good question. When is something a common property?
>>
>> The idea of marking something as not having a pull-up on the reset pin could be considered a common thing I guess.
>> But for now, I am defining it for the goodix driver only, as I am only aware of these devices needing to handle it in a special way.
>>
>> Should I rename it to goodix,no-reset-pull-up?
>
> We already have GPIO_PULL_UP/GPIO_PULL_DOWN flags available in GPIO
> bindings. So maybe the correct way is to have the driver rely on them
> and only leave the reset line in high-impedance mode if GPIO tells it
> that there is a pull-up?

As I understand GPIO_PULL_UP/GPIO_PULL_DOWN flags in bindings, they
indicate that pull-up/pull-down is to be configured for the gpio.

This is different to what I am expressing with goodix,no-reset-pull-up,
as I am expressing the lack of external pull-up on the signal. Without
that, the goodix driver assumes that an external pull-up is mounted, and
that the gpio pin can be set to high impedance, and the external pull-up
will ensure that it stays high.

How do you propose that we can use GPIO_PULL_UP/GPIO_PULL_DOWN flags for
this purpose?

/Esben

Re: [PATCH 1/2] dt-bindings: input: touchscreen: goodix: Add no-reset-pull-up property

[Krzysztof Kozlowski]

On 28/04/2025 09:58, Esben Haabendal wrote:
> On Monday, April 28th, 2025 at 09:48, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On Tue, Apr 22, 2025 at 05:15:02PM GMT, Esben Haabendal wrote:
>>
>>> This should be added for boards where there is no pull-up on the reset pin,
>>> as the driver will otherwise switch the reset signal to high-impedance to
>>> save power, which obviously not safe without pull-up.
>>>
>>> Signed-off-by: Esben Haabendal esben@geanix.com
>>> ---
>>> Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>> index eb4992f708b70fef93bd4b59b9565123f7c6ad5d..7e5c4b98f2cb1ef61798252ea5c573068a46d4aa 100644
>>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>> @@ -45,6 +45,10 @@ properties:
>>> reset-gpios:
>>> maxItems: 1
>>>
>>> + no-reset-pull-up:
>>
>> Is this common property? Where is it defined? Otherwise missing vendor
>> prefix.
> 
> Good question. When is something a common property?

When is defined in common schema and used by more than 2 devices.

> 
> The idea of marking something as not having a pull-up on the reset pin could be considered a common thing I guess.
> But for now, I am defining it for the goodix driver only, as I am only aware of these devices needing to handle it in a special way.
> 
> Should I rename it to goodix,no-reset-pull-up?


Yes


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: input: touchscreen: goodix: Add no-reset-pull-up property

[Esben Haabendal]

"Krzysztof Kozlowski" <krzk@kernel.org> writes:

> On 28/04/2025 09:58, Esben Haabendal wrote:
>> On Monday, April 28th, 2025 at 09:48, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> On Tue, Apr 22, 2025 at 05:15:02PM GMT, Esben Haabendal wrote:
>>>
>>>> This should be added for boards where there is no pull-up on the reset pin,
>>>> as the driver will otherwise switch the reset signal to high-impedance to
>>>> save power, which obviously not safe without pull-up.
>>>>
>>>> Signed-off-by: Esben Haabendal esben@geanix.com
>>>> ---
>>>> Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>>> index eb4992f708b70fef93bd4b59b9565123f7c6ad5d..7e5c4b98f2cb1ef61798252ea5c573068a46d4aa 100644
>>>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>>> @@ -45,6 +45,10 @@ properties:
>>>> reset-gpios:
>>>> maxItems: 1
>>>>
>>>> + no-reset-pull-up:
>>>
>>> Is this common property? Where is it defined? Otherwise missing vendor
>>> prefix.
>>
>> Good question. When is something a common property?
>
> When is defined in common schema and used by more than 2 devices.

Ok. But do we try to predict this in advance? I mean, you can easily add
a property which initially is just for one device (or one vendor?) and
then later on the same property turns out to be needed for other
devices/vendors?

When that happen, do we then define a common property, and then leave
support for the vendor specific variant for backwards compatibility.

>> The idea of marking something as not having a pull-up on the reset pin could be considered a common thing I guess.
>> But for now, I am defining it for the goodix driver only, as I am only aware of these devices needing to handle it in a special way.
>>
>> Should I rename it to goodix,no-reset-pull-up?
>
> Yes

Will do.

/Esben

[PATCH 2/2] Input: goodix - Allow DT specification of missing reset pull-up

[Esben Haabendal]

In commit a2fd46cd3dbb ("Input: goodix - try not to touch the reset-pin on x86/ACPI devices")
a fix for problems on various x86/ACPI devices where external
pull-up is missing were added. The same type of problem can exist on
device-tree platforms, and the fix can be activated by adding the
no-reset-pull-up device-tree property.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/input/touchscreen/goodix.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index a3e8a51c91449533b4d5185746df6b98676053dd..3a55b0f8e5132a5e1fe77bd27de74e058a4afdaf 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -775,7 +775,8 @@ int goodix_reset_no_int_sync(struct goodix_ts_data *ts)
 	 * 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) {
+	if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_GPIO &&
+	    ts->gpiod_rst_flags == GPIOD_IN) {
 		error = gpiod_direction_input(ts->gpiod_rst);
 		if (error)
 			goto error;
@@ -969,6 +970,13 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 	 */
 	ts->gpiod_rst_flags = GPIOD_IN;
 
+	/*
+	 * Devices that does not have pull-up on reset signal should not be
+	 * changed to input
+	 */
+	if (device_property_read_bool(dev, "no-reset-pull-up"))
+		ts->gpiod_rst_flags = GPIOD_ASIS;
+
 	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");

-- 
2.49.0

Re: [PATCH 2/2] Input: goodix - Allow DT specification of missing reset pull-up

[Hans de Goede]

Hi,

On 22-Apr-25 17:15, Esben Haabendal wrote:
> In commit a2fd46cd3dbb ("Input: goodix - try not to touch the reset-pin on x86/ACPI devices")
> a fix for problems on various x86/ACPI devices where external
> pull-up is missing were added. The same type of problem can exist on
> device-tree platforms, and the fix can be activated by adding the
> no-reset-pull-up device-tree property.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>

Thank you for your patch.

> ---
>  drivers/input/touchscreen/goodix.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index a3e8a51c91449533b4d5185746df6b98676053dd..3a55b0f8e5132a5e1fe77bd27de74e058a4afdaf 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -775,7 +775,8 @@ int goodix_reset_no_int_sync(struct goodix_ts_data *ts)
>  	 * 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) {
> +	if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_GPIO &&
> +	    ts->gpiod_rst_flags == GPIOD_IN) {

You can simplify the check to just:

	if (ts->gpiod_rst_flags == GPIOD_IN) {

that will work for the IRQ_PIN_ACCESS_ACPI* access_methods too and
nicely lines up with the gpiod_direction_input() on the next line.

Please also update the comment above the check to reflect the new
situation.

With this fixed, this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans





>  		error = gpiod_direction_input(ts->gpiod_rst);
>  		if (error)
>  			goto error;
> @@ -969,6 +970,13 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
>  	 */
>  	ts->gpiod_rst_flags = GPIOD_IN;
>  
> +	/*
> +	 * Devices that does not have pull-up on reset signal should not be
> +	 * changed to input
> +	 */
> +	if (device_property_read_bool(dev, "no-reset-pull-up"))
> +		ts->gpiod_rst_flags = GPIOD_ASIS;
> +
>  	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");
>