Re: [PATCH v3] Input: ads7846 - don't use scratch for tx_buf when clearing register

Kris Bahnsen
2026-05-05T16:21:54Z

Dmitry,

On 5/4/26 8:01 PM, Dmitry Torokhov wrote:
> Hi Kris,
>
> On Thu, Apr 30, 2026 at 05:37:38PM +0000, Kris Bahnsen wrote:
>> The workaround for XPT2046 clears the command register, giving the
>> touchscreen controller a NOP. The change incorrectly re-uses the
>> req->scratch variable which is used as rx_buf for xfer[5], so by
>> the time xfer[6] occurs, the contents of req->scratch may not be
>> 0. It was found that the touchscreen controller can end up in
>> a completely unresponsive state due to it being given a command
>> the driver does not expect.
>>
>> Instead, rely on the spi_transfer behavior of tx_buf being NULL to
>> transmit all 0 bits and use the scratch variable for the rx_buf for
>> both the 1 byte command to and 2 byte response from the controller.
>>
>> This change was tested on real TSC2046 and ADS7843 controllers,
>> but not the XPT2046 the workaround was originally created for.
>> Confirming that the original modification to clear the command
>> register does not impact either real controller.
>>
>> Fixes: 781a07da9bb94 ("Input: ads7846 - add dummy command register clearing cycle")
>> Cc: stable@vger.kernel.org
>> Co-developed-by: Mark Featherston <mark@embeddedTS.com>
>> Signed-off-by: Mark Featherston <mark@embeddedTS.com>
>> Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
>> ---
>>
>> V1 -> V2: Don't use rx_buf when clearing command reg
>> V2 -> V3: Modify original 2 xfer command to eliminate dev_err()
>> output on xfer with len and NULL buffers
>>
>> drivers/input/touchscreen/ads7846.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
>> index 4b39f7212d35c..488bcc8393293 100644
>> --- a/drivers/input/touchscreen/ads7846.c
>> +++ b/drivers/input/touchscreen/ads7846.c
>> @@ -403,8 +403,7 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
>> 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].rx_buf = &req->scratch;
>
> Sashiko (I believe correctly) pointed out that by doing this "scratch"
> is now write only and this may cause DMA from the device stomp on
> message status and other unrelated data that shares the same cacheline
> with scracth. While it was already a problem before now it is even more
> likely.
>
> Since scratch is now write-only I believe moving it below "sample"
> forces it into separate cacheline and fixes this problem. Could you
> please try making this change?

Apologies, I'm not quite certain I understand what you mean by
"moving it below sample." Do you mean relocating the xfer[6] block
immediately below the xfer[3] block like so? If yes, I can get this
tested and a v4 patch together. If not, can you please clarify?

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 4b39f7212d35..6d57865ff505 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -390,6 +390,11 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
req->xfer[3].len = 2;
spi_message_add_tail(&req->xfer[3], &req->msg);

+ /* clear the command register */
+ req->xfer[6].rx_buf = &req->scratch;
+ req->xfer[6].len = 1;
+ spi_message_add_tail(&req->xfer[6], &req->msg);
+
/* REVISIT: take a few more samples, and compare ... */

/* converter in low power mode & enable PENIRQ */
@@ -402,12 +407,6 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
req->xfer[5].len = 2;
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]);

>
> Thanks.
>

--
Kris Bahnsen
Software Engineer
embeddedTS