| Message ID | 1546617648-23445-2-git-send-email-alex.gonzalez@digi.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Input: goodix - decouple irq and reset lines | expand |
> 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
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.
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
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
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
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 >
> >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.
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.
> >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
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);
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(-)