diff mbox series

[v2,1/2] dt-bindings: iio: dac: AD5766 yaml documentation

Message ID 20201204182043.86899-1-cristian.pop@analog.com
State Changes Requested, archived
Headers show
Series [v2,1/2] dt-bindings: iio: dac: AD5766 yaml documentation | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema fail build log

Commit Message

Cristian Pop Dec. 4, 2020, 6:20 p.m. UTC 
This adds device tree bindings for the AD5766 DAC.

Signed-off-by: Cristian Pop <cristian.pop@analog.com>
---
 Changes in v2:
	- Add "additionalProperties: false" property
	- Remove blank line
 .../bindings/iio/dac/adi,ad5766.yaml          | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad5766.yaml

Comments

Jonathan Cameron Dec. 5, 2020, 6:01 p.m. UTC | #1 
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/ad5766-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?


> ---
>  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.

> + */
> +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.

...
Rob Herring (Arm) Dec. 7, 2020, 4:45 p.m. UTC | #2 
On Fri, 04 Dec 2020 20:20:42 +0200, Cristian Pop wrote:
> This adds device tree bindings for the AD5766 DAC.
> 
> Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> ---
>  Changes in v2:
> 	- Add "additionalProperties: false" property
> 	- Remove blank line
>  .../bindings/iio/dac/adi,ad5766.yaml          | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad5766.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/iio/dac/adi,ad5766.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/iio/dac/adi,ad5766.yaml#
Documentation/devicetree/bindings/iio/dac/adi,ad5766.example.dts:21.13-23: Warning (reg_format): /example-0/ad5766@0:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/iio/dac/adi,ad5766.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/iio/dac/adi,ad5766.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/iio/dac/adi,ad5766.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/iio/dac/adi,ad5766.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/iio/dac/adi,ad5766.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/dac/adi,ad5766.example.dt.yaml: example-0: ad5766@0:reg:0: [0] is too short
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml


See https://patchwork.ozlabs.org/patch/1411214

The base for the patch is generally the last rc1. Any dependencies
should be noted.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Cristian Pop Dec. 8, 2020, 1:30 p.m. UTC | #3 
> -----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.
> 
> ...
Andy Shevchenko Dec. 9, 2020, 3:52 p.m. UTC | #4 
On Fri, Dec 4, 2020 at 8:17 PM 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/ad5766-5767.pdf

Can we use Datasheet: tag instead, please?

>
> Signed-off-by: Cristian Pop <cristian.pop@analog.com>

No blank lines in tag block, please.

...

> + * Analog Devices AD5766, AD5767
> + * Digital to Analog Converters driver

Looks like one line.

...

> +#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>

Keep it sorted?

...

> +#define AD5766_CMD_WR_IN_REG(x)                        (0x10 | ((x) & 0xF))
> +#define AD5766_CMD_WR_DAC_REG(x)               (0x20 | ((x) & 0xF))

Why not GENMASK()?

...

> +#define AD5766_CMD_READBACK_REG(x)             (0x80 | ((x) & 0xF))

Ditto.

...

> +enum ad5766_type {
> +       ID_AD5766,
> +       ID_AD5767,
> +};

This may be problematic in case we switch to device_get_match_data().

...

> +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,

No comma for terminator line.

> +};

...

> +enum {
> +       AD5766_DITHER_PWR,
> +       AD5766_DITHER_INVERT

+ comma

> +};

...

> +/*
> + * 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",
> +};

Can't we rather be simpler, i.e. use 0,1 and -1, where the latter for
NO_DITHER cas?.

...

> +/*
> + * Dither signal can also be scaled.
> + * Available dither scale strings coresponding to "dither_scale" field in

corresponding

> + * "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.
> + */
> +static const char * const ad5766_dither_scales[] = {
> +       "NO_SCALING",
> +       "75%_SCALING",
> +       "50%_SCALING",
> +       "25%_SCALING",
> +};

Oh, no. Please, use plain numbers in percentages.

...

> + * @cached_offset:     Cached range coresponding to the selected offset

corresponding
Please, run checkpatch.pl --code-spell (or how is it called?).

...

> + * @offset_avail:      Offest available table

Ditto.
Offset (I suppose)

...

> +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;

Please,  use get_unaliged_XX() / put_unaligned_XX() and other related
macros / APIs.

> +       return spi_write(st->spi, &st->data[0].b8[0], 3);
> +}

...

> +static int ad5766_reset(struct ad5766_state *st)
> +{
> +       int ret = 0;

In general it's a bad idea and in particular here it's not needed.

> +       return 0;
> +}

...

> +       ret = _ad5766_spi_write(st, AD5766_CMD_DITHER_SIG_1,

> +                         st->dither_source & 0xFFFF);

Do you really need this conjunction? If so, why not GENMASK()?

> +       if (ret)
> +               return ret;

...

> +       ret = _ad5766_spi_write(st, AD5766_CMD_INV_DITHER, st->dither_invert);
> +       if (ret)
> +               return ret;
> +
> +       return 0;

return _ad5766_spi_write(…);

...

> +static int ad5766_set_offset(struct ad5766_state *st, int val, int val2)
> +{
> +       int i;
> +       s32 (*tbl)[AD5766_VOLTAGE_RANGE_MAX][2] = &(st->offset_avail);

Too many parentheses.

> +
> +       for (i = 0; i < AD5766_VOLTAGE_RANGE_MAX; i++) {

ARRAY_SIZE() ?

> +               if ((*tbl)[i][0] == val && (*tbl)[i][1] == val2) {
> +                       st->cached_offset = i;
> +                       return 0;
> +               }
> +       }

Entire function hurts my eyes. Can you give some time and think over it again?

> +       return -EINVAL;
> +}

...

> +static int ad5766_set_scale(struct ad5766_state *st, int val, int val2)

Ditto.

...

> +               *vals = (const int *)st->scale_avail;

Why do you need casting?

...

> +               *vals = (const int *)st->offset_avail;

Ditto.

...

> +static int ad5766_read_raw(struct iio_dev *indio_dev,
> +                          struct iio_chan_spec const *chan,
> +                          int *val,
> +                          int *val2,
> +                          long m)

It may take much less LOCs.

...

> +static int ad5766_write_raw(struct iio_dev *indio_dev,
> +                           struct iio_chan_spec const *chan,
> +                           int val,
> +                           int val2,
> +                           long info)

Ditto.

...

> +               const int max_val = (1 << chan->scan_type.realbits);
> +
> +               if (val >= max_val || val < 0)
> +                       return -EINVAL;
> +               val <<= chan->scan_type.shift;

You can do better, i.e. drop unneeded temporary variable and use fls().

...

> +       return (source >> chan->channel * 2);

Seems parentheses in incorrect place in case you want to increase robustness.

> +}

...

> +       st->dither_source |= (mode << (chan->channel * 2));

It's not LISP, seriously.
I'm wondering if Analog has internal mailing list (and perhaps a wiki
with common tips and tricks for Linux kernel programming) for
review...

...

> +       return (scale >> chan->channel * 2);

As above.

...

> +       st->dither_scale |= (scale << (chan->channel * 2));

As above.

...

> +               return sprintf(buf, "%u\n", 0x01 &
> +                              ~(st->dither_power_ctrl >> chan->channel));

Oh là là.

!(st->dither_power_ctrl & BIT(chan->channel)) ?

...

> +               return sprintf(buf, "%u\n", 0x01 &
> +                              (st->dither_invert >> chan->channel));

Ditto.

...

> +       default:

> +               ret = -EINVAL;
> +               break;

Point of this? Can't return directly?

> +       }
> +
> +       return ret;

...

> +       switch ((u32)private) {

Why casting?

...

> +       default:
> +               ret = -EINVAL;
> +               break;

Why not to return here?

> +       }

> +       return ret ? ret : len;

return len; ?

...

> +               offset = div_s64(offset * 1000000, denom);
> +               st->offset_avail[i][0] = div_s64(offset, 1000000);
> +               div_s64_rem(offset, 1000000, &st->offset_avail[i][1]);

...

> +               scale = div_u64((scale * 1000000000), (1 << realbits));
> +               st->scale_avail[i][0] = (int)div_u64(scale, 1000000);
> +               div_s64_rem(scale, 1000000, &st->scale_avail[i][1]);

Perhaps it's time to extend units.h with mV / V / uV / etc?

...

> +static const struct of_device_id ad5766_dt_match[] = {
> +       { .compatible = "adi,ad5766" },
> +       { .compatible = "adi,ad5767" },

> +       {},

No comma for terninator.

> +};
Jonathan Cameron Dec. 13, 2020, 2:27 p.m. UTC | #5 
On Tue, 8 Dec 2020 13:30:06 +0000
"Pop, Cristian" <Cristian.Pop@analog.com> wrote:

> > -----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.

It's an interesting question of whether anyone actually would use these
parts in a circumstance where they wanted to only access the full range
via a mode switch.  I agree it is theoretically possible, but it's pretty
odd and would smack of curious design decisions to me!
I'm a little cynical in that I suspect the only people who ever change these
ranges are those using devkits to do a PoC.  Production hardware would normally
be designed to work best with a fixed range.

If a range that doesn't correspond to one of the supported ones, actually makes
sense, then having a dt binding that sets max and min values separately and
allowed the driver to check against both would work.

Thanks,

Jonathan

> > 
> >   
> > > ---
> > >  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.
> > 
> > ...
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5766.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5766.yaml
new file mode 100644
index 000000000000..e958fb232357
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5766.yaml
@@ -0,0 +1,53 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2020 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bindings/iio/dac/adi,ad5766.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD5766 DAC device driver
+
+maintainers:
+  - Cristian Pop <cristian.pop@analog.com>
+
+description: |
+  Bindings for the Analog Devices AD5766 current DAC device. Datasheet can be
+  found here:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad5766-5767.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad5766
+      - adi,ad5767
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 1000000
+
+  spi-cpol: true
+
+  reset-gpios:
+    description: GPIO spec for the RESET pin. If specified, it will be
+      asserted during driver probe.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - spi-max-frequency
+  - spi-cpol
+
+additionalProperties: false
+
+examples:
+  - |
+    ad5766@0{
+        compatible = "adi,ad5766";
+        reg = <0>;
+        spi-cpol;
+        spi-max-frequency = <1000000>;
+        reset-gpios = <&gpio 22 0>;
+      };