| 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: <20251115184503.57a071b1@jic23-huawei>
Date: Sat, 15 Nov 2025 18:45:03 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Ajith Anandhan <ajithanandhan0406@...il.com>
Cc: dlechner@...libre.com, 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 Sun, 9 Nov 2025 19:41:19 +0530
Ajith Anandhan <ajithanandhan0406@...il.com> wrote:
> Add driver for the Texas Instruments ADS1120, a precision 16-bit
> analog-to-digital converter with an SPI interface.
>
> The driver supports:
> - Differential and single-ended input channels
> - Configurable gain (1-128 for differential, 1-4 for single-ended)
> - Internal 2.048V reference
> - Single-shot conversion mode
>
> Also update MAINTAINER document.
>
> Signed-off-by: Ajith Anandhan <ajithanandhan0406@...il.com>
Hi Ajith
A few comments from me to add to Andy's review.
Jonathan
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d008f78dc..49c56b459 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -144,6 +144,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
> obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
> +obj-$(CONFIG_TI_ADS1120) += ti-ads1120.o
> obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
> obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
> obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
> diff --git a/drivers/iio/adc/ti-ads1120.c b/drivers/iio/adc/ti-ads1120.c
> new file mode 100644
> index 000000000..1e1871b74
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1120.c
> +
> +/* 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), \
I note that scale is the same for all channels. So why have separate
attributes?
> + .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, \
We very rarely allow extend)name in new drivers.
It is a real pain for userspace code to deal with, so we now
put that info behind the get_label() callback.
It's also fairly rare that we put diagnostic channels out. Might
be better to push those to debugfs and keep the main interface
less confusing.
Shorted is sometimes done as a differential channel with itself
(doesn't matter which one). It's made a little more complex here
as it is specified as shorting both to (AVDD + AVDSS) / 2 .
> + .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"),
> +};
> +
> +/* 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;
> +
> + /* 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));
return spi_sync_transfer()
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static const struct regmap_config ads1120_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = ADS1120_REG_CONFIG3,
> + .cache_type = REGCACHE_FLAT,
Andy covered this already but unless you have strong reason for
something else just use REGCACHE_MAPLE. If you do have a reason
add a comment here to stop it being changed by someone else.
> +};
> +
> +static int ads1120_probe(struct spi_device *spi)
> +{
> + indio_dev->name = "ads1120";
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = ads1120_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ads1120_channels);
> + indio_dev->info = &ads1120_info;
> +
> + ret = ads1120_init(st);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to initialize device\n");
Align as.
return dev_err_probe(dev, ret,
"Failed to initialize device\n");
Same for all other similar cases.
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
Powered by blists - more mailing lists