Re: [PATCH v2 2/7] iio: adc: Add ti-ads1262 driver

["Kurt Borja" <kuurtb@gmail.com>]

On Sun Jun 28, 2026 at 12:15 PM -05, David Lechner wrote:
> On 6/28/26 12:36 AM, Kurt Borja wrote:
>> Add the ti-ads1262 driver with initial support for the primary ADC
>> (ADC1). The ADS1263 auxiliary ADC (ADC2) is handled by a separate driver
>> and interoperability considerations were taken into account.
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  MAINTAINERS                  |    1 +
>>  drivers/iio/adc/Kconfig      |   13 +
>>  drivers/iio/adc/Makefile     |    1 +
>>  drivers/iio/adc/ti-ads1262.c | 1206 ++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 1221 insertions(+)
>
> Ideally, an intial patch would be 1/2 this size. Over 1000 lines in
> a single patch make it take more than twice as long to review as
> if it was split into two patches (or you just don't get a detailed
> review). For example, sample rate and gain can be split out into
> seprate patches.
>
> I certainly don't have time to revew this plus an additonal 1000
> lines of more patches on top of it in a week.

Sure, I'll split it even more.

[...]

>> +static int ads1262_calculate_scale(struct ads1262 *st, u8 realbits, u8 gain,
>> +				   u8 pos_ref, u8 neg_ref, int *val, int *val2)
>> +{
>> +	u64 divd, divr, tmp, rem;
>> +	int pos_uV, neg_uV;
>> +
>> +	switch (pos_ref) {
>> +	case ADS1262_REFMUX_INTERNAL:
>> +		/* Internal voltage reference is 2.5 V */
>> +		pos_uV = 2500000;
>> +		break;
>> +	case ADS1262_REFMUX_AIN0_AIN1...ADS1262_REFMUX_AIN4_AIN5:
>> +		pos_uV = st->refp_uV;
>
> I don't think it is safe to assume the same reference is wired to all of these.

Yes, you're right. I'll add supplies for the other options.

[...]

>> +static int ads1262_channel_enable(struct ads1262 *st,
>> +				  struct ads1262_channel *chan)
>> +{
>> +	u8 mode0, mode2, inpmux, refmux;
>> +	int ret;
>> +
>> +	/* Avoid using guard() here to mitigate AB/BA deadlock warning */
>> +	mutex_lock(&st->chan_lock);
>> +	mode0 = FIELD_PREP(ADS1262_MODE0_INPUT_CHOP_MASK, chan->input_chop) |
>> +		FIELD_PREP(ADS1262_MODE0_IDAC_CHOP_MASK, chan->idac_chop) |
>> +		FIELD_PREP(ADS1262_MODE0_REFREV_MASK, chan->ref_reversal);
>> +	mode2 = FIELD_PREP(ADS1262_MODE2_DR_MASK, chan->data_rate) |
>> +		FIELD_PREP(ADS1262_MODE2_GAIN_MASK, chan->gain) |
>> +		FIELD_PREP(ADS1262_MODE2_BYPASS_MASK, chan->pga_bypass);
>> +	inpmux = FIELD_PREP(ADS1262_INPMUX_MUXN_MASK, chan->input[1]) |
>> +		 FIELD_PREP(ADS1262_INPMUX_MUXP_MASK, chan->input[0]);
>> +	refmux = FIELD_PREP(ADS1262_REFMUX_RMUXN_MASK, chan->reference[1]) |
>> +		 FIELD_PREP(ADS1262_REFMUX_RMUXP_MASK, chan->reference[0]);
>> +	mutex_unlock(&st->chan_lock);
>
> Why does lock not also include actually writing the bits?

Because we take the xfer_lock in that path. Later in the series, I'm
forced to take the xfer_lock first, so that would be AB/BA bug. But...

See the regmap answer below.

[...]

>> +static int ads1262_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan, int *val,
>> +			    int *val2, long mask)
>> +{
>> +	struct ads1262 *st = iio_priv(indio_dev);
>> +	struct ads1262_channel *chan_data = &st->channels[chan->scan_index];
>> +	u8 realbits = chan->scan_type.realbits;
>> +	__be32 raw;
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = ads1262_channel_read(st, chan_data, &raw);
>> +		if (ret)
>> +			return ret;
>> +		*val = sign_extend32(be32_to_cpu(raw), realbits - 1);
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE: {
>> +		guard(mutex)(&st->chan_lock);
>> +
>> +		ret = ads1262_channel_get_scale(st, chan, val, val2);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT_PLUS_NANO;
>> +	}
>> +
>> +	case IIO_CHAN_INFO_HARDWAREGAIN: {
>
> There is only one other ADC that uses "hardwaregain". Usually, we just make
> scale writeable to control the gain. I don't remember what the rules for
> that attribute are. Using it for in_voltage is not documented in the ABI.

I went with hardwaregain because the scale loses too many significant
digits at high gain. With the internal reference and gain = 1, the scale
is at 0.000001164; then at gain = 32, the scale is at 0.000000036.

In this case I expect users to just calculate the scale themselves based
on the hardwaregain. Is this acceptable? If not I'll go with
scale_available.

[...]

>> +static int ads1262_alloc_channels(struct ads1262 *st,
>> +				  struct iio_chan_spec **channels)
>> +{
>> +	struct device *dev = &st->spi->dev;
>> +	struct ads1262_channel *chan_data;
>> +	struct iio_chan_spec *chans;
>> +	unsigned int i, num_channels;
>> +
>> +	/* Account for the timestamp channel */
>> +	num_channels = st->num_channels + 1;
>> +	chans = devm_kcalloc(dev, num_channels, sizeof(*chans), GFP_KERNEL);
>> +	if (!chans)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < st->num_channels; i++) {
>> +		chan_data = &st->channels[i];
>> +		chans[i] = (struct iio_chan_spec) {
>> +			.type = IIO_VOLTAGE,
>> +			.channel = chan_data->input[0],
>> +			.channel2 = chan_data->input[1],
>> +			.scan_index = i,
>> +			.scan_type = {
>> +				.format = IIO_SCAN_FORMAT_SIGNED_INT,
>> +				.realbits = 32,
>> +				.storagebits = 32,
>> +				.endianness = IIO_BE,
>> +			},
>> +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +					      BIT(IIO_CHAN_INFO_SCALE) |
>> +					      BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
>> +					      BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +			.info_mask_shared_by_type_available =
>> +				BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
>
> Gain available might make more sense as separate if there are restrictions
> on gain allowed due to other conditions. 
>
>> +				BIT(IIO_CHAN_INFO_SAMP_FREQ),
>
> Could be the same case with sampling frequency if filters can affect that.

Actually, sampling frequency takes precedence over the filter
configuration. Some rates cause the chip to ignore the filter
configuration.

>
>> +			.indexed = true,
>> +			.differential = true,
>
> These two make more sense closer to the top since they affect the
> channel naming along with type and channel.
>
>> +		};
>> +	}
>
> As mentioned in the DT bindings review, I would make the diagnostic
> channels fixed so that they don't have to be always specified in
> the devicetree.

I agree. I'll add the monitors here.

[...]

>> +static int ads1262_regmap_read(void *context, const void *reg_buf,
>> +			       size_t reg_size, void *val_buf, size_t val_size)
>> +{
>> +	struct ads1262 *st = context;
>> +	struct spi_transfer xfer = {
>> +		.tx_buf = st->tx,
>> +		.rx_buf = st->rx,
>> +		.len = reg_size + 1 + val_size,
>> +	};
>> +	int ret;
>> +
>> +	guard(mutex)(&st->xfer_lock);
>
> SPI bus and regmap both already have their own locking, so putting a lock
> here seems out of place. Instead, the lock should be for higher-level
> operations where there are mulitple register access in a single operation.

I agree. I can definitely move this one to a "higher level". But IMO,
because this also protects tx and rx buffers, it makes sense to have it
here too.

>
>> +
>> +	memset(st->tx, 0, reg_size + 1 + val_size);
>> +
>> +	memcpy(&st->tx[0], reg_buf, 1);
>> +	st->tx[1] = val_size - 1;
>> +
>> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	memcpy(val_buf, &st->rx[2], val_size);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1262_regmap_gather_write(void *context, const void *reg_buf,
>> +				       size_t reg_size, const void *val_buf,
>> +				       size_t val_size)
>> +{
>> +	struct ads1262 *st = context;
>> +	struct spi_transfer xfer = {
>> +		.tx_buf = st->tx,
>> +		.rx_buf = st->rx,
>> +		.len = reg_size + 1 + val_size,
>> +	};
>> +
>> +	guard(mutex)(&st->xfer_lock);
>> +
>> +	memset(st->tx, 0, reg_size + 1 + val_size);
>> +
>> +	memcpy(&st->tx[0], reg_buf, 1);
>> +	st->tx[1] = val_size - 1;
>> +	memcpy(&st->tx[2], val_buf, val_size);
>> +
>> +	return spi_sync_transfer(st->spi, &xfer, 1);
>> +}
>> +
>> +static int ads1262_regmap_write(void *context, const void *data, size_t count)
>> +{
>> +	return ads1262_regmap_gather_write(context, data, 1, data + 1,
>> +					   count - 1);
>> +}
>> +
>> +static const struct regmap_bus ads1262_regmap_bus = {
>> +	.read = ads1262_regmap_read,
>> +	.gather_write = ads1262_regmap_gather_write,
>> +	.write = ads1262_regmap_write,
>> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
>> +	.val_format_endian_default = REGMAP_ENDIAN_BIG,
>> +	/* The first two bytes of the buffer are reserved for the protocol */
>> +	.max_raw_read = ADS1262_XFER_BUFFER_SZ - 2,
>> +	.max_raw_write = ADS1262_XFER_BUFFER_SZ - 2,
>> +};
>
> Why do we need our own bus instead of using REGMAP_SPI?

Because the protocol is different. Read/write protocol for this chip is

	first reg + command | number of regs - 1 | reg values

while regmap_spi is

	first reg | reg values

I had a comment explaining this, but I forgot to restore it.

[...]

>> +static int ads1262_parse_firmware(struct ads1262 *st)
>> +{
>> +	struct device *dev = &st->spi->dev;
>> +	struct clk *clk;
>> +	u32 reg;
>> +	int ret;
>> +
>> +	/* Set the nominal clock frequency */
>> +	clk = devm_clk_get_optional_enabled_with_rate(dev, NULL, 7372800);
>
> This is quite unusual. Usually an external clock would be a fixed clock
> and therefore can't be set.

Really? It can be a crystal of course, but it also can be anything else.
Shouldn't I be trying to set the clock frequency in that case?

>
>> +	if (IS_ERR(clk))
>> +		return dev_err_probe(dev, PTR_ERR(clk),
>> +				     "Failed to get external clock\n");
>> +
>> +	ret = devm_regulator_get_enable(dev, "dvdd");
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to get dvdd regulator\n");
>> +
>> +	st->avdd_uV = devm_regulator_get_enable_read_voltage(dev, "avdd");
>
> We only need the voltage of avdd if it is actually used as a reference, which
> is probably quite rare. Not all regulators provide a voltage value.

Then I should just check for ENODEV here.

>
>> +	if (st->avdd_uV < 0)
>> +		return dev_err_probe(dev, st->avdd_uV, "Failed to get avdd regulator\n");
>> +
>> +	st->refp_uV = devm_regulator_get_enable_read_voltage(dev, "refp");
>> +	if (st->refp_uV < 0 && st->refp_uV != -ENODEV)
>> +		return dev_err_probe(dev, st->refp_uV, "Failed to get refp regulator\n");
>> +
>> +	st->refn_uV = devm_regulator_get_enable_read_voltage(dev, "refn");
>> +	if (st->refn_uV < 0 && st->refn_uV != -ENODEV)
>> +		return dev_err_probe(dev, st->refn_uV, "Failed to get refn regulator\n");
>> +
>> +	st->start_gpiod = devm_gpiod_get_optional(dev, "start", GPIOD_OUT_LOW);
>> +	if (IS_ERR(st->start_gpiod))
>> +		return dev_err_probe(dev, PTR_ERR(st->start_gpiod),
>> +				     "Failed to get start GPIO\n");
>> +
>> +	st->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(st->reset_gpiod))
>> +		return dev_err_probe(dev, PTR_ERR(st->reset_gpiod),
>> +				     "Failed to get reset GPIO\n");
>
> This is currently never used.

It has to be de-asserted for the chip to be in an active state though.

[...]

>> +MODULE_DESCRIPTION("Texas Instruments ADS1262 ADC driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Kurt Borja <kuurtb@gmail.com>");

I agree with all other comments. Thanks a lot for the review!

-- 
Thanks,
 ~ Kurt