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

[Kurt Borja]

On Sat Jun 13, 2026 at 9:10 AM -05, Jonathan Cameron wrote:
> On Fri, 12 Jun 2026 17:46:23 -0500
> Kurt Borja <kuurtb@xxxxxxxxx> 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@xxxxxxxxx>
>
>> 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()

I actually call mutex_destroy() on device .release.

I think it makes more sense that way, otherwise we would UAF?

[...]

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

I'll go for the exported (NS) functions. Much cleaner that way.

I don't know where did I read this callback approach was a way to handle
auxiliary devices and I got fixated on that.

[...]

>> +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),

Sounds good to me.

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

It is. Thanks!

[...]

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

You're right. I'll add a NAME macro.

I don't think we'll ever add entries here.

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

Thanks for your feedback, Jonathan! Apologies if this version was a
little rough... I'm a bit embarrased by the bugs found by Sashiko.

-- 
Thanks,
 ~ Kurt