Re: [PATCH 09/10] iio: dac: add support for AXI DAC IP core

[Jonathan Cameron <jic23@kernel.org>]

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

> From: Nuno Sa <nuno.sa@analog.com>
> 
> Support the Analog Devices Generic AXI DAC IP core. The IP core is used
> for interfacing with digital-to-analog (DAC) converters that require either
> a high-speed serial interface (JESD204B/C) or a source synchronous parallel
> interface (LVDS/CMOS). Typically (for such devices) SPI will be used for
> configuration only, while this IP core handles the streaming of data into
> memory via DMA.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>


A few minor things inline, but mostly seems fine to me.

Jonathan


...

> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> new file mode 100644
> index 000000000000..0022ecb4e4bb
> --- /dev/null
> +++ b/drivers/iio/dac/adi-axi-dac.c


> +
> +enum {
> +	AXI_DAC_FREQ_TONE_1,
> +	AXI_DAC_FREQ_TONE_2,
> +	AXI_DAC_SCALE_TONE_1,
> +	AXI_DAC_SCALE_TONE_2,
> +	AXI_DAC_PHASE_TONE_1,
> +	AXI_DAC_PHASE_TONE_2,
> +};
> +
> +static int __axi_dac_frequency_get(struct axi_dac_state *st, unsigned int chan,
> +				   unsigned int tone, unsigned int *freq)
> +{
> +	u32 reg, raw;
> +	int ret;
> +
> +	if (!st->dac_clk) {
> +		dev_err(st->dev, "Sampling rate is 0...\n");
> +		return -EINVAL;
> +	}
> +
> +	if (tone == AXI_DAC_FREQ_TONE_1)

Given this is matching 2 out of enum with other values, it would be more
locally readable as a switch statement with an error returning default.
Then we wouldn't need to look at the caller.

Or at the caller convert from the enum to 0,1 for all these functions.



> +		reg = AXI_DAC_REG_CHAN_CNTRL_2(chan);
> +	else
> +		reg = AXI_DAC_REG_CHAN_CNTRL_4(chan);
> +
> +	ret = regmap_read(st->regmap, reg, &raw);
> +	if (ret)
> +		return ret;
> +
> +	raw = FIELD_GET(AXI_DAC_FREQUENCY, raw);
> +	*freq = DIV_ROUND_CLOSEST_ULL(raw * st->dac_clk, BIT(16));
> +
> +	return 0;
> +}

...

> +static int axi_dac_scale_set(struct axi_dac_state *st,
> +			     const struct iio_chan_spec *chan,
> +			     const char *buf, size_t len, unsigned int tone)
> +{
> +	int integer, frac, scale;
> +	u32 raw = 0, reg;
> +	int ret;
> +
> +	ret = iio_str_to_fixpoint(buf, 100000, &integer, &frac);
> +	if (ret)
> +		return ret;
> +
> +	scale = integer * MEGA + frac;
> +	if (scale <= -2 * (int)MEGA || scale >= 2 * (int)MEGA)
> +		return -EINVAL;
> +
> +	/*  format is 1.1.14 (sign, integer and fractional bits) */
> +	if (scale < 0) {
> +		raw = FIELD_PREP(AXI_DAC_SCALE_SIGN, 1);
> +		scale *= -1;
> +	}
> +
> +	raw |= div_u64((u64)scale * AXI_DAC_SCALE_INT, MEGA);
> +
> +	if (tone == AXI_DAC_SCALE_TONE_1)
> +		reg = AXI_DAC_REG_CHAN_CNTRL_1(chan->channel);
> +	else
> +		reg = AXI_DAC_REG_CHAN_CNTRL_3(chan->channel);
> +
> +	guard(mutex)(&st->lock);
> +	ret = regmap_write(st->regmap, reg, raw);
> +	if (ret)
> +		return ret;
> +
> +	/* synchronize channels */
> +	ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_1, AXI_DAC_SYNC);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static int axi_dac_phase_set(struct axi_dac_state *st,
> +			     const struct iio_chan_spec *chan,
> +			     const char *buf, size_t len, unsigned int tone)
> +{
> +	int integer, frac, phase;
> +	u32 raw, reg;
> +	int ret;
> +
> +	ret = iio_str_to_fixpoint(buf, 100000, &integer, &frac);

> +	if (ret)
> +		return ret;
> +
> +	phase = integer * MEGA + frac;
> +	if (phase < 0 || phase > AXI_DAC_2_PI_MEGA)
> +		return -EINVAL;
> +
> +	raw = DIV_ROUND_CLOSEST_ULL((u64)phase * U16_MAX, AXI_DAC_2_PI_MEGA);
> +
> +	if (tone == AXI_DAC_PHASE_TONE_1)
Preference for a switch so it's clear there are only 2 choices.
> +		reg = AXI_DAC_REG_CHAN_CNTRL_2(chan->channel);
> +	else
> +		reg = AXI_DAC_REG_CHAN_CNTRL_4(chan->channel);
> +
> +	guard(mutex)(&st->lock);
> +	ret = regmap_update_bits(st->regmap, reg, AXI_DAC_PHASE,
> +				 FIELD_PREP(AXI_DAC_PHASE, raw));
> +	if (ret)
> +		return ret;
> +
> +	/* synchronize channels */
> +	ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_1, AXI_DAC_SYNC);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static int axi_dac_ext_info_set(struct iio_backend *back, uintptr_t private,
> +				const struct iio_chan_spec *chan,
> +				const char *buf, size_t len)
> +{
> +	struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> +	switch (private) {
> +	case AXI_DAC_FREQ_TONE_1:
> +	case AXI_DAC_FREQ_TONE_2:

Same as the get path - convert to which tone here so that the enum becomes
a tone index for the functions called and the mapping to that single enum
is kept clear of the lower level code.

> +		return axi_dac_frequency_set(st, chan, buf, len, private);
> +	case AXI_DAC_SCALE_TONE_1:
> +	case AXI_DAC_SCALE_TONE_2:
> +		return axi_dac_scale_set(st, chan, buf, len, private);
> +	case AXI_DAC_PHASE_TONE_1:
> +	case AXI_DAC_PHASE_TONE_2:
> +		return axi_dac_phase_set(st, chan, buf, len, private);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int axi_dac_ext_info_get(struct iio_backend *back, uintptr_t private,
> +				const struct iio_chan_spec *chan, char *buf)
> +{
> +	struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> +	switch (private) {
> +	case AXI_DAC_FREQ_TONE_1:
> +	case AXI_DAC_FREQ_TONE_2:
> +		return axi_dac_frequency_get(st, chan, buf, private);
I'd break out private as an unsigned int here and then - AXI_DAC_FREQ_TONE_1
so that it is just which tone for all the calls made from here.
Similar for the following ones.

> +	case AXI_DAC_SCALE_TONE_1:
> +	case AXI_DAC_SCALE_TONE_2:
> +		return axi_dac_scale_get(st, chan, buf, private);
> +	case AXI_DAC_PHASE_TONE_1:
> +	case AXI_DAC_PHASE_TONE_2:
> +		return axi_dac_phase_get(st, chan, buf, private);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const struct iio_chan_spec_ext_info axi_dac_ext_info[] = {
> +	IIO_BACKEND_EX_INFO("frequency0", IIO_SEPARATE, AXI_DAC_FREQ_TONE_1),
> +	IIO_BACKEND_EX_INFO("frequency1", IIO_SEPARATE, AXI_DAC_FREQ_TONE_2),
> +	IIO_BACKEND_EX_INFO("scale0", IIO_SEPARATE, AXI_DAC_SCALE_TONE_1),
> +	IIO_BACKEND_EX_INFO("scale1", IIO_SEPARATE, AXI_DAC_SCALE_TONE_2),
> +	IIO_BACKEND_EX_INFO("phase0", IIO_SEPARATE, AXI_DAC_PHASE_TONE_1),
> +	IIO_BACKEND_EX_INFO("phase1", IIO_SEPARATE, AXI_DAC_PHASE_TONE_2),
> +	{}
> +};
> +
> +static int axi_dac_extend_chan(struct iio_backend *back,
> +			       struct iio_chan_spec *chan)
> +{
> +	struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> +	if (chan->type != IIO_ALTVOLTAGE)
> +		return -EINVAL;
> +	if (st->reg_config & AXI_DDS_DISABLE)
> +		/* nothing to extend */
> +		return 0;
> +
> +	chan->ext_info = axi_dac_ext_info;
> +
> +	return 0;
> +}

> +static int axi_dac_set_sample_rate(struct iio_backend *back, unsigned int chan,
> +				   u64 sample_rate)
> +{
> +	struct axi_dac_state *st = iio_backend_get_priv(back);
> +	unsigned int freq;
> +	int ret, tone;
> +
> +	if (!sample_rate)
> +		return -EINVAL;
> +	if (st->reg_config & AXI_DDS_DISABLE)
> +		/* nothing to care if DDS is disabled */
Rephrase this.  Is the point that the sample rate has no meaning without DDS or
that it has meaning but nothing to do here?
> +		return 0;
> +
> +	guard(mutex)(&st->lock);
> +	/*
> +	 * If dac_clk is 0 then this must be the first time we're being notified
> +	 * about the interface sample rate. Hence, just update our internal
> +	 * variable and bail... If it's not 0, then we get the current DDS
> +	 * frequency (for the old rate) and update the registers for the new
> +	 * sample rate.
> +	 */
> +	if (!st->dac_clk) {
> +		st->dac_clk = sample_rate;
> +		return 0;
> +	}
> +
> +	for (tone = 0; tone <= AXI_DAC_FREQ_TONE_2; tone++) {
> +		ret = __axi_dac_frequency_get(st, chan, tone, &freq);
> +		if (ret)
> +			return ret;
> +
> +		ret = __axi_dac_frequency_set(st, chan, sample_rate, tone, freq);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	st->dac_clk = sample_rate;
> +
> +	return 0;
> +}