Re: [PATCH v5 5/7] iio: adc: ad4030: Add SPI offload support

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

On 10/17, Nuno Sá wrote:
> On Tue, 2025-10-14 at 19:22 -0300, Marcelo Schmitt wrote:
> > AD4030 and similar ADCs can capture data at sample rates up to 2 mega
> > samples per second (MSPS). Not all SPI controllers are able to achieve such
> > high throughputs and even when the controller is fast enough to run
> > transfers at the required speed, it may be costly to the CPU to handle
> > transfer data at such high sample rates. Add SPI offload support for AD4030
> > and similar ADCs to enable data capture at maximum sample rates.
> > 
> > Co-developed-by: Trevor Gamblin <tgamblin@baylibre.com>
> > Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> > Co-developed-by: Axel Haslam <ahaslam@baylibre.com>
> > Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> 
...
> > +static int ad4030_offload_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	unsigned int reg_modes;
> > +	int ret, ret2;
> > +
...
> > +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> > +					 &st->offload_trigger_config);
> > +	if (ret)
> > +		goto out_pwm_disable;
> > +
> > +	return 0;
> > +
> > +out_pwm_disable:
> > +	pwm_disable(st->cnv_trigger);
> > +out_unoptimize:
> > +	spi_unoptimize_message(&st->offload_msg);
> > +out_reset_mode:
> > +	/* reenter register configuration mode */
> > +	ret2 = ad4030_enter_config_mode(st);
> 
> nit: if ret2 is not being used at all, maybe just
ret2 is logged on the error message below so I guess I'll keep it as it is.
> 
> if (ad4030_enter_config_mode(st))
> 	
> > +	if (ret2)
> > +		dev_err(&st->spi->dev,
> > +			"couldn't reenter register configuration mode: %d\n",
> > +			ret2);
here we log the error code. We only reach it if reg access fails after
something on offload buffer enable have failed first. We cannot reuse ret here 
because we would be shadowing the original error code.

> > +
> > +	return ret;
> > +}
> > 
> 
> ...
> 
...
> > +	} else {
> > +		/*
> > +		 * One hardware channel is split in two software channels
> > when
> > +		 * using common byte mode. Offloaded SPI transfers can't
> > support
> > +		 * software timestamp so no additional timestamp channel is
> > added.
> > +		 */
> > +		indio_dev->num_channels = 2 * st->chip->num_voltage_inputs;
> 
> Maybe I'm missing something but common mode is not supported for now so isn't
> the above wrong?
> 

Yes, that was buggy. Dropping common-mode channels as last minute change was a
bad idea. I did another set of tests with ADAQ4216 and fixed that for v6. I'll
also re-add common-mode channel support as a separate patch.

Thanks,
Marcelo