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

[David Lechner]

On 3/31/26 5:53 AM, Sabau, Radu bogdan wrote:
> 
> 
>> -----Original Message-----
>> From: David Lechner <dlechner@xxxxxxxxxxxx>
>> Sent: Monday, March 30, 2026 7:46 PM
>> To: Sabau, Radu bogdan <Radu.Sabau@xxxxxxxxxx>; Lars-Peter Clausen
> 
> Hi David, Andy,
> 
>>
>> Which commit is the HDL being built from? It could be that something
>> changed in the FPGA implementation rather than the driver.
>>
> 
> Good call. The bitstream was built from commit 79446d8ee in the ADI HDL
> tree — "library/spi_engine: fix spi engine for SDI backpressure",
> authored by Carlos Souza, merged February 4 2026.
> 
> That commit made two relevant changes to spi_engine_offload.v:
> 
> 1. cmd_valid was decoupled from spi_active. Previously:
> 
>      assign cmd_valid = spi_active;
> 
>    Now cmd_valid is driven by a separate offload_cmd_valid register.
>    The practical effect is cmd_valid is no longer asserted the instant
>    spi_active goes high.
> 
> 2. spi_active now stays asserted until the DMA has consumed all pending
>    SDI data, not just until the last command is accepted. The old
>    deassertion condition was:
> 
>      cmd_ready && (spi_cmd_rd_addr_next == ctrl_cmd_wr_addr)
> 
>    The new condition holds spi_active high while
>    offload_disable_pending && sdi_data_valid. Since interconnect_dir =
>    spi_enable | spi_active, this means the interconnect stays in offload
>    direction for the full duration of the SDI drain phase.
> 
> The February commit also changed the execution module's io_ready1 logic
> (now gated on pending_sdi_data_valid instead of sdi_data_valid directly),
> which shifts when the execution engine signals completion via SYNC relative
> to the last bit being clocked. Together these changes alter the timing of
> the mode boundary between regular SPI and offload. The panic — as shown
> below — is consistent with a race where SPI_ENGINE_INT_CMD_ALMOST_EMPTY
> is left armed in int_enable after host->cur_msg has already been cleared.
> The exact causal chain needs more instrumentation to confirm, but this
> commit is the most significant HDL delta between your test setup and mine.
> 
> So your instinct was right — something changed on the FPGA side,
> specifically the SDI backpressure fix. The driver ordering fix is still
> correct regardless (and I would say is the right thing to do), but
> I wanted to close the loop on the root cause.
> 
> Regarding the race you raised with the new ordering — I tested with
> msleep(100) inserted between ad4695_enter_advanced_sequencer_mode() and
> spi_offload_trigger_enable() to simulate a slow machine. Data came back
> correctly with no index shift. This confirms the expectation: with PWM
> as the CNV source the ADC only converts on an external CNV pulse, and
> the PWM isn't running until spi_offload_trigger_enable() enables the
> trigger, so there is nothing to miss during that gap.

Thanks for clearing this up. I found my notes from when I was working on
this (back in Oct. 2024). It looks like when you aren't using an external
CNV trigger, then ad4695_enter_advanced_sequencer_mode() actually triggers
the first conversion. But it looks like in the end, we ended up only
supporting the case where PWM is connected to CNV.

So the ordering currently in the driver was probably from an earlier
revision that supported more wiring options.

So yes, I'm convinced that changing the ordering for the PWM/CNV case
should be OK.

We'll still want a v2 though with an updated commit message that gives
the reason for the change independent of the AXI SPI Engine changes that
triggered a bug.