Re: [PATCH v1 12/13] iio: adc: ad4134: Support high-speed data capture

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Sep 02, 2026 at 02:25:26PM -0300, Marcelo Schmitt wrote:
> Make use of SPI transfer offloading to speed up data capture, enabling data
> acquisition at faster sample rates (up to 1.496 MSPS).

...

> +static int ad4134_update_conversion_rate(struct ad4134_state *st,
> +					 unsigned int freq_Hz)
> +{
> +	struct spi_offload_trigger_config config = st->offload_trigger_config;
> +	struct pwm_waveform odr_wf = { };
> +	u64 offload_period_ns;
> +	u64 offload_offset_ns;
> +	u64 odr_high_time_ns;
> +	unsigned int count;
> +	u64 target = 10;

Split assignment and move it closer to its first user.

> +	int ret;
> +
> +	if (!in_range(freq_Hz, AD4134_MIN_ODR_FREQ_HZ, AD4134_MAX_ODR_FREQ_HZ))
> +		return -ERANGE;
> +
> +	odr_wf.period_length_ns = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq_Hz);
> +	/*
> +	 * Set the PWM duty cycle to keep ODR high for at least minimum required
> +	 * time. If the rounded PWM's value is less than the minimum required,
> +	 * increase the target value by 10 and attempt to round the waveform
> +	 * again, until the minimum (or try count limit) is reached.
> +	 */
> +	odr_high_time_ns = div64_ul(6ULL * NSEC_PER_SEC, st->sys_clk_hz);

	target = 0;

> +	count = 100;
> +	do {

		target += 10; /* Increment by PWM duty cycle period */

> +		odr_wf.duty_length_ns = target;
> +		ret = pwm_round_waveform_might_sleep(st->odr_pwm, &odr_wf);
> +		if (ret)
> +			return ret;

> +		target += 10; /* Increment by PWM duty cycle period */

In the above way I think it clarifies the initial setting to 10.

Shouldn't be target named as target_ns? Can we use odr_wf.duty_length_ns
directly?

> +	} while (count-- && odr_wf.duty_length_ns < odr_high_time_ns);
> +
> +	/* Check the minimum ODR high time is met */
> +	if (odr_wf.duty_length_ns < odr_high_time_ns)
> +		return -EDOM;
> +
> +	if (odr_wf.period_length_ns < 2 * odr_high_time_ns)
> +		return -EDOM;
> +
> +	/*
> +	 * Configure SPI offload PWM trigger.
> +	 * For gated DCLK, the minimum required time between ODR rising edge
> +	 * and DCLK rising edge is the sum of ODR high time and ODR falling
> +	 * edge to DCLK rising edge time. Delay the offload trigger for at least
> +	 * that amount of time so the ADC sample data will be available when the
> +	 * SPI transfer begin.
> +	 *
> +	 * Use the same period as ODR PWM to avoid timing issues.
> +	 * Convert back from period to frequency for the SPI offload API.
> +	 */
> +	offload_period_ns = odr_wf.period_length_ns;
> +	config.periodic.frequency_hz = DIV_ROUND_UP_ULL(HZ_PER_GHZ, offload_period_ns);
> +	offload_offset_ns = odr_high_time_ns + AD4134_DCLK_RISING_OFFSET_NS;
> +	count = 100;
> +	do {
> +		config.periodic.offset_ns = offload_offset_ns;
> +		ret = spi_offload_trigger_validate(st->offload_trigger, &config);
> +		if (ret)
> +			return ret;
> +
> +		offload_offset_ns += 10;

Does this need the similar comment as per above?

> +	} while (count-- && config.periodic.offset_ns < odr_high_time_ns +
> +							AD4134_DCLK_RISING_OFFSET_NS);
> +
> +	/* Check the minimum ODR to DCLK delay is met */
> +	if (config.periodic.offset_ns < odr_high_time_ns + AD4134_DCLK_RISING_OFFSET_NS)
> +		return -EDOM;
> +
> +	/* Check the PWM periods remain the same */
> +	offload_period_ns = DIV_ROUND_UP_ULL(HZ_PER_GHZ, config.periodic.frequency_hz);
> +	if (odr_wf.period_length_ns != offload_period_ns)
> +		return -EDOM;
> +
> +	ret = pwm_set_waveform_might_sleep(st->odr_pwm, &odr_wf, false);
> +	if (ret)
> +		return ret;
> +
> +	st->offload_trigger_config = config;
> +	st->odr_wf = odr_wf;
> +	st->odr_hz = DIV_ROUND_UP_ULL(NSEC_PER_SEC, odr_wf.period_length_ns);
> +
> +	return 0;
> +}

...

> +static int ad4134_offload_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	int ret, ret2;

These ret2:s in the cases of _set_register_() can be named accordingly.

> +	if (st->spi_mode == AD4134_SPI_MODE_4_WIRE) {
> +		ret = ad4134_set_sample_access(st);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ad4134_prepare_offload_msg(indio_dev);
> +	st->msg.offload = st->offload;
> +	ret = spi_optimize_message(st->spi, &st->msg);
> +	if (ret)
> +		goto out_set_register_input;
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &st->offload_trigger_config);
> +	if (ret)
> +		goto out_unoptimize;
> +
> +	return 0;
> +
> +out_unoptimize:
> +	spi_unoptimize_message(&st->msg);
> +
> +out_set_register_input:
> +	ret2 = ad4134_set_register_access(st);
> +	if (ret2)
> +		dev_err(&st->spi->dev, "reg input select error: %d\n", ret2);
> +
> +	return ret;
> +}

...

> +/* The chip converts and outputs all 4 channels on each sample request */
> +static const unsigned long ad4134_scan_masks[] = {
> +	GENMASK(3, 0),

IIRC Jonathan wants to see this as BIT(x) | BIT(y) | ... to clarify that each
bit is semantically independent.

> +	0
> +};

-- 
With Best Regards,
Andy Shevchenko