Re: [PATCH v3 8/8] iio: adc: ti-ads112c14: add measurement channel support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The parsed `burn-out-current-nanoamp` Devicetree property is never applied to the hardware registers.

Pre-existing issues:
- [High] Short I2C block reads leak uninitialized stack memory to userspace.
- [High] Casting an `s64` array to `const int *` for `IIO_AVAIL_LIST` produces corrupted values on big-endian architectures.
--

--- Patch [8]: [PATCH v3 8/8] iio: adc: ti-ads112c14: add measurement channel support ---
commit d188e14a67542447de91b1747691ed2a77615e7a
Author: David Lechner (TI) <dlechner@baylibre.com>

iio: adc: ti-ads112c14: add measurement channel support

Add support for parsing devicetree properties for measurement channels
and doing direct reads on these.

> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> index 2ce4411a0d86f..a310abd69d8fa 100644
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c

[ ... ]

> @@ -257,12 +302,113 @@ static const struct regmap_config ads112c14_regmap_config = {

[ ... ]

> +static int ads112c14_prepare_measurement_channel(struct ads112c14_data *data,
> +						 const struct iio_chan_spec *chan)
> +{
> +	struct ads112c14_measurement *measurement = &data->measurements[chan->scan_index];
> +	u32 refp_buf_en, refn_buf_en, ref_val, ref_sel;
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, ADS112C14_REG_MUX_CFG,

[ ... ]

> +	ret = regmap_update_bits(data->regmap, ADS112C14_REG_DATA_RATE_CFG,
> +				 ADS112C14_DATA_RATE_CFG_GC_EN,
> +				 FIELD_PREP(ADS112C14_DATA_RATE_CFG_GC_EN,
> +					    measurement->global_chop));
> +	if (ret)
> +		return ret;
> +

[Severity: Medium]
Is the burn-out current setting being applied to the hardware?

In ads112c14_parse_channels(), the burn-out-current-nanoamp property is
parsed and stored in measurement->burnout. However, it does not appear
to be applied to the ADS112C14_REG_DEVICE_CFG register during this channel
configuration step, which might mean the feature remains unconfigured.

> +	refp_buf_en = !data->refp_is_avdd &&
> +		      measurement->vref_source == ADS112C14_VREF_SOURCE_EXTERNAL;
> +	refn_buf_en = !data->refn_is_gnd &&

[ ... ]

> @@ -386,7 +534,8 @@ static int ads112c14_read_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  
> -		*val = sign_extend32(*val, fsr_bits);
> +		if (!measurement || measurement->bipolar)
> +			*val = sign_extend32(*val, fsr_bits);
>  
>  		return IIO_VAL_INT;
>  	}

[Severity: High]
This is a pre-existing issue, but does this sequence safely handle short I2C
block reads?

In ads112c14_read_raw(), an uninitialized buffer is declared on the stack
and passed for conversion:

ads112c14_read_raw()
    ...
    u8 buf[3];
    ...
    ret = ads112c14_single_conversion(data, chan, buf);

Inside ads112c14_single_conversion(), the read is performed:

    return i2c_smbus_read_i2c_block_data(client, ..., buf);

If the block data read returns a positive value less than the requested
number of bytes, checking ret < 0 will not catch the error. The partially
uninitialized stack memory in buf could then be parsed using
get_unaligned_be16() or get_unaligned_be24() and returned to userspace.

[ ... ]

> @@ -436,14 +592,29 @@ static int ads112c14_read_avail(struct iio_dev *indio_dev,
>  {
>  	struct ads112c14_data *data = iio_priv(indio_dev);
>  
> -	if (chan->channel == ADS112C14_SYS_MON_CHANNEL_SHORT) {
> -		*vals = (const int *)data->sys_mon_chan_short_scale_available;
> -		*length = 2 * ARRAY_SIZE(data->sys_mon_chan_short_scale_available);
> -		*type = IIO_VAL_DECIMAL64_PICO;
> -		return IIO_AVAIL_LIST;
> -	}
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->channel < ADS112C14_SYS_MON_CHANNEL_BASE) {
> +			struct ads112c14_measurement *measurement;
> +
> +			measurement = &data->measurements[chan->scan_index];
> +			*vals = (const int *)measurement->scale_available;

[Severity: High]
This is a pre-existing issue, but will casting an s64 array to a const int *
cause incorrect scale values on big-endian architectures?

The IIO core reads this list as pairs of 32-bit integers, combining them
into an s64 by using the first integer as the lower 32 bits and the second
as the upper 32 bits.

On big-endian systems, the upper 32 bits of the s64 are stored first in
memory, meaning the halves could be swapped when read by the IIO core.

> +			*length = 2 * ARRAY_SIZE(measurement->scale_available);
> +			*type = IIO_VAL_DECIMAL64_PICO;
> +			return IIO_AVAIL_LIST;
> +		}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-iio-adc-ti-ads122c14-v3-0-746d52cbf1d0@baylibre.com?part=8