> -----Original Message-----
> From: Jonathan Cameron <ji...@kernel.org>
> Sent: Saturday, December 5, 2020 8:01 PM
> To: Pop, Cristian <cristian....@analog.com>
> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; robh...@kernel.org
> Subject: Re: [PATCH v2 2/2] iio: dac: ad5766: add driver support for AD5766
>
> [External]
>
> On Fri, 4 Dec 2020 20:20:43 +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.
> >
> > Link:
> > https://www.analog.com/media/en/technical-documentation/data-
> sheets/ad
> > 5766-5767.pdf
> >
> > Signed-off-by: Cristian Pop <cristian....@analog.com>
>
> Missing build files + docs for the new ABI.
> Note it doesn't build so a few things to fix on that front!
>
> Docs in appropriate file under Documentation/ABI/testing/sysfs-bus-iio-*
>
> I'm a bit curious about the range being entirely controllable from userspace
> as well. Seems like something that might be dangerous in some systems.
> Perhaps we need some sort of dt binding restriction mechanism?
If you think it is better to restrict the user to a range that is set in device
tree,
please let me know. In some cases it is possible to have an extended range
maybe,
or a combination of multiple ranges.
>
>
> > ---
> > Changes in v2:
> > -Remove forward declarations, arrange code
> > -New ABI docs
> > -Move "max_val" scope in case
> > -Remove blank line
> > -Use bitfield operations, where posible
> > -Change declaration type to int of:
> > int
> scale_avail[AD5766_VOLTAGE_RANGE_MAX][2];
> > int
> offset_avail[AD5766_VOLTAGE_RANGE_MAX][2];
> > -Move initialization down to just above where it is used:
> > "type = spi_get_device_id(spi)->driver_data;"
> >
> > drivers/iio/dac/ad5766.c | 758
> > +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 758 insertions(+)
> > create mode 100644 drivers/iio/dac/ad5766.c
> >
> > diff --git a/drivers/iio/dac/ad5766.c b/drivers/iio/dac/ad5766.c new
> > file mode 100644 index 000000000000..e6d24a41bd4e
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad5766.c
> > @@ -0,0 +1,758 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices AD5766, AD5767
> > + * Digital to Analog Converters driver
> > + *
> > + * Copyright 2019-2020 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/bitfield.h>
> > +
> > +#define AD5766_UPPER_WORD_SPI_MASK GENMASK(31, 16)
> > +#define AD5766_LOWER_WORD_SPI_MASK GENMASK(15, 0)
> > +#define AD5766_DITHER_SOURCE_MASK(x) GENMASK(((2 * x) +
> 1), (2 * x))
> > +#define AD5766_DITHER_SCALE_MASK(x)
> AD5766_DITHER_SOURCE_MASK(x)
> > +
> > +#define AD5766_CMD_NOP_MUX_OUT 0x00
> > +#define AD5766_CMD_SDO_CNTRL 0x01
> > +#define AD5766_CMD_WR_IN_REG(x) (0x10 | ((x)
> & 0xF))
> > +#define AD5766_CMD_WR_DAC_REG(x) (0x20 | ((x) & 0xF))
> > +#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) & 0xF))
> > +#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
> > +
> > +#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,
> > + AD5766_VOLTAGE_RANGE_MAX,
> > +};
> > +
> > +/**
> > + * 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_PWR,
> > + AD5766_DITHER_INVERT
> > +};
> > +
> > +/*
> > + * External dither signal can be composed with the DAC output, if
> activated.
> > + * The dither signals are applied to the N0 and N1 input pins.
> > + * Dither source for each of the channel can be selected by writing
> > +to
> > + * "dither_source",a 32 bit variable and two bits are used for each
> > +of the 16
> > + * channels: 0: NO_DITHER, 1: N0, 2: N1.
> > + * This variable holds available dither source strings.
> > + */
> > +static const char * const ad5766_dither_sources[] = {
> > + "NO_DITHER",
> > + "N0",
> > + "N1",
> > +};
> > +
> > +/*
> > + * Dither signal can also be scaled.
> > + * Available dither scale strings coresponding to "dither_scale"
> > +field in
> > + * "struct ad5766_state".
> > + * "dither_scale" is a 32 bit variable and two bits are used for each
> > +of the 16
> > + * channels: 0: NO_SCALING, 1: 75%_SCALING, 2: 50%_SCALING, 3:
> 25%_SCALING.
>
> Needs explicit ABI docs for a proper discussion. My gut feeling is it should
> be two controls. On/off + a scaling control that takes integer values.
There is a dither on/off, we can also add an on/off control for scale and source
If requested.
>
> > + */
> > +static const char * const ad5766_dither_scales[] = {
> > + "NO_SCALING",
> > + "75%_SCALING",
> > + "50%_SCALING",
> > + "25%_SCALING",
> > +};
> > +
> > +/**
> > + * struct ad5766_state - driver instance specific data
> > + * @spi: SPI device
> > + * @lock: Mutex lock
>
> Say what exactly the scope of the lock is. No interest at all to tell us
> what is
> clear from the type of the structure element.
>
> > + * @chip_info: Chip model specific constants
> > + * @gpio_reset: Reset GPIO, used to reset the device
> > + * @crt_range: Current selected output range
> > + * @cached_offset: Cached range coresponding to the selected offset
> > + * @dither_power_ctrl: Power-down 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 3 possible sources:
> > + * 0: No dither, 1: N0, 2: N1
> > + * Two bits are used for each channel
> > + * @dither_scale: Selects between 4 possible scales:
> > + * 0: No scale, 1: 75%, 2: 50%, 3: 25%
> > + * Two bits are used for each channel
> > + * @scale_avail: Scale available table
> > + * @offset_avail: Offest available table
> > + * @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;
> > + enum ad5766_voltage_range cached_offset;
> > + u16 dither_power_ctrl;
> > + u16 dither_invert;
> > + u32 dither_source;
> > + u32 dither_scale;
> > + int scale_avail[AD5766_VOLTAGE_RANGE_MAX][2];
> > + int offset_avail[AD5766_VOLTAGE_RANGE_MAX][2];
> > + union {
> > + u32 d32;
> > + u16 w16[2];
> > + u8 b8[4];
> > + } data[3] ____cacheline_aligned;
> > +};
> > +
> ...
> > +
> > +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;
> > + st->data[0].b8[1] = (data & 0xFF00) >> 8;
> > + st->data[0].b8[2] = (data & 0x00FF) >> 0;
>
> That's an unaligned put so ideally use put_unaligned_xx16 and friends to
> make that clear.
>
> > +
> > + 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);
>
> Normal convention for this sort of function would be __ rather than _ Looks
> more deliberate.
>
> > + mutex_unlock(&st->lock);
> > +
> > + return ret;
> > +}
> > +
>
> ...
>
> > +
> > +#define _AD5766_CHAN_EXT_INFO(_name, _what, _shared) { \
> > + .name = _name, \
> > + .read = ad5766_read_ext, \
> > + .write = ad5766_write_ext, \
> > + .private = _what, \
> > + .shared = _shared, \
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info ad5766_ext_info[] = {
> > +
> > + _AD5766_CHAN_EXT_INFO("dither_pwr", AD5766_DITHER_PWR,
> IIO_SEPARATE),
> > + _AD5766_CHAN_EXT_INFO("dither_invert",
> AD5766_DITHER_INVERT,
> > + IIO_SEPARATE),
> > + IIO_ENUM("dither_source", IIO_SEPARATE,
> &ad5766_dither_source_enum),
> > + IIO_ENUM_AVAILABLE_SHARED("dither_source",
> > + IIO_SEPARATE,
> > + &ad5766_dither_source_enum),
> > + IIO_ENUM("dither_scale", IIO_SEPARATE,
> &ad5766_dither_scale_enum),
> > + IIO_ENUM_AVAILABLE_SHARED("dither_scale",
>
> That macro doesn't exist in mainline.
>
> > + IIO_SEPARATE,
> > + &ad5766_dither_scale_enum),
> > + {}
> > +};
>
> All the above need ABI docs so we can talk about them without having to
> read data sheets.
>
> ...
