Re: [PATCH v2 1/6] iio: adc: ti-ads1298: fix the data read path

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 24 Oct 2026 20:13:30 -0300
Stephan Costa Barros <stephanbrrs8@gmail.com> wrote:

> Fix five issues in how the driver reads samples:

This is a lot in one patch.  Can we break some of this
up into smaller changes?  The whole thing is probably
to complex for a stable tag, so focus on the most broken
aspects.


> 
> - cmd_buffer serves both the register accessors, under the regmap lock,
>   and the commands, under the IIO device lock, and shares a cacheline
>   with rx_buffer and tx_buffer. A register access can then rewrite a
>   command on the bus, or corrupt an RDATA transfer in flight on a
>   platform without coherent DMA. Give each path its own buffer, each on
>   its own cacheline.

This I'd like in a patch that does just this and allows the commit
message to say why simply using a single lock for the data buffers
would not be cleaner.

> 
> - After STOP, the conversion in progress still raises DRDY, so an RDATA
>   transfer can be in flight when the core frees the active scan mask.
>   Track in the driver whether samples go to the buffer, wait in
>   predisable for the last DRDY and its transfer, and push the sample
>   before marking the transfer done. update_scan_mode reset the transfer
>   counter, which let a new transfer be queued over one in flight; wait
>   for it instead.

Complicated sounding solution. Can we claim buffered mode instead?
That would ensure the update_scan_mode and this don't race and mostly
would be taking an uncontended lock.


> 
> - When a single-shot read times out, its transfer may still be in
>   flight, and the next read then wakes up with the old sample. On
>   unbind, nothing waits for the DRDY interrupt or its transfer. Wait for
>   the transfer before a single-shot read, and on teardown disable the
>   interrupt and wait for the last transfer.

If we get a timeout, we'd not normally worry too much on old data
on the next read. It's an error case that isn't supposed to happen so
if we are talking stale data that is often considered fine. The
second part may make sense - break that out as a separate patch
so we can easily analyse it on its own.


> 
> - The return value of spi_async() and the status of the RDATA message
>   are ignored: a failure leaves the transfer counter stuck, and a failed
>   transfer is reported as a sample. Handle both.

This is fine but not burred in this giant fix patch.

> 
> - read_raw takes the sample out of rx_buffer after releasing direct
>   mode, so a buffer enabled in between can overwrite it. Take it while
>   direct mode is still held.

This one seems valid.

> 
> The first, second and last were found by review and not reproduced.
> The third and fourth were found by review and reproduced on an emulated
> device only.
> 
> Fixes: 00ef7708fa60 ("iio: adc: ti-ads1298: Add driver")
> Signed-off-by: Stephan Costa Barros <stephanbrrs8@gmail.com>

With all the above in mind, I'm not going to spend the time
to unpick the different fixes today but look forward to them
as 5ish individual fix patches.

Thanks,

Jonathan