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

[Kurt Borja]

Hi Jonathan,

On Sat Jun 13, 2026 at 8:45 AM -05, Jonathan Cameron wrote:
> On Fri, 12 Jun 2026 17:46:20 -0500
> Kurt Borja <kuurtb@xxxxxxxxx> wrote:
>
>> Add ti-ads1262 driver for TI ADS1262 and ADS1263 ADCs with initial
>> support for the following features:
>> 
>>   - Power management
>>   - IIO direct and buffer modes
>>   - Channel hot-reloading
>>   - Internal or external oscillator
>>   - Internal or external voltage reference
>>   - Filter configuration
>>   - Sensor bias configuration
>>   - IDAC configuration
>>   - Level-shift voltage configuration
>>   - Auxiliary ADC interoperability considerations
>> 
>> Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
>> ---
>>  MAINTAINERS                  |    1 +
>>  drivers/iio/adc/Kconfig      |   13 +
>>  drivers/iio/adc/Makefile     |    1 +
>>  drivers/iio/adc/ti-ads1262.c | 1816 ++++++++++++++++++++++++++++++++++++++++++
>
> That is rather too big. I think you'll have to work out how to split this
> up into more manageable chunks.  Staying under a 1000 (preferably a lot less)
> per patch makes it much easier for people to review.
>
> Given the complexity of the device this might be one that has to go
> in as several series, building up functionality as we go.

I'll split it up as much as possible for next version.

I was thinking of taking out the hot-reloading stuff for a follow-up
series. In that case I would also add IIO_ACQUIRE_BUFFER_MODE().
What do you think?

>
> I'll ignore all the DT stuff as sounds like that may radically change and
> just take a fairly superficial first look at this.

Yes, I will just address Krzysztof comments and leave that patch until
we can discuss it with David.

>
> Jonathan
>

[...]

>> +#include <linux/lockdep.h>
>
> Fairly unusual to see that header in a driver.
> What's it here for?

I included it for lockdep_assert_held().

[...]

>> +/* IDACMAG constants */
>> +#define ADS1262_IDACMAG_OFF			0
>> +#define ADS1262_IDACMAG_COUNT			11
>> +
>> +/* REFMUX constants */
>
> Naming is good enough I'm not sure I'd bother with the comments
> to say what these are.
>
> On option is to just group them with the register they are about
> and using extra indenting to visually separate them from the register
>
> #define ADS1262_REFMUX_REG			0xxx
> #define   ADS1262_REFMUX_RMUXP_MASK		GENMASK(5, 3)
> #define     ADS1262_RMUX_INTER				0
> #define     ADS1262_RMUX_AIN0_AIN1			1
> #define     ADS1262_RMUX_AIN2_AIN3			2
> #define     ADS1262_RMUX_AIN4_AIN5			3
> #define     ADS1262_RMUX_AVDD_AVSS			4
> #define     ADS1262_RMUX_COUNT				5

I like this...

> However, if you are going to have a terminating entry, an anonymous enum might be better
> with that just as the last item.

...but this sounds good too. I'll go for what looks more organized.

>
> #define   ADS1262_REFMUX_RMUXN_MASK		GENMASK(2, 0)
>
>
>> +#define ADS1262_RMUX_INTER			0
>> +#define ADS1262_RMUX_AIN0_AIN1			1
>> +#define ADS1262_RMUX_AIN2_AIN3			2
>> +#define ADS1262_RMUX_AIN4_AIN5			3
>> +#define ADS1262_RMUX_AVDD_AVSS			4
>> +#define ADS1262_RMUX_COUNT			5
>> +
>> +struct ads1262_channel {
>
> As a general rule we tend to avoid bitfields because of all the problems
> with how loose the C spec is on how these actually get laid out.
> I'd just have this as a suitable 32 bit value and then have
> defines for masks within that.

Are you suggesting storing this whole struct data as a u32 and
reading/writing with FIELD_*() helpers? I think that may be less
readable but it would save memory. I don't know if I understood
correctly though.

I'm dropping the bitfield approach for next version anyway.

[...]

>> +struct ads1262 {
>> +	struct spi_device *spi;
>> +	struct regmap *regmap;
>> +	struct iio_dev *indio_dev;
>> +	struct iio_trigger *trig;
>> +	struct gpio_desc *reset_gpiod;
>> +	struct gpio_desc *start_gpiod;
>> +
>> +	void *scan_buffer;
> I think this is always accessed as a __be32. If so just type it as that.

I was hesitant to do that because of the space reserved at the end for
the timestamp. Didn't feel right to assign __be32 when it would actually
be something like

	struct {
		__be32 buff;
		aligned_s64 ts;
	};

But I have no problem doing it.

[...]

>> +static int ads1262_fill_buffer_mult(struct ads1262 *st)
>> +{
>> +	__be32 val, *scan_buffer = st->scan_buffer;
>
> Avoid mixing pointer and no point, or anything with assignments
> as it makes the code harder to read.
>
>> +	unsigned int chan;
>> +	int i = -1;
>> +	int ret;
>> +
>> +	/*
>> +	 * This routine enables and reads channels in a full-duplex fashion.
>> +	 *
>> +	 * When a channel is enabled, the previous conversion is clocked out of
>> +	 * the shift data register on the same transfer (Section 9.4.7.1). This
>> +	 * allows for low latency software sequencing but forbids any SPI
>> +	 * activity happen in between or data corruption may occur, hence the
>> +	 * need to take the xfer_lock for the whole operation.
>> +	 */
>
> Just to check: Is SPI traffic on the same bus to a different device fine?
> If not you'd need spi_bus_lock(). If it is fine then reword this to talk about
> communications with this device just to avoid confusion.

Yes, to a different device is fine. I'll reword it.

[...]

>> +static int ads1262_read_chip_name(struct ads1262 *st, char **name)
>> +{
>> +	struct device *dev = &st->spi->dev;
>> +	u8 dev_id;
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	ret = regmap_read(st->regmap, ADS1262_ID_REG, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_id = FIELD_GET(ADS1262_DEV_ID_MASK, val);
>> +
>> +	switch (dev_id) {
>> +	case ADS1262_DEV_ID_ADS1262:
>> +		*name = "ads1262";
>
> Given, at somepoint I would guess you'll want to support the auxiliary adc
> on the 1263, I'd start with a struct chip_info  (with the name in there)
> and pick that rather than just the name here.

Makes sense. In that case I can add a dev_warn if the name doesn't match
the internal model. Would that be ok or would you prefer dev_dbg?

>
>> +		break;
>> +	case ADS1262_DEV_ID_ADS1263:
> Not particularly important but common practice to just change the prefix
> for anything device specific.
> 	case ADS1263_DEV_ID

Good to know!

>
>> +		*name = "ads1263";
>> +		break;
>> +	default:
>> +		*name = "ads1262";
> Given we'll ultimately want fallback compatibles to work and so allow
> for firmware to specify which device to fallback to, this should really be
> using the guidance from firmware to select rather than always guessing
> the 1262 variant.  That is safe though given the 'subset' nature so this
> doesn't matter as much as it normally does.

Agreed.

[...]

>> +static const struct reg_default ads1262_reg_defaults[] = {
>> +	{ ADS1262_POWER_REG,		0x11 },
>
> Is it sensible to specify these in terms of the fields that make them up?
> Can make it easier to see what the default state actually means.
> Sometimes it is just too complex, so we don't bother.

I prefer not to do it because it would be too complex. I'll try though.

[...]

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

Ack to the rest of comments!

-- 
Thanks,
 ~ Kurt