On Thu, 2024-03-28 at 15:51 +0000, Jonathan Cameron wrote:
> On Thu, 28 Mar 2024 14:22:34 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
>
> > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> >
> > The AD9739A is a 14-bit, 2.5 GSPS high performance RF DACs that are capable
> > of synthesizing wideband signals from dc up to 3 GHz.
> DC perhaps
>
> >
> > A dual-port, source synchronous, LVDS interface simplifies the digital
> > interface with existing FGPA/ASIC technology. On-chip controllers are used
> > to manage external and internal clock domain variations over temperature to
> > ensure reliable data transfer from the host to the DAC core.
> >
> > Co-developed-by: Dragos Bogdan <dragos.bogdan@xxxxxxxxxx>
> > Signed-off-by: Dragos Bogdan <dragos.bogdan@xxxxxxxxxx>
> > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> Hi Nuno,
>
> A few questions / comments inline but on the whole looking good to me.
>
> Jonathan
>
> > ---
> > Documentation/ABI/testing/sysfs-bus-iio-ad9739a | 17 +
> > MAINTAINERS | 1 +
> > drivers/iio/dac/Kconfig | 16 +
> > drivers/iio/dac/Makefile | 1 +
> > drivers/iio/dac/ad9739a.c | 445 ++++++++++++++++++++++++
> > 5 files changed, 480 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ad9739a
> > b/Documentation/ABI/testing/sysfs-bus-iio-ad9739a
> > new file mode 100644
> > index 000000000000..8a8a5cd10386
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-ad9739a
> > @@ -0,0 +1,17 @@
> > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_operating_mode
> > +KernelVersion: 6.9
> > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > + Dac operating mode. One of the following modes can be selected:
>
> DAC operating mode. ...
>
> > + * normal: This is DAC normal mode.
> > + * mixed-mode: In this mode the output is effectively chopped at the
>
> Spaces and tabs mixed...
>
> > + DAC sample rate. This has the effect of reducing the
> > + power of the fundamental signal while increasing the
> > + power of the images centered around the DAC sample rate,
> > + thus improving the output power of these images.
>
> Any idea why it is called mixed mode? Name doesn't suggest to me what the Docs say
> this does.
Nope, just respecting the datasheet names for the modes. But I may give it another
read. Likely there was a reason for that naming :)
>
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_operating_mode_ava
> > ilable
> > +KernelVersion: 6.9
> > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > + Available operating modes.
>
> > M: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > index 7c0a8caa9a34..ee0d9798d8b4 100644
> > --- a/drivers/iio/dac/Kconfig
> > +++ b/drivers/iio/dac/Kconfig
> > @@ -131,6 +131,22 @@ config AD5624R_SPI
> > Say yes here to build support for Analog Devices AD5624R, AD5644R and
> > AD5664R converters (DAC). This driver uses the common SPI interface.
> >
> > +config AD9739A
> > + tristate "Analog Devices AD9739A RF DAC spi driver"
> > + depends on SPI
> > + select REGMAP_SPI
> > + select IIO_BACKEND
> > + help
> > + Say yes here to build support for Analog Devices AD9739A Digital-to
> > + Analog Converter.
> > +
> > + The driver requires the assistance of the AXI DAC IP core to operate,
>
> Maybe a depends on || COMPILE_TEST to increase build coverage (compared to
> a hard depends on)
>
Can do that...
> > + since SPI is used for configuration only, while data has to be
> > + streamed into memory via DMA.
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called ad9739a.
> > +
>
>
> > diff --git a/drivers/iio/dac/ad9739a.c b/drivers/iio/dac/ad9739a.c
> > new file mode 100644
> > index 000000000000..46431fa345a5
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad9739a.c
>
> > +
> > +enum {
> > + AD9739A_NORMAL_MODE,
> > + AD9739A_MIXED_MODE = 2,
>
> Push these next to the relevant registers and more conventional defines.
> Not seeing why the enum helps much here.
Alright..
>
> > +};
> > +
> > +static int ad9739a_oper_mode_get(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct ad9739a_state *st = iio_priv(indio_dev);
> > + u32 mode;
> > + int ret;
> > +
> > + ret = regmap_read(st->regmap, AD9739A_REG_DEC_CNT, &mode);
> > + if (ret)
> > + return ret;
> > +
> > + mode = FIELD_GET(AD9739A_DAC_DEC, mode);
> > + /* sanity check we get valid values from the HW */
> > + if (mode != AD9739A_NORMAL_MODE && mode != AD9739A_MIXED_MODE)
> > + return -EIO;
> > + if (!mode)
> > + return AD9739A_NORMAL_MODE;
> > +
> > + return AD9739A_MIXED_MODE - 1;
>
> As below. I'd like to see a mapping function, or lookup table or similar
> rather than handling this conversion in code.
>
> > +}
> > +
> > +static int ad9739a_oper_mode_set(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan, u32 mode)
> > +{
> > + struct ad9739a_state *st = iio_priv(indio_dev);
> > +
> > + if (mode == AD9739A_MIXED_MODE - 1)
> > + mode = AD9739A_MIXED_MODE;
>
> Why? Feels like a comment is needed. Or a more obvious conversion function.
>
To match what we want to write in the register... With just two values it's too
simple that opt not to have any helper function or table. Would you be fine with a
comment?
> > +
> > + return regmap_update_bits(st->regmap, AD9739A_REG_DEC_CNT,
> > + AD9739A_DAC_DEC, mode);
> > +}
> > +
> > +static int ad9739a_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct ad9739a_state *st = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + *val = st->sample_rate;
> > + *val2 = 0;
> > + return IIO_VAL_INT_64;
>
> Big numbers :)
My setup is using 2.5Ghz which is big enough to overflow INT but would work on UINT.
>
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
>
>
> > +
> > +/*
> > + * Recommended values (as per datasheet) for the dac clk common mode voltage
> > + * and Mu controller. Look at table 29.
> > + */
> > +static const struct reg_sequence ad9739a_clk_mu_ctrl[] = {
> > + /* DAC clk common mode voltage */
> > + {AD9739A_REG_CROSS_CNT1, 0x0f},
> { AD9739A_REG_CROSS_CNT1, 0x0f },
> etc is more readable in my opinion so is always my preference in IIO.
>
> > + {AD9739A_REG_CROSS_CNT2, 0x0f},
> > + /* Mu controller configuration */
> > + {AD9739A_REG_PHS_DET, 0x30},
> > + {AD9739A_REG_MU_DUTY, 0x80},
> > + {AD9739A_REG_MU_CNT2, 0x44},
> > + {AD9739A_REG_MU_CNT3, 0x6c},
> > +};
> > +
> > +static int ad9739a_init(struct device *dev, const struct ad9739a_state *st)
> > +{
> > + unsigned int i = 0, lock, fsc;
> > + u32 fsc_raw;
> > + int ret;
> > +
> > + ret = regmap_multi_reg_write(st->regmap, ad9739a_clk_mu_ctrl,
> > + ARRAY_SIZE(ad9739a_clk_mu_ctrl));
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * Try to get the MU lock. Repeat the below steps AD9739A_LOCK_N_TRIES
> > + * (as specified by the datasheet) until we get the lock.
> > + */
> > + do {
> > + ret = regmap_write(st->regmap, AD9739A_REG_MU_CNT4,
> > + AD9739A_MU_CNT4_DEFAULT);
> > + if (ret)
> > + return ret;
> > +
> > + /* Enable the Mu controller search and track mode. */
>
> MU for consistency
ack
>
> > + ret = regmap_set_bits(st->regmap, AD9739A_REG_MU_CNT1,
> > + AD9739A_MU_EN_MASK);
> > + if (ret)
> > + return ret;
> > +
> > + /* Ensure the DLL loop is locked */
> > + ret = regmap_read_poll_timeout(st->regmap, AD9739A_REG_MU_STAT1,
> > + lock, lock &
> > AD9739A_MU_LOCK_MASK,
> > + 0, 1000);
> if (ret < 0 && ret != -ETIMEOUT)
> return ret;
>
> i.e. deal with error codes that don't meant it timed out.
>
Oh yes, that makes sense.
> > + } while (ret && ++i < AD9739A_LOCK_N_TRIES);
> > +
> > + if (i == AD9739A_LOCK_N_TRIES)
> > + return dev_err_probe(dev, ret, "Mu lock timeout\n");
> > +
> > + /* Receiver tracking and lock. Same deal as the Mu controller */
>
> MU or Mu. Either fine but be consistent in comments. I have no idea what this is
> so can't say which is better.
>
> > + i = 0;
> > + do {
> > + ret = regmap_update_bits(st->regmap, AD9739A_REG_LVDS_REC_CNT4,
> > + AD9739A_FINE_DEL_SKW_MASK,
> > + FIELD_PREP(AD9739A_FINE_DEL_SKW_MASK,
> > 2));
> > + if (ret)
> > + return ret;
> > +
> > + /* Disable the receiver and the loop. */
> > + ret = regmap_write(st->regmap, AD9739A_REG_LVDS_REC_CNT1, 0);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * Re-enable the loop so it falls out of lock and begins the
> > + * search/track routine again.
> > + */
> > + ret = regmap_set_bits(st->regmap, AD9739A_REG_LVDS_REC_CNT1,
> > + AD9739A_RCVR_LOOP_EN_MASK);
> > + if (ret)
> > + return ret;
> > +
> > + /* Ensure the DLL loop is locked */
> > + ret = regmap_read_poll_timeout(st->regmap,
> > + AD9739A_REG_LVDS_REC_STAT9, lock,
> > + lock ==
> > AD9739A_RCVR_TRACK_AND_LOCK,
> > + 0, 1000);
>
> As above, consider other error codes than -ETIMEOUT;
>
> > + } while (ret && ++i < AD9739A_LOCK_N_TRIES);
> > +
> > + if (i == AD9739A_LOCK_N_TRIES)
> > + return dev_err_probe(dev, ret, "Receiver lock timeout\n");
> > +
> > + ret = device_property_read_u32(dev, "adi,full-scale-microamp", &fsc);
> > + if (ret && ret == -EINVAL)
> > + return 0;
> > + if (ret)
> > + return ret;
> > + if (!in_range(fsc, AD9739A_FSC_MIN, AD9739A_FSC_RANGE))
> > + return dev_err_probe(dev, -EINVAL,
> > + "Invalid full scale current(%u) [%u %u]\n",
> > + fsc, AD9739A_FSC_MIN, AD9739A_FSC_MAX);
> > + /*
> > + * IOUTFS is given by
> > + * Ioutfs = 0.0226 * FSC + 8.58
> > + * and is given in mA. Hence we'll have to multiply by 10 * MILLI in
> > + * order to get rid of the fractional.
> > + */
> > + fsc_raw = DIV_ROUND_CLOSEST(fsc * 10 - 85800, 226);
> > +
> > + ret = regmap_write(st->regmap, AD9739A_REG_FSC_1, fsc_raw & 0xff);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_update_bits(st->regmap, AD9739A_REG_FSC_1,
> > + AD9739A_FSC_MSB, fsc_raw >> 8);
> > +}
>
>
>
> > +
> > +static int ad9739a_probe(struct spi_device *spi)
> > +{
> > + struct device *dev = &spi->dev;
> > + struct iio_dev *indio_dev;
> > + struct ad9739a_state *st;
> > + unsigned int id;
> > + struct clk *clk;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + st = iio_priv(indio_dev);
> > +
> > + clk = devm_clk_get_enabled(dev, NULL);
> > + if (IS_ERR(clk))
> > + return dev_err_probe(dev, PTR_ERR(clk), "Could not get
> > clkin\n");
> > +
> > + st->sample_rate = clk_get_rate(clk);
> > + if (!in_range(st->sample_rate, AD9739A_MIN_DAC_CLK,
> > + AD9739A_DAC_CLK_RANGE))
> > + return dev_err_probe(dev, -EINVAL,
> > + "Invalid dac clk range(%lu) [%lu %lu]\n",
> > + st->sample_rate, AD9739A_MIN_DAC_CLK,
> > + AD9739A_MAX_DAC_CLK);
> > +
> > + st->regmap = devm_regmap_init_spi(spi, &ad9739a_regmap_config);
> > + if (IS_ERR(st->regmap))
> > + return PTR_ERR(st->regmap);
> > +
> > + ret = regmap_read(st->regmap, AD9739A_REG_ID, &id);
> > + if (ret)
> > + return ret;
> > +
> > + if (id != AD9739A_ID)
> > + return dev_err_probe(dev, -ENODEV, "Unrecognized CHIP_ID 0x%X",
> > + id);
> Do we have to give up here? Could it be a compatible future part?
> If so we should fallback on what firmware told us it was + perhaps a
> dev_info() to say we don't recognise the ID register value.
>
I typically prefer to really give up in these cases but no strong opinion... Can turn
this into a dev_warn()...
- Nuno Sá
