Hi Angelo,
Just some minor (not that big of a deal comments)
On Thu, 2024-10-24 at 17:02 +0200, Angelo Dureghello wrote:
> Hi Nuno,
>
> On 24.10.2024 15:05, Nuno Sá wrote:
> > On Tue, 2024-10-22 at 18:40 +0200, Angelo Dureghello wrote:
> > > Hi Nuno,
> > >
> > > On 22.10.2024 14:28, Nuno Sá wrote:
> > > > On Mon, 2024-10-21 at 14:40 +0200, Angelo Dureghello wrote:
> > > > > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > > > >
> > > > > Add High Speed ad3552r platform driver.
> > > > >
> > > > > The ad3552r DAC is controlled by a custom (fpga-based) DAC IP
> > > > > through the current AXI backend, or similar alternative IIO backend.
> > > > >
> > > > > Compared to the existing driver (ad3552r.c), that is a simple SPI
> > > > > driver, this driver is coupled with a DAC IIO backend that finally
> > > > > controls the ad3552r by a fpga-based "QSPI+DDR" interface, to reach
> > > > > maximum transfer rate of 33MUPS using dma stream capabilities.
> > > > >
> > > > > All commands involving QSPI bus read/write are delegated to the backend
> > > > > through the provided APIs for bus read/write.
> > > > >
> > > > > Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/iio/dac/Kconfig | 14 ++
> > > > > drivers/iio/dac/Makefile | 1 +
> > > > > drivers/iio/dac/ad3552r-hs.c | 547
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > > drivers/iio/dac/ad3552r-hs.h | 18 ++
> > > > > drivers/iio/dac/ad3552r.h | 4 +
> > > > > 5 files changed, 584 insertions(+)
> > > > >
> > > > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > > > > index fa091995d002..fc11698e88f2 100644
> > > > > --- a/drivers/iio/dac/Kconfig
> > > > > +++ b/drivers/iio/dac/Kconfig
> > > > > @@ -6,6 +6,20 @@
> > > > >
> > > > > menu "Digital to analog converters"
> > > > >
> > > > > +config AD3552R_HS
> > > > > + tristate "Analog Devices AD3552R DAC High Speed driver"
> > > > > + select ADI_AXI_DAC
> > > > > + help
> > > > > + Say yes here to build support for Analog Devices AD3552R
> > > > > + Digital to Analog Converter High Speed driver.
> > > > > +
> > > > > + The driver requires the assistance of an IP core to operate,
> > > > > + since data is streamed into target device via DMA, sent over a
> > > > > + QSPI + DDR (Double Data Rate) bus.
> > > > > +
> > > > > + To compile this driver as a module, choose M here: the
> > > > > + module will be called ad3552r-hs.
> > > > > +
> > > > > config AD3552R
> > > > > tristate "Analog Devices AD3552R DAC driver"
> > > > > depends on SPI_MASTER
> > > > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> > > > > index c92de0366238..d92e08ca93ca 100644
> > > > > --- a/drivers/iio/dac/Makefile
> > > > > +++ b/drivers/iio/dac/Makefile
> > > > > @@ -4,6 +4,7 @@
> > > > > #
> > > > >
> > > > > # When adding new entries keep the list in alphabetical order
> > > > > +obj-$(CONFIG_AD3552R_HS) += ad3552r-hs.o ad3552r-common.o
> > > > > obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o
> > > > > obj-$(CONFIG_AD5360) += ad5360.o
> > > > > obj-$(CONFIG_AD5380) += ad5380.o
> > > > > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> > > > > new file mode 100644
> > > > > index 000000000000..27bdc35fdc29
> > > > > --- /dev/null
> > > > > +++ b/drivers/iio/dac/ad3552r-hs.c
> > > > > @@ -0,0 +1,547 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/*
> > > > > + * Analog Devices AD3552R
> > > > > + * Digital to Analog converter driver, High Speed version
> > > > > + *
> > > > > + * Copyright 2024 Analog Devices Inc.
> > > > > + */
> > > > > +
> > > > > +#include <linux/bitfield.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > > +#include <linux/iio/backend.h>
> > > > > +#include <linux/iio/buffer.h>
> > > > > +#include <linux/mod_devicetable.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/property.h>
> > > > > +#include <linux/units.h>
> > > > > +
> > > > > +#include "ad3552r.h"
> > > > > +#include "ad3552r-hs.h"
> > > > > +
> > > > > +struct ad3552r_hs_state {
> > > > > + const struct ad3552r_model_data *model_data;
> > > > > + struct gpio_desc *reset_gpio;
> > > > > + struct device *dev;
> > > > > + struct iio_backend *back;
> > > > > + bool single_channel;
> > > > > + struct ad3552r_ch_data ch_data[AD3552R_MAX_CH];
> > > > > + struct ad3552r_hs_platform_data *data;
> > > > > +};
> > > > > +
> > > > > +static int ad3552r_qspi_update_reg_bits(struct ad3552r_hs_state *st,
> > > > > + u32 reg, u32 mask, u32 val,
> > > > > + size_t xfer_size)
> > > > > +{
> > > > > + u32 rval;
> > > > > + int ret;
> > > > > +
> > > > > + ret = st->data->bus_reg_read(st->back, reg, &rval, xfer_size);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + rval = (rval & ~mask) | val;
> > > > > +
> > > > > + return st->data->bus_reg_write(st->back, reg, rval, xfer_size);
> > > > > +}
> > > > > +
> > > > > +static int ad3552r_hs_read_raw(struct iio_dev *indio_dev,
> > > > > + struct iio_chan_spec const *chan,
> > > > > + int *val, int *val2, long mask)
> > > > > +{
> > > > > + struct ad3552r_hs_state *st = iio_priv(indio_dev);
> > > > > + int ret;
> > > > > + int ch = chan->channel;
> > > > > +
> > > > > + switch (mask) {
> > > > > + case IIO_CHAN_INFO_SAMP_FREQ: {
> > > > > + int sclk;
> > > > > +
> > > > > + ret = iio_backend_read_raw(st->back, chan, &sclk, 0,
> > > > > + IIO_CHAN_INFO_FREQUENCY);
> > > > > + if (ret != IIO_VAL_INT)
> > > > > + return -EINVAL;
> > > > > +
> > > >
> > > > I just saw you had some questions on v6 that everyone failed to see. See my
> > > > reply to David here:
> > > >
> > > > https://lore.kernel.org/linux-iio/61cf3072af74a8b2951c948ddc2383ba1e55954d.camel@xxxxxxxxx/
> > > >
> > > > It should be easy and it's something that makes sense (at least to me :))
> > > >
> > >
> > > I understood that we would improve things later in case.
> > >
> > > Could we maybe stay with IIO_CHAN_INFO_FREQUENCY ? It doesn't seems to me
> > > so out of scope. Sorry but i am trying to finalize someway this job,
> > > so i am trying to conatain changes now at v7, if code is not really
> > > totally wrong.
> >
> > I think you're trying to rush in the series. I can understand your frustration
> > but
> > believe me that v7 (or v8) is not so bad :).
> >
> > David already raised concerns about using IIO_CHAN_INFO_FREQUENCY. I'm also not a
> > fan
> > of it and gave you another option that should be trivial and makes sense (given
> > that
> > bus_read and write are already being done through the platform_data interface).
> > So
> > no, I don't think we're going to accept "is not really totally wrong.". IOW, We
> > want
> > it to be totally right - if such a thing exists :).
> >
> > >
>
> i changed this way, using platform_data:
>
> static int axi_dac_bus_clok(struct iio_backend *back)
If we don't have error I would change it to:
static void axi_dac_bus_clock(struct iio_backend *back, u64 *rate) - or at the very
least return u64 and not int.
But alternatively, if you want to take simplicity one step further, you can just save
a u64 bus_clock variable in your platform_data and access it directly (given that
we're only assuming the streaming rate in which case this is constant). And If we
ever have an usecase where we need more flexibility, it should be fairly staright to
bring this the bus_clock() callback.
I'm fine either way so up to you :)
- Nuno Sá
