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

["Nuno Sá" <noname.nuno@gmail.com>]

On Thu, 2024-03-28 at 15:35 +0000, Jonathan Cameron wrote:
> 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.
> 

Ok, will see what of the alternatives looks better.

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

ack..

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

Has no meaning as it's not used with DDS enabled...

- Nuno Sá
>