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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 14 Jun 2026 15:27:11 -0500
"Kurt Borja" <kuurtb@gmail.com> wrote:

> 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@gmail.com> 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@gmail.com>
> >> ---
> >>  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?
Sure, bring it in when needed.  I just missread the code completely ;(
> 
> >
> > 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().
Ah ok.
> 
> [...]
> 
> >> +/* 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.


Yes.  It's a pity that bitfields are loosely defined in C :(

> 
> 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.

I looked again.  For this case we have some magic macros and generally
it's not worth the effort of making this dynamically sized unless you
have hundreds of channels.  Here I would move it to the end of
struct ads1262 (so the _DMA_ part of this does the right thing) and use
	IIO_DECLARE_DMA_BUFFER_WITH_TS(__be32, scan, DEFINE_FOR_MAX_CHANNELS);
That will declare a __be32 array but with the timestamp + padding etc.

> >> +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?

dev_info() probably as if we do see this it isn't supposed to be a problem
(unless DT is broken). Note there is an ongoing discussion with Kryzsztof about
what we should do for detectable parts like this.  So my mental model
of this might not be where that ends up. (See the various versions of the SLF3S
flow sensor driver).

Jonathan