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

[David Lechner]

On 6/28/26 3:00 PM, Kurt Borja wrote:
> On Sun Jun 28, 2026 at 12:15 PM -05, David Lechner wrote:
>> On 6/28/26 12:36 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.
>>>

...

>>> +static int ads1262_read_raw(struct iio_dev *indio_dev,
>>> +			    struct iio_chan_spec const *chan, int *val,
>>> +			    int *val2, long mask)
>>> +{
>>> +	struct ads1262 *st = iio_priv(indio_dev);
>>> +	struct ads1262_channel *chan_data = &st->channels[chan->scan_index];
>>> +	u8 realbits = chan->scan_type.realbits;
>>> +	__be32 raw;
>>> +	int ret;
>>> +
>>> +	switch (mask) {
>>> +	case IIO_CHAN_INFO_RAW:
>>> +		ret = ads1262_channel_read(st, chan_data, &raw);
>>> +		if (ret)
>>> +			return ret;
>>> +		*val = sign_extend32(be32_to_cpu(raw), realbits - 1);
>>> +
>>> +		return IIO_VAL_INT;
>>> +
>>> +	case IIO_CHAN_INFO_SCALE: {
>>> +		guard(mutex)(&st->chan_lock);
>>> +
>>> +		ret = ads1262_channel_get_scale(st, chan, val, val2);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		return IIO_VAL_INT_PLUS_NANO;
>>> +	}
>>> +
>>> +	case IIO_CHAN_INFO_HARDWAREGAIN: {
>>
>> There is only one other ADC that uses "hardwaregain". Usually, we just make
>> scale writeable to control the gain. I don't remember what the rules for
>> that attribute are. Using it for in_voltage is not documented in the ABI.
> 
> I went with hardwaregain because the scale loses too many significant
> digits at high gain. With the internal reference and gain = 1, the scale
> is at 0.000001164; then at gain = 32, the scale is at 0.000000036.
> 
> In this case I expect users to just calculate the scale themselves based
> on the hardwaregain. Is this acceptable? If not I'll go with
> scale_available.

I'm pretty sure there is a series floating around that has proposed
adding new fixed-point IIO_VAL_* types that could be another solution.
We'll see what Jonathan says about this too.


>>> +static int ads1262_regmap_read(void *context, const void *reg_buf,
>>> +			       size_t reg_size, void *val_buf, size_t val_size)
>>> +{
>>> +	struct ads1262 *st = context;
>>> +	struct spi_transfer xfer = {
>>> +		.tx_buf = st->tx,
>>> +		.rx_buf = st->rx,
>>> +		.len = reg_size + 1 + val_size,
>>> +	};
>>> +	int ret;
>>> +
>>> +	guard(mutex)(&st->xfer_lock);
>>
>> SPI bus and regmap both already have their own locking, so putting a lock
>> here seems out of place. Instead, the lock should be for higher-level
>> operations where there are mulitple register access in a single operation.
> 
> I agree. I can definitely move this one to a "higher level". But IMO,
> because this also protects tx and rx buffers, it makes sense to have it
> here too.

It still seems redundant to me to have a lower-level lock if
we have a higher-level lock that is always going to be held
anyway. But maybe there is some case where it still makes sense?


>>> +static int ads1262_parse_firmware(struct ads1262 *st)
>>> +{
>>> +	struct device *dev = &st->spi->dev;
>>> +	struct clk *clk;
>>> +	u32 reg;
>>> +	int ret;
>>> +
>>> +	/* Set the nominal clock frequency */
>>> +	clk = devm_clk_get_optional_enabled_with_rate(dev, NULL, 7372800);
>>
>> This is quite unusual. Usually an external clock would be a fixed clock
>> and therefore can't be set.
> 
> Really? It can be a crystal of course, but it also can be anything else.
> Shouldn't I be trying to set the clock frequency in that case?

According to the datasheet, 7.3728 MHz is just the nominal value while
1 to 8 MHz is allowed. So I would expect the external clock to already
be providing the chosen design-specific rate and we should be getting
the rate here, not setting it.

> 
>>
>>> +	if (IS_ERR(clk))
>>> +		return dev_err_probe(dev, PTR_ERR(clk),
>>> +				     "Failed to get external clock\n");
>>> +
>>> +	ret = devm_regulator_get_enable(dev, "dvdd");
>>> +	if (ret)
>>> +		return dev_err_probe(dev, ret, "Failed to get dvdd regulator\n");
>>> +
>>> +	st->avdd_uV = devm_regulator_get_enable_read_voltage(dev, "avdd");
>>
>> We only need the voltage of avdd if it is actually used as a reference, which
>> is probably quite rare. Not all regulators provide a voltage value.
> 
> Then I should just check for ENODEV here.

Except AVDD is a required supply. They way I did it in the driver I am working
on is first parse all of the channels to see if anything is actually using AVDD
as a reference and only call devm_regulator_get_enable_read_voltage() in that
case, otherwise call devm_regulator_get_enable().

> 
>>
>>> +	if (st->avdd_uV < 0)
>>> +		return dev_err_probe(dev, st->avdd_uV, "Failed to get avdd regulator\n");
>>> +
>>> +	st->refp_uV = devm_regulator_get_enable_read_voltage(dev, "refp");
>>> +	if (st->refp_uV < 0 && st->refp_uV != -ENODEV)
>>> +		return dev_err_probe(dev, st->refp_uV, "Failed to get refp regulator\n");
>>> +
>>> +	st->refn_uV = devm_regulator_get_enable_read_voltage(dev, "refn");
>>> +	if (st->refn_uV < 0 && st->refn_uV != -ENODEV)
>>> +		return dev_err_probe(dev, st->refn_uV, "Failed to get refn regulator\n");
>>> +
>>> +	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->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>>> +	if (IS_ERR(st->reset_gpiod))
>>> +		return dev_err_probe(dev, PTR_ERR(st->reset_gpiod),
>>> +				     "Failed to get reset GPIO\n");
>>
>> This is currently never used.
> 
> It has to be de-asserted for the chip to be in an active state though.
> 
Usually, if we have hardware reset available, we use it to reset instead
of writing a register to do the reset.