| Message ID | 20250422-goodix-no-reset-pull-up-v1-1-3983bb65a1bf@geanix.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | input: touch: goodix: Extend reset pull-up fix to DT platforms | expand |
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
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.
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
"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
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
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(+)