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/ad3552r.pdf
Signed-off-by: Mihail Chindris <[email protected]>
---
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;
+ 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);
+
+ 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, ®);
+ 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);
+ }
+
+ 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:
+ 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),
+ 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, ®);
+ 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,
+ 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.
+ */
+#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, \
+})
+
+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;
+
+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);
+
+ 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);
+
+ 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];
+
+ 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;
+ }
+
+ 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");
+ 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;
+ 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;
+ }
+ }
+
+ 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";
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("Analog Device AD3552R DAC");
+MODULE_LICENSE("GPL v2");
--
2.27.0
Hi Mihail,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linux/master linus/master v5.15-rc4 next-20211008]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Mihail-Chindris/Add-ad3552r-and-ad3542r-driver-support/20211008-204146
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/2d864149611f04bb764b81ebbad7dea5472921c5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mihail-Chindris/Add-ad3552r-and-ad3542r-driver-support/20211008-204146
git checkout 2d864149611f04bb764b81ebbad7dea5472921c5
# save the attached .config to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/iio/dac/ad3552r.c:300:23: error: array type has incomplete element type 'struct iio_chan_spec'
300 | struct iio_chan_spec channels[AD3552R_NUM_CH + 1];
| ^~~~~~~~
drivers/iio/dac/ad3552r.c: In function 'ad3552r_read_raw':
>> drivers/iio/dac/ad3552r.c:767:29: error: implicit declaration of function 'iio_priv'; did you mean 'bio_prio'? [-Werror=implicit-function-declaration]
767 | struct ad3552r_desc *dac = iio_priv(indio_dev);
| ^~~~~~~~
| bio_prio
>> drivers/iio/dac/ad3552r.c:767:29: error: initialization of 'struct ad3552r_desc *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
>> drivers/iio/dac/ad3552r.c:770:14: error: dereferencing pointer to incomplete type 'const struct iio_chan_spec'
770 | u8 ch = chan->channel;
| ^~
>> drivers/iio/dac/ad3552r.c:773:7: error: 'IIO_CHAN_INFO_RAW' undeclared (first use in this function)
773 | case IIO_CHAN_INFO_RAW:
| ^~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:773:7: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/iio/dac/ad3552r.c:790:7: error: 'IIO_CHAN_INFO_ENABLE' undeclared (first use in this function)
790 | case IIO_CHAN_INFO_ENABLE:
| ^~~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ad3552r.c:801:7: error: 'IIO_CHAN_INFO_SCALE' undeclared (first use in this function)
801 | case IIO_CHAN_INFO_SCALE:
| ^~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ad3552r.c:804:10: error: 'IIO_VAL_INT_PLUS_MICRO' undeclared (first use in this function)
804 | return IIO_VAL_INT_PLUS_MICRO;
| ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ad3552r.c:805:7: error: 'IIO_CHAN_INFO_OFFSET' undeclared (first use in this function)
805 | case IIO_CHAN_INFO_OFFSET:
| ^~~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ad3552r.c:813:9: error: 'IIO_VAL_INT' undeclared (first use in this function)
813 | return IIO_VAL_INT;
| ^~~~~~~~~~~
drivers/iio/dac/ad3552r.c: In function 'ad3552r_write_raw':
drivers/iio/dac/ad3552r.c:822:29: error: initialization of 'struct ad3552r_desc *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
822 | struct ad3552r_desc *dac = iio_priv(indio_dev);
| ^~~~~~~~
drivers/iio/dac/ad3552r.c:827:7: error: 'IIO_CHAN_INFO_RAW' undeclared (first use in this function)
827 | case IIO_CHAN_INFO_RAW:
| ^~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:840:7: error: 'IIO_CHAN_INFO_ENABLE' undeclared (first use in this function)
840 | case IIO_CHAN_INFO_ENABLE:
| ^~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c: At top level:
>> drivers/iio/dac/ad3552r.c:872:21: error: variable 'ad3552r_iio_info' has initializer but incomplete type
872 | static const struct iio_info ad3552r_iio_info = {
| ^~~~~~~~
>> drivers/iio/dac/ad3552r.c:873:3: error: 'const struct iio_info' has no member named 'read_raw'
873 | .read_raw = ad3552r_read_raw,
| ^~~~~~~~
drivers/iio/dac/ad3552r.c:873:14: error: excess elements in struct initializer [-Werror]
873 | .read_raw = ad3552r_read_raw,
| ^~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:873:14: note: (near initialization for 'ad3552r_iio_info')
>> drivers/iio/dac/ad3552r.c:874:3: error: 'const struct iio_info' has no member named 'write_raw'
874 | .write_raw = ad3552r_write_raw,
| ^~~~~~~~~
drivers/iio/dac/ad3552r.c:874:15: error: excess elements in struct initializer [-Werror]
874 | .write_raw = ad3552r_write_raw,
| ^~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:874:15: note: (near initialization for 'ad3552r_iio_info')
>> drivers/iio/dac/ad3552r.c:875:3: error: 'const struct iio_info' has no member named 'update_scan_mode'
875 | .update_scan_mode = ad3552r_update_scan_mode
| ^~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:875:22: error: excess elements in struct initializer [-Werror]
875 | .update_scan_mode = ad3552r_update_scan_mode
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:875:22: note: (near initialization for 'ad3552r_iio_info')
drivers/iio/dac/ad3552r.c: In function 'ad3552r_trigger_handler':
>> drivers/iio/dac/ad3552r.c:882:36: error: dereferencing pointer to incomplete type 'struct iio_dev'
882 | struct iio_buffer *buf = indio_dev->buffer;
| ^~
drivers/iio/dac/ad3552r.c:883:29: error: initialization of 'struct ad3552r_desc *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
883 | struct ad3552r_desc *dac = iio_priv(indio_dev);
| ^~~~~~~~
>> drivers/iio/dac/ad3552r.c:888:8: error: implicit declaration of function 'iio_pop_from_buffer'; did you mean 'sg_pcopy_from_buffer'? [-Werror=implicit-function-declaration]
888 | err = iio_pop_from_buffer(buf, buff);
| ^~~~~~~~~~~~~~~~~~~
| sg_pcopy_from_buffer
drivers/iio/dac/ad3552r.c: In function 'ad3552r_configure_custom_gain':
>> drivers/iio/dac/ad3552r.c:1106:22: error: implicit declaration of function 'fwnode_get_named_child_node'; did you mean 'fwnode_get_named_gpiod'? [-Werror=implicit-function-declaration]
1106 | custom_gain_child = fwnode_get_named_child_node(child,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
| fwnode_get_named_gpiod
>> drivers/iio/dac/ad3552r.c:1106:20: error: assignment to 'struct fwnode_handle *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
1106 | custom_gain_child = fwnode_get_named_child_node(child,
| ^
>> drivers/iio/dac/ad3552r.c:1114:8: error: implicit declaration of function 'fwnode_property_read_u32' [-Werror=implicit-function-declaration]
1114 | err = fwnode_property_read_u32(custom_gain_child, "adi,gain-offset",
| ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ad3552r.c:1149:2: error: implicit declaration of function 'fwnode_handle_put' [-Werror=implicit-function-declaration]
1149 | fwnode_handle_put(custom_gain_child);
| ^~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c: In function 'ad3552r_configure_device':
drivers/iio/dac/ad3552r.c:1181:7: error: implicit declaration of function 'device_property_read_bool' [-Werror=implicit-function-declaration]
1181 | if (device_property_read_bool(dev, "adi,vref-out-en"))
| ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1190:8: error: implicit declaration of function 'device_property_read_u32' [-Werror=implicit-function-declaration]
1190 | err = device_property_read_u32(dev, "adi,sdo-drive-strength", &vals[0]);
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1203:16: error: implicit declaration of function 'device_get_child_node_count' [-Werror=implicit-function-declaration]
1203 | dac->num_ch = device_get_child_node_count(dev);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1209:2: error: implicit declaration of function 'device_for_each_child_node'; did you mean 'device_for_each_child'? [-Werror=implicit-function-declaration]
1209 | device_for_each_child_node(dev, child) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
| device_for_each_child
drivers/iio/dac/ad3552r.c:1209:40: error: expected ';' before '{' token
1209 | device_for_each_child_node(dev, child) {
| ^~
| ;
drivers/iio/dac/ad3552r.c:1287:1: error: label 'put_child' defined but not used [-Werror=unused-label]
1287 | put_child:
| ^~~~~~~~~
drivers/iio/dac/ad3552r.c:1161:7: error: unused variable 'is_custom' [-Werror=unused-variable]
1161 | bool is_custom;
| ^~~~~~~~~
drivers/iio/dac/ad3552r.c:1160:15: error: unused variable 'ch' [-Werror=unused-variable]
1160 | u32 vals[2], ch;
| ^~
drivers/iio/dac/ad3552r.c:1159:11: error: unused variable 'cnt' [-Werror=unused-variable]
1159 | int err, cnt = 0, voltage, delta = 100000;
| ^~~
drivers/iio/dac/ad3552r.c: In function 'ad3552r_probe':
drivers/iio/dac/ad3552r.c:1337:14: error: implicit declaration of function 'devm_iio_device_alloc' [-Werror=implicit-function-declaration]
1337 | indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dac));
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1337:12: error: assignment to 'struct iio_dev *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
1337 | indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dac));
| ^
drivers/iio/dac/ad3552r.c:1341:6: error: assignment to 'struct ad3552r_desc *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
1341 | dac = iio_priv(indio_dev);
| ^
drivers/iio/dac/ad3552r.c:1357:21: error: 'INDIO_DIRECT_MODE' undeclared (first use in this function)
1357 | indio_dev->modes = INDIO_DIRECT_MODE;
| ^~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1361:9: error: 'IIO_BUFFER_DIRECTION_OUT' undeclared (first use in this function)
1361 | IIO_BUFFER_DIRECTION_OUT,
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1359:8: error: too many arguments to function 'devm_iio_triggered_buffer_setup_ext'
1359 | err = devm_iio_triggered_buffer_setup_ext(&indio_dev->dev, indio_dev, NULL,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/iio/dac/ad3552r.c:11:
include/linux/iio/triggered_buffer.h:21:5: note: declared here
21 | int devm_iio_triggered_buffer_setup_ext(struct device *dev,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1367:9: error: implicit declaration of function 'devm_iio_device_register'; did you mean 'devm_iio_trigger_register'? [-Werror=implicit-function-declaration]
1367 | return devm_iio_device_register(&spi->dev, indio_dev);
| ^~~~~~~~~~~~~~~~~~~~~~~~
| devm_iio_trigger_register
drivers/iio/dac/ad3552r.c: At top level:
drivers/iio/dac/ad3552r.c:872:30: error: storage size of 'ad3552r_iio_info' isn't known
872 | static const struct iio_info ad3552r_iio_info = {
| ^~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1092:12: error: 'ad3552r_configure_custom_gain' defined but not used [-Werror=unused-function]
1092 | static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1071:12: error: 'ad3552r_find_range' defined but not used [-Werror=unused-function]
1071 | static int ad3552r_find_range(u16 id, u32 *vals)
| ^~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad3552r.c:1024:13: error: 'ad3552r_calc_gain_and_offset' defined but not used [-Werror=unused-function]
1024 | static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +300 drivers/iio/dac/ad3552r.c
292
293 struct ad3552r_desc {
294 /* Used to look the spi bus for atomic operations where needed */
295 struct mutex lock;
296 struct gpio_desc *gpio_reset;
297 struct gpio_desc *gpio_ldac;
298 struct spi_device *spi;
299 struct ad3552r_ch_data ch_data[AD3552R_NUM_CH];
> 300 struct iio_chan_spec channels[AD3552R_NUM_CH + 1];
301 unsigned long enabled_ch;
302 unsigned int num_ch;
303 enum ad3542r_id chip_id;
304 /*
305 * The maximum spi transfer size consist 1 bytes (reg address)
306 * + 2 registers of 3 bytes + 1 reg of 1 byte (SW LDAC)
307 */
308 u8 buf_data[8] ____cacheline_aligned;
309 };
310
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Fri, 8 Oct 2021 12:39:09 +0000
Mihail Chindris <[email protected]> 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/ad3552r.pdf
>
> Signed-off-by: Mihail Chindris <[email protected]>
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, ®);
> + 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, ®);
> + 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.
> +})
> +
> +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 <[email protected]>");
> +MODULE_DESCRIPTION("Analog Device AD3552R DAC");
> +MODULE_LICENSE("GPL v2");
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Sunday, 10 October 2021 18:37
> To: Chindris, Mihail <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> Hennerich, Michael <[email protected]>; Sa, Nuno
> <[email protected]>; Bogdan, Dragos <[email protected]>;
> [email protected]
> Subject: Re: [PATCH v2 2/2] drivers:iio:dac: Add AD3552R driver support
>
> On Fri, 8 Oct 2021 12:39:09 +0000
> Mihail Chindris <[email protected]> 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 <[email protected]>
>
> 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, ®);
> > + 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, ®);
> > + 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 <[email protected]>");
> > +MODULE_DESCRIPTION("Analog Device AD3552R DAC");
> MODULE_LICENSE("GPL
> > +v2");
On Tue, 19 Oct 2021 11:46:26 +0000
"Chindris, Mihail" <[email protected]> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <[email protected]>
> > Sent: Sunday, 10 October 2021 18:37
> > To: Chindris, Mihail <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > Hennerich, Michael <[email protected]>; Sa, Nuno
> > <[email protected]>; Bogdan, Dragos <[email protected]>;
> > [email protected]
> > Subject: Re: [PATCH v2 2/2] drivers:iio:dac: Add AD3552R driver support
> >
> > On Fri, 8 Oct 2021 12:39:09 +0000
> > Mihail Chindris <[email protected]> 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 <[email protected]>
> >
> > 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.
If replying to only a small part, please crop. Cuts down on scrolling fun..
...
> > > +/*
> > > + * 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?
Ah. I see where confusion came from.
In that ABI document capital letters are wild cards for the channel numbers.
I'm not actually sure which driver ever used that one... And digging in the history
I can find the patch that introduced the documentation and.
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?id=98069c3f1cd353eda61d8e11ce40f83d5c5a99f3
Roland did that just before the max517 driver, so looking in there
we have no obvious sign of this today... but
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?id=6ddc5fb43e47e18d434619e08d41260ff121a7f6
and following forwards from there.
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=togreg&id=4cd8d87446d521b7824aa1be84d767c8b5300601
So Lars scrapped it when cleaning that driver up to move it out of staging.
On that basis and the fact we wouldn't do it this way today I'd suggest
we just drop it. Given we now finally have output buffers, doing this
via some in driver magic that checks for the same value for each channel
as uses this path if they match is going to be much nicer.
For reference the X_AND_Z modifier is for axis like you'd find in an
accelerometer where we are really dealing with the x axis and z axis.
It only really applied to events where you have something that require
a value to be (typically) below a threshold to indicate that the device
is on it's side and so all the acceleration due to gravity is in the y
direction.
If you want to do an additional patch to remove that left over
bit of documentation that would be great. If not I'll do it once
I've caught up with the review backlog.
Jonathan
>
> 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 <[email protected]>");
> > > +MODULE_DESCRIPTION("Analog Device AD3552R DAC");
> > MODULE_LICENSE("GPL
> > > +v2");
>