On 1/22/25 7:20 AM, Alisa-Dariana Roman wrote:
> AD7191 is a pin-programmable, ultralow noise 24-bit sigma-delta ADC
ultra-low
> designed for precision bridge sensor measurements. It features two
> differential analog input channels, selectable output rates,
> programmable gain, internal temperature sensor and simultaneous
> 50Hz/60Hz rejection.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@xxxxxxxxxx>
> ---
...
> diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
> new file mode 100644
> index 000000000000..dd8151ad3f3f
> --- /dev/null
> +++ b/drivers/iio/adc/ad7191.c
> @@ -0,0 +1,570 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD7191 ADC driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/adc/ad_sigma_delta.h>
prefer alphabetical order
> +
> +#define ad_sigma_delta_to_ad7191(sigmad) container_of((sigmad), struct ad7191_state, sd)
> +
> +#define AD7191_TEMP_CODES_PER_DEGREE 2815
> +
> +#define AD7191_EXT_CLK_ENABLE 0
> +#define AD7191_INT_CLK_ENABLE 1
> +
> +#define AD7191_CHAN_MASK BIT(0)
> +#define AD7191_TEMP_MASK BIT(1)
> +
> +#define AD7191_MAX_ODR_STATE 3
> +#define AD7191_MAX_PGA_STATE 3
> +
> +enum ad7191_channel {
> + AD7191_CH_AIN1_AIN2 = 0,
0 isn't needed here.
> + AD7191_CH_AIN3_AIN4,
> + AD7191_CH_TEMP
> +};
> +
> +/*
> + * NOTE:
> + * The AD7191 features a dual-use data out ready DOUT/RDY output.
> + * In order to avoid contentions on the SPI bus, it's therefore necessary
> + * to use SPI bus locking.
> + *
> + * The DOUT/RDY output must also be wired to an interrupt-capable GPIO.
Probably worth mentioning that the SPI controller CS gets wired to PDOWN pin
on the ADC here since that isn't very obvious.
> + */
> +
> +struct ad7191_state {
> + struct ad_sigma_delta sd;
> + struct mutex lock; // to protect sensor state
> +
> + struct gpio_descs *odr_gpios;
> + struct gpio_descs *pga_gpios;
> + struct gpio_desc *temp_gpio;
> + struct gpio_desc *chan_gpio;
> + struct gpio_desc *clksel_gpio;
> +
> + u16 int_vref_mv;
> + u32 pga_state;
> + u32 scale_avail[4][2];
> + u32 odr_state;
> + u32 samp_freq_avail[4];
> +
> + struct clk *mclk;
> + u32 clksel_state;
> +};
> +
> +static int ad7191_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
Would be less confusing to me if the channel parameter was changed to "address"
since the actual value is the channel spec .address field.
> +{
> + struct ad7191_state *st = ad_sigma_delta_to_ad7191(sd);
> + u8 temp_gpio_val, chan_gpio_val;
> +
> + if (!FIELD_FIT(AD7191_CHAN_MASK | AD7191_TEMP_MASK, channel))
> + return -EINVAL;
> +
> + chan_gpio_val = FIELD_GET(AD7191_CHAN_MASK, channel);
> + temp_gpio_val = FIELD_GET(AD7191_TEMP_MASK, channel);
> +
> + gpiod_set_value(st->chan_gpio, chan_gpio_val);
> + gpiod_set_value(st->temp_gpio, temp_gpio_val);
> +
> + return 0;
> +}
> +
> +static int set_cs(struct ad_sigma_delta *sigma_delta, int pull_down)
Make it ad7191_set_cs() to be consistent. And "assert" is probably a more common
name instead of pull_down.
> +{
> + struct spi_transfer t = {
> + .len = 0,
> + .cs_change = pull_down,
> + };
> + struct spi_message m;
> +
> + spi_message_init(&m);
> + spi_message_add_tail(&t, &m);
Can make this one line with spi_message_init_with_transfers().
> +
> + return spi_sync_locked(sigma_delta->spi, &m);
> +}
> +
> +static int ad7191_set_mode(struct ad_sigma_delta *sd,
> + enum ad_sigma_delta_mode mode)
> +{
> + struct ad7191_state *st = ad_sigma_delta_to_ad7191(sd);
> +
> + switch (mode) {
> + case AD_SD_MODE_CONTINUOUS:
> + case AD_SD_MODE_SINGLE:
> + return set_cs(&st->sd, 1);
> + case AD_SD_MODE_IDLE:
> + return set_cs(&st->sd, 0);
> + default:
> + return 0;
Should default return an error?
> + }
> +}
> +
> +static const struct ad_sigma_delta_info ad7191_sigma_delta_info = {
> + .set_channel = ad7191_set_channel,
> + .set_mode = ad7191_set_mode,
> + .has_registers = false,
> +};
> +
> +static int ad7191_init_regulators(struct iio_dev *indio_dev)
> +{
> + struct ad7191_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->sd.spi->dev;
> + int ret;
> +
> + ret = devm_regulator_get_enable(dev, "avdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable specified AVdd supply\n");
> +
> + ret = devm_regulator_get_enable(dev, "dvdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable specified DVdd supply\n");
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "vref");
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to get Vref voltage\n");
> +
> + st->int_vref_mv = ret / 1000;
> +
> + return 0;
> +}
> +
> +static int ad7191_gpio_setup(struct iio_dev *indio_dev)
> +{
> + struct ad7191_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->sd.spi->dev;
> +
> + if (device_property_read_u32(dev, "adi,odr-state", &st->odr_state) == 0) {
Usually we check for a specific error. Otherwise, if someone does something like
using a string instead of an int in the .dts file, we will get the default value
rather than an error.
> + if (st->odr_state > AD7191_MAX_ODR_STATE)
> + return dev_err_probe(dev, -EINVAL, "Invalid ODR state.\n");
> +
> + dev_info(dev, "ODR is pin-strapped to %d\n", st->odr_state);
dev_dbg(). or remove it. we can get the info by reading the samping_frequency
attribute if needed.
> + st->odr_gpios = NULL;
> + } else {
> + st->odr_gpios = devm_gpiod_get_array(dev, "odr", GPIOD_OUT_LOW);
> + if (IS_ERR(st->odr_gpios))
> + return dev_err_probe(dev, PTR_ERR(st->odr_gpios),
> + "Failed to get odr gpios.\n");
> + }
> +
> + if (device_property_read_u32(dev, "adi,pga-state", &st->pga_state) == 0) {
> + if (st->odr_state > AD7191_MAX_PGA_STATE)
Looks like copy/paste mistake. Should be checking pga_state here.
> + return dev_err_probe(dev, -EINVAL, "Invalid PGA state.\n");
> +
> + dev_info(dev, "PGA is pin-strapped to %d\n", st->pga_state);
> + st->pga_gpios = NULL;
> + } else {
> + st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
> + if (IS_ERR(st->pga_gpios))
> + return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
> + "Failed to get pga gpios.\n");
> + }
> +
> + if (device_property_read_u32(dev, "adi,clksel-state", &st->clksel_state) == 0) {
> + dev_info(dev, "CLKSEL is pin-strapped to %d\n", st->clksel_state);
> + st->clksel_gpio = NULL;
> + } else {
> + st->clksel_gpio = devm_gpiod_get(dev, "clksel", GPIOD_OUT_HIGH);
> + if (IS_ERR(st->clksel_gpio))
> + return dev_err_probe(dev, PTR_ERR(st->clksel_gpio),
> + "Failed to get clksel gpio.\n");
> + }
> +
> + st->temp_gpio = devm_gpiod_get(dev, "temp", GPIOD_OUT_LOW);
> + if (IS_ERR(st->temp_gpio))
> + return dev_err_probe(dev, PTR_ERR(st->temp_gpio),
> + "Failed to get temp gpio.\n");
> +
> + st->chan_gpio = devm_gpiod_get(dev, "chan", GPIOD_OUT_LOW);
> + if (IS_ERR(st->chan_gpio))
> + return dev_err_probe(dev, PTR_ERR(st->chan_gpio),
> + "Failed to get chan gpio.\n");
> +
> + return 0;
> +}
> +
> +static int ad7191_clock_setup(struct ad7191_state *st)
> +{
As mentioned in the dt-bindings patch review, this could be simpified a bit if
we remove some of the reduant/unnecessary properties.
> + struct device *dev = &st->sd.spi->dev;
> + u8 clksel_value;
> +
> + st->mclk = devm_clk_get_enabled(dev, "mclk");
> + if (IS_ERR(st->mclk)) {
> + if (PTR_ERR(st->mclk) != -ENOENT)
> + return dev_err_probe(dev, PTR_ERR(st->mclk),
> + "Failed to get mclk.\n");
> +
> + /*
> + * No external clock found, default to internal clock.
> + */
> + clksel_value = AD7191_INT_CLK_ENABLE;
> + if (!st->clksel_gpio && st->clksel_state != AD7191_INT_CLK_ENABLE)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid CLKSEL state. To use the internal clock, CLKSEL must be high.\n");
> +
> + dev_info(dev, "Using internal clock.\n");
> + } else {
> + clksel_value = AD7191_EXT_CLK_ENABLE;
> + if (!st->clksel_gpio && st->clksel_state != AD7191_EXT_CLK_ENABLE)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid CLKSEL state. To use the external clock, CLKSEL must be low.\n");
> +
> + dev_info(dev, "Using external clock.\n");
> + }
> +
> + if (st->clksel_gpio)
> + gpiod_set_value(st->clksel_gpio, clksel_value);
> +
> + return 0;
> +}
> +
> +static int ad7191_setup(struct iio_dev *indio_dev, struct device *dev)
> +{
> + struct ad7191_state *st = iio_priv(indio_dev);
> + u64 scale_uv;
> + const int gain[4] = {1, 8, 64, 128};
> + int i, ret;
> +
> + ret = ad7191_init_regulators(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = ad7191_gpio_setup(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = ad7191_clock_setup(st);
> + if (ret)
> + return ret;
> +
> + /*
> + * Sampling frequencies in Hz, available in the documentation, Table 5.
> + */
> + st->samp_freq_avail[0] = 120;
> + st->samp_freq_avail[1] = 60;
> + st->samp_freq_avail[2] = 50;
> + st->samp_freq_avail[3] = 10;
Looks like this one could just be static const data. Or if ODR pins are hard-
wired, maybe this should only have 1 value.
> +
> + for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
> + scale_uv = ((u64)st->int_vref_mv * NANO) >>
> + (indio_dev->channels[0].scan_type.realbits - 1);
> + do_div(scale_uv, gain[i]);
> + st->scale_avail[i][1] = do_div(scale_uv, NANO);
> + st->scale_avail[i][0] = scale_uv;
Same here, if gain pins are hard-wired, then the other options aren't really
available.
> + }
> +
> + return 0;
> +}
> +
> +static int ad7191_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long m)
> +{
> + struct ad7191_state *st = iio_priv(indio_dev);
> +
> + switch (m) {
> + case IIO_CHAN_INFO_RAW:
> + return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + guard(mutex)(&st->lock);
> + *val = st->scale_avail[st->pga_state][0];
> + *val2 = st->scale_avail[st->pga_state][1];
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_TEMP:
> + *val = 0;
> + *val2 = NANO / AD7191_TEMP_CODES_PER_DEGREE;
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_OFFSET:
> + *val = -(1 << (chan->scan_type.realbits - 1));
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + return IIO_VAL_INT;
> + case IIO_TEMP:
> + *val -= 273 * AD7191_TEMP_CODES_PER_DEGREE;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = st->samp_freq_avail[st->odr_state];
> + return IIO_VAL_INT;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad7191_set_gain(struct ad7191_state *st, int gain_index)
> +{
> + unsigned long value = gain_index;
> +
> + if (!st->pga_gpios)
> + return -EPERM;
> +
> + st->pga_state = gain_index;
> +
> + return gpiod_set_array_value(2, st->pga_gpios->desc,
Replace hard-coded 2 with st->pga_gpios->ndescs.
Also, gpiod_set_array_value_cansleep() should be OK.
> + st->pga_gpios->info, &value);
> +
> + return 0;
> +}
> +
> +static int ad7191_set_samp_freq(struct ad7191_state *st, int samp_freq_index)
> +{
> + unsigned long value = samp_freq_index;
> +
> + if (!st->odr_gpios)
> + return -EPERM;
> +
> + st->odr_state = samp_freq_index;
> +
> + return gpiod_set_array_value(2, st->odr_gpios->desc,
> + st->odr_gpios->info, &value);
ditto
> +}
> +
> +static int __ad7191_write_raw(struct ad7191_state *st,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + int i;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + guard(mutex)(&st->lock);
> + for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
> + if (val2 != st->scale_avail[i][1])
> + continue;
> + return ad7191_set_gain(st, i);
> + }
> + return -EINVAL;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (!val)
> + return -EINVAL;
This check seems reduandant since we would get the same result below without it.
> +
> + guard(mutex)(&st->lock);
> + for (i = 0; i < ARRAY_SIZE(st->samp_freq_avail); i++) {
> + if (val != st->samp_freq_avail[i])
> + continue;
> + return ad7191_set_samp_freq(st, i);
> + }
> + return -EINVAL;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
...
> +static const struct iio_chan_spec ad7191_channels[] = {
> + {
> + .type = IIO_TEMP,
> + .address = AD7191_CH_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_OFFSET),
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 24,
> + .storagebits = 24,
IIO buffers are "natrually" aligned, so storagebits for anything > 16, <= 32 is
going to be a 32-bit integer.
> + .endianness = IIO_BE,
> + },
> + },
> + {
> + .type = IIO_VOLTAGE,
> + .differential = 1,
> + .indexed = 1,
> + .channel = 1,
> + .channel2 = 2,
> + .address = AD7191_CH_AIN1_AIN2,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_OFFSET),
A bit odd to have offset separate and scale by type, but I guess it isn't
wrong.
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
> + .scan_index = 1,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 24,
> + .storagebits = 24,
> + .endianness = IIO_BE,
> + },
> + },
> + {
> + .type = IIO_VOLTAGE,
> + .differential = 1,
> + .indexed = 1,
> + .channel = 3,
> + .channel2 = 4,
> + .address = AD7191_CH_AIN3_AIN4,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_OFFSET),
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
> + .scan_index = 2,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 24,
> + .storagebits = 24,
> + .endianness = IIO_BE,
> + },
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
> +static int ad7191_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct ad7191_state *st;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + if (!spi->irq) {
> + dev_err(dev, "no IRQ?\n");
> + return -ENODEV;
return dev_err_probe(...);
Or should we just let ad_sd_init() handle it?
> + }
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> +
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = "ad7191";
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = ad7191_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ad7191_channels);
> + indio_dev->info = &ad7191_info;
> +
> + ad_sd_init(&st->sd, indio_dev, spi, &ad7191_sigma_delta_info);
Need to check return value.
> +
> + ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = ad7191_setup(indio_dev, dev);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct of_device_id ad7191_of_match[] = {
> + {
> + .compatible = "adi,ad7191",
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad7191_of_match);
> +
> +static const struct spi_device_id ad7191_id_table[] = {
> + { "ad7191", 0 },
Could leave out the 0 here.
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, ad7191_id_table);
> +
> +static struct spi_driver ad7191_driver = {
> + .driver = {
> + .name = "ad7191",
> + .of_match_table = ad7191_of_match,
> + },
> + .probe = ad7191_probe,
Still missing .id_table here.
> +};
> +module_spi_driver(ad7191_driver);
> +
> +MODULE_AUTHOR("Alisa-Dariana Roman <alisa.roman@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Analog Devices AD7191 ADC");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
