Re: [PATCH 5/5] iio: adc: Add ti-ads1263-adc2 driver

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 12 Jun 2026 17:46:23 -0500
Kurt Borja <kuurtb@gmail.com> wrote:

> The TI ADS1263 includes an auxiliary, 24-bit, delta-sigma ADC (ADC2).
> ADC2 operation is independent of ADC1, with independent selections of
> input channel, reference voltage, sample rate, and channel gain
> 
> Add support for this ADC as an independent IIO device, through the
> auxiliary bus API.

A few things inline.

> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>

> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> index b33505e7fdc7..1a4b2f934d43 100644
> --- a/drivers/iio/adc/ti-ads1262.c
> +++ b/drivers/iio/adc/ti-ads1262.c

> +static int ads1262_aux_device_setup(struct ads1262 *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct ads1263_adc2_channel *chans;
> +	struct auxiliary_device *adev;
> +	struct ads1263_adc2_ctx *ctx;
> +	struct fwnode_handle *node;
> +	int id, ret;
> +
> +	node = device_get_named_child_node(dev, "adc");
> +	if (!node)
> +		return 0;
> +
> +	ctx = kzalloc_obj(*ctx);
> +	if (!ctx) {
> +		ret = -ENOMEM;
> +		goto out_node_put;
> +	}
> +
> +	id = ida_alloc(&ads1262_ida, GFP_KERNEL);
> +	if (id < 0) {
> +		ret = id;
> +		goto out_free_adc2;
> +	}
> +
> +	chans = kcalloc(st->num_channels, sizeof(*chans), GFP_KERNEL);
> +	if (!chans) {
> +		ret = -ENOMEM;
> +		goto out_free_id;
> +	}
> +
> +	for (unsigned int i = 0; i < st->num_channels; i++) {
> +		chans[i].negative_input = st->channels[i].negative_input;
> +		chans[i].positive_input = st->channels[i].positive_input;
> +	}
> +
> +	ctx->chip = st;
> +	ctx->num_channels = st->num_channels;
> +	ctx->channels = chans;
> +	ctx->enable = ads1263_adc2_enable;
> +	ctx->start = ads1263_adc2_start;
> +	ctx->stop = ads1263_adc2_stop;
> +	ctx->read = ads1263_adc2_read;
> +	mutex_init(&ctx->chan_lock);
devm_mutex_init()

> +
> +	adev = &ctx->adev;
> +	adev->name = "ads1263_adc2";
> +	adev->id = id;
> +	adev->dev.release = ads1262_aux_device_release;
> +	adev->dev.parent = dev;
> +	device_set_node(&adev->dev, no_free_ptr(node));
> +
> +	ret = auxiliary_device_init(adev);
> +	if (ret)
> +		goto out_free_channels;
> +
> +	ret = auxiliary_device_add(adev);
> +	if (ret) {
> +		auxiliary_device_uninit(adev);
> +		return ret;
> +	}
> +
> +	return devm_add_action_or_reset(dev, ads1262_aux_device_destroy, adev);
> +
> +out_free_channels:
> +	kfree(chans);
> +out_free_id:
> +	ida_free(&ads1262_ida, id);
> +out_free_adc2:
> +	kfree(ctx);
> +out_node_put:
> +	fwnode_handle_put(node);
> +
> +	return ret;
> +}

> diff --git a/drivers/iio/adc/ti-ads1262.h b/drivers/iio/adc/ti-ads1262.h
> new file mode 100644
> index 000000000000..98697d771da3
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1262.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Texas Instruments ADS1262 ADC driver
> + *
> + * Copyright (C) 2025 Kurt Borja <kuurtb@gmail.com>
> + */
> +
> +#ifndef _ADS1262_H_
> +#define _ADS1262_H_
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/types.h>
> +
> +struct ads1263_adc2_channel {

As with earlier patch avoid bitfields where layout matters.
Better to just use defines for the field makss.

> +	/* ADC2CFG */
> +	u8 gain:3;
> +	u8 refmux:3;
> +	u8 data_rate:2;
> +
> +	/* ADC2MUX */
> +	u8 negative_input:4;
> +	u8 positive_input:4;
> +};
> +
> +struct ads1263_adc2_ctx {
> +	struct auxiliary_device adev;
> +	struct ads1262 *chip;
> +	/* Protects channel state */
> +	struct mutex chan_lock;
> +	struct ads1263_adc2_channel *channels;
> +	unsigned int num_channels;
> +	int (*enable)(struct ads1263_adc2_ctx *ctx,
> +		      const struct ads1263_adc2_channel *chan);
> +	int (*start)(struct ads1263_adc2_ctx *ctx);
> +	int (*stop)(struct ads1263_adc2_ctx *ctx);
> +	int (*read)(struct ads1263_adc2_ctx *ctx, __be32 *val);

I'm not sure I see this loose coupling as that useful. I'd just export the
functions from the other module and add them to this header.
Maybe I'm missing why you need this complexity.
> +};
> +
> +#endif
> diff --git a/drivers/iio/adc/ti-ads1263-adc2.c b/drivers/iio/adc/ti-ads1263-adc2.c
> new file mode 100644
> index 000000000000..d21f08bbd9ee
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1263-adc2.c

> +
> +static int ads1263_adc2_read_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan,
> +				 int *val, int *val2, long mask)
> +{
> +	struct ads1263_adc2 *st = iio_priv(indio_dev);
> +	struct ads1263_adc2_ctx *ctx = st->ctx;
> +	struct ads1263_adc2_channel *chan_data;
> +	u8 realbits;
> +	__be32 raw;
> +	u32 cnv;
> +	int ret;
> +
> +	chan_data = &st->ctx->channels[chan->scan_index];
> +	realbits = chan->scan_type.realbits;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ads1263_adc2_channel_read(indio_dev, chan_data, &raw);
> +		if (ret)
> +			return ret;
> +
> +		cnv = be32_to_cpu(raw);
> +		cnv >>= chan->scan_type.shift;

Is that really an unaligned be24?  Might be better to handle it as such.

> +		*val = sign_extend32(cnv, realbits - 1);
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		u64 divd, divr, tmp, rem;
> +
> +		mutex_lock(&ctx->chan_lock);
> +		divd = st->vref_uV;
> +		divr = BIT_ULL(chan_data->gain + realbits - 1) * 1000;
> +		mutex_unlock(&ctx->chan_lock);
> +
> +		tmp = div64_u64(divd * 1000000000ULL, divr);
> +		*val = div64_u64_rem(tmp, 1000000000ULL, &rem);

NANO

> +		*val2 = rem;

> +static int
This is oddly different to formatting of most other functions.
static int ads1263_adc2_read_avail(struct iio_dev *indio_dev,
				   struct iio_chan_spec const *chan,
				   const int **vals, int *type,
				   int *length, long mask)
Seems to be under 80 chars + for this sort of thing, going a bit
over is fine anyway.

> +ads1263_adc2_read_avail(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan, const int **vals,
> +			int *type, int *length, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		*type = IIO_VAL_INT;
> +		*vals = ads1263_adc2_gain_avail;
> +		*length = ARRAY_SIZE(ads1263_adc2_gain_avail);
> +		return IIO_AVAIL_LIST;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*type = IIO_VAL_INT;
> +		*vals = ads1263_adc2_data_rate_avail;
> +		*length = ARRAY_SIZE(ads1263_adc2_data_rate_avail);
> +		return IIO_AVAIL_LIST;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int ads1263_adc2_write_raw(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan,
> +				  int val, int val2, long mask)
> +{
> +	struct ads1263_adc2 *st = iio_priv(indio_dev);
> +	struct ads1263_adc2_ctx *ctx = st->ctx;
> +	struct ads1263_adc2_channel *chan_data;
> +	unsigned int i;
> +
> +	chan_data = &ctx->channels[chan->scan_index];
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		for (i = 0; i < ARRAY_SIZE(ads1263_adc2_gain_avail); i++) {
> +			if (val == ads1263_adc2_gain_avail[i])
> +				break;
> +		}
> +		if (i == ARRAY_SIZE(ads1263_adc2_gain_avail))
> +			return -EINVAL;
> +
> +		mutex_lock(&ctx->chan_lock);
> +		chan_data->gain = i;
> +		mutex_unlock(&ctx->chan_lock);
> +
> +		return 0;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		for (i = 0; i < ARRAY_SIZE(ads1263_adc2_data_rate_avail); i++) {
> +			if (val == ads1263_adc2_data_rate_avail[i])
> +				break;
> +		}
> +		if (i == ARRAY_SIZE(ads1263_adc2_data_rate_avail))
> +			return -EINVAL;
> +
> +		mutex_lock(&ctx->chan_lock);

Slightly nicer to add scope got each cases block and then do just guard() for these

> +		chan_data->data_rate = i;
> +		mutex_unlock(&ctx->chan_lock);
> +
> +		return 0;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ads1263_adc2_channel_hot_reload(st, chan);
> +}

> +
> +static int ads1263_adc2_channels_setup(struct iio_dev *indio_dev)
> +{
> +	struct ads1263_adc2 *st = iio_priv(indio_dev);
> +	struct device *dev = &st->ctx->adev.dev;
> +	struct ads1263_adc2_ctx *ctx = st->ctx;
> +	struct iio_chan_spec *chns;
> +	unsigned int i;
> +
> +	/* Account for the timestamp channel */
> +	chns = devm_kcalloc(dev, ctx->num_channels + 1, sizeof(*chns),
> +			    GFP_KERNEL);
> +	if (!chns)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ctx->num_channels; i++) {
> +		guard(mutex)(&ctx->chan_lock);
> +
> +		ctx->channels[i].refmux = st->refmux;
> +
> +		chns[i] = ads1263_adc2_iio_voltage_template;
Rather than using a template like this I'd just set it all here using
a designated initializer.  Means there is one place to see all the fields.

		chns[i] = (struct iio_chan_spec) {
			.type = IIO_VOLTAGE,
			.indexed = true,
			.differential = true, //not sure why this wasn't in your template.
			.channel = ctx->channels[i].positive_input;
			.channel2 = ctx->channels[i].negative_input;
			.scan_index = i,
			.scan_type = {
				.format = IIO_SCAN_FORMAT_SIGNED_INT,
				.realbits = 24,
				.storagebits = 32,
				.shift = 8,
				.endianness = IIO_BE,
			},
			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
				BIT(IIO_CHAN_INFO_SCALE) |
				BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
				BIT(IIO_CHAN_INFO_SAMP_FREQ),
			.info_mask_shared_by_all_available = 
				BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
				BIT(IIO_CHAN_INFO_SAMP_FREQ),
		}
> +		chns[i].scan_index = i;
> +		chns[i].channel = ctx->channels[i].positive_input;
> +		chns[i].channel2 = ctx->channels[i].negative_input;
> +		chns[i].differential = true;
> +	}
> +
> +	chns[i] = (struct iio_chan_spec)
> +		IIO_CHAN_SOFT_TIMESTAMP(ctx->num_channels - 1);
That macro has recently become a designated intializer so
	chns[i] = IIO_CHAN_SOFT_TIMESTAMP(ctx->num_channels - 1);

> +	chns[i].scan_index = i;

Isn't this just overwriting the ctx->num_channels - 1 we just
passed in above?

> +
> +	indio_dev->num_channels = ctx->num_channels + 1;
> +	indio_dev->channels = chns;
> +
> +	return 0;
> +}

> +
> +static int ads1263_adc2_probe(struct auxiliary_device *auxdev,
> +			      const struct auxiliary_device_id *id)
> +{
> +	struct ads1263_adc2_ctx *ctx =
> +		container_of(auxdev, struct ads1263_adc2_ctx, adev);
> +	struct device *dev = &auxdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct ads1263_adc2 *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->ctx = ctx;
> +	st->indio_dev = indio_dev;
> +
> +	ret = ads1263_adc2_regulator_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = (char *)id->driver_data;

See below.

> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ads1263_adc2_iio_info;
> +	ret = ads1263_adc2_channels_setup(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      ads1263_adc2_trigger_handler,
> +					      &ads1263_adc2_buffer_ops);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct auxiliary_device_id ads1263_adc2_auxiliary_match[] = {
> +	{ .name = "ti_ads1262.ads1263_adc2",
> +	  .driver_data = (kernel_ulong_t)"ads1263_adc2" },
	{
		.name = "ti_ads1262.ads1263_adc2",
	  	.driver_data = (kernel_ulong_t)"ads1263_adc2",

Though I really don't like forcing that cast in there and it should be irrelevant
given there is only one entry in this table.  Should be fine to just hard code that
where used.  If you need this later, wrap it in a structure.

	},

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, ads1263_adc2_auxiliary_match);