| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5f15284b-159b-4860-b58b-35c624e2539f@baylibre.com>
Date: Tue, 18 Nov 2025 08:04:58 -0600
From: David Lechner <dlechner@...libre.com>
To: Ajith Anandhan <ajithanandhan0406@...il.com>, jic23@...nel.org
Cc: nuno.sa@...log.com, andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: adc: Add support for TI ADS1120
On 11/9/25 8:11 AM, Ajith Anandhan wrote:
> Add driver for the Texas Instruments ADS1120, a precision 16-bit
> analog-to-digital converter with an SPI interface.
>
...
> +#define ADS1120_CFG0_GAIN_MASK GENMASK(3, 1)
> +#define ADS1120_CFG0_GAIN_1 0
> +#define ADS1120_CFG0_GAIN_2 1
> +#define ADS1120_CFG0_GAIN_4 2
> +#define ADS1120_CFG0_GAIN_8 3
> +#define ADS1120_CFG0_GAIN_16 4
> +#define ADS1120_CFG0_GAIN_32 5
> +#define ADS1120_CFG0_GAIN_64 6
> +#define ADS1120_CFG0_GAIN_128 7
We could avoid these macros by just doing ilog2(gain).
> +
> +#define ADS1120_CFG0_PGA_BYPASS BIT(0)
> +
> +/* Config Register 1 bit definitions */
> +#define ADS1120_CFG1_DR_MASK GENMASK(7, 5)
> +#define ADS1120_CFG1_DR_20SPS 0
> +#define ADS1120_CFG1_DR_45SPS 1
> +#define ADS1120_CFG1_DR_90SPS 2
> +#define ADS1120_CFG1_DR_175SPS 3
> +#define ADS1120_CFG1_DR_330SPS 4
> +#define ADS1120_CFG1_DR_600SPS 5
> +#define ADS1120_CFG1_DR_1000SPS 6
I would avoid writing macros for values we don't use yet. For example,
it may make more sense to have a lookup table when it comes to actually
implementing something that uses this.
Same applies to the rest below.
> +
> +#define ADS1120_CFG1_MODE_MASK GENMASK(4, 3)
> +#define ADS1120_CFG1_MODE_NORMAL 0
> +#define ADS1120_CFG1_MODE_DUTY 1
> +#define ADS1120_CFG1_MODE_TURBO 2
> +
> +#define ADS1120_CFG1_CM_MASK BIT(2)
> +#define ADS1120_CFG1_CM_SINGLE 0
> +#define ADS1120_CFG1_CM_CONTINUOUS 1
> +
> +#define ADS1120_CFG1_TS_EN BIT(1)
> +#define ADS1120_CFG1_BCS_EN BIT(0)
> +
> +/* Config Register 2 bit definitions */
> +#define ADS1120_CFG2_VREF_MASK GENMASK(7, 6)
> +#define ADS1120_CFG2_VREF_INTERNAL 0
> +#define ADS1120_CFG2_VREF_EXT_REFP0 1
> +#define ADS1120_CFG2_VREF_EXT_AIN0 2
> +#define ADS1120_CFG2_VREF_AVDD 3
> +
> +#define ADS1120_CFG2_REJECT_MASK GENMASK(5, 4)
> +#define ADS1120_CFG2_REJECT_OFF 0
> +#define ADS1120_CFG2_REJECT_50_60 1
> +#define ADS1120_CFG2_REJECT_50 2
> +#define ADS1120_CFG2_REJECT_60 3
> +
> +#define ADS1120_CFG2_PSW_EN BIT(3)
> +
> +#define ADS1120_CFG2_IDAC_MASK GENMASK(2, 0)
> +#define ADS1120_CFG2_IDAC_OFF 0
> +#define ADS1120_CFG2_IDAC_10UA 1
> +#define ADS1120_CFG2_IDAC_50UA 2
> +#define ADS1120_CFG2_IDAC_100UA 3
> +#define ADS1120_CFG2_IDAC_250UA 4
> +#define ADS1120_CFG2_IDAC_500UA 5
> +#define ADS1120_CFG2_IDAC_1000UA 6
> +#define ADS1120_CFG2_IDAC_1500UA 7
> +
> +/* Config Register 3 bit definitions */
> +#define ADS1120_CFG3_IDAC1_MASK GENMASK(7, 5)
> +#define ADS1120_CFG3_IDAC1_DISABLED 0
> +#define ADS1120_CFG3_IDAC1_AIN0 1
> +#define ADS1120_CFG3_IDAC1_AIN1 2
> +#define ADS1120_CFG3_IDAC1_AIN2 3
> +#define ADS1120_CFG3_IDAC1_AIN3 4
> +#define ADS1120_CFG3_IDAC1_REFP0 5
> +#define ADS1120_CFG3_IDAC1_REFN0 6
> +
> +#define ADS1120_CFG3_IDAC2_MASK GENMASK(4, 2)
> +#define ADS1120_CFG3_IDAC2_DISABLED 0
> +#define ADS1120_CFG3_IDAC2_AIN0 1
> +#define ADS1120_CFG3_IDAC2_AIN1 2
> +#define ADS1120_CFG3_IDAC2_AIN2 3
> +#define ADS1120_CFG3_IDAC2_AIN3 4
> +#define ADS1120_CFG3_IDAC2_REFP0 5
> +#define ADS1120_CFG3_IDAC2_REFN0 6
> +
> +#define ADS1120_CFG3_DRDYM_MASK BIT(1)
> +#define ADS1120_CFG3_DRDYM_DRDY_ONLY 0
> +#define ADS1120_CFG3_DRDYM_BOTH 1
> +
> +/* Conversion time for 20 SPS */
> +#define ADS1120_CONV_TIME_MS 51
> +
> +/* Internal reference voltage in millivolts */
> +#define ADS1120_VREF_INTERNAL_MV 2048
> +
> +
> +struct ads1120_state {
> + struct spi_device *spi;
> + struct regmap *regmap;
> + /*
> + * Protects chip configuration and ADC reads to ensure
> + * consistent channel/gain settings during conversions.
> + */
> + struct mutex lock;
> +
> + int vref_mv;
> +
> + /* DMA-safe buffer for SPI transfers */
> + u8 data[4] __aligned(IIO_DMA_MINALIGN);
> +};
> +
> +
> +static const int ads1120_gain_values[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
> +static const int ads1120_low_gain_values[] = { 1, 2, 4 };
> +
> +/* Differential channel macro */
> +#define ADS1120_DIFF_CHANNEL(index, chan1, chan2) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = chan1, \
> + .channel2 = chan2, \
> + .differential = 1, \
> + .address = index, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +/* Single-ended channel macro */
> +#define ADS1120_SINGLE_CHANNEL(index, chan) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = chan, \
> + .address = index, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +/* Diagnostic channel macro */
> +#define ADS1120_DIAG_CHANNEL(index, label) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = index, \
> + .address = index, \
> + .extend_name = label, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +static const struct iio_chan_spec ads1120_channels[] = {
> + /* Differential inputs */
> + ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN0_AIN1, 0, 1),
> + ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN0_AIN2, 0, 2),
> + ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN0_AIN3, 0, 3),
> + ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN1_AIN2, 1, 2),
> + ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN1_AIN3, 1, 3),
> + ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN2_AIN3, 2, 3),
> + ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN1_AIN0, 1, 0),
> + ADS1120_DIFF_CHANNEL(ADS1120_CFG0_MUX_AIN3_AIN2, 3, 2),
> + /* Single-ended inputs */
> + ADS1120_SINGLE_CHANNEL(ADS1120_CFG0_MUX_AIN0_AVSS, 0),
> + ADS1120_SINGLE_CHANNEL(ADS1120_CFG0_MUX_AIN1_AVSS, 1),
> + ADS1120_SINGLE_CHANNEL(ADS1120_CFG0_MUX_AIN2_AVSS, 2),
> + ADS1120_SINGLE_CHANNEL(ADS1120_CFG0_MUX_AIN3_AVSS, 3),
> + /* Diagnostic inputs */
> + ADS1120_DIAG_CHANNEL(ADS1120_CFG0_MUX_REFP_REFN_4, "ref_div4"),
> + ADS1120_DIAG_CHANNEL(ADS1120_CFG0_MUX_AVDD_AVSS_4, "avdd_div4"),
> + ADS1120_DIAG_CHANNEL(ADS1120_CFG0_MUX_SHORTED, "shorted"),
> +};
Usually we don't make macros for the index. When it comes to adding
buffer support, having the macros makes it harder to see which scan_index
belongs to which channel. And if buffer support is planned for later,
it would make sense to use .scan_index instead of .address to avoid
duplicating that info later.
> +static int ads1120_set_gain(struct ads1120_state *st, int gain_val)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ads1120_gain_values); i++) {
> + if (ads1120_gain_values[i] == gain_val)
> + break;
> + }
> +
> + if (i == ARRAY_SIZE(ads1120_gain_values))
> + return -EINVAL;
Since there is only one gain register, we should store this in a
per-channel variable, then set the gain in the register in
ads1120_set_mux() instead (and it would make sense to rename
that function as well to something like ads1120_configure_channel()).
> +
> + return regmap_update_bits(st->regmap, ADS1120_REG_CONFIG0,
> + ADS1120_CFG0_GAIN_MASK,
> + FIELD_PREP(ADS1120_CFG0_GAIN_MASK, i));
> +}
> +
...
> +static int ads1120_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ads1120_state *st = iio_priv(indio_dev);
> + int ret, gain;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + guard(mutex)(&st->lock);
> + ret = ads1120_read_measurement(st, chan, val);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + /*
> + * Scale = Vref / (gain * 2^15)
> + * Return in format: val / 2^val2
> + */
> + gain = ads1120_get_gain(st);
> + if (gain < 0)
> + return gain;
> +
> + *val = st->vref_mv;
> + *val2 = gain * 15;
I think this would need to be `*val2 = ilog2(gain) + 15`.
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +
> +/* Regmap read function for ADS1120 */
> +static int ads1120_regmap_read(void *context, const void *reg_buf,
> + size_t reg_size, void *val_buf, size_t val_size)
> +{
> + struct ads1120_state *st = context;
> + u8 reg = *(u8 *)reg_buf;
> + u8 *val = val_buf;
> + int ret;
> + struct spi_transfer xfer[2] = {
> + {
> + .tx_buf = st->data,
> + .len = 1,
> + }, {
> + .rx_buf = val,
> + .len = val_size,
> + }
> + };
> +
> + if (reg > ADS1120_REG_CONFIG3)
> + return -EINVAL;
This check should not be needed because of .maxregister.
> +
> + /* RREG command: 0010rr00 where rr is register address */
> + st->data[0] = ADS1120_CMD_RREG | (reg << 2);
> +
> + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +/* Regmap write function for ADS1120 */
> +static int ads1120_regmap_write(void *context, const void *data, size_t count)
> +{
> + struct ads1120_state *st = context;
> + const u8 *buf = data;
> +
> + if (count != 2)
> + return -EINVAL;
> +
> + /* WREG command: 0100rr00 where rr is register address */
> + st->data[0] = ADS1120_CMD_WREG | (buf[0] << 2);
> + st->data[1] = buf[1];
> +
> + return spi_write(st->spi, st->data, 2);
> +}
I don't see anyting unusal about these read/write functions. We should
be able to use the existing spi_regmap with the proper configuration
instead of making a custom regmap_bus.
> +
> +static const struct regmap_bus ads1120_regmap_bus = {
> + .read = ads1120_regmap_read,
> + .write = ads1120_regmap_write,
> + .reg_format_endian_default = REGMAP_ENDIAN_BIG,
> + .val_format_endian_default = REGMAP_ENDIAN_BIG,
> +};
> +
> +static const struct regmap_config ads1120_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = ADS1120_REG_CONFIG3,
> + .cache_type = REGCACHE_FLAT,
> +};
> +
> +static int ads1120_init(struct ads1120_state *st)
> +{
> + int ret;
It's just a few extra lines to turn on the avdd supply here before
resettting.
> +
> + ret = ads1120_reset(st);
> + if (ret)
> + return dev_err_probe(&st->spi->dev, ret,
> + "Failed to reset device\n");
> +
> + /*
> + * Configure Register 0:
> + * - Input MUX: AIN0/AVSS
> + * - Gain: 1
> + * - PGA bypass enabled. When gain is set > 4, this bit is
> + * automatically ignored by the hardware and PGA is enabled,
> + * so it's safe to leave it set.
> + */
> + ret = regmap_write(st->regmap, ADS1120_REG_CONFIG0,
> + FIELD_PREP(ADS1120_CFG0_MUX_MASK,
> + ADS1120_CFG0_MUX_AIN0_AVSS) |
> + FIELD_PREP(ADS1120_CFG0_GAIN_MASK,
> + ADS1120_CFG0_GAIN_1) |
> + ADS1120_CFG0_PGA_BYPASS);
> + if (ret)
> + return ret;
> +
> + /*
> + * Configure Register 1:
> + * - Data rate: 20 SPS (for single-shot mode)
> + * - Operating mode: Normal
> + * - Conversion mode: Single-shot
> + * - Temperature sensor: Disabled
> + * - Burnout current: Disabled
> + */
> + ret = regmap_write(st->regmap, ADS1120_REG_CONFIG1,
> + FIELD_PREP(ADS1120_CFG1_DR_MASK,
> + ADS1120_CFG1_DR_20SPS) |
> + FIELD_PREP(ADS1120_CFG1_MODE_MASK,
> + ADS1120_CFG1_MODE_NORMAL) |
> + FIELD_PREP(ADS1120_CFG1_CM_MASK,
> + ADS1120_CFG1_CM_SINGLE) |
> + FIELD_PREP(ADS1120_CFG1_TS_EN, 0) |
> + FIELD_PREP(ADS1120_CFG1_BCS_EN, 0));
> + if (ret)
> + return ret;
> +
> + /*
> + * Configure Register 2:
> + * - Voltage reference: Internal 2.048V
> + * - 50/60Hz rejection: Off
> + * - Power switch: Disabled
> + * - IDAC current: Off
> + */
> + ret = regmap_write(st->regmap, ADS1120_REG_CONFIG2,
> + FIELD_PREP(ADS1120_CFG2_VREF_MASK,
> + ADS1120_CFG2_VREF_INTERNAL) |
> + FIELD_PREP(ADS1120_CFG2_REJECT_MASK,
> + ADS1120_CFG2_REJECT_OFF) |
> + FIELD_PREP(ADS1120_CFG2_PSW_EN, 0) |
> + FIELD_PREP(ADS1120_CFG2_IDAC_MASK,
> + ADS1120_CFG2_IDAC_OFF));
> + if (ret)
> + return ret;
> +
> + /*
> + * Configure Register 3:
> + * - IDAC1: Disabled
> + * - IDAC2: Disabled
> + * - DRDY mode: Only reflects data ready status
> + */
> + ret = regmap_write(st->regmap, ADS1120_REG_CONFIG3,
> + FIELD_PREP(ADS1120_CFG3_IDAC1_MASK,
> + ADS1120_CFG3_IDAC1_DISABLED) |
> + FIELD_PREP(ADS1120_CFG3_IDAC2_MASK,
> + ADS1120_CFG3_IDAC2_DISABLED) |
> + FIELD_PREP(ADS1120_CFG3_DRDYM_MASK,
> + ADS1120_CFG3_DRDYM_DRDY_ONLY));
> + if (ret)
> + return ret;
> +
If we want to wait for a later patch to support external reference,
I would consider to check for the devicetree properties here and
print a warning that they aren't supported if present. Maybe others
think that is too much noise though? Especially if you plan to add
it soon.
> + st->vref_mv = ADS1120_VREF_INTERNAL_MV;
> +
> + return 0;
> +}
> +
Powered by blists - more mailing lists