Re: [PATCH RFC v2 2/9] iio: frequency: ad9910: initial driver implementation

[Rodrigo Alencar]

On 26/03/22 04:50PM, Jonathan Cameron wrote:
> On Wed, 18 Mar 2026 17:56:02 +0000
> Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@xxxxxxxxxx> wrote:
> 
> > From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>

...

> > +#include <linux/array_size.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> 
> Generally can avoid including device.h in favour of more specific
> headers.  There are a few exceptions where we can't such as actual
> dereferencing of struct device, but I don't recall seeing a case in here.

I understood that the usage of devm_add_action_or_reset() would justify
the header.

...

> > +#define AD9910_EXT_INFO(_name, _ident, _shared) { \
> > +	.name = _name, \
> > +	.read = ad9910_ext_info_read, \
> > +	.write = ad9910_ext_info_write, \
> > +	.private = _ident, \
> > +	.shared = _shared, \
> 
> If there are only a few of these, I'd put it long hand rather than
> using a macro. Tends to end up easier to read.

Next patches will leverage the macro as more ext_info attrs will be introduced.
I suppose we can build the foundation for later extension.
 
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info ad9910_phy_ext_info[] = {
> > +	AD9910_EXT_INFO("profile", AD9910_PROFILE, IIO_SEPARATE),
> > +	AD9910_EXT_INFO("powerdown", AD9910_POWERDOWN, IIO_SEPARATE),
> > +	{ }
> > +};
> 
> > +static int ad9910_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long info)
> > +{
> > +	struct ad9910_state *st = iio_priv(indio_dev);
> > +	u64 tmp64;
> > +	u32 tmp32;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_FREQUENCY:
> > +		switch (chan->channel) {
> > +		case AD9910_CHANNEL_SINGLE_TONE:
> 
> I haven't read on yet, but if you never have any other cases in here,
> perhaps us an if() as it will reduce indent of the code that follows.

Similar, other channels will be introduced here so additions are easier
to review.

> > +			tmp32 = FIELD_GET(AD9910_PROFILE_ST_FTW_MSK,
> > +					  st->reg[AD9910_REG_PROFILE(st->profile)].val64);
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +		tmp64 = (u64)tmp32 * st->data.sysclk_freq_hz;
> > +		*val = upper_32_bits(tmp64);
> > +		*val2 = upper_32_bits((u64)lower_32_bits(tmp64) * MICRO);

...

> > +
> > +static int ad9910_cfg_sysclk(struct ad9910_state *st, bool update)
> > +{
> > +	u32 tmp32, cfr3 = AD9910_CFR3_OPEN_MSK;
> > +
> > +	cfr3 |= AD9910_CFR3_VCO_SEL_MSK |
> > +		FIELD_PREP(AD9910_CFR3_DRV0_MSK, st->data.refclk_out_drv);
> > +
> > +	if (st->data.pll_enabled) {
> > +		tmp32 = st->data.pll_charge_pump_current - AD9910_ICP_MIN_uA;
> > +		tmp32 = DIV_ROUND_CLOSEST(tmp32, AD9910_ICP_STEP_uA);
> > +		cfr3 |= FIELD_PREP(AD9910_CFR3_ICP_MSK, tmp32) |
> > +			AD9910_CFR3_PLL_EN_MSK;
> > +	} else {
> > +		cfr3 |= AD9910_CFR3_ICP_MSK |
> 
> For this, be explicit what value you are setting, probably be defining a max value
> that the field can take.  Whilst just setting the mask is the same it doesn't
> convey the same meaning to someone reading the code.

This is just the default value from the datasheet, ICP should not really matter
when the PLL is disabled, so removing this should be fine.

> 
> > +			AD9910_CFR3_REFCLK_DIV_RESETB_MSK |
> > +			AD9910_CFR3_PFD_RESET_MSK;
> > +	}
> > +	st->reg[AD9910_REG_CFR3].val32 = cfr3;
> > +
> > +	return ad9910_set_sysclk_freq(st, AD9910_PLL_OUT_MAX_FREQ_HZ, update);
> > +}
> > +
> > +static int ad9910_parse_fw(struct ad9910_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	u32 tmp;
> > +	int ret;
> > +
> > +	st->data.pll_enabled = device_property_read_bool(dev, "adi,pll-enable");
> > +	if (st->data.pll_enabled) {
> > +		tmp = AD9910_ICP_MAX_uA;
> 
> Defaulting to max current seems unusual.  What's the motivation? Normal instinct is
> go minimum if no other info.

ICP_MAX_uA leads to 111 in the CFR3_ICP field, which is the default value when the
device resets or when it powers on. I suppose that if we are not touching that
property, there would be no reason to change that.

...

-- 
Kind regards,

Rodrigo Alencar