Re: [PATCH 10/10] iio: dac: support the ad9739a RF DAC

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 28 Mar 2024 14:22:34 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> 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@analog.com>
> Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
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@vger.kernel.org
> +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.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_operating_mode_available
> +KernelVersion:	6.9
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Available operating modes.

>  M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
> 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)

> +	  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.

> +};
> +
> +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.

> +
> +	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 :)

> +	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

> +		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.

> +	} 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.


> +
> +	ret = ad9739a_reset(dev, st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad9739a_init(dev, st);
> +	if (ret)
> +		return ret;
> +
> +	st->back = devm_iio_backend_get(dev, NULL);
> +	if (IS_ERR(st->back))
> +		return PTR_ERR(st->back);
> +
> +	ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_extend_chan_spec(indio_dev, st->back,
> +					   &ad9739a_channels[0]);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_set_sampling_freq(st->back, 0, st->sample_rate);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_backend_enable(dev, st->back);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "ad9739a";
> +	indio_dev->info = &ad9739a_info;
> +	indio_dev->channels = ad9739a_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad9739a_channels);
> +	indio_dev->setup_ops = &ad9739a_buffer_setup_ops;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}