[PATCH] Input: ads7846 - add dummy command register clearing cycle

[PATCH] Input: ads7846 - add dummy command register clearing cycle

[Marek Vasut]

On STM32MP135F with XPT2046 touch controller attached to SPI bus, it has
been observed that the touch controller locks up after Linux kernel has
finished booting. Adding a dummy cycle on the SPI bus seems to mitigate
the lock up.

The XPTEK XPT2046 controller seems to be an identical clone of TI TSC2046,
the datasheet seems to be a clone of the TI part as well, text seem to be
word to word identical, except all the pictures have been drawn again.

This touch controller is present e.g. on WaveShare 3.2inch RPi LCD (B)
panel, the DTO provided by WaveShare uses 50 kHz SPI clock for this
touch controller, which is unusually low and possibly might have been
used as some sort of workaround for an issue. The SPI LCD on the same
bus uses 16 MHz clock.

SPI bus DT properties spi-cs-setup-delay-ns, spi-cs-hold-delay-ns,
spi-cs-inactive-delay-ns, spi-rx-delay-us, spi-tx-delay-us set to
range of 500ns..5us seem to have no impact on the behavior of the
touch controller, the lock up always occurs. The STM32MP13xx SPI
controller users GPIO control for the nCS pins.

Since the dummy cycle happens after the controller has been put into
power down mode and both ADC and REF regulators have been disabled,
the cycle should have no impact on the configuration of the controller,
i.e. it should be a NOP.

It is unclear whether this problem is specific to this cloned XPT2046
controller, or whether this is also present on TSC2046. A test on
either TSC2046 or ADS7846 would be very welcome.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: kernel@dh-electronics.com
Cc: linux-input@vger.kernel.org
---
 drivers/input/touchscreen/ads7846.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index d2bbb436a77df..90b5251797963 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -404,9 +404,19 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 
 	req->xfer[5].rx_buf = &req->scratch;
 	req->xfer[5].len = 2;
-	CS_CHANGE(req->xfer[5]);
 	spi_message_add_tail(&req->xfer[5], &req->msg);
 
+	/* clear the command register */
+	req->scratch = 0;
+	req->xfer[6].tx_buf = &req->scratch;
+	req->xfer[6].len = 1;
+	spi_message_add_tail(&req->xfer[6], &req->msg);
+
+	req->xfer[7].rx_buf = &req->scratch;
+	req->xfer[7].len = 2;
+	CS_CHANGE(req->xfer[7]);
+	spi_message_add_tail(&req->xfer[7], &req->msg);
+
 	mutex_lock(&ts->lock);
 	ads7846_stop(ts);
 	status = spi_sync(spi, &req->msg);
-- 
2.43.0

Re: [PATCH] Input: ads7846 - add dummy command register clearing cycle

[Marek Vasut]

On 3/20/24 8:23 AM, Marek Vasut wrote:
> On STM32MP135F with XPT2046 touch controller attached to SPI bus, it has
> been observed that the touch controller locks up after Linux kernel has
> finished booting. Adding a dummy cycle on the SPI bus seems to mitigate
> the lock up.
> 
> The XPTEK XPT2046 controller seems to be an identical clone of TI TSC2046,
> the datasheet seems to be a clone of the TI part as well, text seem to be
> word to word identical, except all the pictures have been drawn again.
> 
> This touch controller is present e.g. on WaveShare 3.2inch RPi LCD (B)
> panel, the DTO provided by WaveShare uses 50 kHz SPI clock for this
> touch controller, which is unusually low and possibly might have been
> used as some sort of workaround for an issue. The SPI LCD on the same
> bus uses 16 MHz clock.
> 
> SPI bus DT properties spi-cs-setup-delay-ns, spi-cs-hold-delay-ns,
> spi-cs-inactive-delay-ns, spi-rx-delay-us, spi-tx-delay-us set to
> range of 500ns..5us seem to have no impact on the behavior of the
> touch controller, the lock up always occurs. The STM32MP13xx SPI
> controller users GPIO control for the nCS pins.
> 
> Since the dummy cycle happens after the controller has been put into
> power down mode and both ADC and REF regulators have been disabled,
> the cycle should have no impact on the configuration of the controller,
> i.e. it should be a NOP.
> 
> It is unclear whether this problem is specific to this cloned XPT2046
> controller, or whether this is also present on TSC2046. A test on
> either TSC2046 or ADS7846 would be very welcome.

Hi,

Are there still any open topics with this patch ?

Re: [PATCH] Input: ads7846 - add dummy command register clearing cycle

[Dmitry Torokhov]

On Sun, Jun 23, 2024 at 08:21:00PM +0200, Marek Vasut wrote:
> On 3/20/24 8:23 AM, Marek Vasut wrote:
> > On STM32MP135F with XPT2046 touch controller attached to SPI bus, it has
> > been observed that the touch controller locks up after Linux kernel has
> > finished booting. Adding a dummy cycle on the SPI bus seems to mitigate
> > the lock up.
> > 
> > The XPTEK XPT2046 controller seems to be an identical clone of TI TSC2046,
> > the datasheet seems to be a clone of the TI part as well, text seem to be
> > word to word identical, except all the pictures have been drawn again.
> > 
> > This touch controller is present e.g. on WaveShare 3.2inch RPi LCD (B)
> > panel, the DTO provided by WaveShare uses 50 kHz SPI clock for this
> > touch controller, which is unusually low and possibly might have been
> > used as some sort of workaround for an issue. The SPI LCD on the same
> > bus uses 16 MHz clock.
> > 
> > SPI bus DT properties spi-cs-setup-delay-ns, spi-cs-hold-delay-ns,
> > spi-cs-inactive-delay-ns, spi-rx-delay-us, spi-tx-delay-us set to
> > range of 500ns..5us seem to have no impact on the behavior of the
> > touch controller, the lock up always occurs. The STM32MP13xx SPI
> > controller users GPIO control for the nCS pins.
> > 
> > Since the dummy cycle happens after the controller has been put into
> > power down mode and both ADC and REF regulators have been disabled,
> > the cycle should have no impact on the configuration of the controller,
> > i.e. it should be a NOP.
> > 
> > It is unclear whether this problem is specific to this cloned XPT2046
> > controller, or whether this is also present on TSC2046. A test on
> > either TSC2046 or ADS7846 would be very welcome.
> 
> Hi,
> 
> Are there still any open topics with this patch ?

I am concerned that we are putting workaroud for a single controller
into common function. Can we quirk it based on compatible? If not then I
would like someone to run tests on other controllers. Unfortunately I do
not have such hardware.

Linus, do you have devices with ads7846 or tsc2046 by chance? Spitz?

Thanks.

-- 
Dmitry

Re: [PATCH] Input: ads7846 - add dummy command register clearing cycle

[Linus Walleij]

On Tue, Jun 25, 2024 at 10:50 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> I am concerned that we are putting workaroud for a single controller
> into common function. Can we quirk it based on compatible? If not then I
> would like someone to run tests on other controllers. Unfortunately I do
> not have such hardware.
>
> Linus, do you have devices with ads7846 or tsc2046 by chance? Spitz?

Sadly I do not, but it is inside the Nokia770 so maybe Aaro can test it?
(Not sure if Aaro has the 770 though...)

Yours,
Linus Walleij

Re: [PATCH] Input: ads7846 - add dummy command register clearing cycle

[Aaro Koskinen]

Hi,

On Wed, Jun 26, 2024 at 02:40:00PM +0200, Linus Walleij wrote:
> On Tue, Jun 25, 2024 at 10:50 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > I am concerned that we are putting workaroud for a single controller
> > into common function. Can we quirk it based on compatible? If not then I
> > would like someone to run tests on other controllers. Unfortunately I do
> > not have such hardware.
> >
> > Linus, do you have devices with ads7846 or tsc2046 by chance? Spitz?
> 
> Sadly I do not, but it is inside the Nokia770 so maybe Aaro can test it?
> (Not sure if Aaro has the 770 though...)

I sure have 770, and I can test patches.

However, ads7846 has regressed and it has been broken on 770 for
a couple years already as half-duplex support got suddenly deleted
without any pre-warning to users (770 was using this driver maybe
a decade without any issues). I reported this as a regression but
input maintainers preferred to prioritize "nicer hardware". See e.g.:
https://marc.info/?l=linux-input&m=164783764828520&w=2

I'm planning to restore the half-duplex support for 770, but it's not
on my top prioriority list so it will take a while.

Meanwhile, maybe you can ask the users of such "nicer hardware" to test.

A.

Re: [PATCH] Input: ads7846 - add dummy command register clearing cycle

[Marek Vasut]

On 6/27/24 12:33 AM, Aaro Koskinen wrote:
> Hi,
> 
> On Wed, Jun 26, 2024 at 02:40:00PM +0200, Linus Walleij wrote:
>> On Tue, Jun 25, 2024 at 10:50 PM Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>
>>> I am concerned that we are putting workaroud for a single controller
>>> into common function. Can we quirk it based on compatible? If not then I
>>> would like someone to run tests on other controllers. Unfortunately I do
>>> not have such hardware.
>>>
>>> Linus, do you have devices with ads7846 or tsc2046 by chance? Spitz?
>>
>> Sadly I do not, but it is inside the Nokia770 so maybe Aaro can test it?
>> (Not sure if Aaro has the 770 though...)
> 
> I sure have 770, and I can test patches.
> 
> However, ads7846 has regressed and it has been broken on 770 for
> a couple years already as half-duplex support got suddenly deleted
> without any pre-warning to users (770 was using this driver maybe
> a decade without any issues). I reported this as a regression but
> input maintainers preferred to prioritize "nicer hardware". See e.g.:
> https://marc.info/?l=linux-input&m=164783764828520&w=2
> 
> I'm planning to restore the half-duplex support for 770, but it's not
> on my top prioriority list so it will take a while.
> 
> Meanwhile, maybe you can ask the users of such "nicer hardware" to test.
Seems there is basically no interest anymore . Can this patch be applied 
or how shall we proceed ?

Re: [PATCH] Input: ads7846 - add dummy command register clearing cycle

[Dmitry Torokhov]

On Thu, Oct 31, 2024 at 07:21:52PM +0100, Marek Vasut wrote:
> On 6/27/24 12:33 AM, Aaro Koskinen wrote:
> > Hi,
> > 
> > On Wed, Jun 26, 2024 at 02:40:00PM +0200, Linus Walleij wrote:
> > > On Tue, Jun 25, 2024 at 10:50 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > 
> > > > I am concerned that we are putting workaroud for a single controller
> > > > into common function. Can we quirk it based on compatible? If not then I
> > > > would like someone to run tests on other controllers. Unfortunately I do
> > > > not have such hardware.
> > > > 
> > > > Linus, do you have devices with ads7846 or tsc2046 by chance? Spitz?
> > > 
> > > Sadly I do not, but it is inside the Nokia770 so maybe Aaro can test it?
> > > (Not sure if Aaro has the 770 though...)
> > 
> > I sure have 770, and I can test patches.
> > 
> > However, ads7846 has regressed and it has been broken on 770 for
> > a couple years already as half-duplex support got suddenly deleted
> > without any pre-warning to users (770 was using this driver maybe
> > a decade without any issues). I reported this as a regression but
> > input maintainers preferred to prioritize "nicer hardware". See e.g.:
> > https://marc.info/?l=linux-input&m=164783764828520&w=2
> > 
> > I'm planning to restore the half-duplex support for 770, but it's not
> > on my top prioriority list so it will take a while.
> > 
> > Meanwhile, maybe you can ask the users of such "nicer hardware" to test.
> Seems there is basically no interest anymore . Can this patch be applied or
> how shall we proceed ?

Applied, thank you.

-- 
Dmitry

Re: [PATCH] Input: ads7846 - add dummy command register clearing cycle

[Marek Vasut]

On 6/25/24 10:50 PM, Dmitry Torokhov wrote:
> On Sun, Jun 23, 2024 at 08:21:00PM +0200, Marek Vasut wrote:
>> On 3/20/24 8:23 AM, Marek Vasut wrote:
>>> On STM32MP135F with XPT2046 touch controller attached to SPI bus, it has
>>> been observed that the touch controller locks up after Linux kernel has
>>> finished booting. Adding a dummy cycle on the SPI bus seems to mitigate
>>> the lock up.
>>>
>>> The XPTEK XPT2046 controller seems to be an identical clone of TI TSC2046,
>>> the datasheet seems to be a clone of the TI part as well, text seem to be
>>> word to word identical, except all the pictures have been drawn again.
>>>
>>> This touch controller is present e.g. on WaveShare 3.2inch RPi LCD (B)
>>> panel, the DTO provided by WaveShare uses 50 kHz SPI clock for this
>>> touch controller, which is unusually low and possibly might have been
>>> used as some sort of workaround for an issue. The SPI LCD on the same
>>> bus uses 16 MHz clock.
>>>
>>> SPI bus DT properties spi-cs-setup-delay-ns, spi-cs-hold-delay-ns,
>>> spi-cs-inactive-delay-ns, spi-rx-delay-us, spi-tx-delay-us set to
>>> range of 500ns..5us seem to have no impact on the behavior of the
>>> touch controller, the lock up always occurs. The STM32MP13xx SPI
>>> controller users GPIO control for the nCS pins.
>>>
>>> Since the dummy cycle happens after the controller has been put into
>>> power down mode and both ADC and REF regulators have been disabled,
>>> the cycle should have no impact on the configuration of the controller,
>>> i.e. it should be a NOP.
>>>
>>> It is unclear whether this problem is specific to this cloned XPT2046
>>> controller, or whether this is also present on TSC2046. A test on
>>> either TSC2046 or ADS7846 would be very welcome.
>>
>> Hi,
>>
>> Are there still any open topics with this patch ?
> 
> I am concerned that we are putting workaroud for a single controller
> into common function. Can we quirk it based on compatible?

We can, but there is a slight problem. I came across DTs which describe 
this XPT2046 using TSC2046 or ADS7846 compatible string in those DTs, so 
if those get used, this driver won't work correctly. On the other hand, 
those are random downstream DTs, they are not upstream, so maybe those 
are irrelevant.

> If not then I
> would like someone to run tests on other controllers. Unfortunately I do
> not have such hardware.

I did dig through my pile and I don't have one such controller either.

> Linus, do you have devices with ads7846 or tsc2046 by chance? Spitz?

I would much rather see this tested on at least one of the old 
controllers than add a quirk via DT compatible, because I believe this 
won't have adverse effects on those controllers, and it would help cover 
the odd DTs which consider this to be a drop-in compatible replacement 
for TSC2046 transparently (even if it really is not quite compatible).

Re: [PATCH] Input: ads7846 - add dummy command register clearing cycle

[Kees Bakker]

Op 20-03-2024 om 08:23 schreef Marek Vasut:
> On STM32MP135F with XPT2046 touch controller attached to SPI bus, it has
> been observed that the touch controller locks up after Linux kernel has
> finished booting. Adding a dummy cycle on the SPI bus seems to mitigate
> the lock up.
>
> The XPTEK XPT2046 controller seems to be an identical clone of TI TSC2046,
> the datasheet seems to be a clone of the TI part as well, text seem to be
> word to word identical, except all the pictures have been drawn again.
>
> This touch controller is present e.g. on WaveShare 3.2inch RPi LCD (B)
> panel, the DTO provided by WaveShare uses 50 kHz SPI clock for this
> touch controller, which is unusually low and possibly might have been
> used as some sort of workaround for an issue. The SPI LCD on the same
> bus uses 16 MHz clock.
>
> SPI bus DT properties spi-cs-setup-delay-ns, spi-cs-hold-delay-ns,
> spi-cs-inactive-delay-ns, spi-rx-delay-us, spi-tx-delay-us set to
> range of 500ns..5us seem to have no impact on the behavior of the
> touch controller, the lock up always occurs. The STM32MP13xx SPI
> controller users GPIO control for the nCS pins.
>
> Since the dummy cycle happens after the controller has been put into
> power down mode and both ADC and REF regulators have been disabled,
> the cycle should have no impact on the configuration of the controller,
> i.e. it should be a NOP.
>
> It is unclear whether this problem is specific to this cloned XPT2046
> controller, or whether this is also present on TSC2046. A test on
> either TSC2046 or ADS7846 would be very welcome.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: kernel@dh-electronics.com
> Cc: linux-input@vger.kernel.org
> ---
>   drivers/input/touchscreen/ads7846.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index d2bbb436a77df..90b5251797963 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -404,9 +404,19 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
>   
>   	req->xfer[5].rx_buf = &req->scratch;
>   	req->xfer[5].len = 2;
> -	CS_CHANGE(req->xfer[5]);
>   	spi_message_add_tail(&req->xfer[5], &req->msg);
>   
> +	/* clear the command register */
> +	req->scratch = 0;
> +	req->xfer[6].tx_buf = &req->scratch;
> +	req->xfer[6].len = 1;
> +	spi_message_add_tail(&req->xfer[6], &req->msg);
> +
> +	req->xfer[7].rx_buf = &req->scratch;
> +	req->xfer[7].len = 2;
> +	CS_CHANGE(req->xfer[7]);
> +	spi_message_add_tail(&req->xfer[7], &req->msg);
> +
>   	mutex_lock(&ts->lock);
>   	ads7846_stop(ts);
>   	status = spi_sync(spi, &req->msg);
Sorry for sending a comment this late.

You need to increase the size of req->xfer, because it is an
array of size 6.
     struct spi_transfer     xfer[6];

You're writing beyond the array boundary.
-- 
Kees Bakker

Re: [PATCH] Input: ads7846 - add dummy command register clearing cycle

[Marek Vasut]

On 11/12/24 8:40 PM, Kees Bakker wrote:
> Op 20-03-2024 om 08:23 schreef Marek Vasut:
>> On STM32MP135F with XPT2046 touch controller attached to SPI bus, it has
>> been observed that the touch controller locks up after Linux kernel has
>> finished booting. Adding a dummy cycle on the SPI bus seems to mitigate
>> the lock up.
>>
>> The XPTEK XPT2046 controller seems to be an identical clone of TI 
>> TSC2046,
>> the datasheet seems to be a clone of the TI part as well, text seem to be
>> word to word identical, except all the pictures have been drawn again.
>>
>> This touch controller is present e.g. on WaveShare 3.2inch RPi LCD (B)
>> panel, the DTO provided by WaveShare uses 50 kHz SPI clock for this
>> touch controller, which is unusually low and possibly might have been
>> used as some sort of workaround for an issue. The SPI LCD on the same
>> bus uses 16 MHz clock.
>>
>> SPI bus DT properties spi-cs-setup-delay-ns, spi-cs-hold-delay-ns,
>> spi-cs-inactive-delay-ns, spi-rx-delay-us, spi-tx-delay-us set to
>> range of 500ns..5us seem to have no impact on the behavior of the
>> touch controller, the lock up always occurs. The STM32MP13xx SPI
>> controller users GPIO control for the nCS pins.
>>
>> Since the dummy cycle happens after the controller has been put into
>> power down mode and both ADC and REF regulators have been disabled,
>> the cycle should have no impact on the configuration of the controller,
>> i.e. it should be a NOP.
>>
>> It is unclear whether this problem is specific to this cloned XPT2046
>> controller, or whether this is also present on TSC2046. A test on
>> either TSC2046 or ADS7846 would be very welcome.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: kernel@dh-electronics.com
>> Cc: linux-input@vger.kernel.org
>> ---
>>   drivers/input/touchscreen/ads7846.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/ 
>> touchscreen/ads7846.c
>> index d2bbb436a77df..90b5251797963 100644
>> --- a/drivers/input/touchscreen/ads7846.c
>> +++ b/drivers/input/touchscreen/ads7846.c
>> @@ -404,9 +404,19 @@ static int ads7846_read12_ser(struct device *dev, 
>> unsigned command)
>>       req->xfer[5].rx_buf = &req->scratch;
>>       req->xfer[5].len = 2;
>> -    CS_CHANGE(req->xfer[5]);
>>       spi_message_add_tail(&req->xfer[5], &req->msg);
>> +    /* clear the command register */
>> +    req->scratch = 0;
>> +    req->xfer[6].tx_buf = &req->scratch;
>> +    req->xfer[6].len = 1;
>> +    spi_message_add_tail(&req->xfer[6], &req->msg);
>> +
>> +    req->xfer[7].rx_buf = &req->scratch;
>> +    req->xfer[7].len = 2;
>> +    CS_CHANGE(req->xfer[7]);
>> +    spi_message_add_tail(&req->xfer[7], &req->msg);
>> +
>>       mutex_lock(&ts->lock);
>>       ads7846_stop(ts);
>>       status = spi_sync(spi, &req->msg);
> Sorry for sending a comment this late.
> 
> You need to increase the size of req->xfer, because it is an
> array of size 6.
>      struct spi_transfer     xfer[6];
> 
> You're writing beyond the array boundary.

A fix for that was already posted by Nathan:

[PATCH] Input: ads7846 - Increase xfer array size in 'struct ser_req'

Sorry for the inconvenience.