| Message ID | cover.1711131830.git.marcelo.schmitt@analog.com |
|---|---|
| Headers | show |
| Series | Add support for AD4000 series | expand |
On Fri, 22 Mar 2024 19:05:34 -0300 Marcelo Schmitt <marcelo.schmitt@analog.com> 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@analog.com> > --- > 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. Yes. That's what I suggested inline before reading this properly. Much prefer that as standard tooling will know how to use it. Various comments inline. In particularly look at the spi helpers. It's unusual to see a driver do so much manual handling of transfers. Jonathan > > .../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@vger.kernel.org > +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. When would we turn this on or off from userspace? From this brief description it sounds like either we are trying to run very fast and the SPI bus can't clock quick enough for the non turbo mode. In which case detect that we need it and then turn it on. Right now I have no idea why I'd ever turn this off. Who doesn't want to press the turbo button? > + > +What: /sys/bus/iio/devices/iio:deviceX/span_compression_en > +KernelVersion: 6.9 > +Contact: linux-iio@vger.kernel.org > +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. Can you not control it via in_voltageY_scale then? > + > +What: /sys/bus/iio/devices/iio:deviceX/status_bits_en > +KernelVersion: 6.9 > +Contact: linux-iio@vger.kernel.org > +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. Why would I want this? Which of these isn't something I control? This sounds like a debug feature we shouldn't enable. > + > +What: /sys/bus/iio/devices/iio:deviceX/high_impedance_en > +KernelVersion: 6.9 > +Contact: linux-iio@vger.kernel.org > +Description: > + This attribute is used to enable/disable high impedance mode > + (high-z) which reduces nonlinear charge kickback to the ADC > + input. For this one, do we have precendence in other drivers? I'm find with a control just not sure if this the write ABI to use. > 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> Why? Probably want mod_devicetable.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> You aren't providing a trigger so shouldn't need this one I think. > +#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, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_type = { \ > + .sign = _sign, \ > + .realbits = _real_bits, \ > + .storagebits = 32, \ Why store a 16 bit value in 32 bits? Obviously will make code more complex to handling it differently but the memory saving could be significant if the buffer is large. > + .endianness = IIO_BE, \ > + }, \ > + } \ > + > + > +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); Why can't use init_with_transfers? Though maybe spi_write() is enough here and in other cases. + look at spi_write_then_read() and see if that's appropriate for others. > + > + 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; As below on structure initialization. > + > + 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) > +{ > + 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; Similar to below, use c99 style structure initialization. > + > + 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; Scoped version works here too to avoid the need for delaying the error check. (see below) > + > + 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 ssize_t ad4000_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct ad4000_state *st = iio_priv(indio_dev); > + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); > + unsigned int reg_val; > + bool val; > + int ret; > + > + ret = kstrtobool(buf, &val); > + if (ret < 0) > + return ret; > + > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + > + ret = ad4000_read_reg(st, ®_val); > + if (ret < 0) > + goto err_release; > + > + switch ((u32)this_attr->address) { > + case AD4000_STATUS: > + reg_val &= ~AD4000_STATUS; > + reg_val |= FIELD_PREP(AD4000_STATUS, val); > + ret = ad4000_write_reg(st, reg_val); > + if (ret < 0) > + goto err_release; > + > + st->status_bits = val; > + break; > + case AD4000_SPAN_COMP: > + reg_val &= ~AD4000_SPAN_COMP; > + reg_val |= FIELD_PREP(AD4000_SPAN_COMP, val); > + ret = ad4000_write_reg(st, reg_val); > + if (ret < 0) > + goto err_release; > + > + st->span_comp = val; > + break; > + case AD4000_HIGHZ: > + reg_val &= ~AD4000_HIGHZ; > + reg_val |= FIELD_PREP(AD4000_HIGHZ, val); > + ret = ad4000_write_reg(st, reg_val); > + if (ret < 0) > + goto err_release; > + > + st->high_z_mode = val; > + break; > + case AD4000_TURBO: > + reg_val &= ~AD4000_TURBO; > + reg_val |= FIELD_PREP(AD4000_TURBO, val); > + ret = ad4000_write_reg(st, reg_val); > + if (ret < 0) > + goto err_release; > + > + st->turbo_mode = val; > + break; > + default: > + ret = -EINVAL; > + goto err_release; > + } > + > +err_release: > + iio_device_release_direct_mode(indio_dev); The scoped cleanup.h based version of this is no upstream and would clean# this code up a lot by allowing early returns See iio_device_claim_direct_scoped() > + return ret ? ret : len; > +} > +static irqreturn_t ad4000_trigger_handler(int irq, void *p) > +{ > + 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}; Spaces around the 0, though technically I think you just need {}; > + 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; Better still. struct spi_transfer t = { .rx_buf = &... etc }; > + > + 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); Shouldn't need the cast. It's casting const void * to another const pointer which the c spec allows to be done implicitly > + if (!chip) > + return -EINVAL; > + > + st = iio_priv(indio_dev); > + st->spi = spi; > + > + vref_reg = devm_regulator_get(&spi->dev, "vref"); I'm guessing there are other power supplies? You should enable them as well and given you don't use the voltage of the others you can just use the calls to get them enabled. > + 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"); > + } > + } > + > + 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; For defaults, a nice pattern is to just set them before the attempt to read the property and don't update them if the property isn't available. st->pin_gain = AD4000_1_GAIN; if (device_property_present(&spi->dev, "adi,gain-milli")) { ... } > + } > +static const struct spi_device_id ad4000_id[] = { > + { "ad4000", (kernel_ulong_t)&ad4000_chips[ID_AD4000] }, > + { "ad4001", (kernel_ulong_t)&ad4000_chips[ID_AD4001] }, > + { "ad4002", (kernel_ulong_t)&ad4000_chips[ID_AD4002] }, > + { "ad4003", (kernel_ulong_t)&ad4000_chips[ID_AD4003] }, > + { "ad4004", (kernel_ulong_t)&ad4000_chips[ID_AD4004] }, > + { "ad4005", (kernel_ulong_t)&ad4000_chips[ID_AD4005] }, > + { "ad4006", (kernel_ulong_t)&ad4000_chips[ID_AD4006] }, > + { "ad4007", (kernel_ulong_t)&ad4000_chips[ID_AD4007] }, > + { "ad4008", (kernel_ulong_t)&ad4000_chips[ID_AD4008] }, > + { "ad4010", (kernel_ulong_t)&ad4000_chips[ID_AD4010] }, > + { "ad4011", (kernel_ulong_t)&ad4000_chips[ID_AD4011] }, > + { "ad4020", (kernel_ulong_t)&ad4000_chips[ID_AD4020] }, > + { "ad4021", (kernel_ulong_t)&ad4000_chips[ID_AD4021] }, > + { "ad4022", (kernel_ulong_t)&ad4000_chips[ID_AD4022] }, > + { "adaq4001", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4001] }, > + { "adaq4003", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4003] }, > + { } > +}; > +MODULE_DEVICE_TABLE(spi, ad4000_id); > + > +static const struct of_device_id ad4000_of_match[] = { > + { .compatible = "adi,ad4000", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4000] }, Why the casts? Shouldn't need them as data is a const void * and you are casting from another const pointer. > + { .compatible = "adi,ad4001", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4001] }, > + { .compatible = "adi,ad4002", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4002] }, > + { .compatible = "adi,ad4003", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4003] }, > + { .compatible = "adi,ad4004", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4004] }, > + { .compatible = "adi,ad4005", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4005] }, > + { .compatible = "adi,ad4006", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4006] }, > + { .compatible = "adi,ad4007", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4007] }, > + { .compatible = "adi,ad4008", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4008] }, > + { .compatible = "adi,ad4010", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4010] }, > + { .compatible = "adi,ad4011", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4011] }, > + { .compatible = "adi,ad4020", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4020] }, > + { .compatible = "adi,ad4021", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4021] }, > + { .compatible = "adi,ad4022", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_AD4022] }, > + { .compatible = "adi,adaq4001", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_ADAQ4001] }, > + { .compatible = "adi,adaq4003", > + .data = (struct ad4000_chip_info *)&ad4000_chips[ID_ADAQ4003] }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ad4000_of_match);
On Fri, Mar 22, 2024 at 5:06 PM Marcelo Schmitt <marcelo.schmitt@analog.com> 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@analog.com> > --- > 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@vger.kernel.org > +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@vger.kernel.org > +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@vger.kernel.org > +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@vger.kernel.org > +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@analog.com> > L: linux-iio@vger.kernel.org > 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@analog.com> > 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); > +} > + ...
On Sat, 23 Mar 2024 16:53:17 -0500 > > + > > + /* > > + * 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?) > Good spot. Yes, x86_32 is the one that we most commonly refer to for this. It aligns s64 to only 32 bits whereas IIO ABI is always naturally aligned. > > + } scan; > > + u8 d8[2]; > > + } data __aligned(IIO_DMA_MINALIGN); > > +};
On Sat, Mar 23, 2024 at 4:53 PM David Lechner <dlechner@baylibre.com> wrote: > > On Fri, Mar 22, 2024 at 5:06 PM Marcelo Schmitt > <marcelo.schmitt@analog.com> wrote: > > ... > > + > > + 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"); > > + } > > + } > In a review for a different patch, Jonathan said he would prefer devm_regulator_get() and failing in regulator_get_voltage() rather than using devm_regulator_get_optional() so I think the same would apply here and my suggestion should be overruled.