On 26/05/17 03:47PM, Jonathan Cameron wrote:
> On Fri, 08 May 2026 18:00:19 +0100
> Rodrigo Alencar via B4 Relay <[email protected]>
> wrote:
>
> > From: Rodrigo Alencar <[email protected]>
> >
> > 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 <[email protected]>
> Hi Rodrigo
>
> A few really minor things from a fresh look through.
>
> Jonathan
>
> > diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> > new file mode 100644
> > index 000000000000..c75f2ef178c2
> > --- /dev/null
> > +++ b/drivers/iio/frequency/ad9910.c
>
> > +
> > +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_ENABLE:
> > + switch (chan->channel) {
> > + case AD9910_CHANNEL_PROFILE_0 ... AD9910_CHANNEL_PROFILE_7:
> > + if (ad9910_sw_powerdown_get(st)) {
> > + *val = 0;
> > + } else {
> > + tmp32 = chan->channel -
> > AD9910_CHANNEL_PROFILE_0;
> > + *val = (tmp32 == st->profile);
> > + }
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_FREQUENCY:
> > + switch (chan->channel) {
> > + case AD9910_CHANNEL_PROFILE_0 ... AD9910_CHANNEL_PROFILE_7:
> > + tmp32 = chan->channel - AD9910_CHANNEL_PROFILE_0;
> > + tmp64 = FIELD_GET(AD9910_PROFILE_ST_FTW_MSK,
> > +
> > st->reg[AD9910_REG_PROFILE(tmp32)].val64);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + tmp64 *= st->data.sysclk_freq_hz;
> > + *val = tmp64 >> 32;
> > + *val2 = ((tmp64 & GENMASK_ULL(31, 0)) * MICRO) >> 32;
>
> Why in this particular case have this outside the switch / case whereas in
> others
> you do the full maths and set inside? I'd put it inside and not worry about
> slightly
> long lines.
for frequency, those calculations are going to be common for the other channels
that are
going to be populated by other patches...
DRG up/down and RAM will have tmp64 populated with a FTW value.
>
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_PHASE:
> > + switch (chan->channel) {
> > + case AD9910_CHANNEL_PROFILE_0 ... AD9910_CHANNEL_PROFILE_7:
> > + tmp32 = chan->channel - AD9910_CHANNEL_PROFILE_0;
> > + tmp64 = FIELD_GET(AD9910_PROFILE_ST_POW_MSK,
> > +
> > st->reg[AD9910_REG_PROFILE(tmp32)].val64);
> > + tmp32 = (tmp64 * AD9910_MAX_PHASE_MICRORAD) >> 16;
> > + *val = tmp32 / MICRO;
> > + *val2 = tmp32 % MICRO;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SCALE:
> > + switch (chan->channel) {
> > + case AD9910_CHANNEL_PROFILE_0 ... AD9910_CHANNEL_PROFILE_7:
> > + tmp32 = chan->channel - AD9910_CHANNEL_PROFILE_0;
> > + tmp64 = FIELD_GET(AD9910_PROFILE_ST_ASF_MSK,
> > +
> > st->reg[AD9910_REG_PROFILE(tmp32)].val64);
> > + *val = 0;
> > + *val2 = tmp64 * MICRO >> 14;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + switch (chan->channel) {
> > + case AD9910_CHANNEL_PHY:
> > + *val = st->data.sysclk_freq_hz;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
>
>
> > +
> > +static int ad9910_setup(struct device *dev, struct ad9910_state *st,
> > + struct reset_control *dev_rst)
> > +{
> > + int ret;
> > +
> > + ret = reset_control_deassert(dev_rst);
> > + if (ret)
> > + return ret;
> No need to sleep at all after bringing device out of reset?
>
> Sashiko has reasonably been asking about this in other drivers. If there
> is no period needed or it is so quick as to be irrelevant add a comment here.
I do not see any requirement on that in the datasheet.
> > +
> > + ret = ad9910_reg32_write(st, AD9910_REG_CFR1,
> > + AD9910_CFR1_SDIO_INPUT_ONLY_MSK, false);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(dev, ad9910_sw_powerdown_action, st);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ad9910_reg32_write(st, AD9910_REG_CFR2,
> > + AD9910_CFR2_AMP_SCALE_SINGLE_TONE_MSK |
> > + AD9910_CFR2_SYNC_TIMING_VAL_DISABLE_MSK |
> > + AD9910_CFR2_DRG_NO_DWELL_MSK |
> > + AD9910_CFR2_DATA_ASM_HOLD_LAST_MSK |
> > + AD9910_CFR2_SYNC_CLK_EN_MSK |
> > + AD9910_CFR2_PDCLK_ENABLE_MSK, false);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ad9910_cfg_sysclk(st, false);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ad9910_set_dac_current(st, false);
> > + if (ret)
> > + return ret;
> > +
> > + return ad9910_io_update(st);
> > +}
>
--
Kind regards,
Rodrigo Alencar
