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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 03 Nov 2026 11:53:02 -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>
Really trivial stuff that I might have ignored, but given it looks like
you'll be doing a v4 might as well tidy up.

Thanks,

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..db99fa9f2dda
> --- /dev/null
> +++ b/drivers/iio/frequency/ad9910-axi-backend.c
> @@ -0,0 +1,551 @@

> +
> +static void ad9910_axi_drctl_fromhw(struct ad9910_axi_state *st,
> +				    const struct ad9910_axi_drctl_wfhw *wfhw,
> +				    struct pwm_waveform *wf)
> +{
> +	if (!wfhw->period) {

Maybe a comment on why this one matters, not the width or delay.
If it's a case of all or none, just add a comment on that.

> +		*wf = (struct pwm_waveform){ };
> +		return;
> +	}
> +
> +	wf->period_length_ns = ad9910_axi_cycles_to_ns(st, wfhw->period);
> +	wf->duty_length_ns = ad9910_axi_cycles_to_ns(st, wfhw->width);
> +	wf->duty_offset_ns = ad9910_axi_cycles_to_ns(st, wfhw->delay);
> +}



> +static int ad9910_axi_pwm_read_waveform(struct pwm_chip *chip,
> +					struct pwm_device *pwm, void *_wfhw)
> +{
> +	struct ad9910_axi_state *st = pwmchip_get_drvdata(chip);
> +	struct ad9910_axi_drctl_wfhw *wfhw = _wfhw;
> +	u32 drg;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_read(st->regmap, AD9910_AXI_REG_DRG, &drg);
> +	if (ret)
> +		return ret;
> +
> +	if (!FIELD_GET(AD9910_AXI_REG_DRG_TOGGLE_EN_MSK, drg)) {
> +		*wfhw = (struct ad9910_axi_drctl_wfhw){ };

Given I had to go look to check if you were initializing all fields
in the other paths, maybe just do this zeroing at top of this function?

Doesn't matter much.

> +		return 0;
> +	}
> +
> +	ret = regmap_read(st->regmap, AD9910_AXI_REG_DRCTL_PERIOD, &wfhw->period);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD9910_AXI_REG_DRCTL_WIDTH, &wfhw->width);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_read(st->regmap, AD9910_AXI_REG_DRCTL_DELAY, &wfhw->delay);
> +}

> +
> +static int ad9910_axi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ad9910_axi_state *st;
> +	struct pwm_chip *chip;
> +	void __iomem *base;
> +	struct clk *clk;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*st));

Use local variable dev given you have it.

> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	st = pwmchip_get_drvdata(chip);
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> +
> +	st->dev = dev;
> +	st->regmap = devm_regmap_init_mmio(dev, base,
> +					   &ad9910_axi_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "failed to init register map\n");
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad9910_axi_setup(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to setup device\n");
> +
> +	chip->ops = &ad9910_axi_pwm_ops;
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> +
> +	return devm_iio_backend_register(dev, &ad9910_axi_back_info, st);
> +}