RE: [PATCH v2 2/2] drivers:iio:dac: Add AD3552R driver support

From: Chindris, Mihail
Date: Tue Oct 19 2021 - 07:46:54 EST

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, 10 October 2021 18:37
> To: Chindris, Mihail <Mihail.Chindris@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; lars@xxxxxxxxxx;
> Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>; Sa, Nuno
> <Nuno.Sa@xxxxxxxxxx>; Bogdan, Dragos <Dragos.Bogdan@xxxxxxxxxx>;
> alexandru.ardelean@xxxxxxxxxx
> Subject: Re: [PATCH v2 2/2] drivers:iio:dac: Add AD3552R driver support
>
> On Fri, 8 Oct 2021 12:39:09 +0000
> Mihail Chindris <mihail.chindris@xxxxxxxxxx> wrote:
>
> > The AD3552R-16 is a low drift ultrafast, 16-bit accuracy, current
> > output digital-to-analog converter (DAC) designed to generate multiple
> > output voltage span ranges.
> > The AD3552R-16 operates with a fixed 2.5V reference.
> >
> > Datasheet:
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ad
> > 3552r.pdf
> >
> > Signed-off-by: Mihail Chindris <mihail.chindris@xxxxxxxxxx>
>
> Hi Mihail,
>
> I would reduce the use of the attr and single function does everything approach
> as that is significantly reducing readability. Sometimes it's worth spending
> extra lines of code to remove abstractions that end up with a lot of special
> casing as is happening here.
>
> Other comments inline.
>
> > ---
> > drivers/iio/dac/Kconfig | 10 +
> > drivers/iio/dac/Makefile | 1 +
> > drivers/iio/dac/ad3552r.c | 1396
> > +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 1407 insertions(+)
> > create mode 100644 drivers/iio/dac/ad3552r.c
> >
> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index
> > 75e1f2b48638..ced6428f2c92 100644
> > --- a/drivers/iio/dac/Kconfig
> > +++ b/drivers/iio/dac/Kconfig
> > @@ -6,6 +6,16 @@
> >
> > menu "Digital to analog converters"
> >
> > +config AD3552R
> > + tristate "Analog Devices AD3552R DAC driver"
> > + depends on SPI_MASTER
> > + help
> > + Say yes here to build support for Analog Devices AD3552R
> > + Digital to Analog Converter.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called ad3552r.
> > +
> > config AD5064
> > tristate "Analog Devices AD5064 and similar multi-channel DAC driver"
> > depends on (SPI_MASTER && I2C!=m) || I2C diff --git
> > a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index
> > 33e16f14902a..dffe36efd8ff 100644
> > --- a/drivers/iio/dac/Makefile
> > +++ b/drivers/iio/dac/Makefile
> > @@ -4,6 +4,7 @@
> > #
> >
> > # When adding new entries keep the list in alphabetical order
> > +obj-$(CONFIG_AD3552R) += ad3552r.o
> > obj-$(CONFIG_AD5360) += ad5360.o
> > obj-$(CONFIG_AD5380) += ad5380.o
> > obj-$(CONFIG_AD5421) += ad5421.o
> > diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c new
> > file mode 100644 index 000000000000..42080183b77c
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad3552r.c
> > @@ -0,0 +1,1396 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices AD3552R
> > + * Digital to Analog converter driver
> > + *
> > + * Copyright 2021 Analog Devices Inc.
> > + */
> > +#include <asm/unaligned.h>
> > +#include <linux/device.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/triggered_buffer.h> #include
> > +<linux/iio/trigger_consumer.h> #include <linux/iopoll.h> #include
> > +<linux/kernel.h> #include <linux/regulator/consumer.h> #include
> > +<linux/spi/spi.h>
> > +
> > +/* Register addresses */
> > +/* Primary address space */
> > +#define AD3552R_REG_ADDR_INTERFACE_CONFIG_A 0x00
> > +#define AD3552R_MASK_SOFTWARE_RESET (BIT(7) |
> BIT(0))
> > +#define AD3552R_MASK_ADDR_ASCENSION BIT(5)
> > +#define AD3552R_MASK_SDO_ACTIVE BIT(4)
> > +#define AD3552R_REG_ADDR_INTERFACE_CONFIG_B 0x01
> > +#define AD3552R_MASK_SINGLE_INST BIT(7)
> > +#define AD3552R_MASK_SHORT_INSTRUCTION BIT(3)
> > +#define AD3552R_REG_ADDR_DEVICE_CONFIG 0x02
> > +#define AD3552R_MASK_DEVICE_STATUS(n) BIT(4
> + (n))
> > +#define AD3552R_MASK_CUSTOM_MODES GENMASK(3,
> 2)
> > +#define AD3552R_MASK_OPERATING_MODES
> GENMASK(1, 0)
> > +#define AD3552R_REG_ADDR_CHIP_TYPE 0x03
> > +#define AD3552R_MASK_CLASS GENMASK(7,
> 0)
> > +#define AD3552R_REG_ADDR_PRODUCT_ID_L 0x04
> > +#define AD3552R_REG_ADDR_PRODUCT_ID_H 0x05
> > +#define AD3552R_REG_ADDR_CHIP_GRADE 0x06
> > +#define AD3552R_MASK_GRADE GENMASK(7,
> 4)
> > +#define AD3552R_MASK_DEVICE_REVISION GENMASK(3,
> 0)
> > +#define AD3552R_REG_ADDR_SCRATCH_PAD 0x0A
> > +#define AD3552R_REG_ADDR_SPI_REVISION 0x0B
> > +#define AD3552R_REG_ADDR_VENDOR_L 0x0C
> > +#define AD3552R_REG_ADDR_VENDOR_H 0x0D
> > +#define AD3552R_REG_ADDR_STREAM_MODE 0x0E
> > +#define AD3552R_MASK_LENGTH GENMASK(7,
> 0)
> > +#define AD3552R_REG_ADDR_TRANSFER_REGISTER 0x0F
> > +#define AD3552R_MASK_MULTI_IO_MODE GENMASK(7,
> 6)
> > +#define AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE BIT(2)
> > +#define AD3552R_REG_ADDR_INTERFACE_CONFIG_C 0x10
> > +#define AD3552R_MASK_CRC_ENABLE (GENMASK(7,
> 6) |\
> > + GENMASK(1, 0))
> > +#define AD3552R_MASK_STRICT_REGISTER_ACCESS BIT(5)
> > +#define AD3552R_REG_ADDR_INTERFACE_STATUS_A 0x11
> > +#define AD3552R_MASK_INTERFACE_NOT_READY BIT(7)
> > +#define AD3552R_MASK_CLOCK_COUNTING_ERROR BIT(5)
> > +#define AD3552R_MASK_INVALID_OR_NO_CRC BIT(3)
> > +#define AD3552R_MASK_WRITE_TO_READ_ONLY_REGISTER BIT(2)
> > +#define AD3552R_MASK_PARTIAL_REGISTER_ACCESS BIT(1)
> > +#define AD3552R_MASK_REGISTER_ADDRESS_INVALID BIT(0)
> > +#define AD3552R_REG_ADDR_INTERFACE_CONFIG_D 0x14
> > +#define AD3552R_MASK_ALERT_ENABLE_PULLUP BIT(6)
> > +#define AD3552R_MASK_MEM_CRC_EN BIT(4)
> > +#define AD3552R_MASK_SDO_DRIVE_STRENGTH GENMASK(3,
> 2)
> > +#define AD3552R_MASK_DUAL_SPI_SYNCHROUNOUS_EN BIT(1)
> > +#define AD3552R_MASK_SPI_CONFIG_DDR BIT(0)
> > +#define AD3552R_REG_ADDR_SH_REFERENCE_CONFIG 0x15
> > +#define AD3552R_MASK_IDUMP_FAST_MODE BIT(6)
> > +#define AD3552R_MASK_SAMPLE_HOLD_DIFFERENTIAL_USER_EN BIT(5)
> > +#define AD3552R_MASK_SAMPLE_HOLD_USER_TRIM
> GENMASK(4, 3)
> > +#define AD3552R_MASK_SAMPLE_HOLD_USER_ENABLE BIT(2)
> > +#define AD3552R_MASK_REFERENCE_VOLTAGE_SEL GENMASK(1,
> 0)
> > +#define AD3552R_REG_ADDR_ERR_ALARM_MASK 0x16
> > +#define AD3552R_MASK_REF_RANGE_ALARM BIT(6)
> > +#define AD3552R_MASK_CLOCK_COUNT_ERR_ALARM BIT(5)
> > +#define AD3552R_MASK_MEM_CRC_ERR_ALARM BIT(4)
> > +#define AD3552R_MASK_SPI_CRC_ERR_ALARM BIT(3)
> > +#define AD3552R_MASK_WRITE_TO_READ_ONLY_ALARM BIT(2)
> > +#define AD3552R_MASK_PARTIAL_REGISTER_ACCESS_ALARM BIT(1)
> > +#define AD3552R_MASK_REGISTER_ADDRESS_INVALID_ALARM BIT(0)
> > +#define AD3552R_REG_ADDR_ERR_STATUS 0x17
> > +#define AD3552R_MASK_REF_RANGE_ERR_STATUS BIT(6)
> > +#define AD3552R_MASK_DUAL_SPI_STREAM_EXCEEDS_DAC_ERR_STATUS
> BIT(5)
> > +#define AD3552R_MASK_MEM_CRC_ERR_STATUS BIT(4)
> > +#define AD3552R_MASK_RESET_STATUS BIT(0)
> > +#define AD3552R_REG_ADDR_POWERDOWN_CONFIG 0x18
> > +#define AD3552R_MASK_CH_DAC_POWERDOWN(ch) BIT(4
> + (ch))
> > +#define AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(ch) BIT(ch)
> > +#define AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE 0x19
> > +#define AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch) ((ch) ?
> GENMASK(7, 4) :\
> > + GENMASK(3, 0))
> > +#define AD3552R_REG_ADDR_CH_OFFSET(ch) (0x1B
> + (ch) * 2)
> > +#define AD3552R_MASK_CH_OFFSET_BITS_0_7 GENMASK(7,
> 0)
> > +#define AD3552R_REG_ADDR_CH_GAIN(ch) (0x1C + (ch) *
> 2)
> > +#define AD3552R_MASK_CH_RANGE_OVERRIDE BIT(7)
> > +#define AD3552R_MASK_CH_GAIN_SCALING_N GENMASK(6,
> 5)
> > +#define AD3552R_MASK_CH_GAIN_SCALING_P GENMASK(4,
> 3)
> > +#define AD3552R_MASK_CH_OFFSET_POLARITY BIT(2)
> > +#define AD3552R_MASK_CH_OFFSET_BIT_8 BIT(0)
> > +/*
> > + * Secondary region
> > + * For multibyte registers specify the highest address because the
> > +access is
> > + * done in descending order
> > + */
> > +#define AD3552R_SECONDARY_REGION_START 0x28
> > +#define AD3552R_REG_ADDR_HW_LDAC_16B 0x28
> > +#define AD3552R_REG_ADDR_CH_DAC_16B(ch) (0x2C
> - (1 - ch) * 2)
> > +#define AD3552R_REG_ADDR_DAC_PAGE_MASK_16B 0x2E
> > +#define AD3552R_REG_ADDR_CH_SELECT_16B 0x2F
> > +#define AD3552R_REG_ADDR_INPUT_PAGE_MASK_16B 0x31
> > +#define AD3552R_REG_ADDR_SW_LDAC_16B 0x32
> > +#define AD3552R_REG_ADDR_CH_INPUT_16B(ch) (0x36 - (1 -
> ch) * 2)
> > +/* 3 bytes registers */
> > +#define AD3552R_REG_START_24B 0x37
> > +#define AD3552R_REG_ADDR_HW_LDAC_24B 0x37
> > +#define AD3552R_REG_ADDR_CH_DAC_24B(ch) (0x3D
> - (1 - ch) * 3)
> > +#define AD3552R_REG_ADDR_DAC_PAGE_MASK_24B 0x40
> > +#define AD3552R_REG_ADDR_CH_SELECT_24B 0x41
> > +#define AD3552R_REG_ADDR_INPUT_PAGE_MASK_24B 0x44
> > +#define AD3552R_REG_ADDR_SW_LDAC_24B 0x45
> > +#define AD3552R_REG_ADDR_CH_INPUT_24B(ch) (0x4B - (1 -
> ch) * 3)
> > +
> > +/* Useful defines */
> > +#define AD3552R_NUM_CH 2
> > +#define AD3552R_MASK_CH(ch) BIT(ch)
> > +#define AD3552R_MASK_ALL_CH GENMASK(1,
> 0)
> > +#define AD3552R_PAGE_CH 2
> > +#define AD3552R_MAX_REG_SIZE 3
> > +#define AD3552R_READ_BIT (1 << 7)
> > +#define AD3552R_ADDR_MASK GENMASK(6,
> 0)
> > +#define AD3552R_CRC_ENABLE_VALUE (BIT(6) |
> BIT(1))
> > +#define AD3552R_CRC_DISABLE_VALUE GENMASK(1,
> 0)
> > +#define AD3552R_CRC_POLY 0x07
> > +#define AD3552R_CRC_SEED 0xA5
> > +#define AD3552R_MASK_DAC_12B 0xFFF0
> > +#define AD3552R_DEFAULT_CONFIG_B_VALUE 0x8
> > +#define AD3552R_SCRATCH_PAD_TEST_VAL1 0x34
> > +#define AD3552R_SCRATCH_PAD_TEST_VAL2 0xB2
> > +#define AD3552R_GAIN_SCALE 1000
> > +#define AD3552R_LDAC_PULSE_US 100
> > +
> > +enum ad3552r_ch_vref_select {
> > + /* Internal source with Vref I/O floating */
> > + AD3552R_INTERNAL_VREF_PIN_FLOATING,
> > + /* Internal source with Vref I/O at 2.5V */
> > + AD3552R_INTERNAL_VREF_PIN_2P5V,
> > + /* External source with Vref I/O as input */
> > + AD3552R_EXTERNAL_VREF_PIN_INPUT
> > +};
> > +
> > +enum ad3542r_id {
> > + AD3542R_ID = 0x4008,
> > + AD3552R_ID = 0x4009,
> > +};
> > +
> > +enum ad3552r_ch_output_range {
> > + /* Range from 0 V to 2.5 V. Requires Rfb1x connection */
> > + AD3552R_CH_OUTPUT_RANGE_0__2P5V,
> > + /* Range from 0 V to 5 V. Requires Rfb1x connection */
> > + AD3552R_CH_OUTPUT_RANGE_0__5V,
> > + /* Range from 0 V to 10 V. Requires Rfb2x connection */
> > + AD3552R_CH_OUTPUT_RANGE_0__10V,
> > + /* Range from -5 V to 5 V. Requires Rfb2x connection */
> > + AD3552R_CH_OUTPUT_RANGE_NEG_5__5V,
> > + /* Range from -10 V to 10 V. Requires Rfb4x connection */
> > + AD3552R_CH_OUTPUT_RANGE_NEG_10__10V,
> > +};
> > +
> > +static const s32 ad3552r_ch_ranges[][2] = {
> > + [AD3552R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500},
> > + [AD3552R_CH_OUTPUT_RANGE_0__5V] = {0, 5000},
> > + [AD3552R_CH_OUTPUT_RANGE_0__10V] = {0, 10000},
> > + [AD3552R_CH_OUTPUT_RANGE_NEG_5__5V] = {-5000, 5000},
> > + [AD3552R_CH_OUTPUT_RANGE_NEG_10__10V] = {-10000, 10000}
> > +};
> > +
> > +enum ad3542r_ch_output_range {
> > + /* Range from 0 V to 2.5 V. Requires Rfb1x connection */
> > + AD3542R_CH_OUTPUT_RANGE_0__2P5V,
> > + /* Range from 0 V to 3 V. Requires Rfb1x connection */
> > + AD3542R_CH_OUTPUT_RANGE_0__3V,
> > + /* Range from 0 V to 5 V. Requires Rfb1x connection */
> > + AD3542R_CH_OUTPUT_RANGE_0__5V,
> > + /* Range from 0 V to 10 V. Requires Rfb2x connection */
> > + AD3542R_CH_OUTPUT_RANGE_0__10V,
> > + /* Range from -2.5 V to 7.5 V. Requires Rfb2x connection */
> > + AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V,
> > + /* Range from -5 V to 5 V. Requires Rfb2x connection */
> > + AD3542R_CH_OUTPUT_RANGE_NEG_5__5V,
> > +};
> > +
> > +static const s32 ad3542r_ch_ranges[][2] = {
> > + [AD3542R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500},
> > + [AD3542R_CH_OUTPUT_RANGE_0__3V] = {0, 3000},
> > + [AD3542R_CH_OUTPUT_RANGE_0__5V] = {0, 5000},
> > + [AD3542R_CH_OUTPUT_RANGE_0__10V] = {0, 10000},
> > + [AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V] = {-2500,
> 7500},
> > + [AD3542R_CH_OUTPUT_RANGE_NEG_5__5V] = {-5000, 5000}
> > +};
> > +
> > +enum ad3552r_ch_gain_scaling {
> > + /* Gain scaling of 1 */
> > + AD3552R_CH_GAIN_SCALING_1,
> > + /* Gain scaling of 0.5 */
> > + AD3552R_CH_GAIN_SCALING_0_5,
> > + /* Gain scaling of 0.25 */
> > + AD3552R_CH_GAIN_SCALING_0_25,
> > + /* Gain scaling of 0.125 */
> > + AD3552R_CH_GAIN_SCALING_0_125,
> > +};
> > +
> > +/* Gain * AD3552R_GAIN_SCALE */
> > +static const s32 gains_scaling_table[] = {
> > + [AD3552R_CH_GAIN_SCALING_1] = 1000,
> > + [AD3552R_CH_GAIN_SCALING_0_5] = 500,
> > + [AD3552R_CH_GAIN_SCALING_0_25] = 250,
> > + [AD3552R_CH_GAIN_SCALING_0_125] = 125
> > +};
> > +
> > +enum ad3552r_dev_attributes {
> > + /* - Direct register values */
> > + /* From 0-3 */
> > + AD3552R_SDO_DRIVE_STRENGTH,
> > + /*
> > + * 0 -> Internal Vref, vref_io pin floating (default)
> > + * 1 -> Internal Vref, vref_io driven by internal vref
> > + * 2 or 3 -> External Vref
> > + */
> > + AD3552R_VREF_SELECT,
> > + /* Enable / Disable CRC */
> > + AD3552R_CRC_ENABLE,
> > + /* Spi mode: Strandard, Dual or Quad */
> > + AD3552R_SPI_MULTI_IO_MODE,
> > + /* Spi data rate: Single or dual */
> > + AD3552R_SPI_DATA_RATE,
> > + /* Dual spi synchronous mode */
> > + AD3552R_SPI_SYNCHRONOUS_ENABLE,
> > +
> > + /* - Direct register values (Private) */
> > + /* Read registers in ascending order if set. Else descending */
> > + AD3552R_ADDR_ASCENSION,
> > + /* Single instruction mode if set. Else, stream mode */
> > + AD3552R_SINGLE_INST,
> > + /* Number of addresses to loop on when stream writing. */
> > + AD3552R_STREAM_MODE,
> > + /* Keep stream value if set. */
> > + AD3552R_STREAM_LENGTH_KEEP_VALUE,
> > +};
> > +
> > +enum ad3552r_ch_attributes {
> > + /* DAC powerdown */
> > + AD3552R_CH_DAC_POWERDOWN,
> > + /* DAC amplifier powerdown */
> > + AD3552R_CH_AMPLIFIER_POWERDOWN,
> > + /* Select the output range. Select from enum
> ad3552r_ch_output_range */
> > + AD3552R_CH_OUTPUT_RANGE_SEL,
> > + /*
> > + * Over-rider the range selector in order to manually set the output
> > + * voltage range
> > + */
> > + AD3552R_CH_RANGE_OVERRIDE,
> > + /* Manually set the offset voltage */
> > + AD3552R_CH_GAIN_OFFSET,
> > + /* Sets the polarity of the offset. */
> > + AD3552R_CH_GAIN_OFFSET_POLARITY,
> > + /* PDAC gain scaling */
> > + AD3552R_CH_GAIN_SCALING_P,
> > + /* NDAC gain scaling */
> > + AD3552R_CH_GAIN_SCALING_N,
> > + /* Trigger a software LDAC */
> > + AD3552R_CH_TRIGGER_SOFTWARE_LDAC,
> > + /* Hardware LDAC Mask */
> > + AD3552R_CH_HW_LDAC_MASK,
> > + /* Rfb value */
> > + AD3552R_CH_RFB,
> > + /* Channel select. When set allow Input -> DAC and Mask -> DAC */
> > + AD3552R_CH_SELECT,
> > + /* Raw value to be set to dac */
> > + AD3552R_CH_CODE
> > +};
> > +
> > +struct ad3552r_ch_data {
> > + u16 gain_offset;
> > + bool range_override;
> > + u8 n;
> > + u8 p;
> > + bool offset_polarity;
> > + u16 rfb;
> > + u8 range;
>
> Unless there is a strong reason not to, group types as it will use less memory as
> we need less padding.
>
> > + s32 scale_int;
> > + s32 scale_dec;
> > + s32 offset_int;
> > + s32 offset_dec;
> > +};
> > +
> > +struct ad3552r_desc {
> > + /* Used to look the spi bus for atomic operations where needed */
> > + struct mutex lock;
> > + struct gpio_desc *gpio_reset;
> > + struct gpio_desc *gpio_ldac;
> > + struct spi_device *spi;
> > + struct ad3552r_ch_data ch_data[AD3552R_NUM_CH];
> > + struct iio_chan_spec channels[AD3552R_NUM_CH + 1];
> > + unsigned long enabled_ch;
> > + unsigned int num_ch;
> > + enum ad3542r_id chip_id;
> > + /*
> > + * The maximum spi transfer size consist 1 bytes (reg address)
> > + * + 2 registers of 3 bytes + 1 reg of 1 byte (SW LDAC)
> > + */
> > + u8 buf_data[8] ____cacheline_aligned; };
> > +
> > +static const u16 addr_mask_map[][2] = {
> > + [AD3552R_ADDR_ASCENSION] = {
> > + AD3552R_REG_ADDR_INTERFACE_CONFIG_A,
> > + AD3552R_MASK_ADDR_ASCENSION
> > + },
> > + [AD3552R_SINGLE_INST] = {
> > + AD3552R_REG_ADDR_INTERFACE_CONFIG_B,
> > + AD3552R_MASK_SINGLE_INST
> > + },
> > + [AD3552R_STREAM_MODE] = {
> > + AD3552R_REG_ADDR_STREAM_MODE,
> > + AD3552R_MASK_LENGTH
> > + },
> > + [AD3552R_STREAM_LENGTH_KEEP_VALUE] = {
> > + AD3552R_REG_ADDR_TRANSFER_REGISTER,
> > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE
> > + },
> > + [AD3552R_SDO_DRIVE_STRENGTH] = {
> > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> > + AD3552R_MASK_SDO_DRIVE_STRENGTH
> > + },
> > + [AD3552R_VREF_SELECT] = {
> > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG,
> > + AD3552R_MASK_REFERENCE_VOLTAGE_SEL
> > + },
> > + [AD3552R_CRC_ENABLE] = {
> > + AD3552R_REG_ADDR_INTERFACE_CONFIG_C,
> > + AD3552R_MASK_CRC_ENABLE
> > + },
> > + [AD3552R_SPI_MULTI_IO_MODE] = {
> > + AD3552R_REG_ADDR_TRANSFER_REGISTER,
> > + AD3552R_MASK_MULTI_IO_MODE
> > + },
> > + [AD3552R_SPI_DATA_RATE] = {
> > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> > + AD3552R_MASK_SPI_CONFIG_DDR
> > + },
> > + [AD3552R_SPI_SYNCHRONOUS_ENABLE] = {
> > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> > + AD3552R_MASK_DUAL_SPI_SYNCHROUNOUS_EN
> > + },
> > +};
> > +
> > +/* 0 -> reg addr, 1->ch0 mask, 2->ch1 mask */ static const u16
> > +addr_mask_map_ch[][3] = {
> > + [AD3552R_CH_DAC_POWERDOWN] = {
> > + AD3552R_REG_ADDR_POWERDOWN_CONFIG,
> > + AD3552R_MASK_CH_DAC_POWERDOWN(0),
> > + AD3552R_MASK_CH_DAC_POWERDOWN(1)
> > + },
> > + [AD3552R_CH_AMPLIFIER_POWERDOWN] = {
> > + AD3552R_REG_ADDR_POWERDOWN_CONFIG,
> > + AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(0),
> > + AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(1)
> > + },
> > + [AD3552R_CH_OUTPUT_RANGE_SEL] = {
> > + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE,
> > + AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0),
> > + AD3552R_MASK_CH_OUTPUT_RANGE_SEL(1)
> > + },
> > + /*
> > + * This attributes are update by the chip on 16B and 24B no matter to
> > + * what register the write is done
> > + */
> > + [AD3552R_CH_TRIGGER_SOFTWARE_LDAC] = {
> > + AD3552R_REG_ADDR_SW_LDAC_16B,
> > + AD3552R_MASK_CH(0),
> > + AD3552R_MASK_CH(1)
> > + },
> > + [AD3552R_CH_HW_LDAC_MASK] = {
> > + AD3552R_REG_ADDR_HW_LDAC_16B,
> > + AD3552R_MASK_CH(0),
> > + AD3552R_MASK_CH(1)
> > + },
> > + [AD3552R_CH_SELECT] = {
> > + AD3552R_REG_ADDR_CH_SELECT_16B,
> > + AD3552R_MASK_CH(0),
> > + AD3552R_MASK_CH(1)
> > + }
> > +};
> > +
> > +static u8 _ad3552r_reg_len(u8 addr)
> > +{
> > + switch (addr) {
> > + case AD3552R_REG_ADDR_HW_LDAC_16B:
> > + case AD3552R_REG_ADDR_CH_SELECT_16B:
> > + case AD3552R_REG_ADDR_SW_LDAC_16B:
> > + case AD3552R_REG_ADDR_HW_LDAC_24B:
> > + case AD3552R_REG_ADDR_CH_SELECT_24B:
> > + case AD3552R_REG_ADDR_SW_LDAC_24B:
> > + return 1;
> > + default:
> > + break;
> > + }
> > +
> > + if (addr > AD3552R_REG_ADDR_HW_LDAC_24B)
> > + return 3;
> > + if (addr > AD3552R_REG_ADDR_HW_LDAC_16B)
> > + return 2;
> > +
> > + return 1;
> > +}
> > +
> > +/* SPI transfer to device */
> > +static int ad3552r_transfer(struct ad3552r_desc *dac, u8 addr, u32 len,
> > + u8 *data, bool is_read)
> > +{
> > + int err;
> > + u8 instr;
> > +
> > + instr = addr & AD3552R_ADDR_MASK;
> > + instr |= is_read ? AD3552R_READ_BIT : 0;
> > + dac->buf_data[0] = instr;
> > + if (is_read) {
> > + err = spi_write_then_read(dac->spi, dac->buf_data, 1,
> > + dac->buf_data + 1, len);
> > + if (err)
> > + return err;
> > +
> > + memcpy(data, dac->buf_data + 1, len);
> IIRC spi_write_the_read() doesn't require a dma safe buffer. As such you can
> provide the *data pointer directly to the spi_write_the_read call and avoid
> copying it twice.
>
> Note that's no the case for spi_write(), but you can call spi_write_then_read
> with a zero length n_rx if you want to and avoid the explicit memcpy here.
>
> > +
> > + return 0;
> > + }
> > +
> > + memcpy(dac->buf_data + 1, data, len);
> > + return spi_write(dac->spi, dac->buf_data, len + 1); }
> > +
> > +static int ad3552r_write_reg(struct ad3552r_desc *dac, u8 addr, u16
> > +val) {
> > + u8 reg_len;
> > + u8 buf[AD3552R_MAX_REG_SIZE] = { 0 };
> > +
> > + reg_len = _ad3552r_reg_len(addr);
> > + if (reg_len == 2)
> > + /* Only DAC register are 2 bytes wide */
> > + val &= AD3552R_MASK_DAC_12B;
> > + if (reg_len == 1)
> > + buf[0] = val & 0xFF;
> > + else
> > + /* reg_len can be 2 or 3, but 3rd bytes needs to be set to 0 */
> > + put_unaligned_be16(val, buf);
> > +
> > + return ad3552r_transfer(dac, addr, reg_len, buf, false); }
> > +
> > +static int ad3552r_read_reg(struct ad3552r_desc *dac, u8 addr, u16
> > +*val) {
> > + int err;
> > + u8 reg_len, buf[AD3552R_MAX_REG_SIZE] = { 0 };
> > +
> > + reg_len = _ad3552r_reg_len(addr);
> > + err = ad3552r_transfer(dac, addr, reg_len, buf, true);
> > + if (err)
> > + return err;
> > +
> > + if (reg_len == 1)
> > + *val = buf[0];
> > + else
> > + /* reg_len can be 2 or 3, but only first 2 bytes are relevant */
> > + *val = get_unaligned_be16(buf);
> > +
> > + return 0;
> > +}
> > +
> > +/* Update field of a register, shift val if needed */ static int
> > +ad3552r_update_reg_field(struct ad3552r_desc *dac, u8 addr, u16 mask,
> > + u16 val)
> > +{
> > + int ret;
> > + u16 reg;
> > +
> > + ret = ad3552r_read_reg(dac, addr, &reg);
> > + if (ret < 0)
> > + return ret;
> > +
> > + reg = (reg & ~mask) | (val << __ffs(mask));
> > +
> > + return ad3552r_write_reg(dac, addr, reg); }
> > +
> > +static int ad3552r_set_dev_value(struct ad3552r_desc *dac,
> > + enum ad3552r_dev_attributes attr,
> > + u16 val)
> > +{
> > + switch (attr) {
> > + case AD3552R_SPI_MULTI_IO_MODE:
> > + case AD3552R_SPI_DATA_RATE:
> > + case AD3552R_SPI_SYNCHRONOUS_ENABLE:
> > + case AD3552R_CRC_ENABLE:
> > + /* Not implemented */
> > + return -EINVAL;
> > + default:
> > + return ad3552r_update_reg_field(dac,
> addr_mask_map[attr][0],
> > + addr_mask_map[attr][1], val);
> > + }
> > +
>
> Can't get here so drop this final return. However, given how simple this is I'd
> just put the update_reg inline in the caller locations.
>
> > + return 0;
> > +}
> > +
> > +static int ad3552r_set_offset_value(struct ad3552r_desc *dac, u8 ch,
> > +int val) {
> > + int err;
> > +
> > + err = ad3552r_write_reg(dac, AD3552R_REG_ADDR_CH_OFFSET(ch),
> > + val & AD3552R_MASK_CH_OFFSET_BITS_0_7);
> > + if (err)
> > + return err;
> > +
> > + err = ad3552r_update_reg_field(dac,
> > + AD3552R_REG_ADDR_CH_GAIN(ch),
> > + AD3552R_MASK_CH_OFFSET_BIT_8,
> > + (val >> 8) &
> AD3552R_MASK_CH_OFFSET_BIT_8);
> > + if (err)
> > + return err;
> > +
> > + dac->ch_data[ch].gain_offset = val;
> > +
> > + return 0;
> > +}
> > +
> > +static int ad3552r_set_gain_value(struct ad3552r_desc *dac,
> > + enum ad3552r_ch_attributes attr,
> > + u8 ch,
> > + int val)
> > +{
> > + int reg_mask, err;
> > +
> > + if (attr == AD3552R_CH_GAIN_OFFSET)
> > + return ad3552r_set_offset_value(dac, ch, val);
> > +
> > + switch (attr) {
> > + case AD3552R_CH_RANGE_OVERRIDE:
>
> This is more complex than just having a bunch of functions, one for each case
> (assuming they are called from multiple locations - if not just have the code
> inline).
>
> > + val = !!val;
> > + reg_mask = AD3552R_MASK_CH_RANGE_OVERRIDE;
> > + break;
> > + case AD3552R_CH_GAIN_OFFSET_POLARITY:
> > + val = !!val;
> > + reg_mask = AD3552R_MASK_CH_OFFSET_POLARITY;
> > + break;
> > + case AD3552R_CH_GAIN_SCALING_P:
> > + reg_mask = AD3552R_MASK_CH_GAIN_SCALING_P;
> > + break;
> > + case AD3552R_CH_GAIN_SCALING_N:
> > + reg_mask = AD3552R_MASK_CH_GAIN_SCALING_N;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + err = ad3552r_update_reg_field(dac,
> AD3552R_REG_ADDR_CH_GAIN(ch),
> > + reg_mask, val);
> > + if (err)
> > + return err;
> > +
> > + switch (attr) {
> > + case AD3552R_CH_RANGE_OVERRIDE:
> > + dac->ch_data[ch].range_override = val;
> > + break;
> > + case AD3552R_CH_GAIN_OFFSET_POLARITY:
> > + dac->ch_data[ch].offset_polarity = val;
> > + break;
> > + case AD3552R_CH_GAIN_SCALING_P:
> > + dac->ch_data[ch].p = val;
> > + break;
> > + case AD3552R_CH_GAIN_SCALING_N:
> > + dac->ch_data[ch].n = val;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* Iterate over mask and write required bytes */ static int
> > +ad3552r_write_codes(struct ad3552r_desc *dac, u32 mask, u8 *data) {
> > + int err, i, len, k = 0, ch_mask;
> > + unsigned long lmask = mask;
> > + u8 addr, buff[AD3552R_NUM_CH * AD3552R_MAX_REG_SIZE + 1];
> > + u16 val;
> > +
> > + /* If writing to consecutive registers do just one transfer */
> > +
> > + if (mask == AD3552R_MASK_ALL_CH) {
> > + addr = AD3552R_REG_ADDR_CH_INPUT_24B(1);
> > + /* CH1 */
> > + buff[0] = data[2];
> > + buff[1] = data[3];
> > + buff[2] = 0;
> > + /* CH0 */
> > + buff[3] = data[0];
> > + buff[4] = data[1];
> > + buff[5] = 0;
> > + len = 6;
> > + if (!dac->gpio_ldac) {
> > + /* Software LDAC */
> > + buff[6] = AD3552R_MASK_ALL_CH;
> > + ++len;
> > + }
> > + err = ad3552r_transfer(dac, addr, len, buff, false);
> > + if (err)
> > + return err;
> > +
> > + } else {
> > + k = 0;
> > + for_each_set_bit(i, &lmask, AD3552R_NUM_CH + 1) {
> > + /* Writing to mask CH */
> > + if (i == AD3552R_PAGE_CH) {
> > + addr =
> AD3552R_REG_ADDR_INPUT_PAGE_MASK_24B;
> > + ch_mask = AD3552R_MASK_ALL_CH;
> > + } else {
> > + addr =
> AD3552R_REG_ADDR_CH_INPUT_24B(i);
> > + ch_mask = AD3552R_MASK_CH(i);
> > + }
> > +
> > + val = be16_to_cpu(*((u16 *)(data + k)));
> > + k += 2;
> > + err = ad3552r_write_reg(dac, addr, val);
> > + if (err)
> > + return err;
> > +
> > + if (!dac->gpio_ldac) {
> > + err = ad3552r_write_reg(dac,
> > +
> AD3552R_REG_ADDR_SW_LDAC_24B,
> > + ch_mask);
> > + if (err)
> > + return err;
> > + }
> > + }
> > + }
> > +
> > + if (dac->gpio_ldac) {
> > + gpiod_set_value_cansleep(dac->gpio_ldac, 0);
> > + usleep_range(AD3552R_LDAC_PULSE_US,
> AD3552R_LDAC_PULSE_US + 10);
> > + gpiod_set_value_cansleep(dac->gpio_ldac, 1);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ad3552r_get_ch_value(struct ad3552r_desc *dac,
> > + enum ad3552r_ch_attributes attr,
> > + u8 ch,
> > + u16 *val)
> > +{
> > + int ret;
> > + u16 reg;
> > + u8 addr;
> > + u16 mask;
> > +
> > + /* Attributes not defined in addr_mask_map_ch */
> > + switch (attr) {
> > + case AD3552R_CH_CODE:
> > + return ad3552r_read_reg(dac,
> AD3552R_REG_ADDR_CH_DAC_24B(ch),
>
> As below, just have multiple functions rather than trying to hammer this
> complexity into a single get_ch_value function.
>
> > + val);
> > + case AD3552R_CH_RFB:
> > + *val = dac->ch_data[ch].rfb;
> > + return 0;
> > + default:
> > + break;
> > + }
> > +
> > + addr = addr_mask_map_ch[attr][0];
> > + ret = ad3552r_read_reg(dac, addr, &reg);
> > + if (ret < 0)
> > + return ret;
> > +
> > + mask = addr_mask_map_ch[attr][ch + 1];
> > + *val = (reg & mask) >> __ffs(mask);
> > +
> > + return 0;
> > +}
> > +
> > +static int ad3552r_set_ch_value(struct ad3552r_desc *dac,
>
> Given the amount of 'special' case code in here for the different attrs I'd just
> use separate functions for each of them.
>
> > + enum ad3552r_ch_attributes attr,
> > + u8 ch,
> > + u16 val)
> > +{
> > + int ret;
> > +
> > + /* Attributes not defined in addr_mask_map_ch */
> > + switch (attr) {
> > + case AD3552R_CH_CODE:
> > + return ad3552r_write_reg(dac,
> AD3552R_REG_ADDR_CH_DAC_24B(ch),
> > + val);
> > + case AD3552R_CH_RFB:
> > + dac->ch_data[ch].rfb = val;
> > + return 0;
> > + default:
> > + break;
> > + }
> > +
> > + if (attr >= AD3552R_CH_RANGE_OVERRIDE &&
> > + attr <= AD3552R_CH_GAIN_SCALING_N)
> > + return ad3552r_set_gain_value(dac, attr, ch, val);
> > +
> > + /* Update register related to attributes in chip */
> > + ret = ad3552r_update_reg_field(dac, addr_mask_map_ch[attr][0],
> > + addr_mask_map_ch[attr][ch + 1], val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Update software structures */
> > + if (attr == AD3552R_CH_OUTPUT_RANGE_SEL)
> > + dac->ch_data[ch].range = val;
> > +
> > + return ret;
> > +}
> > +
> > +#define AD3552R_CH_DAC(_idx) ((struct iio_chan_spec) { \
> > + .type = IIO_VOLTAGE, \
> > + .output = true, \
> > + .indexed = true, \
> > + .channel = _idx, \
> > + .scan_index = _idx, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 16, \
> > + .storagebits = 16, \
> > + .endianness = IIO_BE, \
> > + }, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE) | \
> > + BIT(IIO_CHAN_INFO_ENABLE) | \
> > + BIT(IIO_CHAN_INFO_OFFSET), \
> > +})
> > +
> > +/*
> > + * Channel used to update both CH0 and CH1 with the same value by
> > + * writing only one register.
>
> Hmm. This is rather unusual use of the ABI.
> In a similar fashion to us only guaranteeing 'simultaneous sampling'
> when using buffered mode for input devices, I think we should map this only
> being possible if in buffered output mode.
>
> Then this is only useful to either minimise access or when the hardware 'go'
> signal isn't wired up. So I'd suggest that path does a check for the same value
> being written to both channels and uses this if appropriate.
>
> > + */
> > +#define AD3552R_CH_DAC_PAGE(_idx) ((struct iio_chan_spec) { \
> > + .type = IIO_VOLTAGE, \
> > + .output = true, \
> > + .indexed = true, \
> > + .channel = _idx, \
> > + .scan_index = _idx, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 16, \
> > + .storagebits = 16, \
> > + .endianness = IIO_BE, \
> > + }, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .modified = 1, \
> > + .channel2 = IIO_MOD_X_AND_Z, \
> ? Not an obvious choice even if I was happy with this approach in general.

So the chip has this feature of updating both channels at the same time with the same value.
Also, I found this ABI : https://www.kernel.org/doc/html/latest/admin-guide/abi-testing.html#abi-sys-bus-iio-devices-iio-devicex-out-voltagey-z-raw
And as per my understanding it maps well to the chip feature.
I don't know a use I just found the ABI and I though it would make sense.
I searched in to the kernel but it seems this modifier is not used anywhere so I couldn't double check it utility.
Should I keep this channel or remove it?

Mihail

>
> > +})
> > +
> > +static int ad3552r_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val,
> > + int *val2,
> > + long mask)
> > +{
> > + struct ad3552r_desc *dac = iio_priv(indio_dev);
> > + u16 tmp_val;
> > + int err;
> > + u8 ch = chan->channel;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&dac->lock);
> > + if (chan->channel == AD3552R_PAGE_CH)
> > + err = ad3552r_read_reg(dac,
> > +
> AD3552R_REG_ADDR_DAC_PAGE_MASK_24B,
> > + &tmp_val);
> > + else
> > + err = ad3552r_get_ch_value(dac, AD3552R_CH_CODE,
> ch,
> > + &tmp_val);
> > + if (err < 0) {
> > + mutex_unlock(&dac->lock);
> > + return err;
> > + }
> > +
> > + *val = tmp_val;
> > + mutex_unlock(&dac->lock);
> > + break;
> > + case IIO_CHAN_INFO_ENABLE:
> > + mutex_lock(&dac->lock);
> > + err = ad3552r_get_ch_value(dac,
> AD3552R_CH_DAC_POWERDOWN,
> > + ch, &tmp_val);
> > + if (err < 0) {
> > + mutex_unlock(&dac->lock);
> > + return err;
> > + }
> > + *val = !tmp_val;
> > + mutex_unlock(&dac->lock);
> > + break;
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = dac->ch_data[ch].scale_int;
> > + *val2 = dac->ch_data[ch].scale_dec;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_OFFSET:
> > + *val = dac->ch_data[ch].offset_int;
> > + *val2 = dac->ch_data[ch].offset_dec;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return IIO_VAL_INT;
> > +}
> > +
> > +static int ad3552r_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val,
> > + int val2,
> > + long mask)
> > +{
> > + struct ad3552r_desc *dac = iio_priv(indio_dev);
> > + enum ad3552r_ch_attributes attr;
> > + int err = 0;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + if (chan->channel == AD3552R_PAGE_CH) {
> > + mutex_lock(&dac->lock);
> > + err = ad3552r_write_reg(dac,
> > +
> AD3552R_REG_ADDR_DAC_PAGE_MASK_24B,
> > + val);
> > + mutex_unlock(&dac->lock);
> > +
> > + return err;
> > + }
> > +
> > + attr = AD3552R_CH_CODE;
> > + break;
> > + case IIO_CHAN_INFO_ENABLE:
> > + attr = AD3552R_CH_DAC_POWERDOWN;
> > + val = !val;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + mutex_lock(&dac->lock);
> > + err = ad3552r_set_ch_value(dac, attr, chan->channel, val);
> > + mutex_unlock(&dac->lock);
> > +
> > + return err;
> > +}
> > +
> > +static int ad3552r_update_scan_mode(struct iio_dev *indio_dev,
> > + const unsigned long *scan_mask) {
> > + u32 mask;
> > +
> > + mask = *scan_mask;
> > + /* If writing to mask, can't write to other channels */
> > + if ((mask & AD3552R_MASK_CH(AD3552R_PAGE_CH)) &&
> > + (mask & (~AD3552R_MASK_CH(AD3552R_PAGE_CH))))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Device type specific information.
> > + */
> > +static const struct iio_info ad3552r_iio_info = {
> > + .read_raw = ad3552r_read_raw,
> > + .write_raw = ad3552r_write_raw,
> > + .update_scan_mode = ad3552r_update_scan_mode };
> > +
> > +static irqreturn_t ad3552r_trigger_handler(int irq, void *p) {
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct iio_buffer *buf = indio_dev->buffer;
> > + struct ad3552r_desc *dac = iio_priv(indio_dev);
> > + char buff[AD3552R_NUM_CH *
> AD3552R_MAX_REG_SIZE];
> > + int err;
> > +
> > + memset(buff, 0, sizeof(buff));
> > + err = iio_pop_from_buffer(buf, buff);
> > + if (err)
> > + goto end;
> > +
> > + mutex_lock(&dac->lock);
> > + err = ad3552r_write_codes(dac, *indio_dev->active_scan_mask, buff);
> > + mutex_unlock(&dac->lock);
> > + if (err)
> > + goto end;
> Drop this given we are going to exactly where we would otherwise be. Note
> one of the static checkers will probably complain about thsi.
> > +
> > +end:
> > + iio_trigger_notify_done(indio_dev->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int ad3552r_check_scratch_pad(struct ad3552r_desc *dac) {
> > + const u16 val1 = AD3552R_SCRATCH_PAD_TEST_VAL1;
> > + const u16 val2 = AD3552R_SCRATCH_PAD_TEST_VAL2;
> > + u16 val;
> > + int err;
> > +
> > + err = ad3552r_write_reg(dac, AD3552R_REG_ADDR_SCRATCH_PAD,
> val1);
> > + if (err < 0)
> > + return err;
> > +
> > + err = ad3552r_read_reg(dac, AD3552R_REG_ADDR_SCRATCH_PAD,
> &val);
> > + if (err < 0)
> > + return err;
> > +
> > + if (val1 != val)
> > + return -ENODEV;
> > +
> > + err = ad3552r_write_reg(dac, AD3552R_REG_ADDR_SCRATCH_PAD,
> val2);
> > + if (err < 0)
> > + return err;
> > +
> > + err = ad3552r_read_reg(dac, AD3552R_REG_ADDR_SCRATCH_PAD,
> &val);
> > + if (err < 0)
> > + return err;
> > +
> > + if (val2 != val)
> > + return -ENODEV;
> > +
> > + return 0;
> > +}
> > +
> > +struct reg_addr_pool {
> > + struct ad3552r_desc *dac;
> > + u8 addr;
> > +};
> > +
> > +static u16 ad3552r_read_reg_wrapper(struct reg_addr_pool *addr) {
> > + u16 val = 0;
> > +
> > + ad3552r_read_reg(addr->dac, addr->addr, &val);
> What about potential errors? If we need to mask them then add a comment
> here on why.
> > +
> > + return val;
> > +}
> > +
> > +static int ad3552r_reset(struct ad3552r_desc *dac) {
> > + struct reg_addr_pool addr;
> > + int ret;
> > + u16 val;
> > +
> > + dac->gpio_reset = devm_gpiod_get_optional(&dac->spi->dev, "reset",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(dac->gpio_reset))
> > + return PTR_ERR(dac->gpio_reset);
>
> For a lot of similar paths we should use dev_err_probe() so as to register a
> 'reason' if we need to defer + provide a bit more debugging info if it is a
> different error.
>
> > +
> > + if (dac->gpio_reset) {
> > + /* Perform hardware reset */
> > + usleep_range(10, 20);
> > + gpiod_set_value_cansleep(dac->gpio_reset, 1);
> > + } else {
> > + /* Perform software reset if no GPIO provided */
> > + ret = ad3552r_update_reg_field(dac,
> AD3552R_REG_ADDR_INTERFACE_CONFIG_A,
> > +
> AD3552R_MASK_SOFTWARE_RESET,
> > +
> AD3552R_MASK_SOFTWARE_RESET);
> > + if (ret < 0)
> > + return ret;
> > +
> > + }
> > +
> > + addr.dac = dac;
> > + addr.addr = AD3552R_REG_ADDR_INTERFACE_CONFIG_B;
> > + ret = readx_poll_timeout(ad3552r_read_reg_wrapper, &addr, val,
> > + val == AD3552R_DEFAULT_CONFIG_B_VALUE,
> > + 5000, 50000);
> > + if (ret) {
> > + dev_err(&dac->spi->dev, "Err: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = readx_poll_timeout(ad3552r_read_reg_wrapper, &addr, val,
> > + !(val &
> AD3552R_MASK_INTERFACE_NOT_READY),
> > + 5000, 50000);
> > + if (ret) {
> > + dev_err(&dac->spi->dev, "Err: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return ad3552r_set_dev_value(dac, AD3552R_ADDR_ASCENSION, 0); }
> > +
> > +static void ad3552r_get_custom_range(struct ad3552r_desc *dac, s32 i, s32
> *v_min,
> > + s32 *v_max)
> > +{
> > + s64 vref, tmp, common, offset, gn, gp;
> > + /*
> > + * From datasheet formula (In Volts):
> > + * Vmin = 2.5 + [(GainN + Offset / 1024) * 2.5 * Rfb * 1.03]
> > + * Vmax = 2.5 - [(GainP + Offset / 1024) * 2.5 * Rfb * 1.03]
> > + * Calculus are converted to milivolts
> > + */
> > + vref = 2500;
> > + /* 2.5 * 1.03 * 1000 (To mV) */
> > + common = 2575 * dac->ch_data[i].rfb;
> > + offset = dac->ch_data[i].gain_offset;
> > + if (dac->ch_data[i].offset_polarity)
> > + offset *= -1;
> > +
> > + gn = gains_scaling_table[dac->ch_data[i].n];
> > + tmp = (1024 * gn + AD3552R_GAIN_SCALE * offset) * common;
> > + tmp = div_s64(tmp, 1024 * AD3552R_GAIN_SCALE);
> > + *v_max = vref + tmp;
> > +
> > + gp = gains_scaling_table[dac->ch_data[i].p];
> > + tmp = (1024 * gp - AD3552R_GAIN_SCALE * offset) * common;
> > + tmp = div_s64(tmp, 1024 * AD3552R_GAIN_SCALE);
> > + *v_min = vref - tmp;
> > +}
> > +
> > +static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac,
> > +s32 ch) {
> > + s32 idx, v_max, v_min, span, rem;
> > + s64 tmp;
> > +
> > + if (dac->ch_data[ch].range_override) {
> > + ad3552r_get_custom_range(dac, ch, &v_min, &v_max);
> > + } else {
> > + /* Normal range */
> > + idx = dac->ch_data[ch].range;
> > + if (dac->chip_id == AD3542R_ID) {
> > + v_min = ad3542r_ch_ranges[idx][0];
> > + v_max = ad3542r_ch_ranges[idx][1];
> > + } else {
> > + v_min = ad3552r_ch_ranges[idx][0];
> > + v_max = ad3552r_ch_ranges[idx][1];
> > + }
> > + }
> > +
> > + /*
> > + * From datasheet formula:
> > + * Vout = Span * (D / 65536) + Vmin
> > + * Converted to scale and offset:
> > + * Scale = Span / 65536
> > + * Offset = 65536 * Vmin / Span
> > + *
> > + * Reminders are in micros in order to be printed as
> > + * IIO_VAL_INT_PLUS_MICRO
> > + */
> > + span = v_max - v_min;
> > + dac->ch_data[ch].scale_int = div_s64_rem(span, 65536, &rem);
> > + /* Do operations in microvolts */
> > + dac->ch_data[ch].scale_dec = DIV_ROUND_CLOSEST((s64)rem *
> 1000000,
> > + 65536);
> > +
> > + dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536, span,
> > + &rem);
> > + tmp = (s64)rem * 1000000;
> > + dac->ch_data[ch].offset_dec = div_s64(tmp, span); }
> > +
> > +static const char * const gain_dts_names[] = {
> > + "adi,gain-scaling-p",
> > + "adi,gain-scaling-n",
> > + "adi,rfb-ohms"
> > +};
> > +
> > +static int ad3552r_find_range(u16 id, u32 *vals) {
> > + int i, len;
> > + const s32 (*ranges)[2];
>
> Why are we comparing signed and unsigned values in here?
> Should vals be signed as well?
>
> > +
> > + if (id == AD3542R_ID) {
> > + len = ARRAY_SIZE(ad3542r_ch_ranges);
> > + ranges = ad3542r_ch_ranges;
> > + } else {
> > + len = ARRAY_SIZE(ad3552r_ch_ranges);
> > + ranges = ad3552r_ch_ranges;
> > + }
> > +
> > + for (i = 0; i < len; i++)
> > + if (vals[0] == ranges[i][0] * 1000 &&
> > + vals[1] == ranges[i][1] * 1000)
> > + return i;
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
> > + struct fwnode_handle *child,
> > + u32 ch)
> > +{
> > + static const enum ad3552r_ch_attributes gain_attrs[] = {
> > + AD3552R_CH_GAIN_SCALING_P,
> > + AD3552R_CH_GAIN_SCALING_N,
> > + AD3552R_CH_RFB
> > + };
> > + struct device *dev = &dac->spi->dev;
> > + struct fwnode_handle *custom_gain_child;
> > + u32 val;
> > + int err, i;
> > +
> > + custom_gain_child = fwnode_get_named_child_node(child,
> > + "custom-output-
> range-config");
> > + if (IS_ERR(custom_gain_child)) {
> > + dev_err(dev,
> > + "mandatory custom-output-range-config property
> missing\n");
> > + return PTR_ERR(custom_gain_child);
> > + }
> > +
> > + err = fwnode_property_read_u32(custom_gain_child, "adi,gain-offset",
> > + &val);
> > + if (err) {
> > + dev_err(dev,
> > + "mandatory adi,gain-offset property missing\n");
> > + goto put_child;
> > + }
> > +
> > + err = ad3552r_set_ch_value(dac, AD3552R_CH_GAIN_OFFSET, ch,
> > + abs((s32)val));
> > + if (err)
> > + goto put_child;
> > +
> > + err = ad3552r_set_ch_value(dac,
> AD3552R_CH_GAIN_OFFSET_POLARITY,
> > + ch, (s32)val < 0);
> > + if (err)
> > + goto put_child;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gain_attrs); ++i) {
> > + err = fwnode_property_read_u32(custom_gain_child,
> > + gain_dts_names[i], &val);
> > + if (err) {
> > + dev_err(dev, "mandatory %s property missing\n",
> > + gain_dts_names[i]);
> > + goto put_child;
> > + }
> > +
> > + err = ad3552r_set_ch_value(dac, gain_attrs[i], ch, val);
> > + if (err)
> > + goto put_child;
> > + }
> > +
> Don't we need to put the custom_gain_child node in the good path as well as
> the bad one?
> > + return 0;
> > +
> > +put_child:
> > + fwnode_handle_put(custom_gain_child);
> > +
> > + return err;
> > +}
> > +
> > +static int ad3552r_configure_device(struct ad3552r_desc *dac) {
> > + struct device *dev = &dac->spi->dev;
> > + struct fwnode_handle *child;
> > + struct regulator *vref;
> > + int err, cnt = 0, voltage, delta = 100000;
> > + u32 vals[2], ch;
> > + bool is_custom;
> > +
> > + dac->gpio_ldac = devm_gpiod_get_optional(dev, "ldac",
> GPIOD_OUT_HIGH);
> > + if (IS_ERR(dac->gpio_ldac))
> > + return PTR_ERR(dac->gpio_ldac);
> > +
> > + vref = devm_regulator_get_optional(dev, "vref");
> > + if (IS_ERR(vref)) {
> > + if (PTR_ERR(vref) != -ENODEV)
> > + return PTR_ERR(vref);
> > + vref = NULL;
> > + }
> > + if (vref) {
> > + voltage = regulator_get_voltage(vref);
> > + if (voltage > 2500000 + delta || voltage < 2500000 - delta) {
> > + dev_err(dev, "vref-supply must be 2.5V");
>
> I'd be tempted to reduce this to a dev_warn and carry on regardless.
> Whilst the datasheet states it must be a 2.5V reference, I'm not seeing any
> clear guidance on what happens if say it's just beyond your delta.
>
> > + return -EINVAL;
> > + }
> > + vals[0] = AD3552R_EXTERNAL_VREF_PIN_INPUT;
> > + } else {
> > + if (device_property_read_bool(dev, "adi,vref-out-en"))
> > + vals[0] = AD3552R_INTERNAL_VREF_PIN_2P5V;
>
> I would have a separate local variable for the single value and the array cases.
> That will look cleaner than a reader wondering what is in the other elements of
> the array when there is nothing there.
>
> The vals[1] value is only used in a fairly deeply nested statement, so just
> introduce the array in the appropriate place.
>
> > + else
> > + vals[0] = AD3552R_INTERNAL_VREF_PIN_FLOATING;
> > + }
> > + err = ad3552r_set_dev_value(dac, AD3552R_VREF_SELECT, vals[0]);
> > + if (err)
> > + return err;
> > +
> > + err = device_property_read_u32(dev, "adi,sdo-drive-strength",
> &vals[0]);
> > + if (!err) {
> > + if (vals[0] > 3) {
> > + dev_err(dev, "%s must be less than 4\n",
> > + "adi,sdo-drive-strength");
> > + return -EINVAL;
> > + }
> > + err = ad3552r_set_dev_value(dac,
> AD3552R_SDO_DRIVE_STRENGTH,
> > + vals[0]);
> > + if (err)
> > + return err;
> > + }
> > +
> > + dac->num_ch = device_get_child_node_count(dev);
> > + if (!dac->num_ch) {
> > + dev_err(dev, "No channels defined\n");
> > + return -ENODEV;
> > + }
> > +
> > + device_for_each_child_node(dev, child) {
> > + err = fwnode_property_read_u32(child, "reg", &ch);
> > + if (err) {
> > + dev_err(dev, "mandatory reg property missing\n");
> > + goto put_child;
> > + }
> > + if (ch >= AD3552R_NUM_CH) {
> > + dev_err(dev, "reg must be less than %d\n",
> > + AD3552R_NUM_CH);
> > + err = -EINVAL;
> > + goto put_child;
> > + }
> > +
> > + if (fwnode_property_present(child, "adi,output-range-
> microvolt")) {
> > + is_custom = false;
> > + err = fwnode_property_read_u32_array(child,
> > + "adi,output-range-
> microvolt",
> > + vals,
> > + 2);
> > + if (err) {
> > + dev_err(dev,
> > + "mandatory adi,output-range-
> microvolt property missing\n");
> > + goto put_child;
> > + }
> > +
> > + vals[0] = ad3552r_find_range(dac->chip_id, vals);
> > + if ((s32)vals[0] < 0) {
> > + dev_err(dev,
> > + "Invalid adi,output-range-microvolt
> value\n");
> > + err = -EINVAL;
> > + goto put_child;
> > + }
> > + err = ad3552r_set_ch_value(dac,
> > +
> AD3552R_CH_OUTPUT_RANGE_SEL,
> > + ch, vals[0]);
> > + if (err)
> > + goto put_child;
> > + } else {
> > + is_custom = true;
> > + err = ad3552r_configure_custom_gain(dac, child, ch);
> > + if (err)
> > + goto put_child;
> > + }
> > +
> > + ad3552r_calc_gain_and_offset(dac, ch);
> > + err = ad3552r_set_ch_value(dac,
> AD3552R_CH_RANGE_OVERRIDE, ch,
> > + is_custom);
> > + if (err)
> > + goto put_child;
> > +
> > + dac->enabled_ch |= BIT(ch);
> > +
> > + err = ad3552r_set_ch_value(dac, AD3552R_CH_SELECT, ch, 1);
> > + if (err < 0)
> > + return err;
> > +
> > + dac->channels[cnt] = AD3552R_CH_DAC(ch);
> > + ++cnt;
> > +
> > + }
> > +
> > + if (cnt == AD3552R_NUM_CH) {
> > + dac->channels[cnt] =
> AD3552R_CH_DAC_PAGE(AD3552R_PAGE_CH);
> > + ++cnt;
> > + } else {
> > + /* Disable unused channels */
> > + for_each_clear_bit(ch, &dac->enabled_ch,
> AD3552R_PAGE_CH) {
> > + err = ad3552r_set_ch_value(dac,
> > +
> AD3552R_CH_AMPLIFIER_POWERDOWN,
> > + ch,
> > + 0);
> > + if (err)
> > + goto put_child;
>
> Do we need to put the child node out here?
>
> > + }
> > + }
> > +
> > + dac->num_ch = cnt;
> > +
> > +put_child:
> > + fwnode_handle_put(child);
> > +
> > + return err;
> > +}
> > +
> > +static int ad3552r_init(struct ad3552r_desc *dac) {
> > + int err;
> > + u16 val, id;
> > +
> > + err = ad3552r_reset(dac);
> > + if (err) {
> > + dev_err(&dac->spi->dev, "Reset failed\n");
> > + return err;
> > + }
> > +
> > + err = ad3552r_check_scratch_pad(dac);
> > + if (err) {
> > + dev_err(&dac->spi->dev, "Scratch pad test failed\n");
> > + return err;
> > + }
> > +
> > + err = ad3552r_read_reg(dac, AD3552R_REG_ADDR_PRODUCT_ID_L,
> &val);
> > + if (err) {
> > + dev_err(&dac->spi->dev, "Fail read PRODUCT_ID_L\n");
> > + return err;
> > + }
> > + id = val;
> > + err = ad3552r_read_reg(dac, AD3552R_REG_ADDR_PRODUCT_ID_H,
> &val);
> > + if (err) {
> > + dev_err(&dac->spi->dev, "Fail read PRODUCT_ID_H\n");
> > + return err;
> > + }
> > + id |= val << 8;
> > + if (id != dac->chip_id) {
> > + dev_err(&dac->spi->dev, "Product id not matching\n");
> > + return -ENODEV;
> > + }
> > +
> > + return ad3552r_configure_device(dac); }
> > +
> > +static int ad3552r_probe(struct spi_device *spi) {
> > + const struct spi_device_id *id = spi_get_device_id(spi);
> > + struct ad3552r_desc *dac;
> > + struct iio_dev *indio_dev;
> > + int err;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dac));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + dac = iio_priv(indio_dev);
> > + dac->spi = spi;
> > + dac->chip_id = id->driver_data;
> > +
> > + mutex_init(&dac->lock);
> > +
> > + err = ad3552r_init(dac);
> > + if (err)
> > + return err;
> > +
> > + /* Config triggered buffer device */
> > + indio_dev->dev.parent = &spi->dev;
> > + indio_dev->name = "ad3552r";
>
> That name needs to reflect the connected part, not always be ad3552r.
>
> > + indio_dev->info = &ad3552r_iio_info;
> > + indio_dev->num_channels = dac->num_ch;
> > + indio_dev->channels = dac->channels;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + err = devm_iio_triggered_buffer_setup_ext(&indio_dev->dev,
> indio_dev, NULL,
> > + &ad3552r_trigger_handler,
> > +
> IIO_BUFFER_DIRECTION_OUT,
> > + NULL,
> > + NULL);
> > + if (err)
> > + return err;
> > +
> > + return devm_iio_device_register(&spi->dev, indio_dev); }
> > +
> > +static const struct spi_device_id ad3552r_id[] = {
> > + { "ad3542r", AD3542R_ID },
> > + { "ad3552r", AD3552R_ID },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(spi, ad3552r_id);
> > +
> > +static const struct of_device_id ad3552r_of_match[] = {
> > + { .compatible = "adi,ad3542r"},
> > + { .compatible = "adi,ad3552r"},
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ad3552r_of_match);
> > +
> > +static struct spi_driver ad3552r_driver = {
> > + .driver = {
> > + .name = "ad3552r",
> > + .of_match_table = ad3552r_of_match,
> > + },
> > + .probe = ad3552r_probe,
> > + .id_table = ad3552r_id
> > +};
> > +module_spi_driver(ad3552r_driver);
> > +
> > +MODULE_AUTHOR("Mihail Chindris <mihail.chindris@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Analog Device AD3552R DAC");
> MODULE_LICENSE("GPL
> > +v2");