| Message ID | 20241014-wip-bl-ad3552r-axi-v0-iio-testing-v6-7-eeef0c1e0e56@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: add support for the ad3552r AXI DAC IP | expand |
On 10/14/24 5:08 AM, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Add High Speed ad3552r platform driver. > ... > +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; > + > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: { > + int sclk; > + > + ret = iio_backend_read_raw(st->back, chan, &sclk, 0, > + IIO_CHAN_INFO_FREQUENCY); FWIW, this still seems like an odd way to get the stream mode SCLK rate from the backend to me. How does the backend know that we want the stream mode clock rate and not some other frequency value? > + if (ret != IIO_VAL_INT) > + return -EINVAL; > + > + /* Using 4 lanes (QSPI) */ > + *val = DIV_ROUND_CLOSEST(sclk * 4 * (1 + st->ddr_mode), Since DDR is always enabled for buffered reads, I think we should always be multiplying by 2 here instead of (1 + st->ddr_mode). Otherwise the sampling frequency attribute will return the wrong value if it is read when a buffered read is not currently in progress. > + chan->scan_type.storagebits); It would probably be more correct to use realbits here instead of storagebits. Usually realbits is the bits per word being sent over the SPI bus while storagebits can be larger. > + > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_RAW: > + ret = st->data->bus_reg_read(st->back, > + AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), > + val, 2); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + ... > +static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_hs_state *st = iio_priv(indio_dev); > + struct iio_backend_data_fmt fmt = { > + .type = IIO_BACKEND_DATA_UNSIGNED > + }; > + int loop_len, val, err; > + > + /* Inform DAC chip to switch into DDR mode */ > + err = ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + AD3552R_MASK_SPI_CONFIG_DDR, 1); > + if (err) > + return err; > + > + /* Inform DAC IP to go for DDR mode from now on */ > + err = iio_backend_ddr_enable(st->back); > + if (err) { > + dev_warn(st->dev, "could not set DDR mode, not streaming"); > + goto exit_err; > + } > + > + st->ddr_mode = true; > + > + switch (*indio_dev->active_scan_mask) { > + case AD3552R_CH0_ACTIVE: > + st->single_channel = true; > + loop_len = 2; > + val = AD3552R_REG_ADDR_CH_DAC_16B(0); > + break; > + case AD3552R_CH1_ACTIVE: > + st->single_channel = true; > + loop_len = 2; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + case AD3552R_CH0_CH1_ACTIVE: > + st->single_channel = false; > + loop_len = 4; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + default: > + err = -EINVAL; > + goto exit_err_ddr; > + } > + > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > + loop_len, 1); It would be more logical to set this before switching to DDR mode. No need to do a register write with DDR enabled. (And would be necessary for Jonathan's hypothetical 2-SPI-controller backend.) ;-) > + if (err) > + goto exit_err_ddr; > + > + err = iio_backend_data_transfer_addr(st->back, val); > + if (err) > + goto exit_err_ddr; > + > + err = iio_backend_data_format_set(st->back, 0, &fmt); > + if (err) > + goto exit_err_ddr; > + > + err = iio_backend_data_stream_enable(st->back); > + if (err) > + goto exit_err_ddr; > + > + return 0; > + > +exit_err_ddr: > + iio_backend_ddr_disable(st->back); Does it actually work in this order? In ad3552r_hs_buffer_predisable() we call ad3552r_qspi_update_reg_bits() first and then iio_backend_ddr_disable(). If DDR affects register writes, then it seems like disabling DDR in the backend first would cause the register write to disable DDR on the DAC to fail. So the order in ad3552r_hs_buffer_predisable() seems correct. Probably can just drop this part and change all goto exit_err_ddr; to goto exit_err;. > + > +exit_err: > + ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + > + iio_backend_ddr_disable(st->back); > + > + st->ddr_mode = false; > + > + return err; > +} > + > +static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_hs_state *st = iio_priv(indio_dev); > + int err; > + > + err = iio_backend_data_stream_disable(st->back); > + if (err) > + return err; > + > + /* Inform DAC to set in SDR mode */ > + err = ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + if (err) > + return err; > + > + err = iio_backend_ddr_disable(st->back); > + if (err) > + return err; > + > + st->ddr_mode = false; > + > + return 0; > +} > + ... > + > +#define AD3552R_CHANNEL(ch) { \ > + .type = IIO_VOLTAGE, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ As described in [1], the sampling freq attr should be info_mask_separate since it is the sample rate per individual sample, not the sample rate per scan. [1]: https://lore.kernel.org/linux-iio/20240908164940.7c4ffb8a@jic23-huawei/ > + .output = 1, \ > + .indexed = 1, \ > + .channel = (ch), \ > + .scan_index = (ch), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + } \ > +} > +
On Mon, 2024-10-14 at 16:15 -0500, David Lechner wrote: > On 10/14/24 5:08 AM, Angelo Dureghello wrote: > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > Add High Speed ad3552r platform driver. > > > > ... > > > +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; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_SAMP_FREQ: { > > + int sclk; > > + > > + ret = iio_backend_read_raw(st->back, chan, &sclk, 0, > > + IIO_CHAN_INFO_FREQUENCY); > > FWIW, this still seems like an odd way to get the stream mode SCLK > rate from the backend to me. How does the backend know that we want > the stream mode clock rate and not some other frequency value? In this case the backend has a dedicated compatible so sky is the limit :). But yeah, I'm also not extremely happy with IIO_CHAN_INFO_FREQUENCY. But what do you have in mind? Using the sampling frequency INFO or a dedicated OP? > > > + if (ret != IIO_VAL_INT) > > + return -EINVAL; > > + > > + /* Using 4 lanes (QSPI) */ > > + *val = DIV_ROUND_CLOSEST(sclk * 4 * (1 + st->ddr_mode), > > Since DDR is always enabled for buffered reads, I think we should > always be multiplying by 2 here instead of (1 + st->ddr_mode). > > Otherwise the sampling frequency attribute will return the wrong > value if it is read when a buffered read is not currently in > progress. > > > + chan->scan_type.storagebits); > > It would probably be more correct to use realbits here instead of > storagebits. Usually realbits is the bits per word being sent over > the SPI bus while storagebits can be larger. It can go both ways I guess. For channels with eg: status bits in the sample, realbits won't be the exact word on the bus. OTOH, yes, we do have cases for DMA buffering where storage bits is bigger that the actual word. So, yeah, no strong feeling about it. - Nuno Sá
On Mon, 2024-10-14 at 12:08 +0200, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > 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@baylibre.com> > --- Hi Angelo, Some more questions from me on top of David's review... > drivers/iio/dac/Kconfig | 14 ++ > drivers/iio/dac/Makefile | 1 + > drivers/iio/dac/ad3552r-hs.c | 526 +++++++++++++++++++++++++++++++++++++++++++ > drivers/iio/dac/ad3552r-hs.h | 18 ++ > drivers/iio/dac/ad3552r.h | 7 + > 5 files changed, 566 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..cb29a600e141 > --- /dev/null > +++ b/drivers/iio/dac/ad3552r-hs.c > @@ -0,0 +1,526 @@ > +// 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_hs_platform_data *data; > + bool ddr_mode; > +}; > + > +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 err; Be consistent. You have a mixture of err and ret. Personally, slight preference for 'ret'. > + > + err = st->data->bus_reg_read(st->back, reg, &rval, xfer_size); > + if (err) > + return err; > + > + rval &= ~mask; > + rval |= val; > + nit: can be done in one liner... > + 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; > + > + 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; > + > + /* Using 4 lanes (QSPI) */ > + *val = DIV_ROUND_CLOSEST(sclk * 4 * (1 + st->ddr_mode), > + chan->scan_type.storagebits); If we assume ddr always on, don't forget to put that in a comment. In fact, please say that the sampling frequency is only about stream mode (buffering) on. > + > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_RAW: > + ret = st->data->bus_reg_read(st->back, > + AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), > + val, 2); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; Hmm, I think there's an important question here. How useful it is to have "just" raw writes? I don't think there's anything preventing us from supporting SCALE and OFFSET as the SPI driver? Those are important pieces for useland to be able to compute the peak voltage level, right? Or am I missing something? > + default: > + return -EINVAL; > + } > +} > + > +static int ad3552r_hs_write_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); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + return st->data->bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), > + val, 2); > + } > + unreachable(); > + default: > + return -EINVAL; > + } > +} > + > +static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_hs_state *st = iio_priv(indio_dev); > + struct iio_backend_data_fmt fmt = { > + .type = IIO_BACKEND_DATA_UNSIGNED > + }; > + int loop_len, val, err; > + > + /* Inform DAC chip to switch into DDR mode */ > + err = ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + AD3552R_MASK_SPI_CONFIG_DDR, 1); > + if (err) > + return err; > + > + /* Inform DAC IP to go for DDR mode from now on */ > + err = iio_backend_ddr_enable(st->back); > + if (err) { > + dev_warn(st->dev, "could not set DDR mode, not streaming"); To me, this is an error so I would treat it as such. dev_err() > + goto exit_err; > + } > + > + st->ddr_mode = true; > + > + switch (*indio_dev->active_scan_mask) { > + case AD3552R_CH0_ACTIVE: > + st->single_channel = true; > + loop_len = 2; > + val = AD3552R_REG_ADDR_CH_DAC_16B(0); > + break; > + case AD3552R_CH1_ACTIVE: > + st->single_channel = true; > + loop_len = 2; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + case AD3552R_CH0_CH1_ACTIVE: > + st->single_channel = false; > + loop_len = 4; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + default: > + err = -EINVAL; > + goto exit_err_ddr; > + } > + > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > + loop_len, 1); > + if (err) > + goto exit_err_ddr; > + > + err = iio_backend_data_transfer_addr(st->back, val); > + if (err) > + goto exit_err_ddr; > + > + err = iio_backend_data_format_set(st->back, 0, &fmt); > + if (err) > + goto exit_err_ddr; > + > + err = iio_backend_data_stream_enable(st->back); > + if (err) > + goto exit_err_ddr; > + > + return 0; > + > +exit_err_ddr: > + iio_backend_ddr_disable(st->back); > + > +exit_err: > + ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + > + iio_backend_ddr_disable(st->back); > + > + st->ddr_mode = false; 'ddr_mode' is pretty much used for the sampling freq, right? If we go the way of just reporting the buffering sampling freq, I guess you can drop this variable. > + > + return err; > +} > + > +static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_hs_state *st = iio_priv(indio_dev); > + int err; > + > + err = iio_backend_data_stream_disable(st->back); > + if (err) > + return err; > + > + /* Inform DAC to set in SDR mode */ > + err = ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + if (err) > + return err; > + > + err = iio_backend_ddr_disable(st->back); > + if (err) > + return err; > + > + st->ddr_mode = false; > + > + return 0; > +} > + > +static int ad3552r_hs_set_output_range(struct ad3552r_hs_state *st, > + unsigned int mode) > +{ > + return ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE, > + AD3552R_MASK_CH_OUTPUT_RANGE, > + FIELD_PREP(AD3552R_MASK_CH0_RANGE, mode) | > + FIELD_PREP(AD3552R_MASK_CH1_RANGE, mode), > + 1); I think you only call this function once, right? I would do this inline FWIW... > +} > + > +static int ad3552r_hs_reset(struct ad3552r_hs_state *st) > +{ > + int err; > + > + st->reset_gpio = devm_gpiod_get_optional(st->dev, > + "reset", GPIOD_OUT_LOW); > + if (IS_ERR(st->reset_gpio)) > + return PTR_ERR(st->reset_gpio); > + I suspect you actually want GPIOD_OUT_HIGH? Assuming the reset is active low, you need to properly set it as such in DT. Then gpiolib will take care of things for you. Note that, GPIOD_OUT_HIGH means "give me the pin in the asserted state". So if it's active low, then it will be effectively be low. > + if (st->reset_gpio) { > + fsleep(10); > + gpiod_set_value_cansleep(st->reset_gpio, 1); Here you want to bring it out of reset and so de-assert the pin: gpiod_set_value_cansleep(st->reset_gpio, 0); Again, as long as you set the pin as active low in DT, gpiolib will negate the value for you internally. > + } else { > + err = ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_A, > + AD3552R_MASK_SOFTWARE_RESET, > + AD3552R_MASK_SOFTWARE_RESET, 1); > + if (err) > + return err; > + } > + msleep(100); nit: fsleep() > + > + return 0; > +} > + > +static int ad3552r_hs_scratch_pad_test(struct ad3552r_hs_state *st) > +{ > + int err, val; > + > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + AD3552R_SCRATCH_PAD_TEST_VAL1, 1); > + if (err) > + return err; > + > + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + &val, 1); > + if (err) > + return err; > + > + if (val != AD3552R_SCRATCH_PAD_TEST_VAL1) { > + dev_err(st->dev, > + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", > + AD3552R_SCRATCH_PAD_TEST_VAL1, val); > + return -EIO; This is in probing right? dev_err_probe() > + } > + > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + AD3552R_SCRATCH_PAD_TEST_VAL2, 1); > + if (err) > + return err; > + > + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + &val, 1); > + if (err) > + return err; > + > + if (val != AD3552R_SCRATCH_PAD_TEST_VAL2) { > + dev_err(st->dev, > + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", > + AD3552R_SCRATCH_PAD_TEST_VAL2, val); > + return -EIO; ditto > + } > + > + return 0; > +} > + > +static int ad3552r_hs_setup_custom_gain(struct ad3552r_hs_state *st, > + u16 gain, u16 offset) > +{ > + int err; > + > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_OFFSET(0), > + offset, 1); > + if (err) > + return dev_err_probe(st->dev, err, "Error writing register\n"); > + > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_OFFSET(1), > + offset, 1); > + if (err) > + return dev_err_probe(st->dev, err, "Error writing register\n"); > + > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_GAIN(0), > + gain, 1); > + if (err) > + return dev_err_probe(st->dev, err, "Error writing register\n"); > + > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_GAIN(1), > + gain, 1); > + if (err) > + return dev_err_probe(st->dev, err, "Error writing register\n"); > + > + return 0; > +} > + > +static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > +{ > + u8 gs_p, gs_n; > + s16 goffs; > + u16 id, rfb; > + u16 gain = 0, offset = 0; > + u32 val, range; > + int err; > + > + err = ad3552r_hs_reset(st); > + if (err) > + return err; > + > + err = iio_backend_ddr_disable(st->back); > + if (err) > + return err; > + > + err = ad3552r_hs_scratch_pad_test(st); > + if (err) > + return err; > + > + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_L, > + &val, 1); > + if (err) > + return err; > + > + id = val; > + > + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_H, > + &val, 1); > + if (err) > + return err; > + > + id |= val << 8; > + if (id != st->model_data->chip_id) > + dev_info(st->dev, "Chip ID error. Expected 0x%x, Read 0x%x\n", > + AD3552R_ID, id); > + > + err = st->data->bus_reg_write(st->back, > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, > + 0, 1); > + if (err) > + return err; > + > + err = st->data->bus_reg_write(st->back, > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > + AD3552R_MASK_QUAD_SPI | > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > + if (err) > + return err; > + > + err = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); > + if (err) > + return err; > + > + err = iio_backend_data_source_set(st->back, 1, IIO_BACKEND_EXTERNAL); > + if (err) > + return err; > + > + err = ad3552r_get_ref_voltage(st->dev); > + if (err < 0) > + return err; > + > + val = err; Then, 'err' is not an error. I don't really like of mixing return values (errors) with values. Please pass the value as a pointer argument to the function. > + > + err = ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, > + AD3552R_MASK_REFERENCE_VOLTAGE_SEL, > + val, 1); > + if (err) > + return err; > + > + err = ad3552r_get_drive_strength(st->dev, &val); > + if (!err) { > + err = ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SDO_DRIVE_STRENGTH, > + val, 1); > + if (err) > + return err; > + } > + > + struct fwnode_handle *child __free(fwnode_handle) = > + device_get_named_child_node(st->dev, "channel"); > + if (!child) > + return -EINVAL; > + > + /* > + * One of "adi,output-range-microvolt" or "custom-output-range-config" > + * must be available in fdt. > + */ > + err = ad3552r_get_output_range(st->dev, st->model_data, child, &range); > + if (!err) > + return ad3552r_hs_set_output_range(st, range); > + if (err != -ENOENT) > + return err; It seems to me you're already getting the span values so it should be possible to export them via sysfs as the spi driver, right? > + > + err = ad3552r_get_custom_gain(st->dev, child, &gs_p, &gs_n, &rfb, > + &goffs); > + if (err) > + return err; > + > + gain = ad3552r_calc_custom_gain(gs_p, gs_n, goffs); > + offset = abs(goffs); > + > + return ad3552r_hs_setup_custom_gain(st, gain, offset); > +} > + > +static const struct iio_buffer_setup_ops ad3552r_hs_buffer_setup_ops = { > + .postenable = ad3552r_hs_buffer_postenable, > + .predisable = ad3552r_hs_buffer_predisable, > +}; > + > +#define AD3552R_CHANNEL(ch) { \ > + .type = IIO_VOLTAGE, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = (ch), \ > + .scan_index = (ch), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + } \ > +} > + > +static const struct iio_chan_spec ad3552r_hs_channels[] = { > + AD3552R_CHANNEL(0), > + AD3552R_CHANNEL(1), > +}; > + > +static const struct iio_info ad3552r_hs_info = { > + .read_raw = &ad3552r_hs_read_raw, > + .write_raw = &ad3552r_hs_write_raw, > +}; > + > +static int ad3552r_hs_probe(struct platform_device *pdev) > +{ > + struct ad3552r_hs_state *st; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st)); > + if (!indio_dev) > + return -ENOMEM; > + > + st = iio_priv(indio_dev); > + st->dev = &pdev->dev; > + > + st->data = pdev->dev.platform_data; > + if (!st->data) > + dev_err_probe(st->dev, -ENODEV, "No platform data !"); return dev_err_probe() > + > + st->back = devm_iio_backend_get(&pdev->dev, NULL); > + if (IS_ERR(st->back)) > + return PTR_ERR(st->back); > + > + ret = devm_iio_backend_enable(&pdev->dev, st->back); > + if (ret) > + return ret; > + > + st->model_data = device_get_match_data(&pdev->dev); error handling... if (!st->model_data) return -ENODEV; (or -EINVAL) - it seems there's no consensus in what to return here. - Nuno Sá
On 10/15/24 1:37 AM, Nuno Sá wrote: > On Mon, 2024-10-14 at 16:15 -0500, David Lechner wrote: >> On 10/14/24 5:08 AM, Angelo Dureghello wrote: >>> From: Angelo Dureghello <adureghello@baylibre.com> >>> >>> Add High Speed ad3552r platform driver. >>> >> >> ... >> >>> +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; >>> + >>> + switch (mask) { >>> + case IIO_CHAN_INFO_SAMP_FREQ: { >>> + int sclk; >>> + >>> + ret = iio_backend_read_raw(st->back, chan, &sclk, 0, >>> + IIO_CHAN_INFO_FREQUENCY); >> >> FWIW, this still seems like an odd way to get the stream mode SCLK >> rate from the backend to me. How does the backend know that we want >> the stream mode clock rate and not some other frequency value? > > In this case the backend has a dedicated compatible so sky is the limit :). But yeah, > I'm also not extremely happy with IIO_CHAN_INFO_FREQUENCY. But what do you have in > mind? Using the sampling frequency INFO or a dedicated OP? > It think it would be most straightforward to have something like a iio_backend_get_data_stream_clock_rate() callback since that is what we are getting. Re: the other recent discussions about getting too many callbacks. Instead of a dedicated function like this, we could make a set of generic functions: iio_backend_{g,s}et_property_{s,u}(8, 16, 32, 64}() that take an enum parameter for the property. This way, for each new property, we just have to add an enum member instead of creating a get/set callback pair. Unrelated to this particular case, but taking the idea even farther, we could also do the same with enable/disable functions. We talked before about cutting the number of callbacks in half by using a bool parameter instead of separate enable/disable callbacks. But we could cut it down even more by having an enum parameter for the thing we are enabling/disabling.
On 10/15/24 2:15 AM, Nuno Sá wrote: > On Mon, 2024-10-14 at 12:08 +0200, Angelo Dureghello wrote: >> From: Angelo Dureghello <adureghello@baylibre.com> ... >> + } else { >> + err = ad3552r_qspi_update_reg_bits(st, >> + AD3552R_REG_ADDR_INTERFACE_CONFIG_A, >> + AD3552R_MASK_SOFTWARE_RESET, >> + AD3552R_MASK_SOFTWARE_RESET, 1); >> + if (err) >> + return err; >> + } >> + msleep(100); > > nit: fsleep() > fsleep() is microseconds, but we really do want milliseconds here. Datasheet t_18 is 100 ms. (Internally, fsleep() calls msleep() for anything over 20 ms anyway so makes more sense to just call msleep() directly and avoid 3 extra 0s.)
On Tue, 2024-10-15 at 09:38 -0500, David Lechner wrote: > On 10/15/24 1:37 AM, Nuno Sá wrote: > > On Mon, 2024-10-14 at 16:15 -0500, David Lechner wrote: > > > On 10/14/24 5:08 AM, Angelo Dureghello wrote: > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > Add High Speed ad3552r platform driver. > > > > > > > > > > ... > > > > > > > +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; > > > > + > > > > + switch (mask) { > > > > + case IIO_CHAN_INFO_SAMP_FREQ: { > > > > + int sclk; > > > > + > > > > + ret = iio_backend_read_raw(st->back, chan, &sclk, 0, > > > > + IIO_CHAN_INFO_FREQUENCY); > > > > > > FWIW, this still seems like an odd way to get the stream mode SCLK > > > rate from the backend to me. How does the backend know that we want > > > the stream mode clock rate and not some other frequency value? > > > > In this case the backend has a dedicated compatible so sky is the limit :). But > > yeah, > > I'm also not extremely happy with IIO_CHAN_INFO_FREQUENCY. But what do you have > > in > > mind? Using the sampling frequency INFO or a dedicated OP? > > > > It think it would be most straightforward to have something > like a iio_backend_get_data_stream_clock_rate() callback since > that is what we are getting. Hmmm, what about exporting an actual clock? Maybe it's overkill but from a correctness point of view, seems what we should actually do :) > > Re: the other recent discussions about getting too many > callbacks. Instead of a dedicated function like this, we > could make a set of generic functions: > > iio_backend_{g,s}et_property_{s,u}(8, 16, 32, 64}() > Hmm interesting approach. I don't dislike it. Kind of a generic getter/setter thingy. We could then still have optional inline helpers that would call the generic functions with the proper enum value. > that take an enum parameter for the property. This way, > for each new property, we just have to add an enum member > instead of creating a get/set callback pair. > > Unrelated to this particular case, but taking the idea even > farther, we could also do the same with enable/disable > functions. We talked before about cutting the number of > callbacks in half by using a bool parameter instead of > separate enable/disable callbacks. But we could cut it down > even more by having an enum parameter for the thing we are > enabling/disabling. If we don't get too strict about types it could even fall into the above u8 category. Instead of lot of new simple ops we just grow an enum. - Nuno Sá
On 10/15/24 10:00 AM, Nuno Sá wrote: > On Tue, 2024-10-15 at 09:38 -0500, David Lechner wrote: >> On 10/15/24 1:37 AM, Nuno Sá wrote: >>> On Mon, 2024-10-14 at 16:15 -0500, David Lechner wrote: >>>> On 10/14/24 5:08 AM, Angelo Dureghello wrote: >>>>> From: Angelo Dureghello <adureghello@baylibre.com> >>>>> >>>>> Add High Speed ad3552r platform driver. >>>>> >>>> >>>> ... >>>> >>>>> +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; >>>>> + >>>>> + switch (mask) { >>>>> + case IIO_CHAN_INFO_SAMP_FREQ: { >>>>> + int sclk; >>>>> + >>>>> + ret = iio_backend_read_raw(st->back, chan, &sclk, 0, >>>>> + IIO_CHAN_INFO_FREQUENCY); >>>> >>>> FWIW, this still seems like an odd way to get the stream mode SCLK >>>> rate from the backend to me. How does the backend know that we want >>>> the stream mode clock rate and not some other frequency value? >>> >>> In this case the backend has a dedicated compatible so sky is the limit :). But >>> yeah, >>> I'm also not extremely happy with IIO_CHAN_INFO_FREQUENCY. But what do you have >>> in >>> mind? Using the sampling frequency INFO or a dedicated OP? >>> >> >> It think it would be most straightforward to have something >> like a iio_backend_get_data_stream_clock_rate() callback since >> that is what we are getting. > > Hmmm, what about exporting an actual clock? Maybe it's overkill but from a > correctness point of view, seems what we should actually do :) Does seem overkill to me. I wouldn't do it. > >> >> Re: the other recent discussions about getting too many >> callbacks. Instead of a dedicated function like this, we >> could make a set of generic functions: >> >> iio_backend_{g,s}et_property_{s,u}(8, 16, 32, 64}() >> > > Hmm interesting approach. I don't dislike it. Kind of a generic getter/setter thingy. > We could then still have optional inline helpers that would call the generic > functions with the proper enum value. > >> that take an enum parameter for the property. This way, >> for each new property, we just have to add an enum member >> instead of creating a get/set callback pair. >> >> Unrelated to this particular case, but taking the idea even >> farther, we could also do the same with enable/disable >> functions. We talked before about cutting the number of >> callbacks in half by using a bool parameter instead of >> separate enable/disable callbacks. But we could cut it down >> even more by having an enum parameter for the thing we are >> enabling/disabling. > > If we don't get too strict about types it could even fall into the above u8 category. > > Instead of lot of new simple ops we just grow an enum. Sure. For that matter, maybe try to just stick with 32-bit for everything to keep it simple. Probably will eventually need 64-bit for some things, but might be able to get away with avoiding 8 and 16-bit. > > - Nuno Sá >
On Tue, 2024-10-15 at 10:23 -0500, David Lechner wrote: > On 10/15/24 10:00 AM, Nuno Sá wrote: > > On Tue, 2024-10-15 at 09:38 -0500, David Lechner wrote: > > > On 10/15/24 1:37 AM, Nuno Sá wrote: > > > > On Mon, 2024-10-14 at 16:15 -0500, David Lechner wrote: > > > > > On 10/14/24 5:08 AM, Angelo Dureghello wrote: > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > > > Add High Speed ad3552r platform driver. > > > > > > > > > > > > > > > > ... > > > > > > > > > > > +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; > > > > > > + > > > > > > + switch (mask) { > > > > > > + case IIO_CHAN_INFO_SAMP_FREQ: { > > > > > > + int sclk; > > > > > > + > > > > > > + ret = iio_backend_read_raw(st->back, chan, &sclk, 0, > > > > > > + IIO_CHAN_INFO_FREQUENCY); > > > > > > > > > > FWIW, this still seems like an odd way to get the stream mode SCLK > > > > > rate from the backend to me. How does the backend know that we want > > > > > the stream mode clock rate and not some other frequency value? > > > > > > > > In this case the backend has a dedicated compatible so sky is the limit :). > > > > But > > > > yeah, > > > > I'm also not extremely happy with IIO_CHAN_INFO_FREQUENCY. But what do you > > > > have > > > > in > > > > mind? Using the sampling frequency INFO or a dedicated OP? > > > > > > > > > > It think it would be most straightforward to have something > > > like a iio_backend_get_data_stream_clock_rate() callback since > > > that is what we are getting. > > > > Hmmm, what about exporting an actual clock? Maybe it's overkill but from a > > correctness point of view, seems what we should actually do :) > > Does seem overkill to me. I wouldn't do it. > Yes it is. But to me (now that I slept on the matter) a new backend OP is also not the way to go (or at least not coherent). We already have .bus_reg_read() and .bus_reg_write() shared through the platform_data 'struct ad3552r_hs_platform_data' interface. Well, in reality we're asking for the bus clock here so better to add a .bus_clock() to that struct. And since (it seems) we are going the path of just caring about the high speed rate, we might as well just make it a variable for simplicity. > > > > > > > > Re: the other recent discussions about getting too many > > > callbacks. Instead of a dedicated function like this, we > > > could make a set of generic functions: > > > > > > iio_backend_{g,s}et_property_{s,u}(8, 16, 32, 64}() > > > > > > > Hmm interesting approach. I don't dislike it. Kind of a generic getter/setter > > thingy. > > We could then still have optional inline helpers that would call the generic > > functions with the proper enum value. > > > > > that take an enum parameter for the property. This way, > > > for each new property, we just have to add an enum member > > > instead of creating a get/set callback pair. > > > > > > Unrelated to this particular case, but taking the idea even > > > farther, we could also do the same with enable/disable > > > functions. We talked before about cutting the number of > > > callbacks in half by using a bool parameter instead of > > > separate enable/disable callbacks. But we could cut it down > > > even more by having an enum parameter for the thing we are > > > enabling/disabling. > > > > If we don't get too strict about types it could even fall into the above u8 > > category. > > > > Instead of lot of new simple ops we just grow an enum. > > Sure. For that matter, maybe try to just stick with 32-bit > for everything to keep it simple. Probably will eventually > need 64-bit for some things, but might be able to get away > with avoiding 8 and 16-bit. > Agreed. Anyways, nothing that I will take care in the near future (I would first like for things to stabilize a bit). That said, if you want (or anybody else), feel free to send the patches :) - Nuno Sá
On 15.10.2024 17:00, Nuno Sá wrote: > On Tue, 2024-10-15 at 09:38 -0500, David Lechner wrote: > > On 10/15/24 1:37 AM, Nuno Sá wrote: > > > On Mon, 2024-10-14 at 16:15 -0500, David Lechner wrote: > > > > On 10/14/24 5:08 AM, Angelo Dureghello wrote: > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > Add High Speed ad3552r platform driver. > > > > > > > > > > > > > ... > > > > > > > > > +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; > > > > > + > > > > > + switch (mask) { > > > > > + case IIO_CHAN_INFO_SAMP_FREQ: { > > > > > + int sclk; > > > > > + > > > > > + ret = iio_backend_read_raw(st->back, chan, &sclk, 0, > > > > > + IIO_CHAN_INFO_FREQUENCY); > > > > > > > > FWIW, this still seems like an odd way to get the stream mode SCLK > > > > rate from the backend to me. How does the backend know that we want > > > > the stream mode clock rate and not some other frequency value? > > > > > > In this case the backend has a dedicated compatible so sky is the limit :). But > > > yeah, > > > I'm also not extremely happy with IIO_CHAN_INFO_FREQUENCY. But what do you have > > > in > > > mind? Using the sampling frequency INFO or a dedicated OP? > > > > > > > It think it would be most straightforward to have something > > like a iio_backend_get_data_stream_clock_rate() callback since > > that is what we are getting. > > Hmmm, what about exporting an actual clock? Maybe it's overkill but from a > correctness point of view, seems what we should actually do :) > > > > > Re: the other recent discussions about getting too many > > callbacks. Instead of a dedicated function like this, we > > could make a set of generic functions: > > > > iio_backend_{g,s}et_property_{s,u}(8, 16, 32, 64}() > > > > Hmm interesting approach. I don't dislike it. Kind of a generic getter/setter thingy. > We could then still have optional inline helpers that would call the generic > functions with the proper enum value. > > > that take an enum parameter for the property. This way, > > for each new property, we just have to add an enum member > > instead of creating a get/set callback pair. > > > > Unrelated to this particular case, but taking the idea even > > farther, we could also do the same with enable/disable > > functions. We talked before about cutting the number of > > callbacks in half by using a bool parameter instead of > > separate enable/disable callbacks. But we could cut it down > > even more by having an enum parameter for the thing we are > > enabling/disabling. > > If we don't get too strict about types it could even fall into the above u8 category. > > Instead of lot of new simple ops we just grow an enum. so a single call for all enable/disable calls. Looks good to me. What we want to do now ? So if understand, we don't like too much IIO_CHAN_INFO_FREQUENCY but at the same time, we don't want to have several new calls in the backend proposing a design change at this stage, where the patch was (likely) in a good shape. What about to simply add a IIO_CHAN_INFO_BUS_CLK or similar ? > > - Nuno Sá >
On Tue, 2024-10-15 at 09:48 -0500, David Lechner wrote: > On 10/15/24 2:15 AM, Nuno Sá wrote: > > On Mon, 2024-10-14 at 12:08 +0200, Angelo Dureghello wrote: > > > From: Angelo Dureghello <adureghello@baylibre.com> > > ... > > > > + } else { > > > + err = ad3552r_qspi_update_reg_bits(st, > > > + AD3552R_REG_ADDR_INTERFACE_CONFIG_A, > > > + AD3552R_MASK_SOFTWARE_RESET, > > > + AD3552R_MASK_SOFTWARE_RESET, 1); > > > + if (err) > > > + return err; > > > + } > > > + msleep(100); > > > > nit: fsleep() > > > > fsleep() is microseconds, but we really do want milliseconds here. > Datasheet t_18 is 100 ms. (Internally, fsleep() calls msleep() I know. That's why the nitpick :). I just see it as a good practice... > for anything over 20 ms anyway so makes more sense to just call > msleep() directly and avoid 3 extra 0s.) > Anyways, fair enough - Nuno Sá
Hi Nuno, On 15.10.2024 09:15, Nuno Sá wrote: > On Mon, 2024-10-14 at 12:08 +0200, Angelo Dureghello wrote: > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > 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@baylibre.com> > > --- > > Hi Angelo, > > Some more questions from me on top of David's review... > > > drivers/iio/dac/Kconfig | 14 ++ > > drivers/iio/dac/Makefile | 1 + > > drivers/iio/dac/ad3552r-hs.c | 526 +++++++++++++++++++++++++++++++++++++++++++ > > drivers/iio/dac/ad3552r-hs.h | 18 ++ > > drivers/iio/dac/ad3552r.h | 7 + > > 5 files changed, 566 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..cb29a600e141 > > --- /dev/null > > +++ b/drivers/iio/dac/ad3552r-hs.c > > @@ -0,0 +1,526 @@ > > +// 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_hs_platform_data *data; > > + bool ddr_mode; > > +}; > > + > > +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 err; > > Be consistent. You have a mixture of err and ret. Personally, slight preference for > 'ret'. > > > + > > + err = st->data->bus_reg_read(st->back, reg, &rval, xfer_size); > > + if (err) > > + return err; > > + > > + rval &= ~mask; > > + rval |= val; > > + > > nit: can be done in one liner... > > > + 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; > > + > > + 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; > > + > > + /* Using 4 lanes (QSPI) */ > > + *val = DIV_ROUND_CLOSEST(sclk * 4 * (1 + st->ddr_mode), > > + chan->scan_type.storagebits); > > If we assume ddr always on, don't forget to put that in a comment. In fact, please > say that the sampling frequency is only about stream mode (buffering) on. > > > + > > + return IIO_VAL_INT; > > + } > > + case IIO_CHAN_INFO_RAW: > > + ret = st->data->bus_reg_read(st->back, > > + AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), > > + val, 2); > > + if (ret) > > + return ret; > > + > > + return IIO_VAL_INT; > > Hmm, I think there's an important question here. How useful it is to have "just" raw > writes? I don't think there's anything preventing us from supporting SCALE and OFFSET > as the SPI driver? Those are important pieces for useland to be able to compute the > peak voltage level, right? Or am I missing something? > > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int ad3552r_hs_write_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); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > > + return st->data->bus_reg_write(st->back, > > + AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), > > + val, 2); > > + } > > + unreachable(); > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) > > +{ > > + struct ad3552r_hs_state *st = iio_priv(indio_dev); > > + struct iio_backend_data_fmt fmt = { > > + .type = IIO_BACKEND_DATA_UNSIGNED > > + }; > > + int loop_len, val, err; > > + > > + /* Inform DAC chip to switch into DDR mode */ > > + err = ad3552r_qspi_update_reg_bits(st, > > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > > + AD3552R_MASK_SPI_CONFIG_DDR, > > + AD3552R_MASK_SPI_CONFIG_DDR, 1); > > + if (err) > > + return err; > > + > > + /* Inform DAC IP to go for DDR mode from now on */ > > + err = iio_backend_ddr_enable(st->back); > > + if (err) { > > + dev_warn(st->dev, "could not set DDR mode, not streaming"); > > To me, this is an error so I would treat it as such. dev_err() > > > + goto exit_err; > > + } > > + > > + st->ddr_mode = true; > > + > > + switch (*indio_dev->active_scan_mask) { > > + case AD3552R_CH0_ACTIVE: > > + st->single_channel = true; > > + loop_len = 2; > > + val = AD3552R_REG_ADDR_CH_DAC_16B(0); > > + break; > > + case AD3552R_CH1_ACTIVE: > > + st->single_channel = true; > > + loop_len = 2; > > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > > + break; > > + case AD3552R_CH0_CH1_ACTIVE: > > + st->single_channel = false; > > + loop_len = 4; > > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > > + break; > > + default: > > + err = -EINVAL; > > + goto exit_err_ddr; > > + } > > + > > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > > + loop_len, 1); > > + if (err) > > + goto exit_err_ddr; > > + > > + err = iio_backend_data_transfer_addr(st->back, val); > > + if (err) > > + goto exit_err_ddr; > > + > > + err = iio_backend_data_format_set(st->back, 0, &fmt); > > + if (err) > > + goto exit_err_ddr; > > + > > + err = iio_backend_data_stream_enable(st->back); > > + if (err) > > + goto exit_err_ddr; > > + > > + return 0; > > + > > +exit_err_ddr: > > + iio_backend_ddr_disable(st->back); > > + > > +exit_err: > > + ad3552r_qspi_update_reg_bits(st, > > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > > + AD3552R_MASK_SPI_CONFIG_DDR, > > + 0, 1); > > + > > + iio_backend_ddr_disable(st->back); > > + > > + st->ddr_mode = false; > > 'ddr_mode' is pretty much used for the sampling freq, right? If we go the way of just > reporting the buffering sampling freq, I guess you can drop this variable. > > > + > > + return err; > > +} > > + > > +static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev) > > +{ > > + struct ad3552r_hs_state *st = iio_priv(indio_dev); > > + int err; > > + > > + err = iio_backend_data_stream_disable(st->back); > > + if (err) > > + return err; > > + > > + /* Inform DAC to set in SDR mode */ > > + err = ad3552r_qspi_update_reg_bits(st, > > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > > + AD3552R_MASK_SPI_CONFIG_DDR, > > + 0, 1); > > + if (err) > > + return err; > > + > > + err = iio_backend_ddr_disable(st->back); > > + if (err) > > + return err; > > + > > + st->ddr_mode = false; > > + > > + return 0; > > +} > > + > > +static int ad3552r_hs_set_output_range(struct ad3552r_hs_state *st, > > + unsigned int mode) > > +{ > > + return ad3552r_qspi_update_reg_bits(st, > > + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE, > > + AD3552R_MASK_CH_OUTPUT_RANGE, > > + FIELD_PREP(AD3552R_MASK_CH0_RANGE, mode) | > > + FIELD_PREP(AD3552R_MASK_CH1_RANGE, mode), > > + 1); > > I think you only call this function once, right? I would do this inline FWIW... > > > +} > > + > > +static int ad3552r_hs_reset(struct ad3552r_hs_state *st) > > +{ > > + int err; > > + > > + st->reset_gpio = devm_gpiod_get_optional(st->dev, > > + "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(st->reset_gpio)) > > + return PTR_ERR(st->reset_gpio); > > + > > I suspect you actually want GPIOD_OUT_HIGH? Assuming the reset is active low, you > need to properly set it as such in DT. Then gpiolib will take care of things for you. > Note that, GPIOD_OUT_HIGH means "give me the pin in the asserted state". So if it's > active low, then it will be effectively be low. > > > + if (st->reset_gpio) { > > + fsleep(10); > > + gpiod_set_value_cansleep(st->reset_gpio, 1); > > Here you want to bring it out of reset and so de-assert the pin: > > gpiod_set_value_cansleep(st->reset_gpio, 0); > > Again, as long as you set the pin as active low in DT, gpiolib will negate the value > for you internally. > fixed all the rest. And added scale and offset (now readable from sysfs) for the 2 channels, tested that using different ranges on the 2 channels works, including custom ranges. On this point i decided to use the active-high logic (active-low as negated reset was more correct) becouse ad3552r.c is doing the same. I can use the correct active-low logic here anyway. > > + } else { > > + err = ad3552r_qspi_update_reg_bits(st, > > + AD3552R_REG_ADDR_INTERFACE_CONFIG_A, > > + AD3552R_MASK_SOFTWARE_RESET, > > + AD3552R_MASK_SOFTWARE_RESET, 1); > > + if (err) > > + return err; > > + } > > + msleep(100); > > nit: fsleep() > > > + > > + return 0; > > +} > > + > > +static int ad3552r_hs_scratch_pad_test(struct ad3552r_hs_state *st) > > +{ > > + int err, val; > > + > > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > > + AD3552R_SCRATCH_PAD_TEST_VAL1, 1); > > + if (err) > > + return err; > > + > > + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > > + &val, 1); > > + if (err) > > + return err; > > + > > + if (val != AD3552R_SCRATCH_PAD_TEST_VAL1) { > > + dev_err(st->dev, > > + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", > > + AD3552R_SCRATCH_PAD_TEST_VAL1, val); > > + return -EIO; > > This is in probing right? dev_err_probe() > > > + } > > + > > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > > + AD3552R_SCRATCH_PAD_TEST_VAL2, 1); > > + if (err) > > + return err; > > + > > + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > > + &val, 1); > > + if (err) > > + return err; > > + > > + if (val != AD3552R_SCRATCH_PAD_TEST_VAL2) { > > + dev_err(st->dev, > > + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", > > + AD3552R_SCRATCH_PAD_TEST_VAL2, val); > > + return -EIO; > > ditto > > > + } > > + > > + return 0; > > +} > > + > > +static int ad3552r_hs_setup_custom_gain(struct ad3552r_hs_state *st, > > + u16 gain, u16 offset) > > +{ > > + int err; > > + > > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_OFFSET(0), > > + offset, 1); > > + if (err) > > + return dev_err_probe(st->dev, err, "Error writing register\n"); > > + > > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_OFFSET(1), > > + offset, 1); > > + if (err) > > + return dev_err_probe(st->dev, err, "Error writing register\n"); > > + > > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_GAIN(0), > > + gain, 1); > > + if (err) > > + return dev_err_probe(st->dev, err, "Error writing register\n"); > > + > > + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_GAIN(1), > > + gain, 1); > > + if (err) > > + return dev_err_probe(st->dev, err, "Error writing register\n"); > > + > > + return 0; > > +} > > + > > +static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > > +{ > > + u8 gs_p, gs_n; > > + s16 goffs; > > + u16 id, rfb; > > + u16 gain = 0, offset = 0; > > + u32 val, range; > > + int err; > > + > > + err = ad3552r_hs_reset(st); > > + if (err) > > + return err; > > + > > + err = iio_backend_ddr_disable(st->back); > > + if (err) > > + return err; > > + > > + err = ad3552r_hs_scratch_pad_test(st); > > + if (err) > > + return err; > > + > > + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_L, > > + &val, 1); > > + if (err) > > + return err; > > + > > + id = val; > > + > > + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_H, > > + &val, 1); > > + if (err) > > + return err; > > + > > + id |= val << 8; > > + if (id != st->model_data->chip_id) > > + dev_info(st->dev, "Chip ID error. Expected 0x%x, Read 0x%x\n", > > + AD3552R_ID, id); > > + > > + err = st->data->bus_reg_write(st->back, > > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, > > + 0, 1); > > + if (err) > > + return err; > > + > > + err = st->data->bus_reg_write(st->back, > > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > > + AD3552R_MASK_QUAD_SPI | > > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > > + if (err) > > + return err; > > + > > + err = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); > > + if (err) > > + return err; > > + > > + err = iio_backend_data_source_set(st->back, 1, IIO_BACKEND_EXTERNAL); > > + if (err) > > + return err; > > + > > + err = ad3552r_get_ref_voltage(st->dev); > > + if (err < 0) > > + return err; > > + > > + val = err; > > Then, 'err' is not an error. I don't really like of mixing return values (errors) > with values. Please pass the value as a pointer argument to the function. > > > + > > + err = ad3552r_qspi_update_reg_bits(st, > > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, > > + AD3552R_MASK_REFERENCE_VOLTAGE_SEL, > > + val, 1); > > + if (err) > > + return err; > > + > > + err = ad3552r_get_drive_strength(st->dev, &val); > > + if (!err) { > > + err = ad3552r_qspi_update_reg_bits(st, > > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > > + AD3552R_MASK_SDO_DRIVE_STRENGTH, > > + val, 1); > > + if (err) > > + return err; > > + } > > + > > + struct fwnode_handle *child __free(fwnode_handle) = > > + device_get_named_child_node(st->dev, "channel"); > > + if (!child) > > + return -EINVAL; > > + > > + /* > > + * One of "adi,output-range-microvolt" or "custom-output-range-config" > > + * must be available in fdt. > > + */ > > + err = ad3552r_get_output_range(st->dev, st->model_data, child, &range); > > + if (!err) > > + return ad3552r_hs_set_output_range(st, range); > > + if (err != -ENOENT) > > + return err; > > It seems to me you're already getting the span values so it should be possible to > export them via sysfs as the spi driver, right? > > > + > > + err = ad3552r_get_custom_gain(st->dev, child, &gs_p, &gs_n, &rfb, > > + &goffs); > > + if (err) > > + return err; > > + > > + gain = ad3552r_calc_custom_gain(gs_p, gs_n, goffs); > > + offset = abs(goffs); > > + > > + return ad3552r_hs_setup_custom_gain(st, gain, offset); > > +} > > + > > +static const struct iio_buffer_setup_ops ad3552r_hs_buffer_setup_ops = { > > + .postenable = ad3552r_hs_buffer_postenable, > > + .predisable = ad3552r_hs_buffer_predisable, > > +}; > > + > > +#define AD3552R_CHANNEL(ch) { \ > > + .type = IIO_VOLTAGE, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > + .output = 1, \ > > + .indexed = 1, \ > > + .channel = (ch), \ > > + .scan_index = (ch), \ > > + .scan_type = { \ > > + .sign = 'u', \ > > + .realbits = 16, \ > > + .storagebits = 16, \ > > + .endianness = IIO_BE, \ > > + } \ > > +} > > + > > +static const struct iio_chan_spec ad3552r_hs_channels[] = { > > + AD3552R_CHANNEL(0), > > + AD3552R_CHANNEL(1), > > +}; > > + > > +static const struct iio_info ad3552r_hs_info = { > > + .read_raw = &ad3552r_hs_read_raw, > > + .write_raw = &ad3552r_hs_write_raw, > > +}; > > + > > +static int ad3552r_hs_probe(struct platform_device *pdev) > > +{ > > + struct ad3552r_hs_state *st; > > + struct iio_dev *indio_dev; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + st = iio_priv(indio_dev); > > + st->dev = &pdev->dev; > > + > > + st->data = pdev->dev.platform_data; > > + if (!st->data) > > + dev_err_probe(st->dev, -ENODEV, "No platform data !"); > > return dev_err_probe() > > > + > > + st->back = devm_iio_backend_get(&pdev->dev, NULL); > > + if (IS_ERR(st->back)) > > + return PTR_ERR(st->back); > > + > > + ret = devm_iio_backend_enable(&pdev->dev, st->back); > > + if (ret) > > + return ret; > > + > > + st->model_data = device_get_match_data(&pdev->dev); > > error handling... > > if (!st->model_data) > return -ENODEV; (or -EINVAL) - it seems there's no consensus in what to > return here. > > - Nuno Sá >
> > > > > > > > > > > Re: the other recent discussions about getting too many > > > > callbacks. Instead of a dedicated function like this, we > > > > could make a set of generic functions: > > > > > > > > iio_backend_{g,s}et_property_{s,u}(8, 16, 32, 64}() > > > > > > > > > > Hmm interesting approach. I don't dislike it. Kind of a generic getter/setter > > > thingy. > > > We could then still have optional inline helpers that would call the generic > > > functions with the proper enum value. > > > > > > > that take an enum parameter for the property. This way, > > > > for each new property, we just have to add an enum member > > > > instead of creating a get/set callback pair. > > > > > > > > Unrelated to this particular case, but taking the idea even > > > > farther, we could also do the same with enable/disable > > > > functions. We talked before about cutting the number of > > > > callbacks in half by using a bool parameter instead of > > > > separate enable/disable callbacks. But we could cut it down > > > > even more by having an enum parameter for the thing we are > > > > enabling/disabling. > > > > > > If we don't get too strict about types it could even fall into the above u8 > > > category. > > > > > > Instead of lot of new simple ops we just grow an enum. > > > > Sure. For that matter, maybe try to just stick with 32-bit > > for everything to keep it simple. Probably will eventually > > need 64-bit for some things, but might be able to get away > > with avoiding 8 and 16-bit. > > > > Agreed. Anyways, nothing that I will take care in the near future (I would first like > for things to stabilize a bit). That said, if you want (or anybody else), feel free > to send the patches :) Definitely don't do the many size versions. Large signed integers are nice and flexible. Maybe just go s64 from the start. Jonathan > > - Nuno Sá >
On Mon, 14 Oct 2024 12:08:13 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > 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@baylibre.com> Hi Angelo, You have lots of review already. Just one trivial addition from me. Jonathan > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c > new file mode 100644 > index 000000000000..cb29a600e141 > --- /dev/null > +++ b/drivers/iio/dac/ad3552r-hs.c > @@ -0,0 +1,526 @@ > +static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) > +{ > + > + switch (*indio_dev->active_scan_mask) { > + case AD3552R_CH0_ACTIVE: > + st->single_channel = true; > + loop_len = 2; > + val = AD3552R_REG_ADDR_CH_DAC_16B(0); > + break; > + case AD3552R_CH1_ACTIVE: > + st->single_channel = true; > + loop_len = 2; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + case AD3552R_CH0_CH1_ACTIVE: case AD3552R_CH0_ACTIVE | AD3552R_CH1_ACTIVE: > diff --git a/drivers/iio/dac/ad3552r.h b/drivers/iio/dac/ad3552r.h > index 088eb8ecfac6..fc00ed4c2565 100644 > --- a/drivers/iio/dac/ad3552r.h > +++ b/drivers/iio/dac/ad3552r.h > @@ -129,6 +131,11 @@ > #define AD3552R_GAIN_SCALE 1000 > #define AD3552R_LDAC_PULSE_US 100 > > +#define AD3552R_CH0_ACTIVE BIT(0) > +#define AD3552R_CH1_ACTIVE BIT(1) > +#define AD3552R_CH0_CH1_ACTIVE (AD3552R_CH0_ACTIVE | \ > + AD3552R_CH1_ACTIVE) I'd just put that one inline in the case statement. > + > #define AD3552R_MAX_RANGES 5 > #define AD3542R_MAX_RANGES 6 > >
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..cb29a600e141 --- /dev/null +++ b/drivers/iio/dac/ad3552r-hs.c @@ -0,0 +1,526 @@ +// 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_hs_platform_data *data; + bool ddr_mode; +}; + +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 err; + + err = st->data->bus_reg_read(st->back, reg, &rval, xfer_size); + if (err) + return err; + + rval &= ~mask; + rval |= 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; + + 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; + + /* Using 4 lanes (QSPI) */ + *val = DIV_ROUND_CLOSEST(sclk * 4 * (1 + st->ddr_mode), + chan->scan_type.storagebits); + + return IIO_VAL_INT; + } + case IIO_CHAN_INFO_RAW: + ret = st->data->bus_reg_read(st->back, + AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), + val, 2); + if (ret) + return ret; + + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +static int ad3552r_hs_write_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); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { + return st->data->bus_reg_write(st->back, + AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), + val, 2); + } + unreachable(); + default: + return -EINVAL; + } +} + +static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) +{ + struct ad3552r_hs_state *st = iio_priv(indio_dev); + struct iio_backend_data_fmt fmt = { + .type = IIO_BACKEND_DATA_UNSIGNED + }; + int loop_len, val, err; + + /* Inform DAC chip to switch into DDR mode */ + err = ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + AD3552R_MASK_SPI_CONFIG_DDR, + AD3552R_MASK_SPI_CONFIG_DDR, 1); + if (err) + return err; + + /* Inform DAC IP to go for DDR mode from now on */ + err = iio_backend_ddr_enable(st->back); + if (err) { + dev_warn(st->dev, "could not set DDR mode, not streaming"); + goto exit_err; + } + + st->ddr_mode = true; + + switch (*indio_dev->active_scan_mask) { + case AD3552R_CH0_ACTIVE: + st->single_channel = true; + loop_len = 2; + val = AD3552R_REG_ADDR_CH_DAC_16B(0); + break; + case AD3552R_CH1_ACTIVE: + st->single_channel = true; + loop_len = 2; + val = AD3552R_REG_ADDR_CH_DAC_16B(1); + break; + case AD3552R_CH0_CH1_ACTIVE: + st->single_channel = false; + loop_len = 4; + val = AD3552R_REG_ADDR_CH_DAC_16B(1); + break; + default: + err = -EINVAL; + goto exit_err_ddr; + } + + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, + loop_len, 1); + if (err) + goto exit_err_ddr; + + err = iio_backend_data_transfer_addr(st->back, val); + if (err) + goto exit_err_ddr; + + err = iio_backend_data_format_set(st->back, 0, &fmt); + if (err) + goto exit_err_ddr; + + err = iio_backend_data_stream_enable(st->back); + if (err) + goto exit_err_ddr; + + return 0; + +exit_err_ddr: + iio_backend_ddr_disable(st->back); + +exit_err: + ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + AD3552R_MASK_SPI_CONFIG_DDR, + 0, 1); + + iio_backend_ddr_disable(st->back); + + st->ddr_mode = false; + + return err; +} + +static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev) +{ + struct ad3552r_hs_state *st = iio_priv(indio_dev); + int err; + + err = iio_backend_data_stream_disable(st->back); + if (err) + return err; + + /* Inform DAC to set in SDR mode */ + err = ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + AD3552R_MASK_SPI_CONFIG_DDR, + 0, 1); + if (err) + return err; + + err = iio_backend_ddr_disable(st->back); + if (err) + return err; + + st->ddr_mode = false; + + return 0; +} + +static int ad3552r_hs_set_output_range(struct ad3552r_hs_state *st, + unsigned int mode) +{ + return ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE, + AD3552R_MASK_CH_OUTPUT_RANGE, + FIELD_PREP(AD3552R_MASK_CH0_RANGE, mode) | + FIELD_PREP(AD3552R_MASK_CH1_RANGE, mode), + 1); +} + +static int ad3552r_hs_reset(struct ad3552r_hs_state *st) +{ + int err; + + st->reset_gpio = devm_gpiod_get_optional(st->dev, + "reset", GPIOD_OUT_LOW); + if (IS_ERR(st->reset_gpio)) + return PTR_ERR(st->reset_gpio); + + if (st->reset_gpio) { + fsleep(10); + gpiod_set_value_cansleep(st->reset_gpio, 1); + } else { + err = ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_INTERFACE_CONFIG_A, + AD3552R_MASK_SOFTWARE_RESET, + AD3552R_MASK_SOFTWARE_RESET, 1); + if (err) + return err; + } + msleep(100); + + return 0; +} + +static int ad3552r_hs_scratch_pad_test(struct ad3552r_hs_state *st) +{ + int err, val; + + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, + AD3552R_SCRATCH_PAD_TEST_VAL1, 1); + if (err) + return err; + + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, + &val, 1); + if (err) + return err; + + if (val != AD3552R_SCRATCH_PAD_TEST_VAL1) { + dev_err(st->dev, + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", + AD3552R_SCRATCH_PAD_TEST_VAL1, val); + return -EIO; + } + + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, + AD3552R_SCRATCH_PAD_TEST_VAL2, 1); + if (err) + return err; + + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, + &val, 1); + if (err) + return err; + + if (val != AD3552R_SCRATCH_PAD_TEST_VAL2) { + dev_err(st->dev, + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", + AD3552R_SCRATCH_PAD_TEST_VAL2, val); + return -EIO; + } + + return 0; +} + +static int ad3552r_hs_setup_custom_gain(struct ad3552r_hs_state *st, + u16 gain, u16 offset) +{ + int err; + + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_OFFSET(0), + offset, 1); + if (err) + return dev_err_probe(st->dev, err, "Error writing register\n"); + + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_OFFSET(1), + offset, 1); + if (err) + return dev_err_probe(st->dev, err, "Error writing register\n"); + + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_GAIN(0), + gain, 1); + if (err) + return dev_err_probe(st->dev, err, "Error writing register\n"); + + err = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_GAIN(1), + gain, 1); + if (err) + return dev_err_probe(st->dev, err, "Error writing register\n"); + + return 0; +} + +static int ad3552r_hs_setup(struct ad3552r_hs_state *st) +{ + u8 gs_p, gs_n; + s16 goffs; + u16 id, rfb; + u16 gain = 0, offset = 0; + u32 val, range; + int err; + + err = ad3552r_hs_reset(st); + if (err) + return err; + + err = iio_backend_ddr_disable(st->back); + if (err) + return err; + + err = ad3552r_hs_scratch_pad_test(st); + if (err) + return err; + + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_L, + &val, 1); + if (err) + return err; + + id = val; + + err = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_H, + &val, 1); + if (err) + return err; + + id |= val << 8; + if (id != st->model_data->chip_id) + dev_info(st->dev, "Chip ID error. Expected 0x%x, Read 0x%x\n", + AD3552R_ID, id); + + err = st->data->bus_reg_write(st->back, + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, + 0, 1); + if (err) + return err; + + err = st->data->bus_reg_write(st->back, + AD3552R_REG_ADDR_TRANSFER_REGISTER, + AD3552R_MASK_QUAD_SPI | + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); + if (err) + return err; + + err = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); + if (err) + return err; + + err = iio_backend_data_source_set(st->back, 1, IIO_BACKEND_EXTERNAL); + if (err) + return err; + + err = ad3552r_get_ref_voltage(st->dev); + if (err < 0) + return err; + + val = err; + + err = ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, + AD3552R_MASK_REFERENCE_VOLTAGE_SEL, + val, 1); + if (err) + return err; + + err = ad3552r_get_drive_strength(st->dev, &val); + if (!err) { + err = ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + AD3552R_MASK_SDO_DRIVE_STRENGTH, + val, 1); + if (err) + return err; + } + + struct fwnode_handle *child __free(fwnode_handle) = + device_get_named_child_node(st->dev, "channel"); + if (!child) + return -EINVAL; + + /* + * One of "adi,output-range-microvolt" or "custom-output-range-config" + * must be available in fdt. + */ + err = ad3552r_get_output_range(st->dev, st->model_data, child, &range); + if (!err) + return ad3552r_hs_set_output_range(st, range); + if (err != -ENOENT) + return err; + + err = ad3552r_get_custom_gain(st->dev, child, &gs_p, &gs_n, &rfb, + &goffs); + if (err) + return err; + + gain = ad3552r_calc_custom_gain(gs_p, gs_n, goffs); + offset = abs(goffs); + + return ad3552r_hs_setup_custom_gain(st, gain, offset); +} + +static const struct iio_buffer_setup_ops ad3552r_hs_buffer_setup_ops = { + .postenable = ad3552r_hs_buffer_postenable, + .predisable = ad3552r_hs_buffer_predisable, +}; + +#define AD3552R_CHANNEL(ch) { \ + .type = IIO_VOLTAGE, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .output = 1, \ + .indexed = 1, \ + .channel = (ch), \ + .scan_index = (ch), \ + .scan_type = { \ + .sign = 'u', \ + .realbits = 16, \ + .storagebits = 16, \ + .endianness = IIO_BE, \ + } \ +} + +static const struct iio_chan_spec ad3552r_hs_channels[] = { + AD3552R_CHANNEL(0), + AD3552R_CHANNEL(1), +}; + +static const struct iio_info ad3552r_hs_info = { + .read_raw = &ad3552r_hs_read_raw, + .write_raw = &ad3552r_hs_write_raw, +}; + +static int ad3552r_hs_probe(struct platform_device *pdev) +{ + struct ad3552r_hs_state *st; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st)); + if (!indio_dev) + return -ENOMEM; + + st = iio_priv(indio_dev); + st->dev = &pdev->dev; + + st->data = pdev->dev.platform_data; + if (!st->data) + dev_err_probe(st->dev, -ENODEV, "No platform data !"); + + st->back = devm_iio_backend_get(&pdev->dev, NULL); + if (IS_ERR(st->back)) + return PTR_ERR(st->back); + + ret = devm_iio_backend_enable(&pdev->dev, st->back); + if (ret) + return ret; + + st->model_data = device_get_match_data(&pdev->dev); + + indio_dev->name = "ad3552r"; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->setup_ops = &ad3552r_hs_buffer_setup_ops; + indio_dev->channels = ad3552r_hs_channels; + indio_dev->num_channels = ARRAY_SIZE(ad3552r_hs_channels); + indio_dev->info = &ad3552r_hs_info; + + ret = devm_iio_backend_request_buffer(&pdev->dev, st->back, indio_dev); + if (ret) + return ret; + + ret = ad3552r_hs_setup(st); + if (ret) + return ret; + + return devm_iio_device_register(&pdev->dev, indio_dev); +} + +static const struct ad3552r_model_data ad3552r_model_data = { + .model_name = "ad3552r", + .chip_id = AD3552R_ID, + .num_hw_channels = 2, + .ranges_table = ad3552r_ch_ranges, + .num_ranges = ARRAY_SIZE(ad3552r_ch_ranges), +}; + +static const struct of_device_id ad3552r_hs_of_id[] = { + { .compatible = "adi,ad3552r", .data = &ad3552r_model_data }, + { } +}; +MODULE_DEVICE_TABLE(of, ad3552r_hs_of_id); + +static struct platform_driver axi_ad3552r_driver = { + .driver = { + .name = "ad3552r-axi", + .of_match_table = ad3552r_hs_of_id, + }, + .probe = ad3552r_hs_probe, +}; +module_platform_driver(axi_ad3552r_driver); + +MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@analog.com>"); +MODULE_AUTHOR("Angelo Dureghello <adueghello@baylibre.com>"); +MODULE_DESCRIPTION("AD3552R Driver - AXI IP version"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(IIO_AD3552R); diff --git a/drivers/iio/dac/ad3552r-hs.h b/drivers/iio/dac/ad3552r-hs.h new file mode 100644 index 000000000000..dbf71d5e58c1 --- /dev/null +++ b/drivers/iio/dac/ad3552r-hs.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2024 Analog Devices Inc. + * Copyright (c) 2024 Baylibre, SAS + */ +#ifndef __LINUX_PLATFORM_DATA_AD3552R_HS_H__ +#define __LINUX_PLATFORM_DATA_AD3552R_HS_H__ + +struct iio_backend; + +struct ad3552r_hs_platform_data { + int (*bus_reg_read)(struct iio_backend *back, u32 reg, u32 *val, + size_t data_size); + int (*bus_reg_write)(struct iio_backend *back, u32 reg, u32 val, + size_t data_size); +}; + +#endif /* __LINUX_PLATFORM_DATA_AD3552R_HS_H__ */ diff --git a/drivers/iio/dac/ad3552r.h b/drivers/iio/dac/ad3552r.h index 088eb8ecfac6..fc00ed4c2565 100644 --- a/drivers/iio/dac/ad3552r.h +++ b/drivers/iio/dac/ad3552r.h @@ -38,6 +38,8 @@ #define AD3552R_REG_ADDR_TRANSFER_REGISTER 0x0F #define AD3552R_MASK_MULTI_IO_MODE GENMASK(7, 6) #define AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE BIT(2) +#define AD3552R_MASK_DUAL_SPI BIT(6) +#define AD3552R_MASK_QUAD_SPI BIT(7) #define AD3552R_REG_ADDR_INTERFACE_CONFIG_C 0x10 #define AD3552R_MASK_CRC_ENABLE (GENMASK(7, 6) |\ GENMASK(1, 0)) @@ -129,6 +131,11 @@ #define AD3552R_GAIN_SCALE 1000 #define AD3552R_LDAC_PULSE_US 100 +#define AD3552R_CH0_ACTIVE BIT(0) +#define AD3552R_CH1_ACTIVE BIT(1) +#define AD3552R_CH0_CH1_ACTIVE (AD3552R_CH0_ACTIVE | \ + AD3552R_CH1_ACTIVE) + #define AD3552R_MAX_RANGES 5 #define AD3542R_MAX_RANGES 6