RE: [PATCH v2 2/2] iio: dac: ad5766: add driver support for AD5766

["Pop, Cristian" <Cristian.Pop@analog.com>]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, December 5, 2020 8:01 PM
> To: Pop, Cristian <Cristian.Pop@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; robh+dt@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.pop@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.pop@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.
> 
> ...