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

[David Lechner]

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.

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

> +	},
> +	{
> +		.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.

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

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

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

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.

> +
> +	/*
> +	 * 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);
> +}
> +