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

[Kris Bahnsen]

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@xxxxxxxxxxxxxxx
>> Co-developed-by: Mark Featherston <mark@xxxxxxxxxxxxxx>
>> Signed-off-by: Mark Featherston <mark@xxxxxxxxxxxxxx>
>> Signed-off-by: Kris Bahnsen <kris@xxxxxxxxxxxxxx>
>> ---
>>
>> 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