Re: [PATCH v2 2/2] iio: adc: Add support for TI ADS1120
From: Ajith Anandhan
Date: Mon Dec 15 2025 - 11:49:51 EST
On 12/15/25 10:06 PM, David Lechner wrote:
On 12/15/25 10:13 AM, Ajith Anandhan wrote:I understand your concern about unused macros. This was suggested by Jonathan earlier.
On 11/18/25 7:34 PM, David Lechner wrote:I would argue that if they aren't being used then omitting them would
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)We could avoid these macros by just doing ilog2(gain).
+#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
I understand your concern about unused macros. I've kept them for documentation purposes as they map directly to the datasheet register definitions, which makes it easier to verify correctness against hardware specs also I'd prefer to keep it like this since it give more readability Shall i keep this as it is for this initial version?
save us the time of having to verify the correctness in the first place.
I’m fine with removing the unused macros if that is preferred. Please let me know how you’d like me to proceed.
OK. Just mention this in the commit message.+static int ads1120_set_gain(struct ads1120_state *st, int gain_val)Since there is only one gain register, we should store this in a
+{
+ 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;
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()).
I've implemented a global gain that applies to all channels for simplicity. I planed to add
per-channel gain storage in the later patches.
I think you are looking for reg_shift in struct regmap_config.+/* Regmap write function for ADS1120 */I don't see anyting unusal about these read/write functions. We should
+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);
+}
be able to use the existing spi_regmap with the proper configuration
instead of making a custom regmap_bus.
The ADS1120 needs register address shifted by 2 bits
in command byte (reg << 2). I couldn't find a way to do this with standard
SPI regmap. If there's a configuration I'm missing, please point me to it and I'll gladly simplify.
Thanks for the pointer.
I did look at reg_shift, but it doesn’t fit this device. With .reg_shift = 2, regmap would send only (reg << 2) (e.g. 0x0c for reg 3).
The ADS1120 requires the command byte to be CMD | (reg << 2) (e.g. 0x20 | 0x0c = 0x2c for an RREG of reg 3).
Similarly,
.read_flag_mask would produce reg | mask (e.g. 0x03 | 0x20 = 0x23), which is also not the required format.
Unless I’m missing a regmap configuration that can generate (reg << 2) | CMD as a single byte,
a custom regmap bus seems necessary here. Please let me know if there is a way to express this with standard regmap-spi.