Add support for the AD485X DAS familiy.
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad485x.c | 1061 ++++++++++++++++++++++++++++++++++++++
3 files changed, 1074 insertions(+)
create mode 100644 drivers/iio/adc/ad485x.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f60fe85a30d5..83f55229d731 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -36,6 +36,18 @@ config AD4130
To compile this driver as a module, choose M here: the module will be
called ad4130.
+config AD485X
+ tristate "Analog Device AD485x DAS Driver"
+ depends on SPI
+ select REGMAP_SPI
+ select IIO_BACKEND
+ help
+ Say yes here to build support for Analog Devices AD485x high speed
+ data acquisition system (DAS).
+
+ To compile this driver as a module, choose M here: the module will be
+ called ad485x.
+
config AD7091R
tristate
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d370e066544e..26c1670c8913 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -7,6 +7,7 @@
obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
obj-$(CONFIG_AD4130) += ad4130.o
+obj-$(CONFIG_AD485X) += ad485x.o
obj-$(CONFIG_AD7091R) += ad7091r-base.o
obj-$(CONFIG_AD7091R5) += ad7091r5.o
obj-$(CONFIG_AD7091R8) += ad7091r8.o
diff --git a/drivers/iio/adc/ad485x.c b/drivers/iio/adc/ad485x.c
new file mode 100644
index 000000000000..3333cad9ed8f
--- /dev/null
+++ b/drivers/iio/adc/ad485x.c
@@ -0,0 +1,1061 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD485x DAS driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/iio/backend.h>
+#include <linux/iio/iio.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/units.h>
+
+#define AD485X_REG_INTERFACE_CONFIG_A 0x00
+#define AD485X_REG_INTERFACE_CONFIG_B 0x01
+#define AD485X_REG_PRODUCT_ID_L 0x04
+#define AD485X_REG_PRODUCT_ID_H 0x05
+#define AD485X_REG_DEVICE_CTRL 0x25
+#define AD485X_REG_PACKET 0x26
+
+#define AD485X_REG_CH_CONFIG_BASE 0x2A
+#define AD485X_REG_CHX_SOFTSPAN(ch) ((0x12 * (ch)) + AD485X_REG_CH_CONFIG_BASE)
+#define AD485X_REG_CHX_OFFSET(ch) (AD485X_REG_CHX_SOFTSPAN(ch) + 0x01)
+#define AD485X_REG_CHX_OFFSET_LSB(ch) AD485X_REG_CHX_OFFSET(ch)
+#define AD485X_REG_CHX_OFFSET_MID(ch) (AD485X_REG_CHX_OFFSET_LSB(ch) + 0x01)
+#define AD485X_REG_CHX_OFFSET_MSB(ch) (AD485X_REG_CHX_OFFSET_MID(ch) + 0x01)
+#define AD485X_REG_CHX_GAIN(ch) (AD485X_REG_CHX_OFFSET(ch) + 0x03)
+#define AD485X_REG_CHX_GAIN_LSB(ch) AD485X_REG_CHX_GAIN(ch)
+#define AD485X_REG_CHX_GAIN_MSB(ch) (AD485X_REG_CHX_GAIN(ch) + 0x01)
+#define AD485X_REG_CHX_PHASE(ch) (AD485X_REG_CHX_GAIN(ch) + 0x02)
+#define AD485X_REG_CHX_PHASE_LSB(ch) AD485X_REG_CHX_PHASE(ch)
+#define AD485X_REG_CHX_PHASE_MSB(ch) (AD485X_REG_CHX_PHASE_LSB(ch) + 0x01)
+
+#define AD485X_REG_TESTPAT_0(c) (0x38 + (c) * 0x12)
+#define AD485X_REG_TESTPAT_1(c) (0x39 + (c) * 0x12)
+#define AD485X_REG_TESTPAT_2(c) (0x3A + (c) * 0x12)
+#define AD485X_REG_TESTPAT_3(c) (0x3B + (c) * 0x12)
+
+#define AD485X_SW_RESET (BIT(7) | BIT(0))
+#define AD485X_SDO_ENABLE BIT(4)
+#define AD485X_SINGLE_INSTRUCTION BIT(7)
+#define AD485X_ECHO_CLOCK_MODE BIT(0)
+
+#define AD485X_PACKET_FORMAT_0 0
+#define AD485X_PACKET_FORMAT_1 1
+#define AD485X_PACKET_FORMAT_MASK GENMASK(1, 0)
+#define AD485X_OS_EN BIT(7)
+
+#define AD485X_TEST_PAT BIT(2)
+
+#define AD4858_PACKET_SIZE_20 0
+#define AD4858_PACKET_SIZE_24 1
+#define AD4858_PACKET_SIZE_32 2
+
+#define AD4857_PACKET_SIZE_16 0
+#define AD4857_PACKET_SIZE_24 1
+
+#define AD485X_TESTPAT_0_DEFAULT 0x2A
+#define AD485X_TESTPAT_1_DEFAULT 0x3C
+#define AD485X_TESTPAT_2_DEFAULT 0xCE
+#define AD485X_TESTPAT_3_DEFAULT(c) (0x0A + (0x10 * (c)))
+
+#define AD485X_SOFTSPAN_0V_2V5 0
+#define AD485X_SOFTSPAN_N2V5_2V5 1
+#define AD485X_SOFTSPAN_0V_5V 2
+#define AD485X_SOFTSPAN_N5V_5V 3
+#define AD485X_SOFTSPAN_0V_6V25 4
+#define AD485X_SOFTSPAN_N6V25_6V25 5
+#define AD485X_SOFTSPAN_0V_10V 6
+#define AD485X_SOFTSPAN_N10V_10V 7
+#define AD485X_SOFTSPAN_0V_12V5 8
+#define AD485X_SOFTSPAN_N12V5_12V5 9
+#define AD485X_SOFTSPAN_0V_20V 10
+#define AD485X_SOFTSPAN_N20V_20V 11
+#define AD485X_SOFTSPAN_0V_25V 12
+#define AD485X_SOFTSPAN_N25V_25V 13
+#define AD485X_SOFTSPAN_0V_40V 14
+#define AD485X_SOFTSPAN_N40V_40V 15
+
+#define AD485X_MAX_LANES 8
+#define AD485X_MAX_IODELAY 32
+
+#define AD485X_T_CNVH_NS 40
+
+#define AD4858_PROD_ID 0x60
+#define AD4857_PROD_ID 0x61
+#define AD4856_PROD_ID 0x62
+#define AD4855_PROD_ID 0x63
+#define AD4854_PROD_ID 0x64
+#define AD4853_PROD_ID 0x65
+#define AD4852_PROD_ID 0x66
+#define AD4851_PROD_ID 0x67
+#define AD4858I_PROD_ID 0x6F
+
+struct ad485x_chip_info {
+ const char *name;
+ unsigned int product_id;
+ const unsigned int (*scale_table)[2];
+ int num_scales;
+ const int *offset_table;
+ int num_offset;
+ const struct iio_chan_spec *channels;
+ unsigned int num_channels;
+ unsigned long throughput;
+ unsigned int resolution;
+};
+
+struct ad485x_state {
+ struct spi_device *spi;
+ struct pwm_device *cnv;
+ struct iio_backend *back;
+ /*
+ * Synchronize access to members the of driver state, and ensure
+ * atomicity of consecutive regmap operations.
+ */
+ struct mutex lock;
+ struct regmap *regmap;
+ const struct ad485x_chip_info *info;
+ unsigned long sampling_freq;
+ unsigned int (*scales)[2];
+ int *offsets;
+};
+
+static int ad485x_reg_access(struct iio_dev *indio_dev,
+ unsigned int reg,
+ unsigned int writeval,
+ unsigned int *readval)
+{
+ struct ad485x_state *st = iio_priv(indio_dev);
+
+ if (readval)
+ return regmap_read(st->regmap, reg, readval);
+
+ return regmap_write(st->regmap, reg, writeval);
+}
+
+static int ad485x_set_sampling_freq(struct ad485x_state *st, unsigned int freq)
+{
+ struct pwm_state cnv_state = {
+ .duty_cycle = AD485X_T_CNVH_NS,
+ .enabled = true,
+ };
+ int ret;
+
+ if (freq > st->info->throughput)
+ freq = st->info->throughput;
+
+ cnv_state.period = DIV_ROUND_CLOSEST_ULL(1000000000, freq);
+
+ ret = pwm_apply_might_sleep(st->cnv, &cnv_state);
+ if (ret)
+ return ret;
+
+ st->sampling_freq = freq;
+
+ return 0;
+}
+
+static int ad485x_setup(struct ad485x_state *st)
+{
+ unsigned int product_id;
+ int ret;
+
+ ret = ad485x_set_sampling_freq(st, 1000000);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
+ AD485X_SW_RESET);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_B,
+ AD485X_SINGLE_INSTRUCTION);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
+ AD485X_SDO_ENABLE);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(st->regmap, AD485X_REG_PRODUCT_ID_L, &product_id);
+ if (ret)
+ return ret;
+
+ if (product_id != st->info->product_id)
+ dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n",
+ product_id);
+
+ ret = regmap_write(st->regmap, AD485X_REG_DEVICE_CTRL,
+ AD485X_ECHO_CLOCK_MODE);
+ if (ret)
+ return ret;
+
+ return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
+}
+
+static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
+{
+ int i, cnt = 0, max_cnt = 0, start, max_start = 0;
+
+ for (i = 0, start = -1; i < size; i++) {
+ if (field[i] == 0) {
+ if (start == -1)
+ start = i;
+ cnt++;
+ } else {
+ if (cnt > max_cnt) {
+ max_cnt = cnt;
+ max_start = start;
+ }
+ start = -1;
+ cnt = 0;
+ }
+ }
+
+ if (cnt > max_cnt) {
+ max_cnt = cnt;
+ max_start = start;
+ }
+
+ if (!max_cnt)
+ return -EIO;
+
+ *ret_start = max_start;
+
+ return max_cnt;
+}
+
+static int ad485x_calibrate(struct ad485x_state *st)
+{
+ int opt_delay, lane_num, delay, i, s, c;
+ enum iio_backend_interface_type interface_type;
+ bool pn_status[AD485X_MAX_LANES][AD485X_MAX_IODELAY];
+ int ret;
+
+ ret = iio_backend_interface_type_get(st->back, &interface_type);
+ if (ret)
+ return ret;
+
+ if (interface_type == IIO_BACKEND_INTERFACE_CMOS)
+ lane_num = st->info->num_channels;
+ else
+ lane_num = 1;
+
+ if (st->info->resolution == 16) {
+ ret = iio_backend_data_size_set(st->back, 24);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->regmap, AD485X_REG_PACKET,
+ AD485X_TEST_PAT | AD4857_PACKET_SIZE_24);
+ if (ret)
+ return ret;
+ } else {
+ ret = iio_backend_data_size_set(st->back, 32);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->regmap, AD485X_REG_PACKET,
+ AD485X_TEST_PAT | AD4858_PACKET_SIZE_32);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < st->info->num_channels; i++) {
+ ret = regmap_write(st->regmap, AD485X_REG_TESTPAT_0(i),
+ AD485X_TESTPAT_0_DEFAULT);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->regmap, AD485X_REG_TESTPAT_1(i),
+ AD485X_TESTPAT_1_DEFAULT);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->regmap, AD485X_REG_TESTPAT_2(i),
+ AD485X_TESTPAT_2_DEFAULT);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->regmap, AD485X_REG_TESTPAT_3(i),
+ AD485X_TESTPAT_3_DEFAULT(i));
+ if (ret)
+ return ret;
+
+ ret = iio_backend_chan_enable(st->back, i);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < lane_num; i++) {
+ for (delay = 0; delay < AD485X_MAX_IODELAY; delay++) {
+ ret = iio_backend_iodelay_set(st->back, i, delay);
+ if (ret)
+ return ret;
+ ret = iio_backend_chan_status(st->back, i,
+ &pn_status[i][delay]);
+ if (ret)
+ return ret;
+ }
+ }
+
+ for (i = 0; i < lane_num; i++) {
+ c = ad485x_find_opt(&pn_status[i][0], AD485X_MAX_IODELAY, &s);
+ if (c < 0)
+ return c;
+
+ opt_delay = s + c / 2;
+ ret = iio_backend_iodelay_set(st->back, i, opt_delay);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < st->info->num_channels; i++) {
+ ret = iio_backend_chan_disable(st->back, i);
+ if (ret)
+ return ret;
+ }
+
+ ret = iio_backend_data_size_set(st->back, 20);
+ if (ret)
+ return ret;
+
+ return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
+}
+
+static const char *const ad4858_packet_fmts[] = {
+ "20-bit", "24-bit", "32-bit"
+};
+
+static const char *const ad4857_packet_fmts[] = {
+ "16-bit", "24-bit"
+};
+
+static int ad485x_set_packet_format(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int format)
+{
+ struct ad485x_state *st = iio_priv(indio_dev);
+ unsigned int val;
+ int ret;
+
+ if (chan->scan_type.realbits == 20)
+ switch (format) {
+ case 0:
+ val = 20;
+ break;
+ case 1:
+ val = 24;
+ break;
+ case 2:
+ val = 32;
+ break;
+ default:
+ return -EINVAL;
+ }
+ else if (chan->scan_type.realbits == 16)
+ switch (format) {
+ case 0:
+ val = 16;
+ break;
+ case 1:
+ val = 24;
+ break;
+ default:
+ return -EINVAL;
+ }
+ else
+ return -EINVAL;
+
+ ret = iio_backend_data_size_set(st->back, val);
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(st->regmap, AD485X_REG_PACKET,
+ AD485X_PACKET_FORMAT_MASK, format);
+}
+
+static int ad485x_get_packet_format(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct ad485x_state *st = iio_priv(indio_dev);
+ unsigned int format;
+ int ret;
+
+ ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
+ if (ret)
+ return ret;
+
+ format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
+
+ return format;
+}
+
+static const struct iio_enum ad4858_packet_fmt = {
+ .items = ad4858_packet_fmts,
+ .num_items = ARRAY_SIZE(ad4858_packet_fmts),
+ .set = ad485x_set_packet_format,
+ .get = ad485x_get_packet_format,
+};
+
+static const struct iio_enum ad4857_packet_fmt = {
+ .items = ad4857_packet_fmts,
+ .num_items = ARRAY_SIZE(ad4857_packet_fmts),
+ .set = ad485x_set_packet_format,
+ .get = ad485x_get_packet_format,
+};
+
+static int ad485x_get_calibscale(struct ad485x_state *st, int ch, int *val,
+ int *val2)
+{
+ unsigned int reg_val;
+ int gain;
+ int ret;
+
+ guard(mutex)(&st->lock);
+
+ ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
+ ®_val);
+ if (ret)
+ return ret;
+
+ gain = (reg_val & 0xFF) << 8;
+
+ ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
+ ®_val);
+ if (ret)
+ return ret;
+
+ gain |= reg_val & 0xFF;
+
+ *val = gain;
+ *val2 = 32768;
+
+ return IIO_VAL_FRACTIONAL;
+}
+
+static int ad485x_set_calibscale(struct ad485x_state *st, int ch, int val,
+ int val2)
+{
+ unsigned long long gain;
+ unsigned int reg_val;
+ int ret;
+
+ gain = val * 1000000 + val2;
+ gain = gain * 32768;
+ gain = DIV_U64_ROUND_CLOSEST(gain, 1000000);
+
+ reg_val = gain;
+
+ guard(mutex)(&st->lock);
+
+ ret = regmap_write(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
+ reg_val >> 8);
+ if (ret)
+ return ret;
+
+ return regmap_write(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
+ reg_val & 0xFF);
+}
+
+static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val,
+ int *val2)
+{
+ unsigned int lsb, mid, msb;
+ int ret;
+
+ guard(mutex)(&st->lock);
+
+ ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
+ &msb);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
+ &mid);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
+ &lsb);
+ if (ret)
+ return ret;
+
+ if (st->info->resolution == 16) {
+ *val = msb << 8;
+ *val |= mid;
+ *val = sign_extend32(*val, 15);
+ } else {
+ *val = msb << 12;
+ *val |= mid << 4;
+ *val |= lsb >> 4;
+ *val = sign_extend32(*val, 19);
+ }
+
+ return IIO_VAL_INT;
+}
+
+static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
+ int val2)
+{
+ unsigned int lsb, mid, msb;
+ int ret;
+
+ if (st->info->resolution == 16) {
+ lsb = 0;
+ mid = val & 0xFF;
+ msb = (val >> 8) & 0xFF;
+ } else {
+ lsb = (val << 4) & 0xFF;
+ mid = (val >> 4) & 0xFF;
+ msb = (val >> 12) & 0xFF;
+ }
+
+ guard(mutex)(&st->lock);
+
+ ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
+ if (ret)
+ return ret;
+
+ return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);
+}
+
+static const unsigned int ad485x_scale_table[][2] = {
+ {2500, 0x0}, {5000, 0x1}, {5000, 0x2}, {10000, 0x3}, {6250, 0x04},
+ {12500, 0x5}, {10000, 0x6}, {20000, 0x7}, {12500, 0x8}, {25000, 0x9},
+ {20000, 0xA}, {40000, 0xB}, {25000, 0xC}, {50000, 0xD}, {40000, 0xE},
+ {80000, 0xF}
+};
+
+static const int ad4857_offset_table[] = {
+ 0, -32768
+};
+
+static const int ad4858_offset_table[] = {
+ 0, -524288
+};
+
+static const unsigned int ad485x_scale_avail[] = {
+ 2500, 5000, 10000, 6250, 12500, 20000, 25000, 40000, 50000, 80000
+};
+
+static void __ad485x_get_scale(struct ad485x_state *st, int scale_tbl,
+ unsigned int *val, unsigned int *val2)
+{
+ const struct ad485x_chip_info *info = st->info;
+ const struct iio_chan_spec *chan = &info->channels[0];
+ unsigned int tmp;
+
+ tmp = (scale_tbl * 1000000ULL) >> chan->scan_type.realbits;
+ *val = tmp / 1000000;
+ *val2 = tmp % 1000000;
+}
+
+static int ad485x_set_scale(struct ad485x_state *st,
+ const struct iio_chan_spec *chan, int val, int val2)
+{
+ const struct ad485x_chip_info *info = st->info;
+ unsigned int scale_val[2];
+ unsigned int i, j = 0;
+
+ for (i = 0; i < info->num_scales; i++) {
+ __ad485x_get_scale(st, info->scale_table[i][0],
+ &scale_val[0], &scale_val[1]);
+ if (scale_val[0] != val || scale_val[1] != val2)
+ continue;
+
+ /*
+ * Adjust the softspan value (differential or single ended)
+ * based on the scale value selected and current offset of
+ * the channel.
+ *
+ * If the offset is 0 then continue iterations until finding
+ * the next matching scale value which always corresponds to
+ * the single ended mode.
+ */
+ if (!st->offsets[chan->channel] && !j) {
+ j++;
+ continue;
+ }
+
+ return regmap_write(st->regmap,
+ AD485X_REG_CHX_SOFTSPAN(chan->channel),
+ info->scale_table[i][1]);
+ }
+
+ return -EINVAL;
+}
+
+static int ad485x_get_scale(struct ad485x_state *st,
+ const struct iio_chan_spec *chan, int *val,
+ int *val2)
+{
+ const struct ad485x_chip_info *info = st->info;
+ unsigned int i, softspan_val;
+ int ret;
+
+ ret = regmap_read(st->regmap, AD485X_REG_CHX_SOFTSPAN(chan->channel),
+ &softspan_val);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < info->num_scales; i++) {
+ if (softspan_val == info->scale_table[i][1])
+ break;
+ }
+
+ if (i == info->num_scales)
+ return -EIO;
+
+ __ad485x_get_scale(st, info->scale_table[i][0], val, val2);
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ad485x_set_offset(struct ad485x_state *st,
+ const struct iio_chan_spec *chan, int val)
+{
+ guard(mutex)(&st->lock);
+
+ if (st->offsets[chan->channel] != val) {
+ st->offsets[chan->channel] = val;
+ /* Restore to the default range if offset changes */
+ if (st->offsets[chan->channel])
+ return regmap_write(st->regmap,
+ AD485X_REG_CHX_SOFTSPAN(chan->channel),
+ AD485X_SOFTSPAN_N40V_40V);
+ return regmap_write(st->regmap,
+ AD485X_REG_CHX_SOFTSPAN(chan->channel),
+ AD485X_SOFTSPAN_0V_40V);
+ }
+
+ return 0;
+}
+
+static int ad485x_scale_offset_fill(struct ad485x_state *st)
+{
+ unsigned int i, val1, val2;
+
+ st->offsets = devm_kcalloc(&st->spi->dev, st->info->num_channels,
+ sizeof(*st->offsets), GFP_KERNEL);
+ if (!st->offsets)
+ return -ENOMEM;
+
+ st->scales = devm_kmalloc_array(&st->spi->dev, ARRAY_SIZE(ad485x_scale_avail),
+ sizeof(*st->scales), GFP_KERNEL);
+ if (!st->scales)
+ return -ENOMEM;
+
+ for (i = 0; i < ARRAY_SIZE(ad485x_scale_avail); i++) {
+ __ad485x_get_scale(st, ad485x_scale_avail[i], &val1, &val2);
+ st->scales[i][0] = val1;
+ st->scales[i][1] = val2;
+ }
+
+ return 0;
+}
+
+static int ad485x_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long info)
+{
+ struct ad485x_state *st = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *val = st->sampling_freq;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_CALIBSCALE:
+ return ad485x_get_calibscale(st, chan->channel, val, val2);
+ case IIO_CHAN_INFO_SCALE:
+ return ad485x_get_scale(st, chan, val, val2);
+ case IIO_CHAN_INFO_CALIBBIAS:
+ return ad485x_get_calibbias(st, chan->channel, val, val2);
+ case IIO_CHAN_INFO_OFFSET:
+ scoped_guard(mutex, &st->lock)
+ * val = st->offsets[chan->channel];
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad485x_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long info)
+{
+ struct ad485x_state *st = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return ad485x_set_sampling_freq(st, val);
+ case IIO_CHAN_INFO_SCALE:
+ return ad485x_set_scale(st, chan, val, val2);
+ case IIO_CHAN_INFO_CALIBSCALE:
+ return ad485x_set_calibscale(st, chan->channel, val, val2);
+ case IIO_CHAN_INFO_CALIBBIAS:
+ return ad485x_set_calibbias(st, chan->channel, val, val2);
+ case IIO_CHAN_INFO_OFFSET:
+ return ad485x_set_offset(st, chan, val);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad485x_update_scan_mode(struct iio_dev *indio_dev,
+ const unsigned long *scan_mask)
+{
+ struct ad485x_state *st = iio_priv(indio_dev);
+ unsigned int c;
+ int ret;
+
+ for (c = 0; c < st->info->num_channels; c++) {
+ if (test_bit(c, scan_mask))
+ ret = iio_backend_chan_enable(st->back, c);
+ else
+ ret = iio_backend_chan_disable(st->back, c);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ad485x_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ struct ad485x_state *st = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ *vals = (const int *)st->scales;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ /* Values are stored in a 2D matrix */
+ *length = ARRAY_SIZE(ad485x_scale_avail) * 2;
+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_OFFSET:
+ *vals = st->info->offset_table;
+ *type = IIO_VAL_INT;
+ *length = st->info->num_offset;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+#define AD485X_IIO_CHANNEL(index, real, storage, info) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .ext_info = info, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) | \
+ BIT(IIO_CHAN_INFO_CALIBBIAS) | \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_OFFSET), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .info_mask_shared_by_type_available = \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_OFFSET), \
+ .indexed = 1, \
+ .channel = index, \
+ .scan_index = index, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = real, \
+ .storagebits = storage, \
+ }, \
+}
+
+static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
+ IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
+ IIO_ENUM_AVAILABLE("packet_format",
+ IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
+ {},
+};
+
+static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
+ IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
+ IIO_ENUM_AVAILABLE("packet_format",
+ IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
+ {},
+};
+
+static const struct iio_chan_spec ad4858_channels[] = {
+ AD485X_IIO_CHANNEL(0, 20, 32, ad4858_ext_info),
+ AD485X_IIO_CHANNEL(1, 20, 32, ad4858_ext_info),
+ AD485X_IIO_CHANNEL(2, 20, 32, ad4858_ext_info),
+ AD485X_IIO_CHANNEL(3, 20, 32, ad4858_ext_info),
+ AD485X_IIO_CHANNEL(4, 20, 32, ad4858_ext_info),
+ AD485X_IIO_CHANNEL(5, 20, 32, ad4858_ext_info),
+ AD485X_IIO_CHANNEL(6, 20, 32, ad4858_ext_info),
+ AD485X_IIO_CHANNEL(7, 20, 32, ad4858_ext_info),
+};
+
+static const struct iio_chan_spec ad4857_channels[] = {
+ AD485X_IIO_CHANNEL(0, 16, 16, ad4857_ext_info),
+ AD485X_IIO_CHANNEL(1, 16, 16, ad4857_ext_info),
+ AD485X_IIO_CHANNEL(2, 16, 16, ad4857_ext_info),
+ AD485X_IIO_CHANNEL(3, 16, 16, ad4857_ext_info),
+ AD485X_IIO_CHANNEL(4, 16, 16, ad4857_ext_info),
+ AD485X_IIO_CHANNEL(5, 16, 16, ad4857_ext_info),
+ AD485X_IIO_CHANNEL(6, 16, 16, ad4857_ext_info),
+ AD485X_IIO_CHANNEL(7, 16, 16, ad4857_ext_info),
+};
+
+static const struct ad485x_chip_info ad4858_info = {
+ .name = "ad4858",
+ .product_id = AD4858_PROD_ID,
+ .scale_table = ad485x_scale_table,
+ .num_scales = ARRAY_SIZE(ad485x_scale_table),
+ .offset_table = ad4858_offset_table,
+ .num_offset = ARRAY_SIZE(ad4858_offset_table),
+ .channels = ad4858_channels,
+ .num_channels = ARRAY_SIZE(ad4858_channels),
+ .throughput = 1 * MEGA,
+ .resolution = 20,
+};
+
+static const struct ad485x_chip_info ad4857_info = {
+ .name = "ad4857",
+ .product_id = AD4857_PROD_ID,
+ .scale_table = ad485x_scale_table,
+ .num_scales = ARRAY_SIZE(ad485x_scale_table),
+ .offset_table = ad4857_offset_table,
+ .num_offset = ARRAY_SIZE(ad4857_offset_table),
+ .channels = ad4857_channels,
+ .num_channels = ARRAY_SIZE(ad4857_channels),
+ .throughput = 1 * MEGA,
+ .resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4856_info = {
+ .name = "ad4856",
+ .product_id = AD4856_PROD_ID,
+ .scale_table = ad485x_scale_table,
+ .num_scales = ARRAY_SIZE(ad485x_scale_table),
+ .offset_table = ad4858_offset_table,
+ .num_offset = ARRAY_SIZE(ad4858_offset_table),
+ .channels = ad4858_channels,
+ .num_channels = ARRAY_SIZE(ad4858_channels),
+ .throughput = 250 * KILO,
+ .resolution = 20,
+ .resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4855_info = {
+ .name = "ad4855",
+ .product_id = AD4855_PROD_ID,
+ .scale_table = ad485x_scale_table,
+ .num_scales = ARRAY_SIZE(ad485x_scale_table),
+ .offset_table = ad4857_offset_table,
+ .num_offset = ARRAY_SIZE(ad4857_offset_table),
+ .channels = ad4857_channels,
+ .num_channels = ARRAY_SIZE(ad4857_channels),
+ .throughput = 250 * KILO,
+ .resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4854_info = {
+ .name = "ad4854",
+ .product_id = AD4854_PROD_ID,
+ .scale_table = ad485x_scale_table,
+ .num_scales = ARRAY_SIZE(ad485x_scale_table),
+ .offset_table = ad4858_offset_table,
+ .num_offset = ARRAY_SIZE(ad4858_offset_table),
+ .channels = ad4858_channels,
+ .num_channels = ARRAY_SIZE(ad4858_channels),
+ .throughput = 1 * MEGA,
+ .resolution = 20,
+};
+
+static const struct ad485x_chip_info ad4853_info = {
+ .name = "ad4853",
+ .product_id = AD4853_PROD_ID,
+ .scale_table = ad485x_scale_table,
+ .num_scales = ARRAY_SIZE(ad485x_scale_table),
+ .offset_table = ad4857_offset_table,
+ .num_offset = ARRAY_SIZE(ad4857_offset_table),
+ .channels = ad4857_channels,
+ .num_channels = ARRAY_SIZE(ad4857_channels),
+ .throughput = 1 * MEGA,
+ .resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4852_info = {
+ .name = "ad4852",
+ .product_id = AD4852_PROD_ID,
+ .scale_table = ad485x_scale_table,
+ .num_scales = ARRAY_SIZE(ad485x_scale_table),
+ .offset_table = ad4858_offset_table,
+ .num_offset = ARRAY_SIZE(ad4858_offset_table),
+ .channels = ad4858_channels,
+ .num_channels = ARRAY_SIZE(ad4858_channels),
+ .throughput = 250 * KILO,
+ .resolution = 20,
+};
+
+static const struct ad485x_chip_info ad4851_info = {
+ .name = "ad4851",
+ .product_id = AD4851_PROD_ID,
+ .scale_table = ad485x_scale_table,
+ .num_scales = ARRAY_SIZE(ad485x_scale_table),
+ .offset_table = ad4857_offset_table,
+ .num_offset = ARRAY_SIZE(ad4857_offset_table),
+ .channels = ad4857_channels,
+ .num_channels = ARRAY_SIZE(ad4857_channels),
+ .throughput = 250 * KILO,
+ .resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4858i_info = {
+ .name = "ad4858i",
+ .product_id = AD4858I_PROD_ID,
+ .scale_table = ad485x_scale_table,
+ .num_scales = ARRAY_SIZE(ad485x_scale_table),
+ .offset_table = ad4858_offset_table,
+ .num_offset = ARRAY_SIZE(ad4858_offset_table),
+ .channels = ad4858_channels,
+ .num_channels = ARRAY_SIZE(ad4858_channels),
+ .throughput = 1000000,
+ .resolution = 20,
+};
+
+static const struct iio_info ad485x_info = {
+ .debugfs_reg_access = ad485x_reg_access,
+ .read_raw = ad485x_read_raw,
+ .write_raw = ad485x_write_raw,
+ .update_scan_mode = ad485x_update_scan_mode,
+ .read_avail = ad485x_read_avail,
+};
+
+static const struct regmap_config regmap_config = {
+ .reg_bits = 16,
+ .val_bits = 8,
+ .read_flag_mask = BIT(7),
+};
+
+static const char * const ad485x_power_supplies[] = {
+ "vcc", "vdd", "vddh", "vio"
+};
+
+static int ad485x_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct ad485x_state *st;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ st = iio_priv(indio_dev);
+ st->spi = spi;
+
+ mutex_init(&st->lock);
+
+ ret = devm_regulator_bulk_get_enable(&spi->dev,
+ ARRAY_SIZE(ad485x_power_supplies),
+ ad485x_power_supplies);
+ if (ret)
+ return dev_err_probe(&spi->dev, ret,
+ "failed to get and enable supplies\n");
+
+ st->cnv = devm_pwm_get(&spi->dev, NULL);
+ if (IS_ERR(st->cnv))
+ return PTR_ERR(st->cnv);
+
+ st->info = spi_get_device_match_data(spi);
+ if (!st->info)
+ return -ENODEV;
+
+ st->regmap = devm_regmap_init_spi(spi, ®map_config);
+ if (IS_ERR(st->regmap))
+ return PTR_ERR(st->regmap);
+
+ ret = ad485x_scale_offset_fill(st);
+ if (ret)
+ return ret;
+
+ ret = ad485x_setup(st);
+ if (ret)
+ return ret;
+
+ indio_dev->name = st->info->name;
+ indio_dev->channels = st->info->channels;
+ indio_dev->num_channels = st->info->num_channels;
+ indio_dev->info = &ad485x_info;
+
+ st->back = devm_iio_backend_get(&spi->dev, NULL);
+ if (IS_ERR(st->back))
+ return PTR_ERR(st->back);
+
+ ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
+ if (ret)
+ return ret;
+
+ ret = devm_iio_backend_enable(&spi->dev, st->back);
+ if (ret)
+ return ret;
+
+ ret = ad485x_calibrate(st);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ad485x_of_match[] = {
+ { .compatible = "adi,ad4858", .data = &ad4858_info, },
+ { .compatible = "adi,ad4857", .data = &ad4857_info, },
+ { .compatible = "adi,ad4856", .data = &ad4856_info, },
+ { .compatible = "adi,ad4855", .data = &ad4855_info, },
+ { .compatible = "adi,ad4854", .data = &ad4854_info, },
+ { .compatible = "adi,ad4853", .data = &ad4853_info, },
+ { .compatible = "adi,ad4852", .data = &ad4852_info, },
+ { .compatible = "adi,ad4851", .data = &ad4851_info, },
+ { .compatible = "adi,ad4858i", .data = &ad4858i_info, },
+ {}
+};
+
+static const struct spi_device_id ad485x_spi_id[] = {
+ { "ad4858", (kernel_ulong_t)&ad4858_info },
+ { "ad4857", (kernel_ulong_t)&ad4857_info },
+ { "ad4856", (kernel_ulong_t)&ad4856_info },
+ { "ad4855", (kernel_ulong_t)&ad4855_info },
+ { "ad4854", (kernel_ulong_t)&ad4854_info },
+ { "ad4853", (kernel_ulong_t)&ad4853_info },
+ { "ad4852", (kernel_ulong_t)&ad4852_info },
+ { "ad4851", (kernel_ulong_t)&ad4851_info },
+ { "ad4858i", (kernel_ulong_t)&ad4858i_info },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, ad485x_spi_id);
+
+static struct spi_driver ad485x_driver = {
+ .probe = ad485x_probe,
+ .driver = {
+ .name = "ad485x",
+ .of_match_table = ad485x_of_match,
+ },
+ .id_table = ad485x_spi_id,
+};
+module_spi_driver(ad485x_driver);
+
+MODULE_AUTHOR("Sergiu Cuciurean <sergiu.cuciurean@analog.com>");
+MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@analog.com>");
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD485x DAS driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_BACKEND);
--
2.46.0On Mon, 23 Sep 2024 13:10:23 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> Add support for the AD485X DAS familiy.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
A few additional comments from me.
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/ad485x.c b/drivers/iio/adc/ad485x.c
> new file mode 100644
> index 000000000000..3333cad9ed8f
> --- /dev/null
> +++ b/drivers/iio/adc/ad485x.c
> @@ -0,0 +1,1061 @@
> +static int ad485x_setup(struct ad485x_state *st)
> +{
> + unsigned int product_id;
> + int ret;
> +
> + ret = ad485x_set_sampling_freq(st, 1000000);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> + AD485X_SW_RESET);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_B,
> + AD485X_SINGLE_INSTRUCTION);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> + AD485X_SDO_ENABLE);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_PRODUCT_ID_L, &product_id);
> + if (ret)
> + return ret;
> +
> + if (product_id != st->info->product_id)
> + dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> + product_id);
dev_info() for this is probably better. Might be fine afterall if
the new part is fully backwards compatible with this one.
> +
> + ret = regmap_write(st->regmap, AD485X_REG_DEVICE_CTRL,
> + AD485X_ECHO_CLOCK_MODE);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
> +}
> +
> +static int ad485x_get_packet_format(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ad485x_state *st = iio_priv(indio_dev);
> + unsigned int format;
> + int ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
> + if (ret)
> + return ret;
> +
> + format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
> +
> + return format;
return FIELD_GET()
> +}
> +
> +static int ad485x_get_calibscale(struct ad485x_state *st, int ch, int *val,
> + int *val2)
> +{
> + unsigned int reg_val;
> + int gain;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain = (reg_val & 0xFF) << 8;
Use a byte array into which you write the regval then a get_unalignedb_be16
or similar to get the value.
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain |= reg_val & 0xFF;
> +
> + *val = gain;
> + *val2 = 32768;
> +
> + return IIO_VAL_FRACTIONAL;
> +}
> +static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val,
> + int *val2)
> +{
> + unsigned int lsb, mid, msb;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> + &msb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> + &mid);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> + &lsb);
> + if (ret)
> + return ret;
> +
> + if (st->info->resolution == 16) {
> + *val = msb << 8;
> + *val |= mid;
> + *val = sign_extend32(*val, 15);
> + } else {
> + *val = msb << 12;
> + *val |= mid << 4;
> + *val |= lsb >> 4;
As below I'd use a byte array then you can do get_unaligned_be24 to
+ a right shift by 4 of the whole thing.
> + *val = sign_extend32(*val, 19);
> + }
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
> + unsigned int lsb, mid, msb;
> + int ret;
> +
> + if (st->info->resolution == 16) {
> + lsb = 0;
> + mid = val & 0xFF;
> + msb = (val >> 8) & 0xFF;
> + } else {
> + lsb = (val << 4) & 0xFF;
Might as well mask by 0xF0 to make it really clear
nothing in bottom few bits.
> + mid = (val >> 4) & 0xFF;
> + msb = (val >> 12) & 0xFF;
I'd be tempted to shift the whole thing left 4 bits and
then use a put_unaligned_be24 on it into a byte array then pick
out the bytes from that array. Will be easier to read.
Above 16 bit case would be a put_unaligned_be16
> + }
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);
I think you already got this question, but consider bulk writes.
> +}
> +
> +static const unsigned int ad485x_scale_table[][2] = {
> + {2500, 0x0}, {5000, 0x1}, {5000, 0x2}, {10000, 0x3}, {6250, 0x04},
> + {12500, 0x5}, {10000, 0x6}, {20000, 0x7}, {12500, 0x8}, {25000, 0x9},
> + {20000, 0xA}, {40000, 0xB}, {25000, 0xC}, {50000, 0xD}, {40000, 0xE},
> + {80000, 0xF}
Spaces after { and before } preferred.
Maybe just have this as one item per line. I think that is more readable.
> +};
> +
> +static int ad485x_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long info)
> +{
> + struct ad485x_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = st->sampling_freq;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + return ad485x_get_calibscale(st, chan->channel, val, val2);
> + case IIO_CHAN_INFO_SCALE:
> + return ad485x_get_scale(st, chan, val, val2);
> + case IIO_CHAN_INFO_CALIBBIAS:
> + return ad485x_get_calibbias(st, chan->channel, val, val2);
> + case IIO_CHAN_INFO_OFFSET:
> + scoped_guard(mutex, &st->lock)
> + * val = st->offsets[chan->channel];
*val = ...
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +#define AD485X_IIO_CHANNEL(index, real, storage, info) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .ext_info = info, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) | \
> + BIT(IIO_CHAN_INFO_CALIBBIAS) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
Maybe add avail for calibscale and calibbias as well.
> +
> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + {},
> +};
> +
> +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> + {},
I think Andy pointed these out. No trailing comma on terminators.
However, I'm not sure this ABI is practical currently.
Basically I have no idea what it is or how to use it!
> +};
> +
> +static const struct ad485x_chip_info ad4858i_info = {
> + .name = "ad4858i",
> + .product_id = AD4858I_PROD_ID,
> + .scale_table = ad485x_scale_table,
> + .num_scales = ARRAY_SIZE(ad485x_scale_table),
> + .offset_table = ad4858_offset_table,
> + .num_offset = ARRAY_SIZE(ad4858_offset_table),
> + .channels = ad4858_channels,
> + .num_channels = ARRAY_SIZE(ad4858_channels),
> + .throughput = 1000000,
Odd you use MEGA above, but not here.
> + .resolution = 20,
> +};
> +static int ad485x_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ad485x_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + mutex_init(&st->lock);
Ever so slight preference for the new option of.
ret = devm_mutex_init(dev, &st->lock);
if (ret)
return ret;
...
> +}
> +
> +static const struct of_device_id ad485x_of_match[] = {
> + { .compatible = "adi,ad4858", .data = &ad4858_info, },
> + { .compatible = "adi,ad4857", .data = &ad4857_info, },
> + { .compatible = "adi,ad4856", .data = &ad4856_info, },
> + { .compatible = "adi,ad4855", .data = &ad4855_info, },
> + { .compatible = "adi,ad4854", .data = &ad4854_info, },
> + { .compatible = "adi,ad4853", .data = &ad4853_info, },
> + { .compatible = "adi,ad4852", .data = &ad4852_info, },
> + { .compatible = "adi,ad4851", .data = &ad4851_info, },
> + { .compatible = "adi,ad4858i", .data = &ad4858i_info, },
> + {}
{ }
> +};
> +
On Mon, Sep 23, 2024 at 12:17 PM Antoniu Miclaus
<antoniu.miclaus@analog.com> wrote:
>
> Add support for the AD485X DAS familiy.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
...
> +static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
> +{
> + int i, cnt = 0, max_cnt = 0, start, max_start = 0;
> +
> + for (i = 0, start = -1; i < size; i++) {
> + if (field[i] == 0) {
> + if (start == -1)
> + start = i;
> + cnt++;
> + } else {
> + if (cnt > max_cnt) {
> + max_cnt = cnt;
> + max_start = start;
> + }
> + start = -1;
> + cnt = 0;
> + }
> + }
> +
> + if (cnt > max_cnt) {
> + max_cnt = cnt;
> + max_start = start;
> + }
> +
> + if (!max_cnt)
> + return -EIO;
EIO seems an odd choice since this function doesn't actually do any
I/O. Maybe EINVAL would be better?
> +
> + *ret_start = max_start;
> +
> + return max_cnt;
> +}
> +
...
> +static int ad485x_set_calibscale(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
> + unsigned long long gain;
> + unsigned int reg_val;
> + int ret;
> +
Need to check both val and val2 for negative here before converting to unsigned.
if (val < 0 || val2 < 0)
return -EINVAL;
> + gain = val * 1000000 + val2;
> + gain = gain * 32768;
> + gain = DIV_U64_ROUND_CLOSEST(gain, 1000000);
> +
> + reg_val = gain;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> + reg_val >> 8);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> + reg_val & 0xFF);
> +}
> +
> +static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val,
> + int *val2)
> +{
val2 is unused and can be removed
> + unsigned int lsb, mid, msb;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> + &msb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> + &mid);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> + &lsb);
> + if (ret)
> + return ret;
> +
> + if (st->info->resolution == 16) {
> + *val = msb << 8;
> + *val |= mid;
> + *val = sign_extend32(*val, 15);
> + } else {
> + *val = msb << 12;
> + *val |= mid << 4;
> + *val |= lsb >> 4;
> + *val = sign_extend32(*val, 19);
> + }
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
val2 is unused here. It would also be a good idea to implement the
write_raw_get_fmt callback to select IIO_VAL_INT for this attribute to
avoid having to deal with negative val2.
> + unsigned int lsb, mid, msb;
> + int ret;
Should check for negative val here before converting to unsigned.
> +
> + if (st->info->resolution == 16) {
> + lsb = 0;
> + mid = val & 0xFF;
> + msb = (val >> 8) & 0xFF;
> + } else {
> + lsb = (val << 4) & 0xFF;
> + mid = (val >> 4) & 0xFF;
> + msb = (val >> 12) & 0xFF;
> + }
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);
> +}
> +
...
> +static int ad485x_set_offset(struct ad485x_state *st,
> + const struct iio_chan_spec *chan, int val)
> +{
> + guard(mutex)(&st->lock);
> +
> + if (st->offsets[chan->channel] != val) {
> + st->offsets[chan->channel] = val;
> + /* Restore to the default range if offset changes */
> + if (st->offsets[chan->channel])
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + AD485X_SOFTSPAN_N40V_40V);
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + AD485X_SOFTSPAN_0V_40V);
> + }
> +
> + return 0;
> +}
I'm not sure I understand the relationship between softspan and the
offset. A raw value of 0 always means we measured 0V no matter what
the softspan setting is. So it seems like the offset should always be
0.
I'm guessing the intent was to handle bipolar vs. unipolar softspans,
but this doesn't actually work mathematically.
So far, I've only seen inputs that can be bipolar or unipolar
specified in the devicetree. I'm not aware of a way to select this at
runtime.
> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + {},
> +};
> +
> +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> + {},
> +};
Usually, something like this packet format would be automatically
selected when buffered reads are enabled based on what other features
it provides are needed, i.e only enable the status bits when events
are enabled.
(For those that didn't read the datasheet, the different packet
formats basically enable extra status bits per sample. And in the case
of oversampling, one of the formats also selects a reduced number of
sample bits.)
We have quite a few parts in the pipline right like this one that have
per-sample status bits. In the past, these were generally handled with
IIO events, but this doesn't really work for these high-speed backends
since the data is being piped directly to DMA and we don't look at
each sample in the ADC driver. So it would be worthwhile to try to
find some general solution here for handling this sort of thing.
> +
> +static const struct iio_chan_spec ad4858_channels[] = {
> + AD485X_IIO_CHANNEL(0, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(1, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(2, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(3, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(4, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(5, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(6, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(7, 20, 32, ad4858_ext_info),
> +};
> +
> +static const struct iio_chan_spec ad4857_channels[] = {
> + AD485X_IIO_CHANNEL(0, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(1, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(2, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(3, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(4, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(5, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(6, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(7, 16, 16, ad4857_ext_info),
> +};
How does 16-bit storage size work when packet format is 24-bit?
>
> > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > + IIO_ENUM_AVAILABLE("packet_format",
> > + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > + {},
> > +};
> > +
> > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > + IIO_ENUM_AVAILABLE("packet_format",
> > + IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > + {},
> > +};
>
> Usually, something like this packet format would be automatically
> selected when buffered reads are enabled based on what other features
> it provides are needed, i.e only enable the status bits when events
> are enabled.
>
> (For those that didn't read the datasheet, the different packet
> formats basically enable extra status bits per sample. And in the case
> of oversampling, one of the formats also selects a reduced number of
> sample bits.)
>
> We have quite a few parts in the pipline right like this one that have
> per-sample status bits. In the past, these were generally handled with
> IIO events, but this doesn't really work for these high-speed backends
> since the data is being piped directly to DMA and we don't look at
> each sample in the ADC driver. So it would be worthwhile to try to
> find some general solution here for handling this sort of thing.
We have previously talked about schemes to describe metadata
alongside channels. I guess maybe it's time to actually look at how
that works. I'm not sure dynamic control of that metadata
is going to be easy to do though or if we even want to
(as opposed to always on or off for a particular device).
On Sat, 2024-09-28 at 18:30 +0100, Jonathan Cameron wrote:
>
> >
> > > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > + IIO_ENUM_AVAILABLE("packet_format",
> > > + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > + {},
> > > +};
> > > +
> > > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > + IIO_ENUM_AVAILABLE("packet_format",
> > > + IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > + {},
> > > +};
> >
> > Usually, something like this packet format would be automatically
> > selected when buffered reads are enabled based on what other features
> > it provides are needed, i.e only enable the status bits when events
> > are enabled.
> >
> > (For those that didn't read the datasheet, the different packet
> > formats basically enable extra status bits per sample. And in the case
> > of oversampling, one of the formats also selects a reduced number of
> > sample bits.)
> >
> > We have quite a few parts in the pipline right like this one that have
> > per-sample status bits. In the past, these were generally handled with
> > IIO events, but this doesn't really work for these high-speed backends
> > since the data is being piped directly to DMA and we don't look at
> > each sample in the ADC driver. So it would be worthwhile to try to
> > find some general solution here for handling this sort of thing.
I did not read the datasheet that extensively but here it goes my 2 cents
(basically my internal feedback on this one):
So this packet format thingy may be a very "funny" discussion if we really need
to support it. I'm not sure how useful it is the 32 bits format rather than
being used in test pattern. I'm not seeing too much benefit on the channel id or
span id information (we can already get that info with other attributes). The
OR/UR is the one that could be more useful but is there someone using it? Do we
really need to have it close to the sample? If not, there's the status register
and... Also, I think this can be implemented using IIO events (likely what we
will be asked). So what comes to mind could be:
For test_pattern (could be implemented as ext_info or an additional channel I
think - not for now I guess) we can easily look at our word side and dynamically
set the proper packet size. So, to me, this is effectively the only place where
32bits would make sense (assuming we don't implement OR/UR for now).
For oversampling we can have both 20/24 bit averaged data. But from the
datasheet:
"Oversampling is useful in applications requiring lower noise and higher dynamic
range per output data-word, which the AD4858 supports with 24-bit output
resolution and reduced average output data rates"
So from the above it looks like it could make sense to default to 24bit packets
if oversampling is enabled.
Now the question is OR/UR. If that is something we can support with events, we
could see when one of OR/UR is being requested to either enable 24 packets (no
oversampling) or 32 bit packets (oversampling on).
>
> We have previously talked about schemes to describe metadata
> alongside channels. I guess maybe it's time to actually look at how
> that works. I'm not sure dynamic control of that metadata
> is going to be easy to do though or if we even want to
> (as opposed to always on or off for a particular device).
>
Indeed this is something we have been discussing and the ability to have status
alongside a buffered samples is starting to be requested more and more. Some
parts do have the status bit alongside the sample (meaning in the same register
read) which means it basically goes with the sample as part of it's
storage_bits. While not ideal, an application caring about those bits still has
access to the complete raw sample and can access them. It gets more complicated
where the status (sometimes a per device status register) is located in another
register. I guess we can have two case:
1) A device status which applies for all channels being sampled;
2) A per channel status (where the .metada approach could make sense).
But I'm not sure how we could define something like this other than assuming
that raw status data is being sent to userspace (given that every device has
it's own custom status bits and quirks).
- Nuno Sá
On Mon, 30 Sep 2024 09:05:04 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Sat, 2024-09-28 at 18:30 +0100, Jonathan Cameron wrote:
> >
> > >
> > > > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > > > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > > + IIO_ENUM_AVAILABLE("packet_format",
> > > > + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > > + {},
> > > > +};
> > > > +
> > > > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > > > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > > + IIO_ENUM_AVAILABLE("packet_format",
> > > > + IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > > + {},
> > > > +};
> > >
> > > Usually, something like this packet format would be automatically
> > > selected when buffered reads are enabled based on what other features
> > > it provides are needed, i.e only enable the status bits when events
> > > are enabled.
> > >
> > > (For those that didn't read the datasheet, the different packet
> > > formats basically enable extra status bits per sample. And in the case
> > > of oversampling, one of the formats also selects a reduced number of
> > > sample bits.)
> > >
> > > We have quite a few parts in the pipline right like this one that have
> > > per-sample status bits. In the past, these were generally handled with
> > > IIO events, but this doesn't really work for these high-speed backends
> > > since the data is being piped directly to DMA and we don't look at
> > > each sample in the ADC driver. So it would be worthwhile to try to
> > > find some general solution here for handling this sort of thing.
>
> I did not read the datasheet that extensively but here it goes my 2 cents
> (basically my internal feedback on this one):
>
> So this packet format thingy may be a very "funny" discussion if we really need
> to support it. I'm not sure how useful it is the 32 bits format rather than
> being used in test pattern. I'm not seeing too much benefit on the channel id or
> span id information (we can already get that info with other attributes). The
> OR/UR is the one that could be more useful but is there someone using it? Do we
> really need to have it close to the sample? If not, there's the status register
> and... Also, I think this can be implemented using IIO events (likely what we
> will be asked). So what comes to mind could be:
Definite preference for using events, but for a device doing DMA I'm not sure
how we can do that without requiring parsing all the data.
So we would need some metadata description to know it is there.
>
> For test_pattern (could be implemented as ext_info or an additional channel I
> think - not for now I guess) we can easily look at our word side and dynamically
> set the proper packet size. So, to me, this is effectively the only place where
> 32bits would make sense (assuming we don't implement OR/UR for now).
> For oversampling we can have both 20/24 bit averaged data. But from the
> datasheet:
>
> "Oversampling is useful in applications requiring lower noise and higher dynamic
> range per output data-word, which the AD4858 supports with 24-bit output
> resolution and reduced average output data rates"
>
> So from the above it looks like it could make sense to default to 24bit packets
> if oversampling is enabled.
That sounds like what we do for the DMA oversampling cases that change
the resolutions.
>
> Now the question is OR/UR. If that is something we can support with events, we
> could see when one of OR/UR is being requested to either enable 24 packets (no
> oversampling) or 32 bit packets (oversampling on).
>
>
>
> >
> > We have previously talked about schemes to describe metadata
> > alongside channels. I guess maybe it's time to actually look at how
> > that works. I'm not sure dynamic control of that metadata
> > is going to be easy to do though or if we even want to
> > (as opposed to always on or off for a particular device).
> >
>
> Indeed this is something we have been discussing and the ability to have status
> alongside a buffered samples is starting to be requested more and more. Some
> parts do have the status bit alongside the sample (meaning in the same register
> read) which means it basically goes with the sample as part of it's
> storage_bits. While not ideal, an application caring about those bits still has
> access to the complete raw sample and can access them.
This has the advantage that if we come along later and define a metadata
in storage bits description it is backwards compatible. We've been doing
this for years with some devices.
> It gets more complicated
> where the status (sometimes a per device status register) is located in another
> register. I guess we can have two case:
>
> 1) A device status which applies for all channels being sampled;
> 2) A per channel status (where the .metada approach could make sense).
If it's a separate register per channel and optional, we'd have to treat it as a metadata
channel as no guarantee it would be packed next to the main channel.
If we have a description of metadata additions in main channel storage, I'm not
against having a IIO_METADATA channel type.
If it's a single channel I'm not sure how we'd make as channel description
general enough easily as we end up with every field possibly needed an association
with a different channel.
>
> But I'm not sure how we could define something like this other than assuming
> that raw status data is being sent to userspace (given that every device has
> it's own custom status bits and quirks).
That is always fine.
Jonathan
>
> - Nuno Sá
On Sat, 2024-10-05 at 18:26 +0100, Jonathan Cameron wrote:
> On Mon, 30 Sep 2024 09:05:04 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Sat, 2024-09-28 at 18:30 +0100, Jonathan Cameron wrote:
> > >
> > > >
> > > > > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > > > > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL,
> > > > > &ad4858_packet_fmt),
> > > > > + IIO_ENUM_AVAILABLE("packet_format",
> > > > > + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > > > + {},
> > > > > +};
> > > > > +
> > > > > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > > > > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL,
> > > > > &ad4857_packet_fmt),
> > > > > + IIO_ENUM_AVAILABLE("packet_format",
> > > > > + IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > > > + {},
> > > > > +};
> > > >
> > > > Usually, something like this packet format would be automatically
> > > > selected when buffered reads are enabled based on what other features
> > > > it provides are needed, i.e only enable the status bits when events
> > > > are enabled.
> > > >
> > > > (For those that didn't read the datasheet, the different packet
> > > > formats basically enable extra status bits per sample. And in the case
> > > > of oversampling, one of the formats also selects a reduced number of
> > > > sample bits.)
> > > >
> > > > We have quite a few parts in the pipline right like this one that have
> > > > per-sample status bits. In the past, these were generally handled with
> > > > IIO events, but this doesn't really work for these high-speed backends
> > > > since the data is being piped directly to DMA and we don't look at
> > > > each sample in the ADC driver. So it would be worthwhile to try to
> > > > find some general solution here for handling this sort of thing.
> >
> > I did not read the datasheet that extensively but here it goes my 2 cents
> > (basically my internal feedback on this one):
> >
> > So this packet format thingy may be a very "funny" discussion if we really
> > need
> > to support it. I'm not sure how useful it is the 32 bits format rather than
> > being used in test pattern. I'm not seeing too much benefit on the channel
> > id or
> > span id information (we can already get that info with other attributes).
> > The
> > OR/UR is the one that could be more useful but is there someone using it? Do
> > we
> > really need to have it close to the sample? If not, there's the status
> > register
> > and... Also, I think this can be implemented using IIO events (likely what
> > we
> > will be asked). So what comes to mind could be:
>
> Definite preference for using events, but for a device doing DMA I'm not sure
> how we can do that without requiring parsing all the data.
>
> So we would need some metadata description to know it is there.
>
> >
> > For test_pattern (could be implemented as ext_info or an additional channel
> > I
> > think - not for now I guess) we can easily look at our word side and
> > dynamically
> > set the proper packet size. So, to me, this is effectively the only place
> > where
> > 32bits would make sense (assuming we don't implement OR/UR for now).
> > For oversampling we can have both 20/24 bit averaged data. But from the
> > datasheet:
> >
> > "Oversampling is useful in applications requiring lower noise and higher
> > dynamic
> > range per output data-word, which the AD4858 supports with 24-bit output
> > resolution and reduced average output data rates"
> >
> > So from the above it looks like it could make sense to default to 24bit
> > packets
> > if oversampling is enabled.
>
> That sounds like what we do for the DMA oversampling cases that change
> the resolutions.
>
> >
> > Now the question is OR/UR. If that is something we can support with events,
> > we
> > could see when one of OR/UR is being requested to either enable 24 packets
> > (no
> > oversampling) or 32 bit packets (oversampling on).
> >
> >
> >
> > >
> > > We have previously talked about schemes to describe metadata
> > > alongside channels. I guess maybe it's time to actually look at how
> > > that works. I'm not sure dynamic control of that metadata
> > > is going to be easy to do though or if we even want to
> > > (as opposed to always on or off for a particular device).
> > >
> >
> > Indeed this is something we have been discussing and the ability to have
> > status
> > alongside a buffered samples is starting to be requested more and more. Some
> > parts do have the status bit alongside the sample (meaning in the same
> > register
> > read) which means it basically goes with the sample as part of it's
> > storage_bits. While not ideal, an application caring about those bits still
> > has
> > access to the complete raw sample and can access them.
>
> This has the advantage that if we come along later and define a metadata
> in storage bits description it is backwards compatible. We've been doing
> this for years with some devices.
>
> > It gets more complicated
> > where the status (sometimes a per device status register) is located in
> > another
> > register. I guess we can have two case:
> >
> > 1) A device status which applies for all channels being sampled;
> > 2) A per channel status (where the .metada approach could make sense).
>
> If it's a separate register per channel and optional, we'd have to treat it as
> a metadata
> channel as no guarantee it would be packed next to the main channel.
>
> If we have a description of metadata additions in main channel storage, I'm
> not
> against having a IIO_METADATA channel type.
>
I guess you mean that a complete solution would never only be a IIO_METADATA but
also extending 'struct iio_scan_type'?
> If it's a single channel I'm not sure how we'd make as channel description
> general enough easily as we end up with every field possibly needed an
> association
> with a different channel.
Not sure I followed the above... You mean having a single channel (like one
register) pointing to different channels?
What I mean with 1) is for example what happens with IMUs (eg: adis16475). They
have a DIAG_STAT register (which is pretty much device wide status/error
information) that's also part of burst transfers (where we get our samples) and
while some bits might not be meaningful for the sampling "session", others might
make sense to reason about data integrity or correctness.
For the above case, I think the IIO_METADATA channel type would fit.
- Nuno Sá
On Fri, 11 Oct 2024 12:23:49 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Sat, 2024-10-05 at 18:26 +0100, Jonathan Cameron wrote:
> > On Mon, 30 Sep 2024 09:05:04 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > > On Sat, 2024-09-28 at 18:30 +0100, Jonathan Cameron wrote:
> > > >
> > > > >
> > > > > > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > > > > > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL,
> > > > > > &ad4858_packet_fmt),
> > > > > > + IIO_ENUM_AVAILABLE("packet_format",
> > > > > > + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > > > > + {},
> > > > > > +};
> > > > > > +
> > > > > > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > > > > > + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL,
> > > > > > &ad4857_packet_fmt),
> > > > > > + IIO_ENUM_AVAILABLE("packet_format",
> > > > > > + IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > > > > + {},
> > > > > > +};
> > > > >
> > > > > Usually, something like this packet format would be automatically
> > > > > selected when buffered reads are enabled based on what other features
> > > > > it provides are needed, i.e only enable the status bits when events
> > > > > are enabled.
> > > > >
> > > > > (For those that didn't read the datasheet, the different packet
> > > > > formats basically enable extra status bits per sample. And in the case
> > > > > of oversampling, one of the formats also selects a reduced number of
> > > > > sample bits.)
> > > > >
> > > > > We have quite a few parts in the pipline right like this one that have
> > > > > per-sample status bits. In the past, these were generally handled with
> > > > > IIO events, but this doesn't really work for these high-speed backends
> > > > > since the data is being piped directly to DMA and we don't look at
> > > > > each sample in the ADC driver. So it would be worthwhile to try to
> > > > > find some general solution here for handling this sort of thing.
> > >
> > > I did not read the datasheet that extensively but here it goes my 2 cents
> > > (basically my internal feedback on this one):
> > >
> > > So this packet format thingy may be a very "funny" discussion if we really
> > > need
> > > to support it. I'm not sure how useful it is the 32 bits format rather than
> > > being used in test pattern. I'm not seeing too much benefit on the channel
> > > id or
> > > span id information (we can already get that info with other attributes).
> > > The
> > > OR/UR is the one that could be more useful but is there someone using it? Do
> > > we
> > > really need to have it close to the sample? If not, there's the status
> > > register
> > > and... Also, I think this can be implemented using IIO events (likely what
> > > we
> > > will be asked). So what comes to mind could be:
> >
> > Definite preference for using events, but for a device doing DMA I'm not sure
> > how we can do that without requiring parsing all the data.
> >
> > So we would need some metadata description to know it is there.
> >
> > >
> > > For test_pattern (could be implemented as ext_info or an additional channel
> > > I
> > > think - not for now I guess) we can easily look at our word side and
> > > dynamically
> > > set the proper packet size. So, to me, this is effectively the only place
> > > where
> > > 32bits would make sense (assuming we don't implement OR/UR for now).
> > > For oversampling we can have both 20/24 bit averaged data. But from the
> > > datasheet:
> > >
> > > "Oversampling is useful in applications requiring lower noise and higher
> > > dynamic
> > > range per output data-word, which the AD4858 supports with 24-bit output
> > > resolution and reduced average output data rates"
> > >
> > > So from the above it looks like it could make sense to default to 24bit
> > > packets
> > > if oversampling is enabled.
> >
> > That sounds like what we do for the DMA oversampling cases that change
> > the resolutions.
> >
> > >
> > > Now the question is OR/UR. If that is something we can support with events,
> > > we
> > > could see when one of OR/UR is being requested to either enable 24 packets
> > > (no
> > > oversampling) or 32 bit packets (oversampling on).
> > >
> > >
> > >
> > > >
> > > > We have previously talked about schemes to describe metadata
> > > > alongside channels. I guess maybe it's time to actually look at how
> > > > that works. I'm not sure dynamic control of that metadata
> > > > is going to be easy to do though or if we even want to
> > > > (as opposed to always on or off for a particular device).
> > > >
> > >
> > > Indeed this is something we have been discussing and the ability to have
> > > status
> > > alongside a buffered samples is starting to be requested more and more. Some
> > > parts do have the status bit alongside the sample (meaning in the same
> > > register
> > > read) which means it basically goes with the sample as part of it's
> > > storage_bits. While not ideal, an application caring about those bits still
> > > has
> > > access to the complete raw sample and can access them.
> >
> > This has the advantage that if we come along later and define a metadata
> > in storage bits description it is backwards compatible. We've been doing
> > this for years with some devices.
> >
> > > It gets more complicated
> > > where the status (sometimes a per device status register) is located in
> > > another
> > > register. I guess we can have two case:
> > >
> > > 1) A device status which applies for all channels being sampled;
> > > 2) A per channel status (where the .metada approach could make sense).
> >
> > If it's a separate register per channel and optional, we'd have to treat it as
> > a metadata
> > channel as no guarantee it would be packed next to the main channel.
> >
> > If we have a description of metadata additions in main channel storage, I'm
> > not
> > against having a IIO_METADATA channel type.
> >
>
> I guess you mean that a complete solution would never only be a IIO_METADATA but
> also extending 'struct iio_scan_type'?
Yes we needs a way to refer to an existing scan element then we could add additional
channels and refer into them as needed.
>
> > If it's a single channel I'm not sure how we'd make as channel description
> > general enough easily as we end up with every field possibly needed an
> > association
> > with a different channel.
>
> Not sure I followed the above... You mean having a single channel (like one
> register) pointing to different channels?
Both way's around occur. Multiple channels, some normal, some metadata some
separate pointing to a single storage location and per channel meta data
for different 'signals' shoved in one register.
>
> What I mean with 1) is for example what happens with IMUs (eg: adis16475). They
> have a DIAG_STAT register (which is pretty much device wide status/error
> information) that's also part of burst transfers (where we get our samples) and
> while some bits might not be meaningful for the sampling "session", others might
> make sense to reason about data integrity or correctness.
>
> For the above case, I think the IIO_METADATA channel type would fit.
That one is easy. Fiddly case is where we have say overflow bits for
multiple signals (i.e. pins) in a single dmabuffer element.
To make that work cleanly we need a way to not only describe the contents
but cross reference it to the related data.
We've discussed ways to group actual channel (i.e. current and power on same pin)
in the past but doing this for metadata that is packed in multiple channels
is going to be a real pain.
Basically it all needs to be very flexible and any attempt to support a subset
is likely to wall us into an inflexible ABI.
Jonathan
>
> - Nuno Sá
On Sat, Sep 28, 2024 at 8:30 PM Jonathan Cameron <jic23@kernel.org> wrote: ... > > We have quite a few parts in the pipline right like this one that have > > per-sample status bits. In the past, these were generally handled with > > IIO events, but this doesn't really work for these high-speed backends > > since the data is being piped directly to DMA and we don't look at > > each sample in the ADC driver. So it would be worthwhile to try to > > find some general solution here for handling this sort of thing. > > We have previously talked about schemes to describe metadata > alongside channels. I guess maybe it's time to actually look at how > that works. I'm not sure dynamic control of that metadata > is going to be easy to do though or if we even want to > (as opposed to always on or off for a particular device). Time for the kernel to return JSON on a per channel basis? Just saying :-) -- With Best Regards, Andy Shevchenko
On Thu, Sep 26, 2024 at 04:39:18PM +0200, David Lechner wrote:
> On Mon, Sep 23, 2024 at 12:17 PM Antoniu Miclaus
> <antoniu.miclaus@analog.com> wrote:
...
> > +static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
> > +{
> > + int i, cnt = 0, max_cnt = 0, start, max_start = 0;
> > +
> > + for (i = 0, start = -1; i < size; i++) {
> > + if (field[i] == 0) {
> > + if (start == -1)
> > + start = i;
> > + cnt++;
> > + } else {
> > + if (cnt > max_cnt) {
> > + max_cnt = cnt;
> > + max_start = start;
> > + }
> > + start = -1;
> > + cnt = 0;
> > + }
> > + }
> > +
> > + if (cnt > max_cnt) {
> > + max_cnt = cnt;
> > + max_start = start;
> > + }
> > +
> > + if (!max_cnt)
> > + return -EIO;
>
> EIO seems an odd choice since this function doesn't actually do any
> I/O. Maybe EINVAL would be better?
I would even go with -ENOENT as function called 'find'.
> > + *ret_start = max_start;
> > +
> > + return max_cnt;
> > +}
--
With Best Regards,
Andy Shevchenko
Hi Antoniu,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.11]
[cannot apply to jic23-iio/togreg next-20240924]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Antoniu-Miclaus/iio-backend-add-API-for-interface-get/20240923-182050
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240923101206.3753-7-antoniu.miclaus%40analog.com
patch subject: [PATCH 6/7] iio: adc: ad485x: add ad485x driver
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20240924/202409242353.rDAcuGYR-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409242353.rDAcuGYR-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409242353.rDAcuGYR-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/iio/adc/ad485x.c: In function 'ad485x_get_packet_format':
>> drivers/iio/adc/ad485x.c:396:18: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
396 | format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
| ^~~~~~~~~
drivers/iio/adc/ad485x.c: At top level:
drivers/iio/adc/ad485x.c:854:23: warning: initialized field overwritten [-Woverride-init]
854 | .resolution = 16,
| ^~
drivers/iio/adc/ad485x.c:854:23: note: (near initialization for 'ad4856_info.resolution')
vim +/FIELD_GET +396 drivers/iio/adc/ad485x.c
384
385 static int ad485x_get_packet_format(struct iio_dev *indio_dev,
386 const struct iio_chan_spec *chan)
387 {
388 struct ad485x_state *st = iio_priv(indio_dev);
389 unsigned int format;
390 int ret;
391
392 ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
393 if (ret)
394 return ret;
395
> 396 format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
397
398 return format;
399 }
400
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Mon, Sep 23, 2024 at 01:10:23PM +0300, Antoniu Miclaus wrote:
> Add support for the AD485X DAS familiy.
...
> +#include <asm/unaligned.h>
It's better to move this after linux/*.h
> +#include <linux/delay.h>
> +#include <linux/iio/backend.h>
> +#include <linux/iio/iio.h>
This can be split to a group after the other linux/*.h
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
Seems missing headers:
+ array_size.h
+ bits.h
+ device.h
+ err.h
+ mod_devicetable.h
+ module.h
+ mutex.h
+ types.h
...
> +static int ad485x_set_sampling_freq(struct ad485x_state *st, unsigned int freq)
> +{
> + struct pwm_state cnv_state = {
> + .duty_cycle = AD485X_T_CNVH_NS,
> + .enabled = true,
> + };
> + int ret;
> +
> + if (freq > st->info->throughput)
> + freq = st->info->throughput;
clamp() ? (will need minmax.h)
> + cnv_state.period = DIV_ROUND_CLOSEST_ULL(1000000000, freq);
We have time / frequency constants, like HZ_PER_..., NSEC_PER_....
> + ret = pwm_apply_might_sleep(st->cnv, &cnv_state);
> + if (ret)
> + return ret;
> +
> + st->sampling_freq = freq;
> +
> + return 0;
> +}
> +
> +static int ad485x_setup(struct ad485x_state *st)
> +{
> + unsigned int product_id;
> + int ret;
> +
> + ret = ad485x_set_sampling_freq(st, 1000000);
Ditto.
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> + AD485X_SW_RESET);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_B,
> + AD485X_SINGLE_INSTRUCTION);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> + AD485X_SDO_ENABLE);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_PRODUCT_ID_L, &product_id);
> + if (ret)
> + return ret;
> +
> + if (product_id != st->info->product_id)
> + dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> + product_id);
> +
> + ret = regmap_write(st->regmap, AD485X_REG_DEVICE_CTRL,
> + AD485X_ECHO_CLOCK_MODE);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
> +}
...
> +static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
> +{
> + int i, cnt = 0, max_cnt = 0, start, max_start = 0;
Why are they signed?
> + for (i = 0, start = -1; i < size; i++) {
> + if (field[i] == 0) {
> + if (start == -1)
> + start = i;
> + cnt++;
> + } else {
> + if (cnt > max_cnt) {
> + max_cnt = cnt;
> + max_start = start;
> + }
> + start = -1;
> + cnt = 0;
> + }
> + }
> +
> + if (cnt > max_cnt) {
> + max_cnt = cnt;
> + max_start = start;
> + }
> +
> + if (!max_cnt)
> + return -EIO;
> +
> + *ret_start = max_start;
> +
> + return max_cnt;
> +}
...
> +static int ad485x_calibrate(struct ad485x_state *st)
> +{
> + int opt_delay, lane_num, delay, i, s, c;
Why e.g. i is signed?
> + enum iio_backend_interface_type interface_type;
> + bool pn_status[AD485X_MAX_LANES][AD485X_MAX_IODELAY];
Why not a bitmap?
> + int ret;
> +}
...
> +static const char *const ad4858_packet_fmts[] = {
> + "20-bit", "24-bit", "32-bit"
Leave trailing comma, it may help extending the list in case it becomes a
multi-line.
> +};
> +
> +static const char *const ad4857_packet_fmts[] = {
> + "16-bit", "24-bit"
Ditto.
> +};
...
> +static int ad485x_get_packet_format(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ad485x_state *st = iio_priv(indio_dev);
> + unsigned int format;
> + int ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
> + if (ret)
> + return ret;
> + format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
> +
> + return format;
return FIELD_GET(...);
> +}
> +static int ad485x_get_calibscale(struct ad485x_state *st, int ch, int *val,
> + int *val2)
One line?
...
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain = (reg_val & 0xFF) << 8;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain |= reg_val & 0xFF;
Why not bulk read and proper __le16/__be16 type?
...
> +static int ad485x_set_calibscale(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
> + unsigned long long gain;
> + unsigned int reg_val;
> + int ret;
> +
> + gain = val * 1000000 + val2;
MICRO?
> + gain = gain * 32768;
> + gain = DIV_U64_ROUND_CLOSEST(gain, 1000000);
Can be combined into one lien.
> + reg_val = gain;
...
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> + reg_val >> 8);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> + reg_val & 0xFF);
Bulk write?
> +}
...
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> + &msb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> + &mid);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> + &lsb);
> + if (ret)
> + return ret;
Bulk read?
> + if (st->info->resolution == 16) {
> + *val = msb << 8;
> + *val |= mid;
> + *val = sign_extend32(*val, 15);
So, please use respective byteorder conversions...
> + } else {
> + *val = msb << 12;
> + *val |= mid << 4;
> + *val |= lsb >> 4;
> + *val = sign_extend32(*val, 19);
...here as well (we have _be14()/_le24 ones).
> + }
...
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
> + unsigned int lsb, mid, msb;
> + int ret;
> +
> + if (st->info->resolution == 16) {
> + lsb = 0;
> + mid = val & 0xFF;
> + msb = (val >> 8) & 0xFF;
> + } else {
> + lsb = (val << 4) & 0xFF;
> + mid = (val >> 4) & 0xFF;
> + msb = (val >> 12) & 0xFF;
> + }
Ditto.
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);
Bulk write?
> +}
...
> +static const unsigned int ad485x_scale_table[][2] = {
> + {2500, 0x0}, {5000, 0x1}, {5000, 0x2}, {10000, 0x3}, {6250, 0x04},
> + {12500, 0x5}, {10000, 0x6}, {20000, 0x7}, {12500, 0x8}, {25000, 0x9},
> + {20000, 0xA}, {40000, 0xB}, {25000, 0xC}, {50000, 0xD}, {40000, 0xE},
> + {80000, 0xF}
It's more natural to list them in pow-of-two per line manner.
Moreover the last item is redundant. It's equal to the index. Why do you need it?
> +};
...
> +static const int ad4857_offset_table[] = {
> + 0, -32768
Here, and above and everywhere else in such cases leave a trailing comma.
> +};
...
> + for (i = 0; i < info->num_scales; i++) {
> + __ad485x_get_scale(st, info->scale_table[i][0],
> + &scale_val[0], &scale_val[1]);
> + if (scale_val[0] != val || scale_val[1] != val2)
> + continue;
> +
> + /*
> + * Adjust the softspan value (differential or single ended)
> + * based on the scale value selected and current offset of
> + * the channel.
> + *
> + * If the offset is 0 then continue iterations until finding
> + * the next matching scale value which always corresponds to
> + * the single ended mode.
> + */
> + if (!st->offsets[chan->channel] && !j) {
> + j++;
So, j may be named properly and be boolean for the sake of the semantic usage.
> + continue;
> + }
> +
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + info->scale_table[i][1]);
> + }
...
> +{
> + guard(mutex)(&st->lock);
> +
> + if (st->offsets[chan->channel] != val) {
Invert and drop indentation for the next lines.
> + st->offsets[chan->channel] = val;
> + /* Restore to the default range if offset changes */
> + if (st->offsets[chan->channel])
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + AD485X_SOFTSPAN_N40V_40V);
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + AD485X_SOFTSPAN_0V_40V);
> + }
> +
> + return 0;
> +}
...
> +static int ad485x_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long info)
> +{
> + struct ad485x_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = st->sampling_freq;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + return ad485x_get_calibscale(st, chan->channel, val, val2);
> + case IIO_CHAN_INFO_SCALE:
> + return ad485x_get_scale(st, chan, val, val2);
> + case IIO_CHAN_INFO_CALIBBIAS:
> + return ad485x_get_calibbias(st, chan->channel, val, val2);
> + case IIO_CHAN_INFO_OFFSET:
> + scoped_guard(mutex, &st->lock)
> + * val = st->offsets[chan->channel];
This is interesting white space location...
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
...
> + struct ad485x_state *st = iio_priv(indio_dev);
> + unsigned int c;
> + int ret;
> +
> + for (c = 0; c < st->info->num_channels; c++) {
> + if (test_bit(c, scan_mask))
Do we have a helper now for this iterator?
> + ret = iio_backend_chan_enable(st->back, c);
> + else
> + ret = iio_backend_chan_disable(st->back, c);
> + if (ret)
> + return ret;
> + }
...
> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + {},
Please, drop comma in the terminator lines.
> +};
...
> +static const struct ad485x_chip_info ad4858i_info = {
> + .name = "ad4858i",
> + .product_id = AD4858I_PROD_ID,
> + .scale_table = ad485x_scale_table,
> + .num_scales = ARRAY_SIZE(ad485x_scale_table),
> + .offset_table = ad4858_offset_table,
> + .num_offset = ARRAY_SIZE(ad4858_offset_table),
> + .channels = ad4858_channels,
> + .num_channels = ARRAY_SIZE(ad4858_channels),
> + .throughput = 1000000,
How many people wrote this patch? It has inconsistency at least in multipliers
like this all over the code. Who knows what's else also being inconsistent...
> + .resolution = 20,
> +};
...
> +static const struct regmap_config regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 8,
> + .read_flag_mask = BIT(7),
> +};
Why do you need regmap lock?
...
> +static const char * const ad485x_power_supplies[] = {
> + "vcc", "vdd", "vddh", "vio"
Leave trailing comma.
> +};
...
> +static int ad485x_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ad485x_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> + mutex_init(&st->lock);
Why not devm?
> + ret = devm_regulator_bulk_get_enable(&spi->dev,
> + ARRAY_SIZE(ad485x_power_supplies),
> + ad485x_power_supplies);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "failed to get and enable supplies\n");
> +
> + st->cnv = devm_pwm_get(&spi->dev, NULL);
> + if (IS_ERR(st->cnv))
> + return PTR_ERR(st->cnv);
> +
> + st->info = spi_get_device_match_data(spi);
> + if (!st->info)
> + return -ENODEV;
> +
> + st->regmap = devm_regmap_init_spi(spi, ®map_config);
> + if (IS_ERR(st->regmap))
> + return PTR_ERR(st->regmap);
> +
> + ret = ad485x_scale_offset_fill(st);
> + if (ret)
> + return ret;
> +
> + ret = ad485x_setup(st);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = st->info->name;
> + indio_dev->channels = st->info->channels;
> + indio_dev->num_channels = st->info->num_channels;
> + indio_dev->info = &ad485x_info;
> +
> + st->back = devm_iio_backend_get(&spi->dev, NULL);
> + if (IS_ERR(st->back))
> + return PTR_ERR(st->back);
> +
> + ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_backend_enable(&spi->dev, st->back);
> + if (ret)
> + return ret;
> +
> + ret = ad485x_calibrate(st);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
--
With Best Regards,
Andy Shevchenko
© 2016 - 2024 Red Hat, Inc.