[PATCH] Input: goodix - decouple irq and reset lines

[PATCH] Input: goodix - decouple irq and reset lines

[Alex Gonzalez]

The current implementation on the Goodix driver requires the two optional GPIOs
(RESET and INT) to be provided.

However, it is possible to designs the RESET line pulled-up and only provide
the INT line. Such designs do not work with the current implementation.

The patch attached allows for designs that only provide the INT line to have
a functional touch on the default I2C address, even though some functionality
like the sleep mode will not be available.

This has been tested on v4.20 with a gt911 based touch panel design that only
provides the INT line.

Alex Gonzalez (1):
  Input: goodix - decouple irq and reset lines

 drivers/input/touchscreen/goodix.c | 59 ++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

[PATCH] Input: goodix - decouple irq and reset lines

[Alex Gonzalez]

The current implementation on the Goodix driver requires the two optional GPIOs
(RESET and INT) to be provided.

However, it is possible to designs the RESET line pulled-up and only provide
the INT line. Such designs do not work with the current implementation.

The patch attached allows for designs that only provide the INT line to have
a functional touch on the default I2C address, even though some functionality
like the sleep mode will not be available.

This has been tested on v4.20 with a gt911 based touch panel design that only
provides the INT line.

Alex Gonzalez (1):
  Input: goodix - decouple irq and reset lines

 drivers/input/touchscreen/goodix.c | 59 ++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

[PATCH] Input: goodix - decouple irq and reset lines

[Alex Gonzalez]

The Goodix touch controller allows the use of two optional GPIOs (RESET
and INT) to reset the touch controller, select the I2C address of the
device and exit the device from sleep mode.

The current implementation requires both GPIOs to be provided, however,
it is possible to provide only the INT line and not to have the RESET line
available but pulled-up.

Designs that only provide the INT line are able to operate the touch on
the default I2C address but will not be able to reset the touch via
software or place the device in sleep mode.

Signed-off-by: Alex Gonzalez <alex.gonzalez@digi.com>
---
 drivers/input/touchscreen/goodix.c | 59 ++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f2d9c2c41885..8d96b0b771b6 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -484,34 +484,39 @@ static int goodix_reset(struct goodix_ts_data *ts)
 {
 	int error;
 
-	/* begin select I2C slave addr */
-	error = gpiod_direction_output(ts->gpiod_rst, 0);
-	if (error)
-		return error;
+	if (ts->gpiod_rst) {
+		/* begin select I2C slave addr */
+		error = gpiod_direction_output(ts->gpiod_rst, 0);
+		if (error)
+			return error;
 
-	msleep(20);				/* T2: > 10ms */
+		msleep(20);				/* T2: > 10ms */
 
-	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
-	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
-	if (error)
-		return error;
+		/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
+		error = gpiod_direction_output(ts->gpiod_int,
+				ts->client->addr == 0x14);
+		if (error)
+			return error;
 
-	usleep_range(100, 2000);		/* T3: > 100us */
+		usleep_range(100, 2000);		/* T3: > 100us */
 
-	error = gpiod_direction_output(ts->gpiod_rst, 1);
-	if (error)
-		return error;
+		error = gpiod_direction_output(ts->gpiod_rst, 1);
+		if (error)
+			return error;
 
-	usleep_range(6000, 10000);		/* T4: > 5ms */
+		usleep_range(6000, 10000);		/* T4: > 5ms */
 
-	/* end select I2C slave addr */
-	error = gpiod_direction_input(ts->gpiod_rst);
-	if (error)
-		return error;
+		/* end select I2C slave addr */
+		error = gpiod_direction_input(ts->gpiod_rst);
+		if (error)
+			return error;
+	}
 
-	error = goodix_int_sync(ts);
-	if (error)
-		return error;
+	if (ts->gpiod_int) {
+		error = goodix_int_sync(ts);
+		if (error)
+			return error;
+	}
 
 	return 0;
 }
@@ -786,13 +791,11 @@ static int goodix_ts_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
-	if (ts->gpiod_int && ts->gpiod_rst) {
-		/* reset the controller */
-		error = goodix_reset(ts);
-		if (error) {
-			dev_err(&client->dev, "Controller reset failed.\n");
-			return error;
-		}
+	/* reset the controller */
+	error = goodix_reset(ts);
+	if (error) {
+		dev_err(&client->dev, "Controller reset failed.\n");
+		return error;
 	}
 
 	error = goodix_i2c_test(client);

[PATCH] Input: goodix - decouple irq and reset lines

[Alex Gonzalez]

The Goodix touch controller allows the use of two optional GPIOs (RESET
and INT) to reset the touch controller, select the I2C address of the
device and exit the device from sleep mode.

The current implementation requires both GPIOs to be provided, however,
it is possible to provide only the INT line and not to have the RESET line
available but pulled-up.

Designs that only provide the INT line are able to operate the touch on
the default I2C address but will not be able to reset the touch via
software or place the device in sleep mode.

Signed-off-by: Alex Gonzalez <alex.gonzalez@digi.com>
---
 drivers/input/touchscreen/goodix.c | 59 ++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f2d9c2c41885..8d96b0b771b6 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -484,34 +484,39 @@ static int goodix_reset(struct goodix_ts_data *ts)
 {
 	int error;
 
-	/* begin select I2C slave addr */
-	error = gpiod_direction_output(ts->gpiod_rst, 0);
-	if (error)
-		return error;
+	if (ts->gpiod_rst) {
+		/* begin select I2C slave addr */
+		error = gpiod_direction_output(ts->gpiod_rst, 0);
+		if (error)
+			return error;
 
-	msleep(20);				/* T2: > 10ms */
+		msleep(20);				/* T2: > 10ms */
 
-	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
-	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
-	if (error)
-		return error;
+		/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
+		error = gpiod_direction_output(ts->gpiod_int,
+				ts->client->addr == 0x14);
+		if (error)
+			return error;
 
-	usleep_range(100, 2000);		/* T3: > 100us */
+		usleep_range(100, 2000);		/* T3: > 100us */
 
-	error = gpiod_direction_output(ts->gpiod_rst, 1);
-	if (error)
-		return error;
+		error = gpiod_direction_output(ts->gpiod_rst, 1);
+		if (error)
+			return error;
 
-	usleep_range(6000, 10000);		/* T4: > 5ms */
+		usleep_range(6000, 10000);		/* T4: > 5ms */
 
-	/* end select I2C slave addr */
-	error = gpiod_direction_input(ts->gpiod_rst);
-	if (error)
-		return error;
+		/* end select I2C slave addr */
+		error = gpiod_direction_input(ts->gpiod_rst);
+		if (error)
+			return error;
+	}
 
-	error = goodix_int_sync(ts);
-	if (error)
-		return error;
+	if (ts->gpiod_int) {
+		error = goodix_int_sync(ts);
+		if (error)
+			return error;
+	}
 
 	return 0;
 }
@@ -786,13 +791,11 @@ static int goodix_ts_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
-	if (ts->gpiod_int && ts->gpiod_rst) {
-		/* reset the controller */
-		error = goodix_reset(ts);
-		if (error) {
-			dev_err(&client->dev, "Controller reset failed.\n");
-			return error;
-		}
+	/* reset the controller */
+	error = goodix_reset(ts);
+	if (error) {
+		dev_err(&client->dev, "Controller reset failed.\n");
+		return error;
 	}
 
 	error = goodix_i2c_test(client);

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Andreas Gohr]

> Designs that only provide the INT line are able to operate the touch on
> the default I2C address but will not be able to reset the touch via
> software or place the device in sleep mode.

This sounds exactly like the problem I'm having with the One-Mix Yoga
2 I described a few days ago.

I applied your patch on top of https://github.com/hadess/gt9xx, built
the goodix_backport module and loaded it successfully. The touchscreen
continued to work fine. Unfortunately, after a suspend and wakeup the
results are the same as without the patch - the touchscreen is
unresponsive. Unloading and loading the module again does not help. No
error messages whatsoever in dmesg.

Andreas

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Dmitry Torokhov]

Hi Alex,

On Fri, Jan 04, 2019 at 05:00:48PM +0100, Alex Gonzalez wrote:
> The Goodix touch controller allows the use of two optional GPIOs (RESET
> and INT) to reset the touch controller, select the I2C address of the
> device and exit the device from sleep mode.
> 
> The current implementation requires both GPIOs to be provided, however,
> it is possible to provide only the INT line and not to have the RESET line
> available but pulled-up.
> 
> Designs that only provide the INT line are able to operate the touch on
> the default I2C address but will not be able to reset the touch via
> software or place the device in sleep mode.

I do not have a datasheet for the device, so I am not sure if reset line
is actually needed to put the device into sleep mode. As far as I can
see from the code we suspend it by pulsing INT line and then sending a
command to the controller, and resuming by pulsing the INT line again.
So it sounds to me INT only designs _could_ place device in sleep mode.

As far as the patch goes, if you do not need to execute reset or put
device into low power mode, you do not need to specify any of the GPIOs
as GPIO resources. Simply specify the INT GPIO as your interrupt source
(GpioInt() in ACPI world or "interrupts = <&gpio NNN
IRQF_TRIGGER_WHATEVER>" in DT world and be done with it.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Gonzalez, Alex]

Hi Dmitry,

Thanks for your quick reply.

>
>I do not have a datasheet for the device, so I am not sure if reset line
>is actually needed to put the device into sleep mode. As far as I can
>see from the code we suspend it by pulsing INT line and then sending a
>command to the controller, and resuming by pulsing the INT line again.
>So it sounds to me INT only designs _could_ place device in sleep mode.
>

The way I read the gt911 dataheet I also understand that only the INT line is
required to enter sleep mode but I don't other for the other supported 
controllers.  My comment is based on both the suspend() and resume() functions 
returning in the absence of either gpiod_int or gpiod_rst and not progressing 
to the sleep sequence.

>As far as the patch goes, if you do not need to execute reset or put
>device into low power mode, you do not need to specify any of the GPIOs
>as GPIO resources. Simply specify the INT GPIO as your interrupt source
>(GpioInt() in ACPI world or "interrupts = <&gpio NNN
>IRQF_TRIGGER_WHATEVER>" in DT world and be done with it.
>

That was my first impression too. However this does not work for my device. My
hypothesis is that the touch controller I2C address setting sequence is not 
happening as it should, so I need at least the control of the INT line in 
order to fix the I2C address.

I am unsure though whether this is a problem specific to the design I am 
testing with or all designs will have problems setting the I2C address without 
controlling the INT GPIO line.

Regards,
Alex


>Thanks.
>
>--
>Dmitry

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Bastien Nocera]

On Sat, 2019-01-05 at 22:51 +0000, Dmitry Torokhov wrote:
> Hi Alex,
> 
> On Fri, Jan 04, 2019 at 05:00:48PM +0100, Alex Gonzalez wrote:
> > The Goodix touch controller allows the use of two optional GPIOs
> > (RESET
> > and INT) to reset the touch controller, select the I2C address of
> > the
> > device and exit the device from sleep mode.
> > 
> > The current implementation requires both GPIOs to be provided,
> > however,
> > it is possible to provide only the INT line and not to have the
> > RESET line
> > available but pulled-up.
> > 
> > Designs that only provide the INT line are able to operate the
> > touch on
> > the default I2C address but will not be able to reset the touch via
> > software or place the device in sleep mode.
> 
> I do not have a datasheet for the device, so I am not sure if reset
> line

Data sheets for a lot of the Goodix devices were shared a couple of
years ago on this list. You'll find the one for the GT911 in here:
https://drive.google.com/drive/folders/0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg?usp=sharing

You can probably translate it using Google Translate's Documents
upload, but I haven't had much luck at all...

> is actually needed to put the device into sleep mode. As far as I can
> see from the code we suspend it by pulsing INT line and then sending
> a
> command to the controller, and resuming by pulsing the INT line
> again.
> So it sounds to me INT only designs _could_ place device in sleep
> mode.
> 
> As far as the patch goes, if you do not need to execute reset or put
> device into low power mode, you do not need to specify any of the
> GPIOs
> as GPIO resources. Simply specify the INT GPIO as your interrupt
> source
> (GpioInt() in ACPI world or "interrupts = <&gpio NNN
> IRQF_TRIGGER_WHATEVER>" in DT world and be done with it.

Given that we do have access to the datasheet, it would also be useful
for the patch to mention where in the datasheet it says that the reset
line can be left pulled-up, or mention on which shipping device this
setup is already used (and if so, what the DTS or ACPI snippet that
declares those is).

Cheers

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Bastien Nocera]

On Mon, 2019-01-07 at 16:56 +0100, Bastien Nocera wrote:
> Given that we do have access to the datasheet, it would also be
> useful
> for the patch to mention where in the datasheet it says that the
> reset
> line can be left pulled-up, or mention on which shipping device this
> setup is already used (and if so, what the DTS or ACPI snippet that
> declares those is).

Or alternatively, and as Andreas pointed out in another thread, find
the code in the vendor driver that does support this setup:
https://github.com/goodix/gt9xx_driver_android
https://github.com/goodix/goodix_gt9xx_public

If it doesn't work with the vendor code, then we might not want to make
it work with our driver either.

Cheers

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Gonzalez, Alex]

Hi Bastien,

>Given that we do have access to the datasheet, it would also be useful
>for the patch to mention where in the datasheet it says that the reset
>line can be left pulled-up,

The pin description table on section 4, on the "Reset pin" row, contains a 
remark as follows:

External 10K pull-up resistor required, active-low reset

This comes from a newer revision of the datasheet though:
http://focuslcds.com/content/GT911.pdf

I guess it's open to interpretation whether driving the reset line is 
optional. The code seemed to imply it by using devm_gpiod_get_optional() to 
obtain the GPIO.

>or mention on which shipping device this
>setup is already used (and if so, what the DTS or ACPI snippet that
>declares those is).
>

I am testing with an LCD application kit for the ConnectCore 6UL SBC Pro:

https://www.digi.com/products/models/cc-acc-lcdw-10
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/imx6ul-ccimx6ulsbcpro.dts?h=v5.0-rc1#n120

This display in particular does not have the reset line available on the 
connector. The only way to make it work seems to be to use the INT line to fix 
an I2C address.

>Cheers
>

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Gonzalez, Alex]

>
>If it doesn't work with the vendor code, then we might not want to make
>it work with our driver either.
>

Thanks Bastien. The vendor code does seem to require both INT and RESET gpios.

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Dmitry Torokhov]

On Mon, Jan 07, 2019 at 04:42:26PM +0000, Gonzalez, Alex wrote:
> Hi Bastien,
> 
> >Given that we do have access to the datasheet, it would also be useful
> >for the patch to mention where in the datasheet it says that the reset
> >line can be left pulled-up,
> 
> The pin description table on section 4, on the "Reset pin" row, contains a 
> remark as follows:
> 
> External 10K pull-up resistor required, active-low reset
> 
> This comes from a newer revision of the datasheet though:
> http://focuslcds.com/content/GT911.pdf
> 
> I guess it's open to interpretation whether driving the reset line is 
> optional. The code seemed to imply it by using devm_gpiod_get_optional() to 
> obtain the GPIO.

They are optional in the sense that driver should work without them, but
if they specified we need both.

> 
> >or mention on which shipping device this
> >setup is already used (and if so, what the DTS or ACPI snippet that
> >declares those is).
> >
> 
> I am testing with an LCD application kit for the ConnectCore 6UL SBC Pro:
> 
> https://www.digi.com/products/models/cc-acc-lcdw-10
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/imx6ul-ccimx6ulsbcpro.dts?h=v5.0-rc1#n120
> 
> This display in particular does not have the reset line available on the 
> connector. The only way to make it work seems to be to use the INT line to fix 
> an I2C address.

Do you have to use 0x14 address? Can you used the default 0x5d?

My concern with trying to do the address selection without RST line is
that it is quite unreliable, as it really depends on timings between the
chip reset, INT line being driven by the host and then being switched to
input.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: goodix - decouple irq and reset lines

[Gonzalez, Alex]

>
>My concern with trying to do the address selection without RST line is
>that it is quite unreliable, as it really depends on timings between the
>chip reset, INT line being driven by the host and then being switched to
>input.

I think you are right. thinking twice I should be able to fake the reset line
by controlling a regulator to the voltage source for the pull-up. It seems a 
more solid solution.

Thanks a lot for your help and sorry for the noise.

Alex