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

[Jonathan Cameron <jic23@kernel.org>]

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,