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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Probe silently succeeds with corrupted state when `ti,refmux` property is out of bounds.
- [High] Unportable C bitfields used for hardware registers break big-endian architectures.
- [High] Missing `select AUXILIARY_BUS` in Kconfig causes a build failure.
- [High] Heap out-of-bounds access if only the timestamp channel is enabled in buffer mode.
- [Medium] Dead code in `ads1263_adc2_write_raw()` silently prevents hardware updates on the fly (hot reload).
- [Medium] Permanent `fwnode_handle` resource leak on the error path of `ads1262_aux_device_setup()`.
- [Low] Semantic API misuse: releasing direct mode lock instead of buffer mode lock.
--

commit 6e165e44885d4dd5616f87d79875344d0f1cd3cb
Author: Kurt Borja <kuurtb@gmail.com>

iio: adc: Add ti-ads1263-adc2 driver

The TI ADS1263 includes an auxiliary, 24-bit, delta-sigma ADC (ADC2) which
operates independently of ADC1. This patch adds support for ADC2 as an
independent IIO device using the auxiliary bus API.

> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1809,6 +1809,19 @@ config TI_ADS1262
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-ads1262.
>  
> +config TI_ADS1263_ADC2
> +	tristate "Texas Instruments ADS1263 auxiliary ADC (ADC2) driver"
> +	depends on TI_ADS1262
> +	select AUXILIARY_BUS

[Severity: High]
Since ti-ads1262.c unconditionally calls auxiliary bus APIs like
auxiliary_device_init(), does config TI_ADS1262 also need to
select AUXILIARY_BUS? 

If CONFIG_TI_ADS1262=y and CONFIG_TI_ADS1263_ADC2=n, the auxiliary bus code
won't be compiled, which could result in undefined reference linker errors
when building the main driver.

[ ... ]
> --- /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 {
> +	/* ADC2CFG */
> +	u8 gain:3;
> +	u8 refmux:3;
> +	u8 data_rate:2;
> +
> +	/* ADC2MUX */
> +	u8 negative_input:4;
> +	u8 positive_input:4;
> +};

[Severity: High]
Will this structure layout cause issues on big-endian architectures? 

The memory layout of C bitfields is implementation-defined, and this
structure appears to be written directly to the SPI hardware using
regmap_bulk_write() in ads1263_adc2_enable(). On big-endian systems, the
bits could be packed in reverse order and write invalid configuration data.

[ ... ]
> --- a/drivers/iio/adc/ti-ads1262.c
> +++ b/drivers/iio/adc/ti-ads1262.c
[ ... ]
> +static int ads1262_aux_device_setup(struct ads1262 *st)
> +{
[ ... ]
> +	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);

[Severity: Medium]
Does this error path leak the fwnode reference? 

When device_set_node() is called above, no_free_ptr(node) unconditionally
sets the local node pointer to NULL. If auxiliary_device_init() later fails,
jumping to out_node_put will call fwnode_handle_put(NULL). This becomes a
no-op, leaving the reference permanently leaked.

[ ... ]
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1263-adc2.c
[ ... ]
> +static int ads1263_adc2_channel_hot_reload(struct ads1263_adc2 *st,
> +					   const struct iio_chan_spec *chan)
> +{
> +	struct ads1263_adc2_ctx *ctx = st->ctx;
> +	unsigned long i;
> +	int ret;
> +
> +	/* Hot reloading is only required on buffer mode */
> +	if (!iio_device_try_claim_buffer_mode(st->indio_dev))
> +		return 0;
> +
> +	i = find_first_bit(st->indio_dev->active_scan_mask,
> +			   iio_get_masklength(st->indio_dev));
> +	if (i != chan->scan_index) {
> +		iio_device_release_direct(st->indio_dev);

[Severity: Low]
Should this use iio_device_release_buffer_mode() instead of
iio_device_release_direct()? 

While both macros currently expand to the same internal unlock function,
this breaks the IIO API contract which expects symmetric lock and unlock
function pairs.

> +		return 0;
> +	}
[ ... ]
> +static int ads1263_adc2_write_raw(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan,
> +				  int val, int val2, long mask)
> +{
[ ... ]
> +	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;

[Severity: Medium]
Will ads1263_adc2_channel_hot_reload() at the end of this function ever be
reached? 

Both the IIO_CHAN_INFO_HARDWAREGAIN and IIO_CHAN_INFO_SAMP_FREQ cases use
a return 0 instead of break after updating the software state. This causes
the function to exit immediately, which silently prevents hardware updates
during an active buffer mode.

> +
> +	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);
> +		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_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ads1263_adc2 *st = iio_priv(indio_dev);
> +	struct ads1263_adc2_ctx *ctx = st->ctx;
> +	struct device *dev = &ctx->adev.dev;
> +	unsigned long i;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev->parent);
> +	if (ret)
> +		return ret;
> +
> +	i = find_first_bit(indio_dev->active_scan_mask,
> +			   iio_get_masklength(indio_dev));
> +	ret = ctx->enable(ctx, &ctx->channels[i]);

[Severity: High]
Is there a potential out-of-bounds array access here if only the timestamp
channel is enabled?

The iio_validate_scan_mask_onehot function allows a single channel to be
active. If the user only enables the timestamp channel, find_first_bit()
will return the timestamp channel's index, which is ctx->num_channels.

Since ctx->channels is allocated with exactly num_channels elements, accessing
ctx->channels[num_channels] reads out-of-bounds memory and passes that
data to the hardware via ctx->enable().

> +	if (ret)
> +		goto out_runtime_autosuspend;
> +
> +	ret = ctx->start(ctx);
> +	if (ret)
> +		goto out_runtime_autosuspend;
> +
> +	return 0;
[ ... ]
> +static int ads1263_adc2_regulator_setup(struct ads1263_adc2 *st)
> +{
> +	struct device *dev = &st->ctx->adev.dev;
> +	const char *reg_id, *propname;
> +	u32 refmux = 0;
> +	int ret;
> +
> +	propname = "ti,refmux";
> +	ret = device_property_read_u32(dev, propname, &refmux);
> +	if (refmux >= ADS1263_ADC2_REF2_COUNT)
> +		return dev_err_probe(dev, ret, "%s out of range\n", propname);

[Severity: High]
Does this silently succeed on failure? 

If the ti,refmux value exceeds ADS1263_ADC2_REF2_COUNT, the preceding
device_property_read_u32() was successful, meaning ret is 0. 

Because ret is 0, dev_err_probe() will return 0, causing the probe to
incorrectly succeed with an uninitialized vref_uV value. This could lead
to a divide-by-zero during scale calculations.

> +	st->refmux = refmux;
> +
> +	if (refmux == ADS1263_ADC2_REF2_INTER) {
> +		/* The internal voltage reference is 2.5 V */
> +		st->vref_uV = 2500000;
> +		return 0;
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-ads126x-v1-0-894c788d03ed@gmail.com?part=5