Re: [PATCH v5 06/13] iio: frequency: ad9910: initial driver implementation

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 17 May 2026 19:37:50 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Add the core AD9910 DDS driver infrastructure with single tone mode
> support. This includes SPI register access, profile management via GPIO
> pins, PLL/DAC configuration from firmware properties, and single tone
> frequency/phase/amplitude control through IIO attributes.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
Hi Rodrigo

A couple of potential nice to haves.

Jonathan


> +
> +static int ad9910_parse_fw(struct ad9910_state *st)
> +{
> +	static const char * const refclk_out_drv0[] = {
> +		"disabled", "low", "medium", "high",
> +	};
> +	struct device *dev = &st->spi->dev;
> +	u32 tmp[2];
> +	int ret;
> +
> +	st->data.pll_enabled = device_property_read_bool(dev, "adi,pll-enable");
> +	if (st->data.pll_enabled) {
> +		tmp[0] = AD9910_ICP_MIN_uA;
> +		device_property_read_u32(dev, "adi,charge-pump-current-microamp", &tmp[0]);
Might be a good idea to move to the pattern that seems to be becoming
the preferred way to do this and do
		if (device_property_present()) {
			ret = device_property_read_u32()...
...
		} else {
			...
		}

That is slightly nicer ad picks up malformed DT.  I know I was the advocate for
the set a default and don't check ret but I'm learning!


> +		if (tmp[0] < AD9910_ICP_MIN_uA || tmp[0] > AD9910_ICP_MAX_uA)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "invalid charge pump current %u\n", tmp[0]);
> +		st->data.pll_charge_pump_current = tmp[0];
> +
> +		ret = device_property_match_property_string(dev,
> +							    "adi,refclk-out-drive-strength",
> +							    refclk_out_drv0,
> +							    ARRAY_SIZE(refclk_out_drv0));
> +		if (ret < 0)
Similarly good to know if failure to match actually means wasn't there or not.

> +			st->data.refclk_out_drv = AD9910_REFCLK_OUT_DRV_DISABLED;
> +		else
> +			st->data.refclk_out_drv = ret;
> +	}
> +
> +	tmp[1] = AD9910_DAC_IOUT_DEFAULT_uA;
And similar again.

> +	device_property_read_u32_array(dev, "output-range-microamp", tmp,
> +				       ARRAY_SIZE(tmp));
> +	if (tmp[1] < AD9910_DAC_IOUT_MIN_uA || tmp[1] > AD9910_DAC_IOUT_MAX_uA)
> +		return dev_err_probe(dev, -ERANGE,
> +				     "Invalid DAC output current %u uA\n", tmp[1]);
> +	st->data.dac_output_current = tmp[1];
> +
> +	return 0;
> +}


> +static const struct spi_device_id ad9910_id[] = {
> +	{ "ad9910" },

Request to simplify what Uwe is busy doing (assuming he'll get to spi
at somepoint). Please use a named initializer like we always do for
of_device_id.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ad9910_id);