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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 07 Aug 2026 22:58:24 -0500
Kurt Borja <kuurtb@gmail.com> 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.
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
Hi Kurt,

Just a few trivial things from me.

Jonathan

> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> new file mode 100644
> index 000000000000..d78e5e3ae13e
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1262.c

> +enum {
> +	ADS1262_RUNMODE_CONTINUOUS,
> +	ADS1262_RUNMODE_PULSE,
> +};
> +
> +enum {
> +	ADS1262_FILTER_SINC1,
> +	ADS1262_FILTER_SINC2,
> +	ADS1262_FILTER_SINC3,
> +	ADS1262_FILTER_SINC4,
> +	ADS1262_FILTER_FIR,
Where the values align with register field values, better
to assign them explicitly as easier to compare with datasheets
etc. 
> +};


> +
> +static int ads1262_channel_enable(struct ads1262 *st,
> +				  const struct iio_chan_spec *spec)
> +{
> +	u8 val;
> +
> +	guard(mutex)(&st->xfer_lock);
> +	guard(mutex)(&st->chan_lock);
> +
> +	val = FIELD_PREP(ADS1262_INPMUX_MUXN_MASK, spec->channel2) |
> +	      FIELD_PREP(ADS1262_INPMUX_MUXP_MASK, spec->channel);
> +	return regmap_update_bits(st->regmap, ADS1262_INPMUX_REG,
> +				 ADS1262_INPMUX_MUXN_MASK |
> +				 ADS1262_INPMUX_MUXP_MASK, val);

I'd put val on next line.  Feel free to go longer on the lines
in place like this where it perhaps improves readability by
keeping masks and values in the same call.

	return regmap_update_bits(st->regmap, ADS1262_INPMUX_REG,
				 ADS1262_INPMUX_MUXN_MASK |
				 ADS1262_INPMUX_MUXP_MASK,
				 FIELD_PREP(ADS1262_INPMUX_MUXN_MASK, spec->channel2) |
			 	 FIELD_PREP(ADS1262_INPMUX_MUXP_MASK, spec->channel);

> +}
> +



> +static int ads1262_dev_configure(struct ads1262 *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +
> +	ret = ads1262_dev_reset(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to reset device\n");
> +
> +	guard(mutex)(&st->xfer_lock);
> +
> +	ret = regmap_clear_bits(st->regmap, ADS1262_POWER_REG,
> +				ADS1262_POWER_RESET_MASK);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(st->regmap, ADS1262_INTERFACE_REG,
> +				ADS1262_INTERFACE_STATUS_MASK |
> +				ADS1262_INTERFACE_CRC_MASK);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

I don't think this gets modified later - if it doesn't
	return regmap_cear_bits()

> +}



> +
> +static const struct reg_default ads1262_reg_defaults[] = {
> +	{ ADS1262_INTERFACE_REG,
> +	  FIELD_PREP_CONST(ADS1262_INTERFACE_STATUS_MASK, true) |
> +	  FIELD_PREP_CONST(ADS1262_INTERFACE_CRC_MASK, true) },
> +	{ ADS1262_MODE0_REG,		0x00 },
> +	{ ADS1262_MODE1_REG,
> +	  FIELD_PREP_CONST(ADS1262_MODE1_FILTER_MASK, ADS1262_FILTER_FIR) },
> +	{ ADS1262_MODE2_REG,
> +	  FIELD_PREP_CONST(ADS1262_MODE2_DR_MASK, ADS1262_DR_20_SPS) },
> +	{ ADS1262_INPMUX_REG,
> +	  FIELD_PREP_CONST(ADS1262_INPMUX_MUXN_MASK, ADS1262_INPMUX_AIN1) },
> +	{ ADS1262_IDACMUX_REG,
> +	  FIELD_PREP_CONST(ADS1262_IDACMUX_MUX2_MASK, ADS1262_IDACMUX_NO_CONN) |
> +	  FIELD_PREP_CONST(ADS1262_IDACMUX_MUX1_MASK, ADS1262_IDACMUX_NO_CONN) },
> +	{ ADS1262_IDACMAG_REG,		0x00 },
> +	{ ADS1262_REFMUX_REG,		0x00 },
> +	{ ADS1262_TDACP_REG,		0x00 },
> +	{ ADS1262_TDACN_REG,		0x00 },
> +	{ ADS1262_GPIOCON_REG,		0x00 },
> +	{ ADS1262_GPIODIR_REG,		0x00 },
> +	{ ADS1262_ADC2CFG_REG,		0x00 },
> +	{ ADS1262_ADC2MUX_REG,
> +	  FIELD_PREP_CONST(ADS1262_ADC2MUX_MUXN2_MASK, ADS1262_INPMUX_AIN1) },
> +};
I'm not particularly keen on this formatting from a readability point of view.
Maybe using named intializers would help:

static const struct reg_default ads1262_reg_defaults[] = {
	{ 
		.reg = ADS1262_INTERFACE_REG,
		.def = FIELD_PREP_CONST(ADS1262_INTERFACE_STATUS_MASK, true) |
		       FIELD_PREP_CONST(ADS1262_INTERFACE_CRC_MASK, true),
//extra indent fine here for readability reasons if we end up with some long lines
	},
	{ .reg = ADS1262_MODE0_REG, .def = 0x00 },
etc or just maybe use the more open form of the first element for all of them.
burns a bunch of lines, but is easy to read. 




> +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;
> +	u8 tx[2];
> +
> +	lockdep_assert_held(&st->xfer_lock);
> +
> +	/*
> +	 * The register read operation uses a two byte command header followed
> +	 * by the register data:
> +	 *
> +	 *	byte 0:   RREG opcode | register address
> +	 *	byte 1:   number of registers to transfer, minus one
> +	 *	byte 2..: register data
> +	 */
> +	memcpy(tx, reg_buf, 1);
> +	tx[0] |= ADS1262_OPCODE_RREG;

Similar to below. I think more readable without the memcpy()

> +	tx[1] = val_size - 1;
> +
> +	return spi_write_then_read(st->spi, tx, sizeof(tx), val_buf, val_size);
> +}
> +
> +static int ads1262_regmap_gather_write(void *context, const void *reg_buf,
> +				       size_t reg_size, const void *val_buf,
> +				       size_t val_size)
> +{
> +	struct ads1262 *st = context;
> +	u8 tx[ADS1262_MAX_REGMAP_WRITE + 2];
> +
> +	lockdep_assert_held(&st->xfer_lock);
> +
> +	/*
> +	 * The register write operation uses a two byte command header followed
> +	 * by the register data:
> +	 *
> +	 *	byte 0:   WREG opcode | register address
> +	 *	byte 1:   number of registers to transfer, minus one
> +	 *	byte 2..: register data
> +	 */
> +	memcpy(tx, reg_buf, 1);

This is odd and I guess there because you don't want to cast reg_buf.
Even so I'd either use a local variable or just cast it.

	tx[0] = *((u8 *)regbuf) | ADS1262_OPCODE_WREG;

> +	tx[0] |= ADS1262_OPCODE_WREG;
> +	tx[1] = val_size - 1;
> +	memcpy(&tx[2], val_buf, val_size);
> +
> +	return spi_write_then_read(st->spi, tx, 2 + val_size, NULL, 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);
> +}
> +
> +static const struct regmap_bus ads1262_regmap_bus = {
> +	.read = ads1262_regmap_read,
> +	.gather_write = ads1262_regmap_gather_write,
> +	.write = ads1262_regmap_write,
> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> +	.val_format_endian_default = REGMAP_ENDIAN_BIG,
> +	.max_raw_write = ADS1262_MAX_REGMAP_WRITE,
> +};
> +
> +static int ads1262_gpio_setup(struct ads1262 *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +
> +	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");
> +
> +	/*
> +	 * The power transition timing requirement is 65536 clock cycles, at the
> +	 * minimum clock frequency this is 65536 microseconds.
> +	 */
> +	fsleep(65536);
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	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, "%s: failed to read single-channel\n",
> +					     fwnode_get_name(node));
> +
> +		pins[1] = ADS1262_INPMUX_AINCOM;
> +		fwnode_property_read_u32(node, "common-mode-channel", &pins[1]);
> +	} 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, "%s: failed to read diff-channels\n",
> +					     fwnode_get_name(node));
> +
> +		if (pins[0] <= ADS1262_INPMUX_AINCOM || pins[1] <= ADS1262_INPMUX_AINCOM)
> +			spec->differential = true;
> +	} else {
> +		return dev_err_probe(dev, -ENXIO,
> +				     "%s: one of single-channel or diff-channels is required\n",
> +				     fwnode_get_name(node));
> +	}
> +
> +	if (pins[0] >= ADS1262_INPMUX_FLOAT || pins[1] >= ADS1262_INPMUX_FLOAT)
> +		return dev_err_probe(dev, -EINVAL, "%s: input channels not in range\n",
> +				     fwnode_get_name(node));
> +
> +	if ((pins[0] >= ADS1262_INPMUX_TEMP ||
> +	     pins[1] >= ADS1262_INPMUX_TEMP) && pins[0] != pins[1])
> +		return dev_err_probe(dev, -EINVAL,
> +				     "%s: monitor channels must be selected symmetrically\n",
> +				     fwnode_get_name(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 *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);
> +	}
> +
> +	specs[num_specs - 1] = IIO_CHAN_SOFT_TIMESTAMP(num_specs - 1);
> +
> +	indio_dev->channels = specs;
> +	indio_dev->num_channels = num_specs;
> +
> +	return 0;
> +}