Re: [PATCH v2] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[David Lechner]

On 4/1/26 5:56 AM, Radu Sabau via B4 Relay wrote:
> From: Radu Sabau <radu.sabau@xxxxxxxxxx>
> 
> ad4695_enter_advanced_sequencer_mode() was called after
> spi_offload_trigger_enable(). That is wrong because
> ad4695_enter_advanced_sequencer_mode() issues regular SPI transfers to
> put the ADC into advanced sequencer mode, and spi_offload_trigger_enable()
> enables the PWM trigger that drives CNV. Once the PWM is running, CNV
> pulses start arriving and the offload engine owns the SPI bus; any
> concurrent regular SPI transfer produces undefined behaviour.

I think it would be more accurate to say that not all SPI offload
capable controllers may support regular SPI xfer when offloading is
enabled.

> 
> Fix this by calling ad4695_enter_advanced_sequencer_mode() before
> spi_offload_trigger_enable(), so the ADC is fully configured before the
> first CNV pulse can occur. This is consistent with the same constraint
> that already applies to the BUSY_GP_EN write above it.
> 
> Update the error unwind labels accordingly: add err_exit_conversion_mode
> so that a failure of spi_offload_trigger_enable() correctly exits
> conversion mode before clearing BUSY_GP_EN.
> 
> Fixes: f09f140e3ea8 ("iio: adc: ad4695: Add support for SPI offload")
> Reviewed-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
> ---
> ad4695_enter_advanced_sequencer_mode() issues regular SPI transfers to
> configure the ADC. These must complete before spi_offload_trigger_enable()
> enables the PWM/CNV trigger, because once CNV pulses are live the offload
> engine owns the SPI bus.
> 
> This fixes the call ordering and updates the error unwind path accordingly.
> ---
> Changes in v2:
> - Reword commit message to explain the correct bus-ownership
>   invariant directly, without reference to the HDL bug that
>   exposed it.
> - Remove unnecessary comment since the error path changed.
> - Link to v1: https://lore.kernel.org/r/20260330-ad4696-fix-v1-1-e841e96451b2@xxxxxxxxxx
> ---
>  drivers/iio/adc/ad4695.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index cda419638d9a..4471acd6fd5f 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -876,14 +876,14 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err_unoptimize_message;
>  
> -	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> -					 &config);
> +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>  	if (ret)
>  		goto err_disable_busy_output;
>  
> -	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);
>  	if (ret)
> -		goto err_offload_trigger_disable;
> +		goto err_exit_conversion_mode;
>  
>  	mutex_lock(&st->cnv_pwm_lock);
>  	pwm_get_state(st->cnv_pwm, &state);
> @@ -895,22 +895,15 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
>  	ret = pwm_apply_might_sleep(st->cnv_pwm, &state);
>  	mutex_unlock(&st->cnv_pwm_lock);
>  	if (ret)
> -		goto err_offload_exit_conversion_mode;
> +		goto err_trigger_disable;
>  
>  	return 0;
>  
> -err_offload_exit_conversion_mode:
> -	/*
> -	 * We have to unwind in a different order to avoid triggering offload.
> -	 * ad4695_exit_conversion_mode() triggers a conversion, so it has to be
> -	 * done after spi_offload_trigger_disable().
> -	 */
> +err_trigger_disable:

I would have left this as err_offload_trigger_disable to reduce the
diff and avoid having to explain why it was renamed. But not a big
deal.

>  	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> -	ad4695_exit_conversion_mode(st);
> -	goto err_disable_busy_output;
>  
> -err_offload_trigger_disable:
> -	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +err_exit_conversion_mode:
> +	ad4695_exit_conversion_mode(st);
>  
>  err_disable_busy_output:
>  	regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE,
> 
> ---
> base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
> change-id: 20260330-ad4696-fix-186955a8c511
> 
> Best regards,

Reviewed-by: David Lechner <dlechner@xxxxxxxxxxxx>