On Mon, 2024-10-14 at 16:14 -0500, David Lechner wrote:
> On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> >
> > Extend AXI-DAC backend with new features required to interface
> > to the ad3552r DAC. Mainly, a new compatible string is added to
> > support the ad3552r-axi DAC IP, very similar to the generic DAC
> > IP but with some customizations to work with the ad3552r.
> >
> > Then, a serie of generic functions has been added to match with
>
> spelling: series
>
> > ad3552r needs. Function names has been kept generic as much as
> > possible, to allow re-utilization from other frontend drivers.
> >
> > Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > ---
>
> ...
>
> > +static int axi_dac_read_raw(struct iio_backend *back,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct axi_dac_state *st = iio_backend_get_priv(back);
> > + int err, reg;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_FREQUENCY:
> > +
> > + if (!st->info->has_dac_clk)
> > + return -EOPNOTSUPP;
> > +
> > + /*
> > + * As from ad3552r AXI IP documentation,
> > + * returning the SCLK depending on the stream mode.
> > + */
> > + err = regmap_read(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, ®);
> > + if (err)
> > + return err;
> > +
> > + if (reg & AXI_DAC_CUSTOM_CTRL_STREAM)
> > + *val = st->dac_clk_rate / 2;
> > + else
> > + *val = st->dac_clk_rate / 8;
>
> To get the DAC sample rate, we only care about the streaming mode
> rate, so this should just always be / 2 and not / 8. Otherwise
> the sampling_frequency attribute in the DAC driver will return
> the wrong value when the buffer is not enabled. We never do buffered
> writes without enabling streaming mode.
But the question then is, what do we return when streaming mode is off? Dividing by 2
in that case won't report the actual SCLK. But you do have a point and I think a very
common pattern from userspace is to first get the sampling frequency and only then
starting buffering. In this case, yes, we get the wrong sampling frequency. Bottom
line I agree with David and we should just care about returning the max sampling
frequency which is the one that apps ultimately care about.
So, I would say to divide it by 2 during probe and just return that value in here.
- Nuno Sá
>
> > +
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int axi_dac_bus_reg_write(struct iio_backend *back, u32 reg, u32 val,
> > + size_t data_size)
> > +{
> > + struct axi_dac_state *st = iio_backend_get_priv(back);
> > + int ret;
> > + u32 ival;
> > +
> > + if (data_size == sizeof(u16))
> > + ival = FIELD_PREP(AXI_DAC_CUSTOM_WR_DATA_16, val);
> > + else
> > + ival = FIELD_PREP(AXI_DAC_CUSTOM_WR_DATA_8, val);
> > +
> > + ret = regmap_write(st->regmap, AXI_DAC_CUSTOM_WR_REG, ival);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know
>
> I'm guessing these got renamed. REG_CNTRL_2 = AXI_DAC_CNTRL_2_REG
> and AXI_DAC_CNTRL_DATA_WR = AXI_DAC_CUSTOM_WR_REG?
>
> > + * the data size. So keeping data size control here only,
> > + * since data size is mandatory for the current transfer.
> > + * DDR state handled separately by specific backend calls,
> > + * generally all raw register writes are SDR.
> > + */
> > + if (data_size == sizeof(u8))
> > + ret = regmap_set_bits(st->regmap, AXI_DAC_CNTRL_2_REG,
> > + AXI_DAC_CNTRL_2_SYMB_8B);
> > + else
> > + ret = regmap_clear_bits(st->regmap, AXI_DAC_CNTRL_2_REG,
> > + AXI_DAC_CNTRL_2_SYMB_8B);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
> > + AXI_DAC_CUSTOM_CTRL_ADDRESS,
> > + FIELD_PREP(AXI_DAC_CUSTOM_CTRL_ADDRESS, reg));
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
> > + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA,
> > + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_read_poll_timeout(st->regmap,
> > + AXI_DAC_CUSTOM_CTRL_REG, ival,
> > + ival & AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA,
> > + 10, 100 * KILO);
> > + if (ret)
> > + return ret;
>
> Should we also clear AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA on timeout
> so that we don't leave things in a bad state?
>
> > +
> > + return regmap_clear_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
> > + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA);
> > +}
> > +
>
> ...
>
> > static int axi_dac_probe(struct platform_device *pdev)
> > {
> > - const unsigned int *expected_ver;
> > struct axi_dac_state *st;
> > void __iomem *base;
> > unsigned int ver;
> > @@ -566,15 +793,26 @@ static int axi_dac_probe(struct platform_device *pdev)
> > if (!st)
> > return -ENOMEM;
> >
> > - expected_ver = device_get_match_data(&pdev->dev);
> > - if (!expected_ver)
> > + st->info = device_get_match_data(&pdev->dev);
> > + if (!st->info)
> > return -ENODEV;
> >
> > - clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > + clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
>
> This will break existing users that don't have clock-names
> in the DT. It should be fine to leave it as NULL in which
> case it will get the clock at index 0 in the clocks array
> even if there is more than one clock.
Good catch...
- Nuno Sá
