Re: [PATCH v4 03/10] iio: adc: add the ti-ads1262 driver

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

On Mon Aug 31, 2026 at 5:22 PM -03, David Lechner wrote:
> On 8/28/26 1:38 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.
>> 
>
> ...
>
>> +#define ADS1262_FW_CHANNEL_COUNT		16
>> +#define ADS1262_MON_CHANNEL_COUNT		4
>> +#define ADS1262_REGMAP_WRITE_SZ			8
>> +#define ADS1262_MONITOR_ADDR_OFFSET		100
>
> Where does this offset come from? I would make the address the value that
> gets written to MUXP/MUXN. But it looks like we are using the same value
> for the .channel, so setting .address to that would be redundant.

I'm using .address to map the firmware 'reg' to channels in
fwnode_xlate. That's why I would need to move the monitors forward. More
on this discussion below.

>
>> +
>> +#define ADS1262_ADC1_RESOLUTION			32
>> +
>> +struct ads1262 {
>> +	struct spi_device *spi;
>> +	struct regmap *regmap;
>> +	struct gpio_desc *start_gpiod;
>> +	/* protects concurrent SPI transfers */
>> +	struct mutex xfer_lock;
>> +	/* protects channel state */
>> +	struct mutex chan_lock;
>> +	struct completion drdy;
>> +	unsigned long clk_rate;
>> +	u8 dev_id;
>> +};
>> +
>> +static const char * const ads1262_device_id_to_name[] = {
>> +	[ADS1262_DEV_ID] = "ads1262",
>> +	[ADS1263_DEV_ID] = "ads1263",
>> +};
>> +
>> +static const struct iio_chan_spec ads1262_monitor_chan_specs[] = {
>> +	{
>> +		.type = IIO_TEMP,
>> +		.channel = ADS1262_INPMUX_TEMP,
>> +		.channel2 = ADS1262_INPMUX_TEMP,
>
> Since these are the same, I would just not set .channel2 and later say
> MUXN = spec->differential ? spec->channel2 : spec->channel. Same applies
> to others below.
>
>> +		.address = ADS1262_MONITOR_ADDR_OFFSET + 0,
>> +		.scan_type = {
>> +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
>> +			.realbits = ADS1262_ADC1_RESOLUTION,
>> +			.storagebits = 32,
>> +			.endianness = IIO_BE,
>> +		},
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>
> Where is SCALE and OFFSET?

Missing.

I'm pretty sure I tested this channel though so maybe there's something
wrong in my tests.

>
>> +	},
>> +	{
>> +		.type = IIO_VOLTAGE,
>> +		.channel = ADS1262_INPMUX_AVDD,
>> +		.channel2 = ADS1262_INPMUX_AVDD,
>> +		.indexed = 1,
>> +		.address = ADS1262_MONITOR_ADDR_OFFSET + 1,
>> +		.scan_type = {
>> +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
>> +			.realbits = ADS1262_ADC1_RESOLUTION,
>> +			.storagebits = 32,
>> +			.endianness = IIO_BE,
>> +		},
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +	},
>> +	{
>> +		.type = IIO_VOLTAGE,
>> +		.channel = ADS1262_INPMUX_DVDD,
>> +		.channel2 = ADS1262_INPMUX_DVDD,
>> +		.indexed = 1,
>> +		.address = ADS1262_MONITOR_ADDR_OFFSET + 2,
>> +		.scan_type = {
>> +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
>> +			.realbits = ADS1262_ADC1_RESOLUTION,
>> +			.storagebits = 32,
>> +			.endianness = IIO_BE,
>> +		},
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +	},
>> +	{
>> +		.type = IIO_VOLTAGE,
>> +		.channel = ADS1262_INPMUX_TDAC,
>> +		.channel2 = ADS1262_INPMUX_TDAC,
>
> Hmm... a differential where channel == channel2 usually means a shorted
> input. TDACP and TDACN can be controlled indepedantly, so really are two
> separate channels.

They can be controlled independently but the user would have to define a
common mode channel for that. I went with this because its only a test
channel and we making these channels static. Otherwise we would have to
allow the TDAC channel in devicetree. Would that be preferable?

>
>> +		.indexed = 1,
>> +		.differential = 1,
>> +		.address = ADS1262_MONITOR_ADDR_OFFSET + 3,
>> +		.scan_type = {
>> +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
>> +			.realbits = ADS1262_ADC1_RESOLUTION,
>> +			.storagebits = 32,
>> +			.endianness = IIO_BE,
>> +		},
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +	},
>> +};
>> +
>
> ...
>
>> +static int ads1262_channel_read(struct iio_dev *indio_dev,
>> +				const struct iio_chan_spec *spec, __be32 *val)
>> +{
>> +	struct ads1262 *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
>> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
>> +		return -EBUSY;
>> +
>> +	ret = ads1262_set_runmode(st, ADS1262_RUNMODE_PULSE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ads1262_channel_enable(st, spec);
>> +	if (ret)
>> +		return ret;
>> +
>> +	reinit_completion(&st->drdy);
>> +
>> +	ret = ads1262_dev_start_one(st);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ads1262_wait_for_conversion(st);
>> +	if (ret)
>
> Since wait is interruptable, do we need to do something to stop the
> conversion here?

The conversions are already stopped by ads1262_dev_start_one(). I
believe there is no way to cancel the current conversion. The only
downside here would the stale data stuff pointed out by Sashiko.

>
>> +		return ret;
>> +
>> +	return ads1262_dev_read_by_cmd(st, ADS1262_OPCODE_RDATA1, val);
>> +}
>> +
>
> ...
>
>> +static int ads1262_fwnode_xlate(struct iio_dev *indio_dev,
>> +				const struct fwnode_reference_args *iiospec)
>> +{
>> +	/* REVISIT: the auxiliary ADC (ADC2) is currently not supported */
>> +	if (iiospec->nargs > 1 && iiospec->args[1])
>> +		return -EINVAL;
>> +
>> +	if (!iiospec->nargs)
>> +		return 0;
>> +
>> +	for (unsigned int i = 0; i < indio_dev->num_channels; i++) {
>
> Won't this include the timestamp channel?

Yes, I'll fix it.

>
>> +		if (indio_dev->channels[i].address == iiospec->args[0])
>
> I don't think .address is the right thing to use here (it is coming from
> reg in the devcietree). I would expect channel. Otherwise consumers in the
> devicetree have to be away of how channels were assigned rather than picking
> the datasheet channel number.

I was very confused about what approach should I take here. All channels
in this chip are actually differential. In that case should I make
#io-channel-cells = 3 i.e. positive, negative and ADC?

>
> And the devicetree bindings should mention the monitor channel numbers (11 - 14).
>
>> +			return i;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>
> ...
>
>> +static int ads1262_spi_probe(struct spi_device *spi)
>> +{
>> +	struct device *dev = &spi->dev;
>> +	struct iio_dev *indio_dev;
>> +	struct ads1262 *st;
>> +	unsigned long rate;
>> +	struct clk *clk;
>> +	int irq;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->info = &ads1262_iio_info;
>> +
>> +	st = iio_priv(indio_dev);
>> +	st->spi = spi;
>> +	init_completion(&st->drdy);
>> +
>> +	ret = devm_mutex_init(dev, &st->chan_lock);
>> +	if (ret)
>> +		return ret;
>> +	ret = devm_mutex_init(dev, &st->xfer_lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ads1262_parse_channels(indio_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ads1262_supply_setup(st);
>> +	if (ret)
>> +		return ret;
>> +
>> +	clk = devm_clk_get_optional_enabled(dev, NULL);
>> +	if (IS_ERR(clk))
>> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get external clock\n");
>> +
>> +	rate = clk_get_rate(clk);
>> +	if (clk && !rate)
>> +		return dev_err_probe(dev, -EINVAL, "failed to get clock rate\n");
>> +	st->clk_rate = rate ? rate : ADS1262_NOMINAL_CLK_RATE;
>> +
>> +	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->regmap = devm_regmap_init(dev, &ads1262_regmap_bus, st,
>> +				      &ads1262_regmap_config);
>> +	if (IS_ERR(st->regmap))
>> +		return PTR_ERR(st->regmap);
>> +
>> +	ret = ads1262_dev_configure(st);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to configure device\n");
>> +
>> +	indio_dev->name = ads1262_device_id_to_name[st->dev_id];
>
> Not so sure about this. Almost always, this is coming from the compatible
> match data. So unless we plan on trusting the device ID returned by the
> chip over the devicetree when we add more to the device id tables and looking
> up per-chip behavior from there instead of the compatible, I would go with
> the traditional approach. That way the name userpace sees match the driver
> behavior that goes with the other chip-specific match data that is likely
> to be added in the future.

You're right. I'll revert this.

>
>> +
>> +	/*
>> +	 * REVISIT: This chip has software polling capabilities, which could be
>> +	 * used to stop depending on the DRDY signal.
>> +	 *
>> +	 * Additionally, the MISO pin also can be used as a DRDY IRQ, in which
>> +	 * case the interrupt would be named 'doutdrdy', but requires extra
>> +	 * timing and synchronization considerations to be reliable.
>> +	 */
>> +	irq = fwnode_irq_get_byname(dev_fwnode(dev), "drdy");
>> +	if (irq < 0)
>> +		return dev_err_probe(dev, irq,
>> +				     "the 'drdy' IRQ is currently required for operation\n");
>> +
>> +	ret = devm_request_irq(dev, irq, ads1262_irq_handler, IRQF_NO_THREAD,
>> +			       indio_dev->name, st);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}
>> +

-- 
Thanks,
 ~ Kurt