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@xxxxxxxxxx> wrote:
>
> > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
>
>
> 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á
>
