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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 7 May 2026 13:52:20 +0000
"Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

> > -----Original Message-----
> > From: Sabau, Radu bogdan <Radu.Sabau@analog.com>
> > Sent: Thursday, May 7, 2026 3:17 PM  
> 
> ...
> 
> > >  
> > 
> > Hi everyone,
> > 
> > Uhmm..., why was this fix marked as Changes Requested? Is it something I am
> > missing?
> > 
> > Best Regards,
> > Radu  
> 
> I have also had a look here :
> https://sashiko.dev/#/patchset/20260428-ad_sigma_delta-fix-v1-1-8e3f925ee8d2%40analog.com
> 
> Therefore here is the reply to Sashiko's review:
> 
> > For devices that lack the optional disable_one callback,
> > ad_sigma_delta_disable_one() is a no-op. In that scenario,
> > ad_sigma_delta_set_mode() executes the final SPI transfer for the
> > conversion.
> >
> > Because sigma_delta->keep_cs_asserted is still true when set_mode is
> > called, its transfer will carry cs_change = 1. Since disable_one
> > does not issue a subsequent transfer with cs_change = 0, will the SPI
> > framework skip teardown and leave CS permanently stuck low?
> >
> > Could sigma_delta->keep_cs_asserted be cleared before calling
> > ad_sigma_delta_set_mode() to ensure proper teardown, or should an
> > explicit dummy transfer be added?  
> 
> Good catch, thank you. The previous version only moved disable_one()
> after clearing keep_cs_asserted, but left set_mode(AD_SD_MODE_IDLE) in
> the out: block where keep_cs_asserted is still true. For devices that
> do not implement disable_one, set_mode() issues the last SPI transfer,
> so it carries cs_change=1 and CS stays stuck — the same bug reproduced
> on a different code path.
> 
> Fixed by moving set_mode(IDLE) into the out_unlock: block as well, so
> both calls execute after keep_cs_asserted is cleared. This matches the
> pattern already used correctly in ad_sd_calibrate(), which clears
> keep_cs_asserted before calling set_mode(IDLE) and disable_one() in its
> single cleanup block.
> 
> ...
> 
> > I noticed an additional state tracking issue in the error path of
> > ad_sd_buffer_postenable() that wasn't introduced by this patch.
> > When operations like ad_sigma_delta_clear_pending_event() fail, the error
> > path calls spi_bus_unlock() but leaves sigma_delta->bus_locked = true and
> > sigma_delta->keep_cs_asserted = true. Since the IIO core does not invoke
> > predisable on a postenable failure, this state persists.
> >
> > Subsequent SPI operations like direct mode reads will observe
> > bus_locked == true and incorrectly invoke spi_sync_locked(). As the bus
> > is not actually locked at the controller level, this bypasses the
> > SPI framework's bus_lock_mutex.
> >
> > Does this allow concurrent access to the SPI controller from multiple
> > threads, potentially corrupting SPI bus data?  
> 
> Yes, the concern is valid. With bus_locked == true but the controller's
> bus_lock_mutex not held, any concurrent spi_sync() from another driver
> can proceed unobstructed while our driver issues spi_sync_locked() —
> exactly the race bus_lock_mutex is meant to prevent.
> 
> Fixed in the same patch by resetting both keep_cs_asserted and
> bus_locked to false in err_unlock: before calling spi_bus_unlock(),
> consistent with the teardown sequence in predisable().
With that lot in mind I'll drop this version.

thanks,

Jonathan