Re: [PATCH v3 2/9] iio: adc: add the ti-ads1262 driver

On Sat Aug 8, 2026 at 1:39 PM -05, David Lechner wrote:
> On 8/7/26 10:58 PM, 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.
>
> Should probably mention here that IIO_CHAN_INFO_SCALE is intentionally
> left out here. (Or just implement it using internal reference voltage
> to start with.)

I'll mention the scale is left out.

>
>> 
>> Signed-off-by: Kurt Borja <[email protected]>
>> ---

[...]

>> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c

[...]

>> +static int ads1262_dev_reset(struct ads1262 *st)
>> +{
>> +	int ret;
>> +
>> +	if (st->reset_gpiod) {
>> +		ret = gpiod_set_value_cansleep(st->reset_gpiod, 1);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/*
>> +		 * The RESET pulse timing requirement is 4 clock cycles, at the
>> +		 * minimum clock rate this is 4 microseconds.
>> +		 */
>> +		fsleep(4);
>
> How long do we have to hold reset before the chip powers down?

For power down 65536 clk cycles. Less than that is simple reset.

[...]

>> +static int ads1262_wait_for_conversion(struct ads1262 *st)
>> +{
>> +	u64 max_lat_ms;
>> +	long ret;
>> +
>> +	/*
>> +	 * The first conversion latency is affected by the channel's data rate,
>> +	 * filter, the configurable conversion delay and whether chop mode
>> +	 * and/or IDAC rotation mode are enabled.
>> +	 *
>> +	 * The worst possible latency is calculated by taking the lowest data
>> +	 * rate (2.5 SPS) and the sinc4 filter. This gives a latency of 1600 ms
>> +	 * (Table 9-13). Then we scale it by the actual clock rate and multiply
>> +	 * by 4 to account for chop and IDAC rotation modes (Equation 20).
>> +	 */
>> +	max_lat_ms = 4 * div_u64(mul_u32_u32(1600, 7372800), st->clk_rate);
>
> These are constant values, so don't need mul_u32_u32(). Also, given the wide
> range of possible sampling rates, I would include the current sampling rate
> in the calculation. No need to wate 1.6 seconds for something that should
> take a few 10s of microseconds.

Can we leave it like this until I implement the settlingtime? That way I
can calculate the actual first conversion latency.

[...]

>> +static int ads1262_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>> +				      unsigned int writeval, unsigned int *readval)
>> +{
>> +	struct ads1262 *st = iio_priv(indio_dev);
>> +
>> +	guard(mutex)(&st->xfer_lock);
>> +
>> +	if (readval)
>> +		return regmap_read_bypassed(st->regmap, reg, readval);
>
> Don't trust the cache? :-)

I don't trust myself :-) This was relevant early in development, I'll go
with the normal version.

[...]

>> +static int ads1262_parse_channels(struct iio_dev *indio_dev)
>> +{
>> +	struct ads1262 *st = iio_priv(indio_dev);
>> +	struct device *dev = &st->spi->dev;
>> +	struct iio_chan_spec *specs;
>> +	unsigned long used_regs = 0;
>> +	int num_specs;
>> +	u32 reg;
>> +	int ret;
>> +
>> +	st->num_channels = device_get_named_child_node_count(dev, "channel");
>> +	if (!st->num_channels)
>> +		return dev_err_probe(dev, -ENXIO, "no 'channel' nodes configured\n");
>> +	if (st->num_channels > ADS1262_MAX_CHANNEL_COUNT)
>> +		return dev_err_probe(dev, -EINVAL, "too many channels\n");
>> +
>> +	/* Account for the timestamp channel */
>> +	num_specs = st->num_channels + 1;
>> +	specs = devm_kcalloc(dev, num_specs, sizeof(*specs), GFP_KERNEL);
>> +	if (!specs)
>> +		return -ENOMEM;
>> +
>> +	device_for_each_named_child_node_scoped(dev, node, "channel") {
>> +		ret = fwnode_property_read_u32(node, "reg", &reg);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret, "%s: failed to read channel reg\n",
>> +					     fwnode_get_name(node));
>> +		if (reg >= st->num_channels)
>> +			return dev_err_probe(dev, -EINVAL, "%s: reg out of range\n",
>> +					     fwnode_get_name(node));
>> +
>> +		static_assert(ADS1262_MAX_CHANNEL_COUNT < BITS_PER_LONG);
>> +		if (__test_and_set_bit(reg, &used_regs))
>> +			return dev_err_probe(dev, -EINVAL, "%s: duplicated channel reg\n",
>> +					     fwnode_get_name(node));
>> +
>> +		specs[reg].scan_index = reg;
>> +		specs[reg].scan_type = (struct iio_scan_type) {
>> +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
>> +			.realbits = ADS1262_ADC1_RESOLUTION,
>> +			.storagebits = 32,
>> +			.endianness = IIO_BE,
>> +		};
>> +
>> +		ret = ads1262_parse_channel_node(st, &specs[reg], node);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (specs[reg].channel == ADS1262_INPMUX_TEMP)
>> +			specs[reg].type = IIO_TEMP;
>> +		else
>> +			specs[reg].type = IIO_VOLTAGE;
>> +
>> +		if (specs[reg].channel != ADS1262_INPMUX_TEMP)
>> +			specs[reg].indexed = true;
>> +
>> +		specs[reg].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	}
>
> If we are going to use reg to determine the scan index, we need to
> make sure there are no holes in specs that didn't get filled in.
>
> device_for_each_named_child_node_scoped() will skip `status = "disabled"`
> channels, so this could be a possibility.

Ah, I didn't know some channels could be skipped. I'll check for holes.

[...]

-- 
Thanks,
 ~ Kurt