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

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.

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>

> +#include <linux/compiler_attributes.h>

This one is covered by types.h.

> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/lockdep.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>

...

> +/*
> + * The power transition timing requirement is 65536 clock cycles, at the minimum
> + * clock frequency this is 65536 microseconds.
> + */
> +#define ADS1262_POWER_TRANS_USECS		65536

_US as a suffix is enough. It's how in plenty of cases we do in the Linux
kernel.

> +#define ADS1262_NOMINAL_CLK_RATE		7372800

And here perhaps _Hz? What is the unit for this value?

...

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

...

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

> +	if (reset_gpiod) {
> +		/*
> +		 * Wait a power transition cycle to ensure we are in a
> +		 * powered-off state after acquiring the RESET GPIO.
> +		 */
> +		fsleep(ADS1262_POWER_TRANS_USECS);
> +
> +		ret = gpiod_set_value_cansleep(reset_gpiod, 0);
> +		if (ret)
> +			return ret;
> +
> +		fsleep(ADS1262_POWER_TRANS_USECS);
> +	} else {
> +		ret = ads1262_dev_send_cmd(st, ADS1262_OPCODE_RESET);
> +		if (ret)
> +			return ret;
> +		/*
> +		 * The RESET timing requirement is 8 clock cycles, at the
> +		 * minimum clock rate this is 8 microseconds.
> +		 */
> +		fsleep(8);
> +	}
> +
> +	return 0;
> +}

...

> +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(1600ULL * ADS1262_NOMINAL_CLK_RATE, st->clk_rate);
> +	ret = wait_for_completion_interruptible_timeout(&st->drdy,
> +							msecs_to_jiffies(max_lat_ms));
> +	if (ret < 0)
> +		return ret;
> +	if (!ret)

In this case it's better to have number comparison as the semantics of 0 is
different to usual (success) code.

	if (ret == 0)

> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}

...

> +static int ads1262_regmap_write(void *context, const void *data, size_t count)
> +{
> +	return ads1262_regmap_gather_write(context, data, 1, data + 1,
> +					   count - 1);

I would go for a single line of 82 characters.

> +}

...

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

> +	if (fwnode_property_present(node, "single-channel")) {
> +		ret = fwnode_property_read_u32(node, "single-channel", &pins[0]);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "%pfwP: failed to read single-channel\n",
> +					     node);
> +
> +		pins[1] = ADS1262_INPMUX_AINCOM;
> +
> +		if (fwnode_property_present(node, "common-mode-channel")) {
> +			ret = fwnode_property_read_u32(node, "common-mode-channel", &pins[1]);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "%pfwP: failed to read common-mode-channel\n",
> +						     node);
> +		}
> +	} else if (fwnode_property_present(node, "diff-channels")) {
> +		ret = fwnode_property_read_u32_array(node, "diff-channels", pins,
> +						     ARRAY_SIZE(pins));
> +		if (ret)
> +			return dev_err_probe(dev, ret, "%pfwP: failed to read diff-channels\n",
> +					     node);
> +
> +		spec->differential = true;
> +	} else {
> +		return dev_err_probe(dev, -EINVAL,
> +				     "%pfwP: one of single-channel or diff-channels is required\n",
> +				     node);
> +	}
> +
> +	if (pins[0] > ADS1262_INPMUX_AINCOM || pins[1] > ADS1262_INPMUX_AINCOM)
> +		return dev_err_probe(dev, -EINVAL, "%pfwP: input channels not in range\n", node);
> +
> +	spec->channel = pins[0];
> +	spec->channel2 = pins[1];
> +
> +	return 0;
> +}

...

> +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 *chan_specs;
> +	unsigned int num_fw_channels, num_specs;
> +	unsigned int i = 0;

Split assignment. Move it closer to the first user.

> +	u32 reg;
> +	int ret;
> +
> +	num_fw_channels = device_get_named_child_node_count(dev, "channel");
> +	if (num_fw_channels > ADS1262_FW_CHANNEL_COUNT)
> +		return dev_err_probe(dev, -EINVAL, "too many channels\n");
> +
> +	/* Account for the monitor channels and timestamp */
> +	num_specs = num_fw_channels + ADS1262_MON_CHANNEL_COUNT + 1;
> +	chan_specs = devm_kcalloc(dev, num_specs, sizeof(*chan_specs), GFP_KERNEL);
> +	if (!chan_specs)
> +		return -ENOMEM;
> +
> +	device_for_each_named_child_node_scoped(dev, node, "channel") {
> +		struct iio_chan_spec *spec = &chan_specs[i];
> +
> +		ret = fwnode_property_read_u32(node, "reg", &reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "%pfwP: failed to read channel reg\n", node);
> +		if (reg >= ADS1262_MONITOR_ADDR_OFFSET)
> +			return dev_err_probe(dev, -EINVAL, "%pfwP: reg out of range\n", node);
> +
> +		ret = ads1262_parse_channel_node(st, spec, node);
> +		if (ret)
> +			return ret;
> +
> +		spec->type = IIO_VOLTAGE;
> +		spec->indexed = true;
> +		spec->scan_index = i;
> +		spec->address = reg;
> +		spec->scan_type = (struct iio_scan_type) {
> +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
> +			.realbits = ADS1262_ADC1_RESOLUTION,
> +			.storagebits = 32,
> +			.endianness = IIO_BE,
> +		};
> +		spec->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +
> +		i++;
> +	}
> +
> +	memcpy(&chan_specs[i], ads1262_monitor_chan_specs,
> +	       sizeof(ads1262_monitor_chan_specs));
> +
> +	for (unsigned int mon = 0; mon < ADS1262_MON_CHANNEL_COUNT; mon++) {
> +		chan_specs[i].scan_index = i;
> +		i++;
> +	}
> +
> +	chan_specs[i] = IIO_CHAN_SOFT_TIMESTAMP(i);
> +	i++;
> +
> +	indio_dev->channels = chan_specs;
> +	indio_dev->num_channels = i;
> +
> +	return 0;
> +}

...

> +	st->clk_rate = rate ? rate : ADS1262_NOMINAL_CLK_RATE;

Can use Elvis.

-- 
With Best Regards,
Andy Shevchenko