On Fri, 18 Dec 2020 19:12:31 +0200
Cristian Pop <cristian....@analog.com> wrote:
> The AD5766/AD5767 are 16-channel, 16-bit/12-bit, voltage output dense DACs
> Digital-to-Analog converters.
>
> This change adds support for these DACs.
>
> Signed-off-by: Cristian Pop <cristian....@analog.com>
Hi Cristian,
The addressing in this device 'interesting'. I'm not sure it can be handled
much better than you have done though.
Otherwise, a few minor bits and pieces inline.
Thanks,
Jonathan
> ---
> Changelog v4:
> - Select output range
> - Set possible dither source to: 0 - N0 and 1 for N1.
> - Better macros usage
> - Invert reset logic
> - Use GENMASK
> - Add/remove some commas
> drivers/iio/dac/Kconfig | 10 +
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/ad5766.c | 668 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 679 insertions(+)
> create mode 100644 drivers/iio/dac/ad5766.c
>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 6f6074a5d3db..cea07b4cced1 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -189,6 +189,16 @@ config AD5764
> To compile this driver as a module, choose M here: the
> module will be called ad5764.
>
> +config AD5766
> + tristate "Analog Devices AD5766/AD5767 DAC driver"
> + depends on SPI_MASTER
> + help
> + Say yes here to build support for Analog Devices AD5766, AD5767
> + Digital to Analog Converter.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad5766.
> +
> config AD5770R
> tristate "Analog Devices AD5770R IDAC driver"
> depends on SPI_MASTER
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 2fc481167724..33e16f14902a 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_AD5755) += ad5755.o
> obj-$(CONFIG_AD5755) += ad5758.o
> obj-$(CONFIG_AD5761) += ad5761.o
> obj-$(CONFIG_AD5764) += ad5764.o
> +obj-$(CONFIG_AD5766) += ad5766.o
> obj-$(CONFIG_AD5770R) += ad5770r.o
> obj-$(CONFIG_AD5791) += ad5791.o
> obj-$(CONFIG_AD5686) += ad5686.o
> diff --git a/drivers/iio/dac/ad5766.c b/drivers/iio/dac/ad5766.c
> new file mode 100644
> index 000000000000..b7405185f3d8
> --- /dev/null
> +++ b/drivers/iio/dac/ad5766.c
> @@ -0,0 +1,668 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD5766, AD5767
> + * Digital to Analog Converters driver
> + * Copyright 2019-2020 Analog Devices Inc.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned/be_byteshift.h>
> +
> +#define AD5766_UPPER_WORD_SPI_MASK GENMASK(31, 16)
> +#define AD5766_LOWER_WORD_SPI_MASK GENMASK(15, 0)
> +#define AD5766_DITHER_SOURCE_MASK(ch) GENMASK(((2 * ch) + 1),
> (2 * ch))
> +#define AD5766_DITHER_SOURCE(ch, source) BIT((ch * 2) + source)
> +#define AD5766_DITHER_SCALE_MASK(x) AD5766_DITHER_SOURCE_MASK(x)
> +#define AD5766_DITHER_SCALE(ch, scale) (scale << (ch * 2))
> +#define AD5766_DITHER_ENABLE_MASK(ch) BIT(ch)
> +#define AD5766_DITHER_ENABLE(ch, state) ((!state) << ch)
> +#define AD5766_DITHER_INVERT_MASK(ch) BIT(ch)
> +#define AD5766_DITHER_INVERT(ch, state) (state << ch)
> +
> +#define AD5766_CMD_NOP_MUX_OUT 0x00
> +#define AD5766_CMD_SDO_CNTRL 0x01
> +#define AD5766_CMD_WR_IN_REG(x) (0x10 | ((x) &
> GENMASK(3, 0)))
> +#define AD5766_CMD_WR_DAC_REG(x) (0x20 | ((x) & GENMASK(3, 0)))
> +#define AD5766_CMD_SW_LDAC 0x30
> +#define AD5766_CMD_SPAN_REG 0x40
> +#define AD5766_CMD_WR_PWR_DITHER 0x51
> +#define AD5766_CMD_WR_DAC_REG_ALL 0x60
> +#define AD5766_CMD_SW_FULL_RESET 0x70
> +#define AD5766_CMD_READBACK_REG(x) (0x80 | ((x) & GENMASK(3, 0)))
> +#define AD5766_CMD_DITHER_SIG_1 0x90
> +#define AD5766_CMD_DITHER_SIG_2 0xA0
> +#define AD5766_CMD_INV_DITHER 0xB0
> +#define AD5766_CMD_DITHER_SCALE_1 0xC0
> +#define AD5766_CMD_DITHER_SCALE_2 0xD0
These addresses struck me as 'unusual' so I went and had a look at the
datasheet.
I would separate the definition of C3-C0 and A3-A0 as then it will look a lot
more obvious as we will just have registers 0x0 to 0xD, some of which are banked
with the address bits acting as selector.
However. There are at least 2 special cases where the address bits aren't acting
to specify a banked per output channel register *sigh* so perhaps what you have
is the best we can do.
> +
> +#define AD5766_FULL_RESET_CODE 0x1234
> +
> +enum ad5766_type {
> + ID_AD5766,
> + ID_AD5767,
> +};
> +
> +enum ad5766_voltage_range {
> + AD5766_VOLTAGE_RANGE_M20V_0V,
> + AD5766_VOLTAGE_RANGE_M16V_to_0V,
> + AD5766_VOLTAGE_RANGE_M10V_to_0V,
> + AD5766_VOLTAGE_RANGE_M12V_to_14V,
> + AD5766_VOLTAGE_RANGE_M16V_to_10V,
> + AD5766_VOLTAGE_RANGE_M10V_to_6V,
> + AD5766_VOLTAGE_RANGE_M5V_to_5V,
> + AD5766_VOLTAGE_RANGE_M10V_to_10V,
> +};
> +
> +/**
> + * struct ad5766_chip_info - chip specific information
> + * @num_channels: number of channels
> + * @channel: channel specification
> + */
> +struct ad5766_chip_info {
> + unsigned int num_channels;
> + const struct iio_chan_spec *channels;
> +};
> +
> +enum {
> + AD5766_DITHER_ENABLE,
> + AD5766_DITHER_INVERT,
> + AD5766_DITHER_SOURCE,
> +};
> +
> +/*
> + * Dither signal can also be scaled.
> + * Available dither scale strings corresponding to "dither_scale" field in
> + * "struct ad5766_state".
> + */
> +static const char * const ad5766_dither_scales[] = {
> + "100",
> + "75",
> + "50",
> + "25",
> +};
> +
> +/**
> + * struct ad5766_state - driver instance specific data
> + * @spi: SPI device
> + * @lock: Lock used to restrict concurent access to SPI device
> + * @chip_info: Chip model specific constants
> + * @gpio_reset: Reset GPIO, used to reset the device
> + * @crt_range: Current selected output range
> + * @dither_enable: Power enable bit for each channel dither block (for
> + * example, D15 = DAC 15,D8 = DAC 8, and D0 = DAC 0)
> + * 0 - Normal operation, 1 - Power down
> + * @dither_invert: Inverts the dither signal applied to the selected DAC
> + * outputs
> + * @dither_source: Selects between 2 possible sources:
> + * 1: N0, 2: N1
> + * Two bits are used for each channel
> + * @dither_scale: Two bits are used for each of the 16 channels:
> + * 0: 100% SCALING, 1: 75% SCALING, 2: 50% SCALING,
> + * 3: 25% SCALING.
> + * @data: SPI transfer buffers
> + */
> +struct ad5766_state {
> + struct spi_device *spi;
> + struct mutex lock;
> + const struct ad5766_chip_info *chip_info;
> + struct gpio_desc *gpio_reset;
> + enum ad5766_voltage_range crt_range;
> + u16 dither_enable;
> + u16 dither_invert;
> + u32 dither_source;
> + u32 dither_scale;
> + union {
> + u32 d32;
> + u16 w16[2];
> + u8 b8[4];
> + } data[3] ____cacheline_aligned;
> +};
> +
> +struct ad5766_span_tbl {
> + int min;
> + int max;
> +};
> +
> +static const struct ad5766_span_tbl ad5766_span_tbl[] = {
> + [AD5766_VOLTAGE_RANGE_M20V_0V] = {
Giving naming you 'could' just assume the min, max element was clear enough
and go with
[AD5766_VOLTAGE_RANGE_M20V_0V] = {-20, 0}, etc
I don't think there would be any significant loss in readability.
> + .min = -20,
> + .max = 0,
> + },
> + [AD5766_VOLTAGE_RANGE_M16V_to_0V] = {
> + .min = -16,
> + .max = 0,
> + },
> + [AD5766_VOLTAGE_RANGE_M10V_to_0V] = {
> + .min = -10,
> + .max = 0,
> + },
> + [AD5766_VOLTAGE_RANGE_M12V_to_14V] = {
> + .min = -12,
> + .max = 14,
> + },
> + [AD5766_VOLTAGE_RANGE_M16V_to_10V] = {
> + .min = -16,
> + .max = 10,
> + },
> + [AD5766_VOLTAGE_RANGE_M10V_to_6V] = {
> + .min = -10,
> + .max = 6,
> + },
> + [AD5766_VOLTAGE_RANGE_M5V_to_5V] = {
> + .min = -5,
> + .max = 5,
> + },
> + [AD5766_VOLTAGE_RANGE_M10V_to_10V] = {
> + .min = -10,
> + .max = 10,
> + },
> +};
> +
> +static int __ad5766_spi_read(struct ad5766_state *st, u8 dac, int *val)
> +{
> + int ret;
> + struct spi_transfer xfers[] = {
> + {
> + .tx_buf = &st->data[0].d32,
> + .bits_per_word = 8,
> + .len = 3,
> + .cs_change = 1,
> + }, {
> + .tx_buf = &st->data[1].d32,
> + .rx_buf = &st->data[2].d32,
> + .bits_per_word = 8,
> + .len = 3,
> + },
> + };
> +
> + st->data[0].d32 = AD5766_CMD_READBACK_REG(dac);
> + st->data[1].d32 = AD5766_CMD_NOP_MUX_OUT;
> +
> + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> + if (ret)
> + return ret;
> +
> + *val = st->data[2].w16[1];
> +
> + return ret;
> +}
> +
> +static int __ad5766_spi_write(struct ad5766_state *st, u8 command, u16 data)
> +{
> + st->data[0].b8[0] = command;
> + put_unaligned_be16(data, &st->data[0].b8[1]);
> +
> + return spi_write(st->spi, &st->data[0].b8[0], 3);
> +}
> +
> +static int ad5766_read(struct iio_dev *indio_dev, u8 dac, int *val)
> +{
> + struct ad5766_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = __ad5766_spi_read(st, dac, val);
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int ad5766_write(struct iio_dev *indio_dev, u8 dac, u16 data)
> +{
> + struct ad5766_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = __ad5766_spi_write(st, AD5766_CMD_WR_DAC_REG(dac), data);
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int ad5766_reset(struct ad5766_state *st)
> +{
> + int ret;
> +
> + if (st->gpio_reset) {
> + gpiod_set_value_cansleep(st->gpio_reset, 1);
> + ndelay(100); /* t_reset >= 100ns */
> + gpiod_set_value_cansleep(st->gpio_reset, 0);
> + } else {
> + ret = __ad5766_spi_write(st, AD5766_CMD_SW_FULL_RESET,
> + AD5766_FULL_RESET_CODE);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /*
> + * Minimum time between a reset and the subsequent successful write is
> + * typically 25 ns
> + */
> + ndelay(25);
> +
> + return 0;
> +}
> +
> +static int ad5766_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long m)
> +{
> + struct ad5766_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (m) {
> + case IIO_CHAN_INFO_RAW:
> + ret = ad5766_read(indio_dev, chan->address, val);
> + if (ret)
> + return ret;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_OFFSET:
> + *val = ad5766_span_tbl[st->crt_range].min;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = ad5766_span_tbl[st->crt_range].max -
> + ad5766_span_tbl[st->crt_range].min;
> + *val2 = st->chip_info->channels[0].scan_type.realbits;
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad5766_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val,
> + int val2,
> + long info)
> +{
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + {
> + const int max_val = GENMASK(chan->scan_type.realbits - 1, 0);
> +
> + if (val > max_val || val < 0)
> + return -EINVAL;
> + val <<= chan->scan_type.shift;
> + return ad5766_write(indio_dev, chan->address, val);
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info ad5766_info = {
> + .read_raw = ad5766_read_raw,
> + .write_raw = ad5766_write_raw,
> +};
> +
> +static int ad5766_get_dither_source(struct iio_dev *dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ad5766_state *st = iio_priv(dev);
> + u32 source;
> +
> + source = st->dither_source & AD5766_DITHER_SOURCE_MASK(chan->channel);
> + source = source >> (chan->channel * 2);
> + source -= 1;
> +
> + return source;
> +}
> +
> +static int ad5766_set_dither_source(struct iio_dev *dev,
> + const struct iio_chan_spec *chan,
> + unsigned int source)
> +{
> + struct ad5766_state *st = iio_priv(dev);
> + uint16_t val;
> + int ret;
> +
> + st->dither_source &= ~AD5766_DITHER_SOURCE_MASK(chan->channel);
> + st->dither_source |= AD5766_DITHER_SOURCE(chan->channel, source);
> +
> + val = FIELD_GET(AD5766_LOWER_WORD_SPI_MASK, st->dither_source);
> + ret = ad5766_write(dev, AD5766_CMD_DITHER_SIG_1, val);
> + if (ret)
> + return ret;
> +
> + val = FIELD_GET(AD5766_UPPER_WORD_SPI_MASK, st->dither_source);
> +
> + return ad5766_write(dev, AD5766_CMD_DITHER_SIG_2, val);
> +}
> +
> +static int ad5766_get_dither_scale(struct iio_dev *dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ad5766_state *st = iio_priv(dev);
> + u32 scale;
> +
> + scale = st->dither_scale & AD5766_DITHER_SCALE_MASK(chan->channel);
> +
> + return (scale >> (chan->channel * 2));
> +}
> +
> +static int ad5766_set_dither_scale(struct iio_dev *dev,
> + const struct iio_chan_spec *chan,
> + unsigned int scale)
> +{
> + int ret;
> + struct ad5766_state *st = iio_priv(dev);
> + uint16_t val;
> +
> + st->dither_scale &= ~AD5766_DITHER_SCALE_MASK(chan->channel);
> + st->dither_scale |= AD5766_DITHER_SCALE(chan->channel, scale);
> +
> + val = FIELD_GET(AD5766_LOWER_WORD_SPI_MASK, st->dither_scale);
> + ret = ad5766_write(dev, AD5766_CMD_DITHER_SCALE_1, val);
> + if (ret)
> + return ret;
> + val = FIELD_GET(AD5766_UPPER_WORD_SPI_MASK, st->dither_scale);
> +
> + return ad5766_write(dev, AD5766_CMD_DITHER_SCALE_2, val);
> +}
> +
> +static const struct iio_enum ad5766_dither_scale_enum = {
> + .items = ad5766_dither_scales,
> + .num_items = ARRAY_SIZE(ad5766_dither_scales),
> + .set = ad5766_set_dither_scale,
> + .get = ad5766_get_dither_scale,
> +};
> +
> +static ssize_t ad5766_read_ext(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + char *buf)
> +{
> + struct ad5766_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (private) {
> + case AD5766_DITHER_ENABLE:
> + return sprintf(buf, "%u\n",
> + !(st->dither_enable & BIT(chan->channel)));
> + break;
> + case AD5766_DITHER_INVERT:
> + return sprintf(buf, "%u\n",
> + !!(st->dither_invert & BIT(chan->channel)));
> + break;
> + case AD5766_DITHER_SOURCE:
> + return sprintf(buf, "%u\n",
> + ad5766_get_dither_source(indio_dev, chan));
> + default:
> + return -EINVAL;
> + }
> +
Can't get here
> + return ret;
> +}
> +
> +static ssize_t ad5766_write_ext(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len)
> +{
> + struct ad5766_state *st = iio_priv(indio_dev);
> + bool readin;
> + int ret;
> +
> + ret = kstrtobool(buf, &readin);
> + if (ret)
> + return ret;
> +
> + switch (private) {
> + case AD5766_DITHER_ENABLE:
> + st->dither_enable &= ~AD5766_DITHER_ENABLE_MASK(chan->channel);
> + st->dither_enable |= AD5766_DITHER_ENABLE(chan->channel,
> + readin);
> + ret = ad5766_write(indio_dev, AD5766_CMD_WR_PWR_DITHER,
> + st->dither_enable);
> + break;
> + case AD5766_DITHER_INVERT:
> + st->dither_invert &= ~AD5766_DITHER_INVERT_MASK(chan->channel);
> + st->dither_invert |= AD5766_DITHER_INVERT(chan->channel,
> + readin);
> + ret = ad5766_write(indio_dev, AD5766_CMD_INV_DITHER,
> + st->dither_invert);
> + break;
> + case AD5766_DITHER_SOURCE:
> + ret = ad5766_set_dither_source(indio_dev, chan, readin);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return ret ? ret : len;
> +}
> +
> +#define _AD5766_CHAN_EXT_INFO(_name, _what, _shared) { \
> + .name = _name, \
> + .read = ad5766_read_ext, \
> + .write = ad5766_write_ext, \
> + .private = _what, \
> + .shared = _shared, \
> +}
> +
> +#define IIO_ENUM_AVAILABLE_SHARED(_name, _shared, _e) \
> +{ \
> + .name = (_name "_available"), \
> + .shared = _shared, \
> + .read = iio_enum_available_read, \
> + .private = (uintptr_t)(_e), \
> +}
> +
> +static const struct iio_chan_spec_ext_info ad5766_ext_info[] = {
> +
> + _AD5766_CHAN_EXT_INFO("dither_enable", AD5766_DITHER_ENABLE,
> + IIO_SEPARATE),
> + _AD5766_CHAN_EXT_INFO("dither_invert", AD5766_DITHER_INVERT,
> + IIO_SEPARATE),
> + _AD5766_CHAN_EXT_INFO("dither_source", AD5766_DITHER_SOURCE,
> + IIO_SEPARATE),
> + IIO_ENUM("dither_scale", IIO_SEPARATE, &ad5766_dither_scale_enum),
> + IIO_ENUM_AVAILABLE_SHARED("dither_scale",
> + IIO_SEPARATE,
> + &ad5766_dither_scale_enum),
> + {}
> +};
> +
> +#define AD576x_CHANNEL(_chan, _bits) {
> \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .output = 1, \
> + .channel = (_chan), \
> + .address = (_chan), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = (_bits), \
> + .storagebits = 16, \
> + .shift = 16 - (_bits), \
> + }, \
> + .ext_info = ad5766_ext_info, \
> +}
> +
> +#define DECLARE_AD576x_CHANNELS(_name, _bits) \
> +const struct iio_chan_spec _name[] = { \
> + AD576x_CHANNEL(0, (_bits)), \
> + AD576x_CHANNEL(1, (_bits)), \
> + AD576x_CHANNEL(2, (_bits)), \
> + AD576x_CHANNEL(3, (_bits)), \
> + AD576x_CHANNEL(4, (_bits)), \
> + AD576x_CHANNEL(5, (_bits)), \
> + AD576x_CHANNEL(6, (_bits)), \
> + AD576x_CHANNEL(7, (_bits)), \
> + AD576x_CHANNEL(8, (_bits)), \
> + AD576x_CHANNEL(9, (_bits)), \
> + AD576x_CHANNEL(10, (_bits)), \
> + AD576x_CHANNEL(11, (_bits)), \
> + AD576x_CHANNEL(12, (_bits)), \
> + AD576x_CHANNEL(13, (_bits)), \
> + AD576x_CHANNEL(14, (_bits)), \
> + AD576x_CHANNEL(15, (_bits)), \
> +}
> +
> +static DECLARE_AD576x_CHANNELS(ad5766_channels, 16);
> +static DECLARE_AD576x_CHANNELS(ad5767_channels, 12);
> +
> +static const struct ad5766_chip_info ad5766_chip_infos[] = {
> + [ID_AD5766] = {
> + .num_channels = ARRAY_SIZE(ad5766_channels),
> + .channels = ad5766_channels,
> + },
> + [ID_AD5767] = {
> + .num_channels = ARRAY_SIZE(ad5767_channels),
> + .channels = ad5767_channels,
> + },
> +};
> +
> +static int ad5766_get_output_range(struct ad5766_state *st)
> +{
> + int i, ret, min, max, tmp[2];
> +
> + ret = device_property_read_u32_array(&st->spi->dev,
> + "output-range",
> + tmp, 2);
> + if (ret)
> + return ret;
> +
> + min = tmp[0];
> + max = tmp[1];
> + for (i = 0; i < ARRAY_SIZE(ad5766_span_tbl); i++) {
> + if (ad5766_span_tbl[i].min != min ||
> + ad5766_span_tbl[i].max != max)
> + continue;
> +
> + st->crt_range = i;
> +
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad5766_default_setup(struct ad5766_state *st)
> +{
> + uint16_t val;
> + int ret, i;
> +
> + /* Always issue a reset before writing to the span register. */
> + ret = ad5766_reset(st);
> + if (ret)
> + return ret;
> +
> + ret = ad5766_get_output_range(st);
> + if (ret)
> + return ret;
> +
> + /* Dither power down */
> + st->dither_enable = GENMASK(15, 0);
> + ret = __ad5766_spi_write(st, AD5766_CMD_WR_PWR_DITHER,
> + st->dither_enable);
> + if (ret)
> + return ret;
> +
> + st->dither_source = 0;
> + for (i = 0; i < ARRAY_SIZE(ad5766_channels); i++)
> + st->dither_source |= AD5766_DITHER_SOURCE(i, 0);
> + val = FIELD_GET(AD5766_LOWER_WORD_SPI_MASK, st->dither_source);
> + ret = __ad5766_spi_write(st, AD5766_CMD_DITHER_SIG_1, val);
> + if (ret)
> + return ret;
> +
> + val = FIELD_GET(AD5766_UPPER_WORD_SPI_MASK, st->dither_source);
> + ret = __ad5766_spi_write(st, AD5766_CMD_DITHER_SIG_2, val);
> + if (ret)
> + return ret;
> +
> + st->dither_scale = 0;
> + val = FIELD_GET(AD5766_LOWER_WORD_SPI_MASK, st->dither_scale);
> + ret = __ad5766_spi_write(st, AD5766_CMD_DITHER_SCALE_1, val);
> + if (ret)
> + return ret;
> +
> + val = FIELD_GET(AD5766_UPPER_WORD_SPI_MASK, st->dither_scale);
This is a rather unusual use of FIELD_GET to split a value that we then
write to two 16 bit registers. I guess it works, but does feel a bit odd.
If you are happy with it, I guess I can cope with it confusing me.
> + ret = __ad5766_spi_write(st, AD5766_CMD_DITHER_SCALE_2, val);
> + if (ret)
> + return ret;
> +
> + st->dither_invert = 0;
> + ret = __ad5766_spi_write(st, AD5766_CMD_INV_DITHER, st->dither_invert);
> + if (ret)
> + return ret;
> +
> + return __ad5766_spi_write(st, AD5766_CMD_SPAN_REG, st->crt_range);
> +}
> +
> +static int ad5766_probe(struct spi_device *spi)
> +{
> + enum ad5766_type type;
> + struct iio_dev *indio_dev;
> + struct ad5766_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + mutex_init(&st->lock);
> +
> + st->spi = spi;
> + type = spi_get_device_id(spi)->driver_data;
> + st->chip_info = &ad5766_chip_infos[type];
> +
> + indio_dev->channels = st->chip_info->channels;
> + indio_dev->num_channels = st->chip_info->num_channels;
> + indio_dev->info = &ad5766_info;
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->dev.of_node = spi->dev.of_node;
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset",
> + GPIOD_OUT_LOW);
Minor point, but in theory this might return an error code rather than
NULL (which it will return if no gpio was specified). Should probably
handle that.
> +
> + ret = ad5766_default_setup(st);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
...
