RE: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC
From: Tinaco, Mariel
Date: Mon Sep 09 2024 - 05:48:53 EST
> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Friday, September 6, 2024 8:50 AM
> To: Tinaco, Mariel <Mariel.Tinaco@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jonathan Cameron
> <jic23@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Hennerich,
> Michael <Michael.Hennerich@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>;
> Dimitri Fedrau <dima.fedrau@xxxxxxxxx>; Nuno Sá
> <noname.nuno@xxxxxxxxx>
> Subject: Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC
>
> [External]
>
> On 9/3/24 9:30 PM, Mariel Tinaco wrote:
> > The AD8460 is a “bits in, power out” high voltage, high-power,
> > high-speed driver optimized for large output current (up to ±1 A) and
> > high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into
> > capacitive loads.
> >
> > A digital engine implements user-configurable features: modes for
> > digital input, programmable supply current, and fault monitoring and
> > programmable protection settings for output current, output voltage,
> > and junction temperature. The AD8460 operates on high voltage dual
> > supplies up to ±55 V and a single low voltage supply of 5 V.
> >
> > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@xxxxxxxxxx>
> > ---
> > MAINTAINERS | 1 +
> > drivers/iio/dac/Kconfig | 13 +
> > drivers/iio/dac/Makefile | 1 +
> > drivers/iio/dac/ad8460.c | 932
> > +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 947 insertions(+)
> > create mode 100644 drivers/iio/dac/ad8460.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > e0509c9f5545..30746a3f0bbd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1326,6 +1326,7 @@ L: linux-iio@xxxxxxxxxxxxxxx
> > S: Supported
> > W: https://ez.analog.com/linux-software-drivers
> > F: Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > +F: drivers/iio/dac/ad8460.c
> >
> > ANALOG DEVICES INC AD9739a DRIVER
> > M: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index
> > 1cfd7e2a622f..fa091995d002 100644
> > --- a/drivers/iio/dac/Kconfig
> > +++ b/drivers/iio/dac/Kconfig
> > @@ -301,6 +301,19 @@ config AD7303
> > To compile this driver as module choose M here: the module will be
> called
> > ad7303.
> >
> > +config AD8460
> > + tristate "Analog Devices AD8460 DAC driver"
> > + depends on SPI
> > + select REGMAP_SPI
> > + select IIO_BUFFER
> > + select IIO_BUFFER_DMAENGINE
> > + help
> > + Say yes here to build support for Analog Devices AD8460 Digital to
> > + Analog Converters (DAC).
> > +
> > + To compile this driver as a module choose M here: the module will be
> called
> > + ad8460.
> > +
> > config AD8801
> > tristate "Analog Devices AD8801/AD8803 DAC driver"
> > depends on SPI_MASTER
> > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index
> > 2cf148f16306..621d553bd6e3 100644
> > --- a/drivers/iio/dac/Makefile
> > +++ b/drivers/iio/dac/Makefile
> > @@ -28,6 +28,7 @@ obj-$(CONFIG_AD5686_SPI) += ad5686-spi.o
> > obj-$(CONFIG_AD5696_I2C) += ad5696-i2c.o
> > obj-$(CONFIG_AD7293) += ad7293.o
> > obj-$(CONFIG_AD7303) += ad7303.o
> > +obj-$(CONFIG_AD8460) += ad8460.o
> > obj-$(CONFIG_AD8801) += ad8801.o
> > obj-$(CONFIG_AD9739A) += ad9739a.o
> > obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o diff --git
> > a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new file mode
> > 100644 index 000000000000..6428bfaf0bd7
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad8460.c
> > @@ -0,0 +1,932 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * AD8460 Waveform generator DAC Driver
> > + *
> > + * Copyright (C) 2024 Analog Devices, Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/clk.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/buffer-dma.h>
> > +#include <linux/iio/buffer-dmaengine.h> #include
> > +<linux/iio/consumer.h> #include <linux/iio/events.h> #include
> > +<linux/iio/iio.h> #include <linux/kernel.h> #include <linux/module.h>
> > +#include <linux/mod_devicetable.h> #include <linux/regmap.h> #include
> > +<linux/regulator/consumer.h> #include <linux/spi/spi.h>
> > +
> > +#define AD8460_CTRL_REG(x) (x)
> > +#define AD8460_HVDAC_DATA_WORD_LOW(x) (0x60 + (2 * (x)))
> > +#define AD8460_HVDAC_DATA_WORD_HIGH(x) (0x61 + (2 * (x)))
>
> I don't see AD8460_HVDAC_DATA_WORD_HIGH() used anywhere, so we
> could remove that and rename AD8460_HVDAC_DATA_WORD_LOW() to
> AD8460_HVDAC_DATA_WORD().
>
> > +
> > +#define AD8460_HV_RESET_MSK BIT(7)
> > +#define AD8460_HV_SLEEP_MSK BIT(4)
> > +#define AD8460_WAVE_GEN_MODE_MSK BIT(0)
> > +
> > +#define AD8460_HVDAC_SLEEP_MSK BIT(3)
> > +
> > +#define AD8460_FAULT_ARM_MSK BIT(7)
> > +#define AD8460_FAULT_LIMIT_MSK GENMASK(6, 0)
> > +
> > +#define AD8460_APG_MODE_ENABLE_MSK BIT(5)
> > +#define AD8460_PATTERN_DEPTH_MSK GENMASK(3, 0)
> > +
> > +#define AD8460_QUIESCENT_CURRENT_MSK GENMASK(7,
> 0)
> > +
> > +#define AD8460_SHUTDOWN_FLAG_MSK BIT(7)
> > +
> > +#define AD8460_DATA_BYTE_LOW_MSK GENMASK(7, 0)
> > +#define AD8460_DATA_BYTE_HIGH_MSK GENMASK(5, 0)
> > +#define AD8460_DATA_BYTE_FULL_MSK GENMASK(13, 0)
> > +
> > +#define AD8460_DEFAULT_FAULT_PROTECT 0x00
> > +#define AD8460_DATA_BYTE_WORD_LENGTH 2
> > +#define AD8460_NUM_DATA_WORDS 16
> > +#define AD8460_NOMINAL_VOLTAGE_SPAN 80
> > +#define AD8460_MIN_EXT_RESISTOR_OHMS 2000
> > +#define AD8460_MAX_EXT_RESISTOR_OHMS 20000
> > +#define AD8460_MIN_VREFIO_UV 120000
> > +#define AD8460_MAX_VREFIO_UV 1200000
> > +#define AD8460_ABS_MAX_OVERVOLTAGE_UV 55000000
> > +#define AD8460_ABS_MAX_OVERCURRENT_UA 1000000
> > +#define AD8460_MAX_OVERTEMPERATURE_MC 150000
> > +#define AD8460_MIN_OVERTEMPERATURE_MC 20000
> > +#define AD8460_CURRENT_LIMIT_CONV(x) ((x) / 15625)
> > +#define AD8460_VOLTAGE_LIMIT_CONV(x) ((x) / 1953000)
> > +#define AD8460_TEMP_LIMIT_CONV(x) (((x) + 266640) /
> 6510)
>
> It looks like there are issues with mixed tabs and spaces in the macros above
> that should be cleaned up.
>
> > +
> > +enum ad8460_fault_type {
> > + AD8460_OVERCURRENT_SRC,
> > + AD8460_OVERCURRENT_SNK,
> > + AD8460_OVERVOLTAGE_POS,
> > + AD8460_OVERVOLTAGE_NEG,
> > + AD8460_OVERTEMPERATURE,
> > +};
> > +
> > +struct ad8460_state {
> > + struct spi_device *spi;
> > + struct regmap *regmap;
> > + struct iio_channel *tmp_adc_channel;
> > + struct clk *sync_clk;
> > + /* lock to protect against multiple access to the device and shared data
> */
> > + struct mutex lock;
> > + int refio_1p2v_mv;
> > + u32 ext_resistor_ohms;
> > + /*
> > + * DMA (thus cache coherency maintenance) requires the
> > + * transfer buffers to live in their own cache lines.
> > + */
> > + u8 spi_tx_buf[2] __aligned(IIO_DMA_MINALIGN);
>
> If we make this `__le16 spi_tx_buf` instead of `u8 spi_tx_buf[2]`, then we
> don't need the unaligned access functions.
>
> > +};
> > +
> > +static int ad8460_hv_reset(struct ad8460_state *state) {
> > + int ret;
> > +
> > + ret = regmap_update_bits(state->regmap,
> AD8460_CTRL_REG(0x00),
> > + AD8460_HV_RESET_MSK,
> > + FIELD_PREP(AD8460_HV_RESET_MSK, 1));
>
> Can be simplified with regmap_set_bits().
>
> > + if (ret)
> > + return ret;
> > +
> > + fsleep(20);
> > +
> > + return regmap_update_bits(state->regmap,
> AD8460_CTRL_REG(0x00),
> > + AD8460_HV_RESET_MSK,
> > + FIELD_PREP(AD8460_HV_RESET_MSK, 0));
>
> and regmap_clear_bits().
>
> > +}
> > +
> > +static int ad8460_reset(const struct ad8460_state *state) {
> > + struct device *dev = &state->spi->dev;
> > + struct gpio_desc *reset;
> > +
> > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > + if (IS_ERR(reset))
> > + return dev_err_probe(dev, PTR_ERR(reset),
> > + "Failed to get reset gpio");
> > + if (reset) {
> > + /* minimum duration of 10ns */
> > + ndelay(10);
> > + gpiod_set_value_cansleep(reset, 1);
> > + return 0;
> > + }
> > +
> > + /* bring all registers to their default state */
> > + return regmap_write(state->regmap, AD8460_CTRL_REG(0x03), 1); }
> > +
> > +static int ad8460_enable_apg_mode(struct ad8460_state *state, int
> > +val) {
> > + int ret;
> > +
> > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02),
> > + AD8460_APG_MODE_ENABLE_MSK,
> > +
> FIELD_PREP(AD8460_APG_MODE_ENABLE_MSK, val));
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_update_bits(state->regmap,
> AD8460_CTRL_REG(0x00),
> > + AD8460_WAVE_GEN_MODE_MSK,
> > +
> FIELD_PREP(AD8460_WAVE_GEN_MODE_MSK, val)); }
> > +
> > +static int ad8460_read_shutdown_flag(struct ad8460_state *state, u64
> > +*flag) {
> > + int ret, val;
> > +
> > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x0E), &val);
> > + if (ret)
> > + return ret;
> > +
> > + *flag = FIELD_GET(AD8460_SHUTDOWN_FLAG_MSK, val);
> > + return 0;
> > +}
> > +
> > +static int ad8460_get_hvdac_word(struct ad8460_state *state, int
> > +index, int *val) {
> > + int ret;
> > +
> > + ret = regmap_bulk_read(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > + &state->spi_tx_buf,
> AD8460_DATA_BYTE_WORD_LENGTH);
> > + if (ret)
> > + return ret;
> > +
> > + *val = get_unaligned_le16(state->spi_tx_buf);
>
> With spi_tx_buf changed to __le16, this can use le16_to_cpu() instead of
> get_unaligned_le16().
>
I suppose we use the cpu_to_le16 for the set_hvdac_word function?
static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val)
{
state->spi_tx_buf = cpu_to_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val));
return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD(index),
&state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH);
}
> > +
> > + return ret;
> > +}
> > +
> > +static int ad8460_set_hvdac_word(struct ad8460_state *state, int
> > +index, int val) {
> > + put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK,
> val),
> > + &state->spi_tx_buf);
> > +
> > + return regmap_bulk_write(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > + state->spi_tx_buf,
> AD8460_DATA_BYTE_WORD_LENGTH); }
> > +
> > +static ssize_t ad8460_dac_input_read(struct iio_dev *indio_dev, uintptr_t
> private,
> > + const struct iio_chan_spec *chan, char
> *buf) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int reg;
> > + int ret;
> > +
> > + ret = ad8460_get_hvdac_word(state, private, ®);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "%d\n", reg);
>
> %u instead of %d since reg is unsigned
>
> > +}
> > +
> > +static ssize_t ad8460_dac_input_write(struct iio_dev *indio_dev, uintptr_t
> private,
> > + const struct iio_chan_spec *chan,
> > + const char *buf, size_t len) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int reg;
> > + int ret;
> > +
> > + ret = kstrtou32(buf, 10, ®);
> > + if (ret)
> > + return ret;
> > +
> > + guard(mutex)(&state->lock);
> > +
> > + return ad8460_set_hvdac_word(state, private, reg); }
> > +
> > +static ssize_t ad8460_read_symbol(struct iio_dev *indio_dev, uintptr_t
> private,
> > + const struct iio_chan_spec *chan, char *buf)
> {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int reg;
> > + int ret;
> > +
> > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "%ld\n",
> FIELD_GET(AD8460_PATTERN_DEPTH_MSK,
> > +reg)); }
>
> I guess FIELD_GET() makes it long? but probalby should be %lu.
>
> > +
> > +static ssize_t ad8460_write_symbol(struct iio_dev *indio_dev, uintptr_t
> private,
> > + const struct iio_chan_spec *chan,
> > + const char *buf, size_t len)
> > +{
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + uint16_t sym;
> > + int ret;
> > +
> > + ret = kstrtou16(buf, 10, &sym);
> > + if (ret)
> > + return ret;
> > +
> > + guard(mutex)(&state->lock);
> > +
> > + return regmap_update_bits(state->regmap,
> > + AD8460_CTRL_REG(0x02),
> > + AD8460_PATTERN_DEPTH_MSK,
> > +
> FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, sym)); }
> > +
> > +static ssize_t ad8460_read_toggle_en(struct iio_dev *indio_dev, uintptr_t
> private,
> > + const struct iio_chan_spec *chan, char
> *buf) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int reg;
> > + int ret;
> > +
> > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "%ld\n",
> > +FIELD_GET(AD8460_APG_MODE_ENABLE_MSK, reg)); }
> > +
> > +static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev, uintptr_t
> private,
> > + const struct iio_chan_spec *chan,
> > + const char *buf, size_t len) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + bool toggle_en;
> > + int ret;
> > +
> > + ret = kstrtobool(buf, &toggle_en);
> > + if (ret)
> > + return ret;
> > +
> > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> > + return ad8460_enable_apg_mode(state, toggle_en);
> > + unreachable();
>
> Hmm... do we need to make an unscoped version of
> iio_device_claim_direct_scoped()?
>
So iio_device_claim_direct_scoped is used here because the buffer enable/disable
accesses this enable_apg_mode function. Is it also a standard practice to put the
kstrobool() util inside the scope?
> > +}
> > +
> > +static ssize_t ad8460_read_powerdown(struct iio_dev *indio_dev,
> uintptr_t private,
> > + const struct iio_chan_spec *chan, char
> *buf) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int reg;
> > + int ret;
> > +
> > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x01), ®);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "%ld\n",
> FIELD_GET(AD8460_HVDAC_SLEEP_MSK,
> > +reg)); }
> > +
> > +static ssize_t ad8460_write_powerdown(struct iio_dev *indio_dev,
> uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + const char *buf, size_t len) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + bool pwr_down;
> > + u64 sdn_flag;
> > + int ret;
> > +
> > + ret = kstrtobool(buf, &pwr_down);
> > + if (ret)
> > + return ret;
> > +
> > + guard(mutex)(&state->lock);
> > +
> > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
> > + AD8460_HVDAC_SLEEP_MSK,
> > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK,
> pwr_down));
> > + if (ret)
> > + return ret;
> > +
> > + if (!pwr_down) {
> > + ret = ad8460_read_shutdown_flag(state, &sdn_flag);
> > + if (ret)
> > + return ret;
> > +
> > + if (sdn_flag) {
> > + ret = ad8460_hv_reset(state);
> > + if (ret)
> > + return ret;
> > + }
> > + }
> > +
> > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00),
> > + AD8460_HV_SLEEP_MSK,
> > + FIELD_PREP(AD8460_HV_SLEEP_MSK,
> !pwr_down));
> > + if (ret)
> > + return ret;
>
> Some comments explaining the logic in this function would be helpful.
> Without reading the datasheet, it looks like this puts it in powerdown mode
> and takes it right back out before returning.
>
> > +
> > + return len;
> > +}
> > +
> > +static const char * const ad8460_powerdown_modes[] = {
> > + "three_state",
> > +};
> > +
> > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan) {
> > + return 0;
> > +}
> > +
> > +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + unsigned int type)
> > +{
> > + return 0;
> > +}
> > +
> > +static int ad8460_set_sample(struct ad8460_state *state, int val) {
> > + int ret;
> > +
> > + ret = ad8460_enable_apg_mode(state, 1);
> > + if (ret)
> > + return ret;
> > +
> > + guard(mutex)(&state->lock);
> > + ret = ad8460_set_hvdac_word(state, 0, val);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_update_bits(state->regmap,
> > + AD8460_CTRL_REG(0x02),
> > + AD8460_PATTERN_DEPTH_MSK,
> > +
> FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0)); }
> > +
> > +static int ad8460_set_fault_threshold(struct ad8460_state *state,
> > + enum ad8460_fault_type fault,
> > + unsigned int threshold)
> > +{
> > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08
> + fault),
> > + AD8460_FAULT_LIMIT_MSK,
> > + FIELD_PREP(AD8460_FAULT_LIMIT_MSK,
> threshold)); }
> > +
> > +static int ad8460_get_fault_threshold(struct ad8460_state *state,
> > + enum ad8460_fault_type fault,
> > + unsigned int *threshold)
> > +{
> > + unsigned int val;
> > + int ret;
> > +
> > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault),
> &val);
> > + if (ret)
> > + return ret;
> > +
> > + *threshold = FIELD_GET(AD8460_FAULT_LIMIT_MSK, val);
> > +
> > + return ret;
> > +}
> > +
> > +static int ad8460_set_fault_threshold_en(struct ad8460_state *state,
> > + enum ad8460_fault_type fault, bool
> en) {
> > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08
> + fault),
> > + AD8460_FAULT_ARM_MSK,
> > + FIELD_PREP(AD8460_FAULT_ARM_MSK,
> en)); }
> > +
> > +static int ad8460_get_fault_threshold_en(struct ad8460_state *state,
> > + enum ad8460_fault_type fault, bool
> *en) {
> > + unsigned int val;
> > + int ret;
> > +
> > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault),
> &val);
> > + if (ret)
> > + return ret;
> > +
> > + *en = FIELD_GET(AD8460_FAULT_ARM_MSK, val);
> > +
> > + return 0;
> > +}
> > +
> > +static int ad8460_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int val, int val2,
> > + long mask)
> > +{
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_VOLTAGE:
> > + iio_device_claim_direct_scoped(return -EBUSY,
> indio_dev)
> > + return ad8460_set_sample(state, val);
> > + unreachable();
> > + case IIO_CURRENT:
> > + return regmap_write(state->regmap,
> AD8460_CTRL_REG(0x04),
> > +
> FIELD_PREP(AD8460_QUIESCENT_CURRENT_MSK, val));
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad8460_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec
> const *chan,
> > + int *val, int *val2, long mask) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + int data, ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_VOLTAGE:
> > + scoped_guard(mutex, &state->lock) {
> > + ret = ad8460_get_hvdac_word(state, 0,
> &data);
> > + if (ret)
> > + return ret;
> > + }
> > + *val = data;
> > + return IIO_VAL_INT;
> > + case IIO_CURRENT:
> > + ret = regmap_read(state->regmap,
> AD8460_CTRL_REG(0x04),
> > + &data);
> > + if (ret)
> > + return ret;
> > + *val = data;
> > + return IIO_VAL_INT;
> > + case IIO_TEMP:
> > + ret = iio_read_channel_raw(state->tmp_adc_channel,
> &data);
> > + if (ret)
> > + return ret;
> > + *val = data;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + *val = clk_get_rate(state->sync_clk);
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_SCALE:
> > + /*
> > + * vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V
> > + * vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V
> > + * vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V
> > + * vADJ = vCONV * (2000 / rSET) * (vREF / 1.2)
> > + * vSPAN = vADJ_MAX - vADJ_MIN
> > + * See datasheet page 49, section FULL-SCALE REDUCTION
> > + */
> > + *val = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state-
> >refio_1p2v_mv;
> > + *val2 = state->ext_resistor_ohms * 1200;
> > + return IIO_VAL_FRACTIONAL;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad8460_select_fault_type(int chan_type, enum
> > +iio_event_direction dir) {
> > + switch (chan_type) {
> > + case IIO_VOLTAGE:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return AD8460_OVERVOLTAGE_POS;
> > + case IIO_EV_DIR_FALLING:
> > + return AD8460_OVERVOLTAGE_NEG;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CURRENT:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return AD8460_OVERCURRENT_SRC;
> > + case IIO_EV_DIR_FALLING:
> > + return AD8460_OVERCURRENT_SNK;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_TEMP:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return AD8460_OVERTEMPERATURE;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad8460_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info, int val, int val2) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int fault;
> > +
> > + if (type != IIO_EV_TYPE_THRESH)
> > + return -EINVAL;
> > +
> > + if (info != IIO_EV_INFO_VALUE)
> > + return -EINVAL;
> > +
> > + fault = ad8460_select_fault_type(chan->type, dir);
> > + if (fault < 0)
> > + return fault;
> > +
> > + return ad8460_set_fault_threshold(state, fault, val); }
> > +
> > +static int ad8460_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info, int *val, int *val2)
> {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int fault;
> > +
> > + if (type != IIO_EV_TYPE_THRESH)
> > + return -EINVAL;
> > +
> > + if (info != IIO_EV_INFO_VALUE)
> > + return -EINVAL;
> > +
> > + fault = ad8460_select_fault_type(chan->type, dir);
> > + if (fault < 0)
> > + return fault;
> > +
> > + return ad8460_get_fault_threshold(state, fault, val); }
> > +
> > +static int ad8460_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir, int val) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int fault;
> > +
> > + if (type != IIO_EV_TYPE_THRESH)
> > + return -EINVAL;
> > +
> > + fault = ad8460_select_fault_type(chan->type, dir);
> > + if (fault < 0)
> > + return fault;
> > +
> > + return ad8460_set_fault_threshold_en(state, fault, val); }
> > +
> > +static int ad8460_read_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int fault;
> > + bool en;
> > + int ret;
> > +
> > + if (type != IIO_EV_TYPE_THRESH)
> > + return -EINVAL;
> > +
> > + fault = ad8460_select_fault_type(chan->type, dir);
> > + if (fault < 0)
> > + return fault;
> > +
> > + ret = ad8460_get_fault_threshold_en(state, fault, &en);
> > + if (ret)
> > + return ret;
> > +
> > + return en;
> > +}
> > +
> > +static int ad8460_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> > + unsigned int writeval, unsigned int *readval) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > +
> > + if (readval)
> > + return regmap_read(state->regmap, reg, readval);
> > +
> > + return regmap_write(state->regmap, reg, writeval); }
> > +
> > +static int ad8460_buffer_preenable(struct iio_dev *indio_dev) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > +
> > + return ad8460_enable_apg_mode(state, 0); }
> > +
> > +static int ad8460_buffer_postdisable(struct iio_dev *indio_dev) {
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > +
> > + return ad8460_enable_apg_mode(state, 1); }
> > +
> > +static const struct iio_buffer_setup_ops ad8460_buffer_setup_ops = {
> > + .preenable = &ad8460_buffer_preenable,
> > + .postdisable = &ad8460_buffer_postdisable, };
> > +
> > +static const struct iio_info ad8460_info = {
> > + .read_raw = &ad8460_read_raw,
> > + .write_raw = &ad8460_write_raw,
> > + .write_event_value = &ad8460_write_event_value,
> > + .read_event_value = &ad8460_read_event_value,
> > + .write_event_config = &ad8460_write_event_config,
> > + .read_event_config = &ad8460_read_event_config,
> > + .debugfs_reg_access = &ad8460_reg_access, };
> > +
> > +static const struct iio_enum ad8460_powerdown_mode_enum = {
> > + .items = ad8460_powerdown_modes,
> > + .num_items = ARRAY_SIZE(ad8460_powerdown_modes),
> > + .get = ad8460_get_powerdown_mode,
> > + .set = ad8460_set_powerdown_mode,
> > +};
> > +
> > +#define AD8460_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {
> \
>
> Looks like _shared is always IIO_SEPARATE, so we could omit that parameter.
>
> > + .name = _name,
> \
> > + .read = (_read), \
> > + .write = (_write), \
> > + .private = (_what), \
> > + .shared = (_shared), \
> > +}
> > +
> > +static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
> > + AD8460_CHAN_EXT_INFO("raw0", 0, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw1", 1, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw2", 2, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw3", 3, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw4", 4, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw5", 5, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw6", 6, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw7", 7, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw8", 8, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw9", 9, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw10", 10, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw11", 11, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw12", 12, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw13", 13, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw14", 14, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw15", 15, IIO_SEPARATE,
> ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("toggle_en", 0, IIO_SEPARATE,
> ad8460_read_toggle_en,
> > + ad8460_write_toggle_en),
> > + AD8460_CHAN_EXT_INFO("symbol", 0, IIO_SEPARATE,
> ad8460_read_symbol,
> > + ad8460_write_symbol),
> > + AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> ad8460_read_powerdown,
> > + ad8460_write_powerdown),
> > + IIO_ENUM("powerdown_mode", IIO_SEPARATE,
> &ad8460_powerdown_mode_enum),
> > + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> > + &ad8460_powerdown_mode_enum),
> > + {}
> > +};
> > +
> > +static const struct iio_event_spec ad8460_events[] = {
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > +};
> > +
> > +#define AD8460_VOLTAGE_CHAN { \
> > + .type = IIO_VOLTAGE, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > + BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE), \
> > + .output = 1, \
> > + .indexed = 1, \
> > + .channel = 0, \
> > + .scan_index = 0, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 14, \
> > + .storagebits = 16, \
> > + .endianness = IIO_LE, \
>
> Data is going over DMA, so should this be IIO_CPU?
>
> > + }, \
> > + .ext_info = ad8460_ext_info, \
> > + .event_spec = ad8460_events, \
> > + .num_event_specs = ARRAY_SIZE(ad8460_events), \
> > +}
> > +
> > +#define AD8460_CURRENT_CHAN { \
> > + .type = IIO_CURRENT, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .output = 0, \
>
> Since this is writeable, I assume this is an output? (so = 1, not 0)
>
> > + .indexed = 1, \
> > + .channel = 0, \
> > + .scan_index = -1, \
> > + .event_spec = ad8460_events, \
> > + .num_event_specs = ARRAY_SIZE(ad8460_events), \
> > +}
> > +
> > +#define AD8460_TEMP_CHAN { \
> > + .type = IIO_TEMP, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .output = 1, \
> > + .indexed = 1, \
> > + .channel = 0, \
> > + .scan_index = -1, \
> > + .event_spec = ad8460_events, \
> > + .num_event_specs = 1, \
> > +}
> > +
> > +static const struct iio_chan_spec ad8460_channels[] = {
> > + AD8460_VOLTAGE_CHAN,
> > + AD8460_CURRENT_CHAN,
> > +};
> > +
> > +static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = {
> > + AD8460_VOLTAGE_CHAN,
> > + AD8460_CURRENT_CHAN,
> > + AD8460_TEMP_CHAN,
> > +};
> > +
> > +static const struct regmap_config ad8460_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = 0x7F,
> > +};
> > +
> > +static int ad8460_probe(struct spi_device *spi) {
> > + struct ad8460_state *state;
> > + struct iio_dev *indio_dev;
> > + struct device *dev;
> > + u32 tmp[2], temp;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + state = iio_priv(indio_dev);
> > + mutex_init(&state->lock);
> > +
> > + indio_dev->name = "ad8460";
> > + indio_dev->info = &ad8460_info;
> > +
> > + state->spi = spi;
> > + dev = &spi->dev;
> > +
> > + state->regmap = devm_regmap_init_spi(spi,
> &ad8460_regmap_config);
> > + if (IS_ERR(state->regmap))
> > + return dev_err_probe(dev, PTR_ERR(state->regmap),
> > + "Failed to initialize regmap");
> > +
> > + state->sync_clk = devm_clk_get_enabled(dev, NULL);
> > + if (IS_ERR(state->sync_clk))
> > + return dev_err_probe(dev, PTR_ERR(state->sync_clk),
> > + "Failed to get sync clk\n");
> > +
> > + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460-
> tmp");
>
> Should we check for specific errors here? For example what if we get -
> EPROBE_DEFER? Also, it doesn't like we could ever get NULL, so IS_ERR()
> should be sufficient.
>
> > + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) {
> > + indio_dev->channels = ad8460_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> > + } else {
> > + indio_dev->channels = ad8460_channels_with_tmp_adc;
> > + indio_dev->num_channels =
> ARRAY_SIZE(ad8460_channels_with_tmp_adc);
> > + }
> > +
> > + ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v");
> > + if (ret == -ENODEV)
> > + state->refio_1p2v_mv = 1200;
> > + else if (ret >= 0)
> > + state->refio_1p2v_mv = ret / 1000;
> > + else
> > + return dev_err_probe(dev, ret, "Failed to get reference
> > +voltage\n");
>
> The pattern we have been using in other drivers is:
>
> ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v");
> if (ret < 0 && ret != -ENODEV)
> return dev_err_probe(dev, ret, "Failed to get reference
> voltage\n");
>
> state->refio_1p2v_mv = ret == -ENODEV ? 1200 : ret / 1000;
>
> It saves a bit of reading.
>
> > +
> > + if (!in_range(state->refio_1p2v_mv, AD8460_MIN_VREFIO_UV,
> > + AD8460_MAX_VREFIO_UV))
>
> It looks like millivolts is being compared to microvolts here.
>
> > + return dev_err_probe(dev, -EINVAL,
> > + "Invalid ref voltage range(%u mV) [%u mV,
> %u mV]\n",
> > + state->refio_1p2v_mv,
> > + AD8460_MIN_VREFIO_UV / 1000,
> > + AD8460_MAX_VREFIO_UV / 1000);
> > +
> > + ret = device_property_read_u32(dev, "adi,external-resistor-ohms",
> > + &state->ext_resistor_ohms);
> > + if (ret)
> > + state->ext_resistor_ohms = 2000;
> > + else if (!in_range(state->ext_resistor_ohms,
> AD8460_MIN_EXT_RESISTOR_OHMS,
> > + AD8460_MAX_EXT_RESISTOR_OHMS))
> > + return dev_err_probe(dev, -EINVAL,
> > + "Invalid resistor set range(%u) [%u, %u]\n",
> > + state->ext_resistor_ohms,
> > + AD8460_MIN_EXT_RESISTOR_OHMS,
> > + AD8460_MAX_EXT_RESISTOR_OHMS);
> > +
> > + /* Arm the device by default */
>
> Comment doesn't match the code.
>
> > + ret = device_property_read_u32_array(dev, "adi,range-microamp",
> > + tmp, ARRAY_SIZE(tmp));
> > + if (!ret) {
> > + if (in_range(tmp[1], 0,
> AD8460_ABS_MAX_OVERCURRENT_UA))
> > + regmap_write(state->regmap,
> AD8460_CTRL_REG(0x08),
> > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > + AD8460_CURRENT_LIMIT_CONV(tmp[1]));
> > +
> > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA,
> 0))
> > + regmap_write(state->regmap,
> AD8460_CTRL_REG(0x09),
> > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > +
> AD8460_CURRENT_LIMIT_CONV(abs(tmp[0])));
> > + }
> > +
> > + ret = device_property_read_u32_array(dev, "adi,range-microvolt",
> > + tmp, ARRAY_SIZE(tmp));
> > + if (!ret) {
> > + if (in_range(tmp[1], 0,
> AD8460_ABS_MAX_OVERVOLTAGE_UV))
> > + regmap_write(state->regmap,
> AD8460_CTRL_REG(0x0A),
> > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > + AD8460_VOLTAGE_LIMIT_CONV(tmp[1]));
> > +
> > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERVOLTAGE_UV,
> 0))
> > + regmap_write(state->regmap,
> AD8460_CTRL_REG(0x0B),
> > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > +
> AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0])));
> > + }
> > +
> > + ret = device_property_read_u32(dev, "adi,max-millicelsius", &temp);
> > + if (!ret) {
> > + if (in_range(temp, AD8460_MIN_OVERTEMPERATURE_MC,
> > + AD8460_MAX_OVERTEMPERATURE_MC))
> > + regmap_write(state->regmap,
> AD8460_CTRL_REG(0x0C),
> > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > + AD8460_TEMP_LIMIT_CONV(abs(temp)));
> > + }
> > +
> > + ret = ad8460_reset(state);
> > + if (ret)
> > + return ret;
> > +
> > + /* Enables DAC by default */
> > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
> > + AD8460_HVDAC_SLEEP_MSK,
> > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK,
> 0));
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> > +
> > + ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, "tx",
> > +
> IIO_BUFFER_DIRECTION_OUT);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Failed to get DMA buffer\n");
> > +
> > + return devm_iio_device_register(dev, indio_dev); }
> > +
> > +static const struct of_device_id ad8460_of_match[] = {
> > + { .compatible = "adi, ad8460" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ad8460_of_match);
> > +
> > +static struct spi_driver ad8460_driver = {
> > + .driver = {
> > + .name = "ad8460",
> > + .of_match_table = ad8460_of_match,
> > + },
> > + .probe = ad8460_probe,
> > +};
> > +module_spi_driver(ad8460_driver);
> > +
> > +MODULE_AUTHOR("Mariel Tinaco <mariel.tinaco@xxxxxxxxxx");
> > +MODULE_DESCRIPTION("AD8460 DAC driver"); MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER);