[PATCH v2] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[PATCH v2] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Radu Sabau via B4 Relay]

From: Radu Sabau <radu.sabau@analog.com>

Commit 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous
ready signals") introduced a new out_unlock: label for an early-exit
path and moved sigma_delta->keep_cs_asserted = false there.
Unfortunately it left both ad_sigma_delta_set_mode(AD_SD_MODE_IDLE)
and ad_sigma_delta_disable_one() in the out: block above that label,
so both SPI writes are issued while keep_cs_asserted is still true.
keep_cs_asserted feeds directly into the cs_change field of the
spi_transfer, so the final write to the device carries cs_change=1.
The SPI framework only calls spi_set_cs() to deassert CS as part of
message teardown when cs_change is not set on the last transfer; with
cs_change=1, that teardown is skipped and CS stays physically asserted.
No further transfer to the device is made, so the driver never
deasserts CS.  The framework provides no automatic CS deassert on bus
unlock or when switching to another device, so CS remains stuck low
until the next SPI operation to that same device.

On a shared SPI bus this is problematic: with CS stuck low the ADC
remains selected and subsequent SPI traffic intended for another device
is interpreted as commands by the ADC.  Devices like the AD7124 that
multiplex /RDY onto the MISO line will start spurious conversions and
pull MISO low, corrupting reads from the other device and causing a
deadlock.

Fix both calls by moving them into the out_unlock: block, after
keep_cs_asserted is cleared, mirroring the existing correct pattern in
ad_sd_calibrate().  With keep_cs_asserted false, both set_mode(IDLE)
and disable_one() execute with cs_change=0, so the SPI framework
deasserts CS as part of normal message teardown.  For devices that do
not implement the optional disable_one callback, set_mode(IDLE) issues
the last SPI transfer and CS is properly released in all cases.

Also fix a pre-existing state leak in ad_sd_buffer_postenable(): the
err_unlock: error path called spi_bus_unlock() without first resetting
bus_locked and keep_cs_asserted to false.  Subsequent SPI calls would
observe bus_locked == true and invoke spi_sync_locked() on a controller
that is no longer locked, potentially allowing concurrent bus access.

Fixes: 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous ready signals")
Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Signed-off-by: Radu Sabau <radu.sabau@analog.com>
---
Changes in v2:
- Move set_mode(AD_SD_MODE_IDLE) into out_unlock: as well, not only
  disable_one(); v1 left set_mode() above the label where
  keep_cs_asserted is still true, so devices without the optional
  disable_one callback still had CS stuck after that transfer.
- Fix pre-existing state leak in ad_sd_buffer_postenable() err_unlock:
  reset bus_locked and keep_cs_asserted before spi_bus_unlock() to
  prevent spi_sync_locked() being called on an unlocked controller.
- Link to v1: https://lore.kernel.org/r/20260428-ad_sigma_delta-fix-v1-1-8e3f925ee8d2@analog.com
---
 drivers/iio/adc/ad_sigma_delta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index a955556f9ec8..a33a7e8c264f 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -441,11 +441,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 out:
 	ad_sd_disable_irq(sigma_delta);
 
-	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
-	ad_sigma_delta_disable_one(sigma_delta, chan->address);
-
 out_unlock:
 	sigma_delta->keep_cs_asserted = false;
+	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
+	ad_sigma_delta_disable_one(sigma_delta, chan->address);
 	sigma_delta->bus_locked = false;
 	spi_bus_unlock(sigma_delta->spi->controller);
 out_release:
@@ -578,6 +577,8 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
 	return 0;
 
 err_unlock:
+	sigma_delta->keep_cs_asserted = false;
+	sigma_delta->bus_locked = false;
 	spi_bus_unlock(sigma_delta->spi->controller);
 	spi_unoptimize_message(&sigma_delta->sample_msg);
 

---
base-commit: 3b3bea6d4b9c162f9e555905d96b8c1da67ecd5b
change-id: 20260428-ad_sigma_delta-fix-bb65d56ccbb0

Best regards,
-- 
Radu Sabau <radu.sabau@analog.com>

Re: [PATCH v2] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Jonathan Cameron]

On Thu, 07 May 2026 17:00:38 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Commit 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous
> ready signals") introduced a new out_unlock: label for an early-exit
> path and moved sigma_delta->keep_cs_asserted = false there.
> Unfortunately it left both ad_sigma_delta_set_mode(AD_SD_MODE_IDLE)
> and ad_sigma_delta_disable_one() in the out: block above that label,
> so both SPI writes are issued while keep_cs_asserted is still true.
> keep_cs_asserted feeds directly into the cs_change field of the
> spi_transfer, so the final write to the device carries cs_change=1.
> The SPI framework only calls spi_set_cs() to deassert CS as part of
> message teardown when cs_change is not set on the last transfer; with
> cs_change=1, that teardown is skipped and CS stays physically asserted.
> No further transfer to the device is made, so the driver never
> deasserts CS.  The framework provides no automatic CS deassert on bus
> unlock or when switching to another device, so CS remains stuck low
> until the next SPI operation to that same device.
> 
> On a shared SPI bus this is problematic: with CS stuck low the ADC
> remains selected and subsequent SPI traffic intended for another device
> is interpreted as commands by the ADC.  Devices like the AD7124 that
> multiplex /RDY onto the MISO line will start spurious conversions and
> pull MISO low, corrupting reads from the other device and causing a
> deadlock.
> 
> Fix both calls by moving them into the out_unlock: block, after
> keep_cs_asserted is cleared, mirroring the existing correct pattern in
> ad_sd_calibrate().  With keep_cs_asserted false, both set_mode(IDLE)
> and disable_one() execute with cs_change=0, so the SPI framework
> deasserts CS as part of normal message teardown.  For devices that do
> not implement the optional disable_one callback, set_mode(IDLE) issues
> the last SPI transfer and CS is properly released in all cases.
> 
> Also fix a pre-existing state leak in ad_sd_buffer_postenable(): the
> err_unlock: error path called spi_bus_unlock() without first resetting
> bus_locked and keep_cs_asserted to false.  Subsequent SPI calls would
> observe bus_locked == true and invoke spi_sync_locked() on a controller
> that is no longer locked, potentially allowing concurrent bus access.
> 
> Fixes: 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous ready signals")
> Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
Applied to the fixes-togreg branch of iio.git

Depending on when I do a pull request (maybe later today) I might
not include this one as it won't have had build bot exposure by then.
It'll go in the next pull if that happens.

Thanks

Jonathan

> ---
> Changes in v2:
> - Move set_mode(AD_SD_MODE_IDLE) into out_unlock: as well, not only
>   disable_one(); v1 left set_mode() above the label where
>   keep_cs_asserted is still true, so devices without the optional
>   disable_one callback still had CS stuck after that transfer.
> - Fix pre-existing state leak in ad_sd_buffer_postenable() err_unlock:
>   reset bus_locked and keep_cs_asserted before spi_bus_unlock() to
>   prevent spi_sync_locked() being called on an unlocked controller.
> - Link to v1: https://lore.kernel.org/r/20260428-ad_sigma_delta-fix-v1-1-8e3f925ee8d2@analog.com
> ---
>  drivers/iio/adc/ad_sigma_delta.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index a955556f9ec8..a33a7e8c264f 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -441,11 +441,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  out:
>  	ad_sd_disable_irq(sigma_delta);
>  
> -	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> -	ad_sigma_delta_disable_one(sigma_delta, chan->address);
> -
>  out_unlock:
>  	sigma_delta->keep_cs_asserted = false;
> +	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> +	ad_sigma_delta_disable_one(sigma_delta, chan->address);
>  	sigma_delta->bus_locked = false;
>  	spi_bus_unlock(sigma_delta->spi->controller);
>  out_release:
> @@ -578,6 +577,8 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
>  	return 0;
>  
>  err_unlock:
> +	sigma_delta->keep_cs_asserted = false;
> +	sigma_delta->bus_locked = false;
>  	spi_bus_unlock(sigma_delta->spi->controller);
>  	spi_unoptimize_message(&sigma_delta->sample_msg);
>  
> 
> ---
> base-commit: 3b3bea6d4b9c162f9e555905d96b8c1da67ecd5b
> change-id: 20260428-ad_sigma_delta-fix-bb65d56ccbb0
> 
> Best regards,

Re: [PATCH v2] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Jonathan Cameron]

On Thu, 07 May 2026 17:00:38 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Commit 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous
> ready signals") introduced a new out_unlock: label for an early-exit
> path and moved sigma_delta->keep_cs_asserted = false there.
> Unfortunately it left both ad_sigma_delta_set_mode(AD_SD_MODE_IDLE)
> and ad_sigma_delta_disable_one() in the out: block above that label,
> so both SPI writes are issued while keep_cs_asserted is still true.
> keep_cs_asserted feeds directly into the cs_change field of the
> spi_transfer, so the final write to the device carries cs_change=1.
> The SPI framework only calls spi_set_cs() to deassert CS as part of
> message teardown when cs_change is not set on the last transfer; with
> cs_change=1, that teardown is skipped and CS stays physically asserted.
> No further transfer to the device is made, so the driver never
> deasserts CS.  The framework provides no automatic CS deassert on bus
> unlock or when switching to another device, so CS remains stuck low
> until the next SPI operation to that same device.
> 
> On a shared SPI bus this is problematic: with CS stuck low the ADC
> remains selected and subsequent SPI traffic intended for another device
> is interpreted as commands by the ADC.  Devices like the AD7124 that
> multiplex /RDY onto the MISO line will start spurious conversions and
> pull MISO low, corrupting reads from the other device and causing a
> deadlock.
> 
> Fix both calls by moving them into the out_unlock: block, after
> keep_cs_asserted is cleared, mirroring the existing correct pattern in
> ad_sd_calibrate().  With keep_cs_asserted false, both set_mode(IDLE)
> and disable_one() execute with cs_change=0, so the SPI framework
> deasserts CS as part of normal message teardown.  For devices that do
> not implement the optional disable_one callback, set_mode(IDLE) issues
> the last SPI transfer and CS is properly released in all cases.
> 
> Also fix a pre-existing state leak in ad_sd_buffer_postenable(): the
> err_unlock: error path called spi_bus_unlock() without first resetting
> bus_locked and keep_cs_asserted to false.  Subsequent SPI calls would
> observe bus_locked == true and invoke spi_sync_locked() on a controller
> that is no longer locked, potentially allowing concurrent bus access.
> 
> Fixes: 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous ready signals")
> Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>

Actually - dropped again. I forgot to check Sashiko.
(Note please do this yourself in future and reply to the patch if changes are
needed).  It raises some concerns about devices that don't provide
the callbacks and hence won't ever use that keep_cs_asserted = false callback.

I'm struggling a bit on how the max11205 works at all as there seems
to be a status register read on a device which claims to have no registers.
ad_sigma_delta_clear_pending_event() as the binding suggests this device
doesn't have a drdy_gpio

Anyhow, please take a look at the feedback and if it's wrong please provide
an explanation of why in this thread.

Thanks

Jonathan


> ---
> Changes in v2:
> - Move set_mode(AD_SD_MODE_IDLE) into out_unlock: as well, not only
>   disable_one(); v1 left set_mode() above the label where
>   keep_cs_asserted is still true, so devices without the optional
>   disable_one callback still had CS stuck after that transfer.
> - Fix pre-existing state leak in ad_sd_buffer_postenable() err_unlock:
>   reset bus_locked and keep_cs_asserted before spi_bus_unlock() to
>   prevent spi_sync_locked() being called on an unlocked controller.
> - Link to v1: https://lore.kernel.org/r/20260428-ad_sigma_delta-fix-v1-1-8e3f925ee8d2@analog.com
> ---
>  drivers/iio/adc/ad_sigma_delta.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index a955556f9ec8..a33a7e8c264f 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -441,11 +441,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  out:
>  	ad_sd_disable_irq(sigma_delta);
>  
> -	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> -	ad_sigma_delta_disable_one(sigma_delta, chan->address);
> -
>  out_unlock:
>  	sigma_delta->keep_cs_asserted = false;
> +	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> +	ad_sigma_delta_disable_one(sigma_delta, chan->address);
>  	sigma_delta->bus_locked = false;
>  	spi_bus_unlock(sigma_delta->spi->controller);
>  out_release:
> @@ -578,6 +577,8 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
>  	return 0;
>  
>  err_unlock:
> +	sigma_delta->keep_cs_asserted = false;
> +	sigma_delta->bus_locked = false;
>  	spi_bus_unlock(sigma_delta->spi->controller);
>  	spi_unoptimize_message(&sigma_delta->sample_msg);
>  
> 
> ---
> base-commit: 3b3bea6d4b9c162f9e555905d96b8c1da67ecd5b
> change-id: 20260428-ad_sigma_delta-fix-bb65d56ccbb0
> 
> Best regards,

RE: [PATCH v2] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Tuesday, May 12, 2026 2:13 PM

...

> > Fixes: 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous ready
> signals")
> > Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> 
> Actually - dropped again. I forgot to check Sashiko.
> (Note please do this yourself in future and reply to the patch if changes are
> needed).  It raises some concerns about devices that don't provide
> the callbacks and hence won't ever use that keep_cs_asserted = false callback.
> 
> I'm struggling a bit on how the max11205 works at all as there seems
> to be a status register read on a device which claims to have no registers.
> ad_sigma_delta_clear_pending_event() as the binding suggests this device
> doesn't have a drdy_gpio
> 
> Anyhow, please take a look at the feedback and if it's wrong please provide
> an explanation of why in this thread.
> 

Hi Jonathan,

First of all sorry for forgetting about Sashiko. I had a look at max11205 and it
seems like the device doesn't have a CS so therefore the concern Sashiko
raises regarding CS may not be valid, I will make sure to mention
that in the commit message.

On the other hand, I see the IC has a shared pin for DRDY and DOUT and
the bindings specify some interrupt though no specific rdy-gpios are
mentioned there. The device reads a register that doesn't exist which
in this case means it blindly clocks out whatever comes on MISO...

However I may have a fix for this and would appear in a second commit.
In clear_pending_event where rdy_gpiod is checked there could be yet
another else branch that could simply return 0 and here is why I think this would work:

Since the IRQ is requested with IRQF_NO_AUTOEN and IRQ_DISABLE_UNLAZY, the latter
keeps the IRQ hardware-unmasked even while software-disabled, so any falling edge
from a completed conversion is latched by the IRQ controller. When ad_sd_enable_irq()
is subsequently called, the latched edge fires immediately, and ad_sd_read_reg() reads
the complete stale result without any prior partial clock corruption, so the result is not
read by mistake as it could currently happen.

Let me know your thoughts on this,
Radu

Re: [PATCH v2] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[David Lechner]

On 5/14/26 5:07 AM, Sabau, Radu bogdan wrote:
>> -----Original Message-----
>> From: Jonathan Cameron <jic23@kernel.org>
>> Sent: Tuesday, May 12, 2026 2:13 PM
> 
> ...
> 
>>> Fixes: 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous ready
>> signals")
>>> Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
>>> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
>>
>> Actually - dropped again. I forgot to check Sashiko.
>> (Note please do this yourself in future and reply to the patch if changes are
>> needed).  It raises some concerns about devices that don't provide
>> the callbacks and hence won't ever use that keep_cs_asserted = false callback.
>>
>> I'm struggling a bit on how the max11205 works at all as there seems
>> to be a status register read on a device which claims to have no registers.
>> ad_sigma_delta_clear_pending_event() as the binding suggests this device
>> doesn't have a drdy_gpio
>>
>> Anyhow, please take a look at the feedback and if it's wrong please provide
>> an explanation of why in this thread.
>>
> 
> Hi Jonathan,
> 
> First of all sorry for forgetting about Sashiko. I had a look at max11205 and it
> seems like the device doesn't have a CS so therefore the concern Sashiko
> raises regarding CS may not be valid, I will make sure to mention
> that in the commit message.
> 
> On the other hand, I see the IC has a shared pin for DRDY and DOUT and
> the bindings specify some interrupt though no specific rdy-gpios are
> mentioned there. The device reads a register that doesn't exist which
> in this case means it blindly clocks out whatever comes on MISO...
> 
> However I may have a fix for this and would appear in a second commit.
> In clear_pending_event where rdy_gpiod is checked there could be yet
> another else branch that could simply return 0 and here is why I think this would work:
> 
> Since the IRQ is requested with IRQF_NO_AUTOEN and IRQ_DISABLE_UNLAZY, the latter
> keeps the IRQ hardware-unmasked even while software-disabled, so any falling edge

This should depend on the IRQ controller. There are some past discussions
on this on the mailing list. The ideal behavior should be that if the
IRQ controller can fully disable the interrupt so that we don't get the
spurious interrupt on enable (from the normal SPI data, not DRDY). If
the interrupt controller can't do this, then it requires the rdy-gpios
to be able to distinguish DRDY from SPI data.

I have used this on ad7173 on a ZedBoard without rdy-gpios and the
interrupt was working correctly. So unless something changed with
the interrupt config in this code since the last time I was using,
I would not expect to see this problem on _all_ interrupt controllers.

> from a completed conversion is latched by the IRQ controller. When ad_sd_enable_irq()
> is subsequently called, the latched edge fires immediately, and ad_sd_read_reg() reads
> the complete stale result without any prior partial clock corruption, so the result is not
> read by mistake as it could currently happen.
> 
> Let me know your thoughts on this,
> Radu
>