On Fri, Mar 22, 2024 at 5:06 PM Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> wrote: > > Add support for AD4000 series of high accuracy, high speed, low power, > successive aproximation register (SAR) ADCs. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> > --- > Pasting relevant comment from cover letter here to aid reviewers. > > Differently from AD7944, AD4000 devices have a configuration register to > toggle some features. For instance, turbo mode is set through configuration > register rather than an external pin. This simplifies hardware connections, > but then requires software interface. So, additional ABI being proposed > in sysfs-bus-iio-adc-ad4000. The one I'm most in doubt about is > span_compression_en which affects the in_voltageY_scale attribute. > That might be instead supported by providing _scale_available and allowing write > to _scale. > > .../ABI/testing/sysfs-bus-iio-adc-ad4000 | 36 + > MAINTAINERS | 2 + > drivers/iio/adc/Kconfig | 12 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ad4000.c | 666 ++++++++++++++++++ > 5 files changed, 717 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad4000 > create mode 100644 drivers/iio/adc/ad4000.c > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4000 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4000 > new file mode 100644 > index 000000000000..98fb7539ad6d > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4000 > @@ -0,0 +1,36 @@ > +What: /sys/bus/iio/devices/iio:deviceX/turbo_en > +KernelVersion: 6.9 > +Contact: linux-iio@xxxxxxxxxxxxxxx > +Description: > + This attribute is used to enable/disable turbo mode allowing > + slower SPI clock rates (at a minimum SCK rate of 75 MHz) to > + achieve the maximum throughput of 2 MSPS. > + > +What: /sys/bus/iio/devices/iio:deviceX/span_compression_en > +KernelVersion: 6.9 > +Contact: linux-iio@xxxxxxxxxxxxxxx > +Description: > + This attribute is used to enable/disable the input span > + compression feature that reduces the ADC input range by 10% from > + the top and bottom of the range while still accessing all > + available ADC codes. Enabling span compression causes a > + decrease in ADC scale which is reflected in the channel > + in_voltageY_scale attribute. > + > +What: /sys/bus/iio/devices/iio:deviceX/status_bits_en > +KernelVersion: 6.9 > +Contact: linux-iio@xxxxxxxxxxxxxxx > +Description: > + This attribute is used to make the chip append a 6-bit status > + word at the end of conversion results. The 6 status bits contain > + the configuration register fields for OV clamp flag, span > + compression, high-z mode, and turbo mode. > + I agree with Jonathan that the three attributes above are not needed (for the reasons he mentioned). > +What: /sys/bus/iio/devices/iio:deviceX/high_impedance_en > +KernelVersion: 6.9 > +Contact: linux-iio@xxxxxxxxxxxxxxx > +Description: > + This attribute is used to enable/disable high impedance mode > + (high-z) which reduces nonlinear charge kickback to the ADC > + input. > + As I mentioned in the DT patch, this one seems like it should be a DT property, not a runtime setting. > diff --git a/MAINTAINERS b/MAINTAINERS > index 3ca90f842298..2ae98c700ff0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1140,7 +1140,9 @@ M: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> > L: linux-iio@xxxxxxxxxxxxxxx > S: Supported > W: https://ez.analog.com/linux-software-drivers > +F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad4000 > F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml > +F: drivers/iio/adc/ad4000.c > > ANALOG DEVICES INC AD4130 DRIVER > M: Cosmin Tanislav <cosmin.tanislav@xxxxxxxxxx> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 0d9282fa67f5..15db35f00ef0 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -21,6 +21,18 @@ config AD_SIGMA_DELTA > select IIO_BUFFER > select IIO_TRIGGERED_BUFFER > > +config AD4000 > + tristate "Analog Devices AD4000 ADC Driver" > + depends on SPI > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + help > + Say yes here to build support for Analog Devices AD4000 high speed > + SPI analog to digital converters (ADC). > + > + To compile this driver as a module, choose M here: the module will be > + called ad4000. > + > config AD4130 > tristate "Analog Device AD4130 ADC Driver" > depends on SPI > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index b3c434722364..f535d617ae89 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -6,6 +6,7 @@ > # When adding new entries keep the list in alphabetical order > obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o > obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o > +obj-$(CONFIG_AD4000) += ad4000.o > obj-$(CONFIG_AD4130) += ad4130.o > obj-$(CONFIG_AD7091R) += ad7091r-base.o > obj-$(CONFIG_AD7091R5) += ad7091r5.o > diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c > new file mode 100644 > index 000000000000..f77104755979 > --- /dev/null > +++ b/drivers/iio/adc/ad4000.c > @@ -0,0 +1,666 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * AD4000 SPI ADC driver > + * > + * Copyright 2024 Analog Devices Inc. > + */ > +#include <asm/unaligned.h> > +#include <linux/bits.h> > +#include <linux/bitfield.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/kernel.h> > +#include <linux/math.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/gpio/consumer.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > +#include <linux/sysfs.h> > +#include <linux/units.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/trigger.h> > +#include <linux/iio/triggered_buffer.h> > +#include <linux/iio/trigger_consumer.h> > + > +#define AD400X_READ_COMMAND 0x54 > +#define AD400X_WRITE_COMMAND 0x14 > + > +#define AD4000_CONFIG_REG_MSK 0xFF > + > +/* AD4000 Configuration Register programmable bits */ > +#define AD4000_STATUS BIT(4) /* Status bits output */ > +#define AD4000_SPAN_COMP BIT(3) /* Input span compression */ > +#define AD4000_HIGHZ BIT(2) /* High impedance mode */ > +#define AD4000_TURBO BIT(1) /* Turbo mode */ > + > +#define AD4000_16BIT_MSK GENMASK(31, 16) > +#define AD4000_18BIT_MSK GENMASK(31, 14) > +#define AD4000_20BIT_MSK GENMASK(31, 12) > + > +#define AD4000_CHANNEL(_sign, _real_bits) \ > + { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ Some chips are fully differential and some are pseudo-differential. For the fully differential chips, I would expect .differential = 1, As discussed on other recent drivers, psudo-differential are .differential = 0 though. > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_type = { \ > + .sign = _sign, \ > + .realbits = _real_bits, \ > + .storagebits = 32, \ > + .endianness = IIO_BE, \ I think this should be IIO_CPU and we should make use of .bits_per_word in the SPI xfers like we do in the ad7944 driver. Otherwise, I think we might have difficutly to achieve max sample rate for the 18 and 20-bit chips once we get SPI offload support added. > + }, \ > + } \ > + > +enum ad4000_ids { > + ID_AD4000, > + ID_AD4001, > + ID_AD4002, > + ID_AD4003, > + ID_AD4004, > + ID_AD4005, > + ID_AD4006, > + ID_AD4007, > + ID_AD4008, > + ID_AD4010, > + ID_AD4011, > + ID_AD4020, > + ID_AD4021, > + ID_AD4022, > + ID_ADAQ4001, > + ID_ADAQ4003, > +}; IMHO, these types of enums just make more work (noise) for people reading the code and don't add anything useful. > + > +struct ad4000_chip_info { > + const char *dev_name; > + struct iio_chan_spec chan_spec; > +}; > + > +static const struct ad4000_chip_info ad4000_chips[] = { > + [ID_AD4000] = { > + .dev_name = "ad4000", > + .chan_spec = AD4000_CHANNEL('u', 16), > + }, > + [ID_AD4001] = { > + .dev_name = "ad4001", > + .chan_spec = AD4000_CHANNEL('s', 16), > + }, > + [ID_AD4002] = { > + .dev_name = "ad4002", > + .chan_spec = AD4000_CHANNEL('u', 18), > + }, > + [ID_AD4003] = { > + .dev_name = "ad4003", > + .chan_spec = AD4000_CHANNEL('s', 18), > + }, > + [ID_AD4004] = { > + .dev_name = "ad4004", > + .chan_spec = AD4000_CHANNEL('u', 16), > + }, > + [ID_AD4005] = { > + .dev_name = "ad4005", > + .chan_spec = AD4000_CHANNEL('s', 16), > + }, > + [ID_AD4006] = { > + .dev_name = "ad4006", > + .chan_spec = AD4000_CHANNEL('u', 18), > + }, > + [ID_AD4007] = { > + .dev_name = "ad4007", > + .chan_spec = AD4000_CHANNEL('s', 18), > + }, > + [ID_AD4008] = { > + .dev_name = "ad4008", > + .chan_spec = AD4000_CHANNEL('u', 16), > + }, > + [ID_AD4010] = { > + .dev_name = "ad4010", > + .chan_spec = AD4000_CHANNEL('u', 18), > + }, > + [ID_AD4011] = { > + .dev_name = "ad4011", > + .chan_spec = AD4000_CHANNEL('s', 18), > + }, > + [ID_AD4020] = { > + .dev_name = "ad4020", > + .chan_spec = AD4000_CHANNEL('s', 20), > + }, > + [ID_AD4021] = { > + .dev_name = "ad4021", > + .chan_spec = AD4000_CHANNEL('s', 20), > + }, > + [ID_AD4022] = { > + .dev_name = "ad4022", > + .chan_spec = AD4000_CHANNEL('s', 20), > + }, > + [ID_ADAQ4001] = { > + .dev_name = "adaq4001", > + .chan_spec = AD4000_CHANNEL('s', 16), > + }, > + [ID_ADAQ4003] = { > + .dev_name = "adaq4003", > + .chan_spec = AD4000_CHANNEL('s', 18), > + }, > +}; As mentioned in my DT bindings reply, I think it would be helpful for reviewers (and git history) to move adding ADAQ support to a separate patch since they have some significant differences from the AD parts. > + > +enum ad4000_gains { > + AD4000_0454_GAIN = 0, > + AD4000_0909_GAIN = 1, > + AD4000_1_GAIN = 2, > + AD4000_1900_GAIN = 3, > + AD4000_GAIN_LEN > +}; > + > +/* > + * Gains stored and computed as fractions to avoid introducing rounding erros. spelling: errors > + */ > +static const int ad4000_gains_frac[AD4000_GAIN_LEN][2] = { > + [AD4000_0454_GAIN] = { 227, 500 }, > + [AD4000_0909_GAIN] = { 909, 1000 }, > + [AD4000_1_GAIN] = { 1, 1 }, > + [AD4000_1900_GAIN] = { 19, 10 }, > +}; > + > +struct ad4000_state { > + struct spi_device *spi; > + struct gpio_desc *cnv_gpio; > + int vref; > + bool status_bits; > + bool span_comp; > + bool turbo_mode; > + bool high_z_mode; > + > + enum ad4000_gains pin_gain; > + int scale_tbl[AD4000_GAIN_LEN][2]; > + > + /* > + * DMA (thus cache coherency maintenance) requires the > + * transfer buffers to live in their own cache lines. > + */ > + union { > + struct { > + u8 sample_buf[4]; > + s64 timestamp; Usually we see __aligned(8) applied to the timestamp (I'm guessing some archs need it?) > + } scan; > + u8 d8[2]; > + } data __aligned(IIO_DMA_MINALIGN); > +}; > + > +static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits) > +{ > + int val, val2, tmp0, tmp1, i; > + u64 tmp2; > + > + val2 = scale_bits; > + for (i = 0; i < AD4000_GAIN_LEN; i++) { > + val = st->vref / 1000; > + /* Multiply by MILLI here to avoid losing precision */ > + val = mult_frac(val, ad4000_gains_frac[i][1] * MILLI, > + ad4000_gains_frac[i][0]); > + /* Would multiply by NANO here but we already multiplied by MILLI */ > + tmp2 = shift_right((u64)val * MICRO, val2); > + tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1); > + st->scale_tbl[i][0] = tmp0; /* Integer part */ > + st->scale_tbl[i][1] = abs(tmp1); /* Fractional part */ > + } > +} > + > +static int ad4000_write_reg(struct ad4000_state *st, uint8_t val) > +{ > + struct spi_transfer t = { > + .tx_buf = st->data.d8, > + .len = 2, > + }; > + struct spi_message m; > + > + put_unaligned_be16(AD400X_WRITE_COMMAND << BITS_PER_BYTE | val, st->data.d8); > + > + spi_message_init(&m); > + spi_message_add_tail(&t, &m); > + > + return spi_sync(st->spi, &m); > +} > + > +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val) > +{ > + struct spi_transfer t = {0}; > + struct spi_message m; > + int ret; > + > + st->data.d8[0] = AD400X_READ_COMMAND; > + > + t.rx_buf = st->data.d8; > + t.tx_buf = st->data.d8; > + t.len = 2; > + > + spi_message_init_with_transfers(&m, &t, 1); > + > + ret = spi_sync(st->spi, &m); > + if (ret < 0) > + return ret; > + > + *val = FIELD_GET(AD4000_CONFIG_REG_MSK, get_unaligned_be16(st->data.d8)); > + > + return ret; > +} > + > +static int ad4000_read_sample(struct ad4000_state *st, uint32_t *val) As with the ad7944 driver, I would expect different handling for the different SPI wiring modes. This looks like it only works with 4-wire mode. > +{ > + struct spi_transfer t = {0}; > + struct spi_message m; > + int ret; > + > + t.rx_buf = &st->data.scan.sample_buf; > + t.len = 4; > + t.delay.value = 60; > + t.delay.unit = SPI_DELAY_UNIT_NSECS; > + > + spi_message_init_with_transfers(&m, &t, 1); > + > + if (st->cnv_gpio) > + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_HIGH); > + > + ret = spi_sync(st->spi, &m); > + if (ret < 0) > + return ret; > + > + if (st->cnv_gpio) > + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_LOW); > + > + *val = get_unaligned_be32(&st->data.scan.sample_buf); > + > + return 0; > +} > + > +static int ad4000_single_conversion(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, int *val) > +{ > + struct ad4000_state *st = iio_priv(indio_dev); > + u32 sample; > + int ret; > + > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + > + ret = ad4000_read_sample(st, &sample); > + > + iio_device_release_direct_mode(indio_dev); > + > + if (ret) > + return ret; > + > + switch (chan->scan_type.realbits) { > + case 16: > + sample = FIELD_GET(AD4000_16BIT_MSK, sample); > + break; > + case 18: > + sample = FIELD_GET(AD4000_18BIT_MSK, sample); > + break; > + case 20: > + sample = FIELD_GET(AD4000_20BIT_MSK, sample); > + break; > + default: > + return -EINVAL; > + } > + > + if (chan->scan_type.sign == 's') > + *val = sign_extend32(sample, chan->scan_type.realbits - 1); > + > + return IIO_VAL_INT; > +} > + > +static int ad4000_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long info) > +{ > + struct ad4000_state *st = iio_priv(indio_dev); > + > + switch (info) { > + case IIO_CHAN_INFO_RAW: > + return ad4000_single_conversion(indio_dev, chan, val); > + case IIO_CHAN_INFO_SCALE: > + *val = st->scale_tbl[st->pin_gain][0]; > + *val2 = st->scale_tbl[st->pin_gain][1]; > + if (st->span_comp) > + *val2 = DIV_ROUND_CLOSEST(*val2 * 4, 5); > + return IIO_VAL_INT_PLUS_NANO; > + default: > + break; > + } > + > + return -EINVAL; > +} > + ... > +static irqreturn_t ad4000_trigger_handler(int irq, void *p) I would expect different handling for the different SPI wiring modes here too. > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct ad4000_state *st = iio_priv(indio_dev); > + struct spi_transfer t = {0}; > + struct spi_message m; > + int ret; > + > + t.rx_buf = &st->data.scan.sample_buf; > + t.len = 4; > + t.delay.value = 60; > + t.delay.unit = SPI_DELAY_UNIT_NSECS; > + > + spi_message_init_with_transfers(&m, &t, 1); > + > + if (st->cnv_gpio) > + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_HIGH); > + > + ret = spi_sync(st->spi, &m); > + if (ret < 0) > + goto err_out; > + > + if (st->cnv_gpio) > + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_LOW); > + > + iio_push_to_buffers_with_timestamp(indio_dev, &st->data.scan, > + iio_get_time_ns(indio_dev)); > + > +err_out: > + iio_trigger_notify_done(indio_dev->trig); > + return IRQ_HANDLED; > +} > + > +static const struct iio_info ad4000_info = { > + .read_raw = &ad4000_read_raw, > + .attrs = &ad4000_attribute_group, > +}; > + > +static void ad4000_regulator_disable(void *reg) > +{ > + regulator_disable(reg); > +} > + > +static int ad4000_probe(struct spi_device *spi) > +{ > + const struct ad4000_chip_info *chip; > + struct regulator *vref_reg; > + struct iio_dev *indio_dev; > + struct ad4000_state *st; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > + if (!indio_dev) > + return -ENOMEM; > + > + chip = (const struct ad4000_chip_info *)device_get_match_data(&spi->dev); Should this be spi_get_device_match_data()? Also don't need the cast here. > + if (!chip) > + return -EINVAL; > + > + st = iio_priv(indio_dev); > + st->spi = spi; > + > + vref_reg = devm_regulator_get(&spi->dev, "vref"); This should to be devm_regulator_get_optional(), otherwise it can return a "dummy" regulator if one is missing in the devicetree which will fail when getting the voltage. > + if (IS_ERR(vref_reg)) > + return dev_err_probe(&spi->dev, PTR_ERR(vref_reg), > + "Failed to get vref regulator\n"); > + > + ret = regulator_enable(vref_reg); > + if (ret < 0) > + return dev_err_probe(&spi->dev, ret, > + "Failed to enable voltage regulator\n"); > + > + ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable, vref_reg); > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "Failed to add regulator disable action\n"); > + > + st->vref = regulator_get_voltage(vref_reg); > + if (st->vref < 0) > + return dev_err_probe(&spi->dev, st->vref, "Failed to get vref\n"); > + > + if (!device_property_present(&spi->dev, "adi,spi-cs-mode")) { > + st->cnv_gpio = devm_gpiod_get(&spi->dev, "cnv", GPIOD_OUT_HIGH); > + if (IS_ERR(st->cnv_gpio)) { > + if (PTR_ERR(st->cnv_gpio) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio), > + "Failed to get CNV GPIO"); > + } > + } Since the DT bindings should be the same for the SPI wirting modes, we should have the same property and logic as the ad7944 driver here. > + > + indio_dev->name = chip->dev_name; > + indio_dev->info = &ad4000_info; > + indio_dev->channels = &chip->chan_spec; > + indio_dev->num_channels = 1; > + > + if (device_property_present(&spi->dev, "adi,gain-milli")) { > + u32 val; > + > + ret = device_property_read_u32(&spi->dev, "adi,gain-milli", &val); > + if (ret) > + return ret; > + > + switch (val) { > + case 454: > + st->pin_gain = AD4000_0454_GAIN; > + break; > + case 909: > + st->pin_gain = AD4000_0909_GAIN; > + break; > + case 1000: > + st->pin_gain = AD4000_1_GAIN; > + break; > + case 1900: > + st->pin_gain = AD4000_1900_GAIN; > + break; > + default: > + return dev_err_probe(&spi->dev, -EINVAL, > + "Invalid firmware provided gain\n"); > + } > + } else { > + st->pin_gain = AD4000_1_GAIN; > + } > + > + /* > + * ADCs that output twos complement code have one less bit to express > + * voltage magnitude. > + */ > + if (chip->chan_spec.scan_type.sign == 's') > + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits - 1); > + else > + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits); > + AFAICT, the gain stuff only applies to ADAQ chips, so it seems odd to call this for all chips (and have the scale attribute report this for chips that don't support it). > + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, > + &iio_pollfunc_store_time, > + &ad4000_trigger_handler, NULL); > + if (ret) > + return ret; > + > + return devm_iio_device_register(&spi->dev, indio_dev); > +} > + ...
