Re: [PATCH v2 06/10] iio: frequency: ad9910: add axi backend driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 22 Oct 2026 12:19:04 -0300
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Add the driver for the AD9910 AXI backend IP core, which interfaces with
> the data and control pins of the AD9910 DDS. It registers as an IIO
> backend that provides:
> 
> - a DMA output buffer that streams data into the parallel data port;
> - the parallel data destination (F[1:0] pins) and data rate, derived
>   from the PDCLK frequency;
> - the PROFILE[2:0] pins control, through profile channel enables.
> 
> It also registers a PWM chip with a single output driving the DRCTL
> pin, which the AD9910 driver uses to control the digital ramp generator
> direction. The PWM implements the waveform callbacks, with period, duty
> length and duty offset programmed in SYNC_CLK cycles. The duty offset
> is realized by a delay applied once, before DRCTL starts toggling. The
> waveform is reprogrammed when the SYNC_CLK rate changes, so that the PWM
> output keeps its timing.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>

A few small things inline. In general I'll be looking for a
tag from Nuno on this like most IIO backends.

Jonathan

> diff --git a/drivers/iio/frequency/ad9910-axi-backend.c b/drivers/iio/frequency/ad9910-axi-backend.c
> new file mode 100644
> index 000000000000..f5374aeb2afe
> --- /dev/null
> +++ b/drivers/iio/frequency/ad9910-axi-backend.c

> +static int ad9910_axi_drctl_write(struct ad9910_axi_state *st,
> +				  const struct ad9910_axi_drctl_wfhw *wfhw)
> +{
> +	int ret;
> +
> +	/*
> +	 * DRCTL toggling restarts, applying the delay, when enabled or when
> +	 * its period or width changes. Changing the delay alone does not
> +	 * restart it, so disable toggling while updating the waveform.
> +	 */
> +	ret = regmap_clear_bits(st->regmap, AD9910_AXI_REG_DRG,
> +				AD9910_AXI_REG_DRG_TOGGLE_EN_MSK);
> +	if (ret || !wfhw->period)

Split conditions as one returns success and that's not obvious coded
like this.  Add a comment there on why it is a success.

> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD9910_AXI_REG_DRCTL_DELAY, wfhw->delay);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD9910_AXI_REG_DRCTL_WIDTH, wfhw->width);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD9910_AXI_REG_DRCTL_PERIOD, wfhw->period);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Register values reach the SYNC_CLK domain in snapshots, taken every
> +	 * 64 AXI clock cycles. Make sure toggling being disabled is captured
> +	 * by one of them before enabling it again.
> +	 */
> +	fsleep(2);
> +
> +	return regmap_set_bits(st->regmap, AD9910_AXI_REG_DRG,
> +			       AD9910_AXI_REG_DRG_TOGGLE_EN_MSK);
> +}
> +
> +static int ad9910_axi_chan_enable(struct iio_backend *back, unsigned int chan)
> +{
> +	struct ad9910_axi_state *st = iio_backend_get_priv(back);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (chan) {
> +	/*
> +	 * Profiles are mutually exclusive, as the PROFILE[2:0] pins always
> +	 * select one of them. Enabling a profile channel selects it, which
> +	 * implicitly disables the previously selected one, hence there is
> +	 * nothing to do when disabling a profile channel.
> +	 */
> +	case AD9910_CHAN_IDX_PROFILE_0 ... AD9910_CHAN_IDX_PROFILE_7:
> +		chan -= AD9910_CHAN_IDX_PROFILE_0;
> +		return regmap_update_bits(st->regmap, AD9910_AXI_REG_PROFILE,
> +					  AD9910_AXI_REG_PROFILE_MSK,
> +					  FIELD_PREP(AD9910_AXI_REG_PROFILE_MSK, chan));
> +	case AD9910_CHAN_IDX_PARALLEL_AMP ... AD9910_CHAN_IDX_PARALLEL_POLAR_PHASE:
> +		/*
> +		 * F[1:0] destination follows the parallel channel order:
> +		 * amplitude, phase, frequency and polar.
> +		 */
> +		chan = min(chan - AD9910_CHAN_IDX_PARALLEL_AMP,
> +			   FIELD_MAX(AD9910_AXI_REG_F_CFG_MSK));

Why min() rather than a FIELD_FIT() and reject if it doesn't?
Or just use FIELD_PREP below and paper over it as you are doing
here.

> +		ret = regmap_update_bits(st->regmap, AD9910_AXI_REG_F_CFG,
> +					 AD9910_AXI_REG_F_CFG_MSK, chan);
Not immediately obvious that the field starts at bit zero so even though it
does I'd use a FIELD_PREP().


> +		if (ret)
> +			return ret;
> +
> +		return regmap_set_bits(st->regmap, AD9910_AXI_REG_UPDATE_CTRL,
> +				       AD9910_AXI_REG_UPDATE_PAR_IF_EN_MSK);
> +	default:
> +		return -EINVAL;
> +	}
> +}