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

[David Lechner <dlechner@baylibre.com>]

On 9/6/26 3:17 PM, Kurt Borja wrote:
> 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.

I think it was just added in a later patch.

> 
>>
>>> +	},
>>> +	{
>>> +		.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?
> 
Sounds like Jonathan is OK with it, so OK with me too to leave it as-is.