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

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

On Fri Aug 28, 2026 at 5:09 AM -03, Andy Shevchenko wrote:
> On Fri, Aug 28, 2026 at 01:38:18AM -0500, 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.
>> 
>> Various features such as accurate timeout delays, per-channel reference
>> sources, scale, offset, settling latency, excitation currents and
>> diagnostics are intentionally left out for future support.
>
> ...
>

...

>> +static int ads1262_dev_send_cmd(struct ads1262 *st, u8 opcode)
>> +{
>> +	guard(mutex)(&st->xfer_lock);
>> +
>> +	return spi_write_then_read(st->spi, &opcode, sizeof(opcode), NULL, 0);
>> +}
>> +
>> +static int ads1262_dev_read_by_cmd(struct ads1262 *st, u8 cmd, __be32 *val)
>> +{
>> +	guard(mutex)(&st->xfer_lock);
>> +
>> +	return spi_write_then_read(st->spi, &cmd, sizeof(cmd), val, sizeof(*val));
>> +}
>
> How do these do not conflict or race with regmap SPI communication?

No because the regmap interface only covers registers accesible by the
WREG (write) and RREG (read) commands. The data holding register is
accessed by a different command (OPCODE) RDATA1/RDATA2.

>
> ...
>
>> +static int ads1262_dev_reset(struct ads1262 *st)
>> +{
>> +	struct device *dev = &st->spi->dev;
>> +	struct gpio_desc *reset_gpiod;
>> +	int ret;
>> +
>> +	reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(reset_gpiod))
>> +		return dev_err_probe(dev, PTR_ERR(reset_gpiod),
>> +				     "failed to get reset GPIO\n");
>
>> +
>
> Unneeded blank line. But can you use reset-gpio driver instead?

I'll look into reset-gpio. But I think it may not be worth to use it
because it may not change much here. We'd still need to manage these
timing constraints and we also only use this GPIO in the probe anyway.

...

>> +static int ads1262_parse_channel_node(struct ads1262 *st,
>> +				      struct iio_chan_spec *spec,
>> +				      struct fwnode_handle *node)
>> +{
>> +	struct device *dev = &st->spi->dev;
>> +	u32 pins[2];
>> +	int ret;
>
> Can this use the property names for 'single-channel' and 'diff-channels'?
> This will deduplicate the same in a few places and reduce potential typos.

You mean storing the property name in a variable? Sure.

...

>> +	st->clk_rate = rate ? rate : ADS1262_NOMINAL_CLK_RATE;
>
> Can use Elvis.

I'm not really sure what you meant here.

...

Thanks Andy! I'll address the rest of the comments too.

-- 
Thanks,
 ~ Kurt