Re: [PATCH v7 11/11] iio: adc: hx711: add support for HX710B

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The HX710B differential input is incorrectly exposed as two separate IIO channels based on its sampling rate.
- [High] Iterating over the software timestamp channel in `hx711_trigger` causes an out-of-bounds array write and executes invalid hardware reads that corrupt the ADC state.
- [High] The driver fails to update `channel_set` when `hx711_wait_for_ready()` fails after a successful `hx711_read()`, leading to desynchronization between software and hardware states.
- [High] The trigger handler `hx711_trigger` pushes negative error codes to the IIO buffer as valid data if `hx711_reset_read` fails.
--

commit 6e9823cf49ecd30f482dc404287cf0c66583e3dc
Author: Piyush Patle <piyushpatle228@gmail.com>
Date:   Sun May 10 20:26:55 2026 +0000

    iio: adc: hx711: add support for HX710B

Add support for the AVIA HX710B ADC to the existing hx711 driver.
The patch models the HX710B with variant-specific channel tables
and tracks the active channel across conversions.

> diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
> index a0e73aa475de0..fece0dc81058a 100644
> --- a/drivers/iio/adc/hx711.c
> +++ b/drivers/iio/adc/hx711.c
[ ... ]
> @@ -99,14 +104,22 @@ struct hx711_data {
>  	int			gain_set;	/* gain set on device */
>  	int			gain_chan_a;	/* gain for channel A */
>  	int			gain_scale[HX711_GAIN_MAX];
> +	int			channel_set;	/* HX710B active channel */
>  	const struct hx711_chip_info	*chip_info;
>  	struct mutex		lock;
>  	/*
>  	 * triggered buffer
> -	 * 2x32-bit channel + 64-bit naturally aligned timestamp
> +	 * up to 3x32-bit channels + 64-bit naturally aligned timestamp
> +	 *
> +	 * aligned_s64 satisfies the 8-byte alignment requirement for the
> +	 * timestamp. For HX711 (at most 2 active channels), iio_push_to_
> +	 * buffers_with_timestamp() places the timestamp at offset 8
> +	 * (scan_bytes=8, already 8-byte aligned), identical to the original
> +	 * 2-channel layout. The extra channel slot for HX710B does not affect
> +	 * the HX711 ABI.
>  	 */
>  	struct {
> -		u32 channel[2];
> +		u32 channel[3];
>  		aligned_s64 timestamp;
>  	} buffer;

Does iterating over the software timestamp channel in hx711_trigger() cause
an out-of-bounds array write?

If all 3 physical channels and the timestamp are enabled, the iteration over
active channels in hx711_trigger() will execute 4 times. Since buffer.channel
only has 3 elements, won't this write past the end of the array on the 4th
iteration?

Also, when hx711_reset_read() is called for the timestamp channel,
chan->address is implicitly 0. hx711_set_hx710b_channel() then calls
hx711_read() which sends exactly 24 pulses to the hardware. The HX710B
requires 25-27 pulses. Could sending 24 pulses leave the hardware in an
undefined state where DOUT never goes low, causing the subsequent
hx711_wait_for_ready() to block the interrupt thread?

[ ... ]
> @@ -283,9 +300,36 @@ static int hx711_set_hx711_channel(struct hx711_data *hx711_data,
>  	return 0;
>  }
>  
> +/*
> + * Switch the HX710B to the requested channel for the next conversion.
> + * chan->address holds the trailing pulse count (Table 3 in datasheet).
> + * channel_set is updated only after both reads succeed.
> + */
> +static int hx711_set_hx710b_channel(struct hx711_data *hx711_data,
> +				    const struct iio_chan_spec *chan)
> +{
> +	int ret;
> +
> +	if (hx711_data->channel_set == chan->channel)
> +		return 0;
> +
> +	ret = hx711_read(hx711_data, chan->address);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hx711_wait_for_ready(hx711_data);
> +	if (ret)
> +		return ret;
> +
> +	hx711_data->channel_set = chan->channel;
> +
> +	return 0;
> +}

Could this lead to a desynchronization between software and hardware states
if hx711_wait_for_ready() fails?

If hx711_read() succeeds, the hardware state is immediately updated for the
new channel. But if the subsequent hx711_wait_for_ready() times out, the
function returns without updating hx711_data->channel_set.

If the next trigger event is delayed enough for the device to become ready,
hx711_reset() would skip the hardware reset sequence. When attempting to read
the old channel, it seems the driver would see channel_set == chan->channel,
skip sending the configuration pulses, and incorrectly read data from the new
channel instead.

[ ... ]
>  static int hx711_reset_read(struct hx711_data *hx711_data,
>  			    const struct iio_chan_spec *chan)
>  {
> +	const struct hx711_chip_info *info = hx711_data->chip_info;
>  	int trailing_pulses;
>  	int ret;
>  
> @@ -298,9 +342,16 @@ static int hx711_reset_read(struct hx711_data *hx711_data,
>  		return -EIO;
>  	}
>  
> -	ret = hx711_set_hx711_channel(hx711_data, chan, &trailing_pulses);
> -	if (ret < 0)
> -		return ret;
> +	if (info->has_fixed_gain) {
> +		ret = hx711_set_hx710b_channel(hx711_data, chan);
> +		if (ret < 0)
> +			return ret;
> +		trailing_pulses = chan->address;
> +	} else {
> +		ret = hx711_set_hx711_channel(hx711_data, chan, &trailing_pulses);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	return hx711_read(hx711_data, trailing_pulses);
>  }

This isn't a regression introduced by this patch, but does hx711_trigger()
push negative error codes to the IIO buffer as valid data if
hx711_reset_read() fails?

hx711_reset_read() can return negative error codes like -EIO. Are these
return values assigned directly to the u32 array elements in
hx711_data->buffer.channel?

It appears these error codes would be implicitly cast to large unsigned
integers and pushed to userspace via iio_push_to_buffers_with_timestamp()
without validation.

[ ... ]
> +/*
> + * HX710B channels (Table 3 in datasheet).
> + * 25 pulses (1 trailing): differential input, 10 SPS -> channel 0
> + * 26 pulses (2 trailing): DVDD-AVDD supply monitor, 40 SPS -> channel 2
> + * 27 pulses (3 trailing): differential input, 40 SPS -> channel 3
> + * .address stores the trailing pulse count for hx711_set_hx710b_channel().
> + * Channel 2 is used for the supply monitor to avoid aliasing the
> + * channel2 terminal of the first differential pair.
> + */
> +static const struct iio_chan_spec hx710b_chan_spec[] = {
> +	{
> +		.type = IIO_VOLTAGE,
> +		.differential = 1,
> +		.channel = 0,
> +		.channel2 = 1,
> +		.indexed = 1,
> +		.address = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 24,
> +			.storagebits = 32,
> +			.endianness = IIO_CPU,
> +		},
> +	},
[ ... ]
> +	{
> +		.type = IIO_VOLTAGE,
> +		.differential = 1,
> +		.channel = 3,
> +		.channel2 = 4,
> +		.indexed = 1,
> +		.address = 3,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 2,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 24,
> +			.storagebits = 32,
> +			.endianness = IIO_CPU,
> +		},
> +	},

Is it correct to expose the HX710B differential input as two separate IIO
channels based on its sampling rate?

Both the 10 SPS mode (channel 0) and the 40 SPS mode (channel 3) appear to
measure the exact same differential input. Should different sampling frequencies
for the same physical input be exposed via the IIO_CHAN_INFO_SAMP_FREQ
attribute on a single channel instead of duplicating the channel?

Exposing them as separate channels could allow userspace to enable both in a
single triggered buffer scan, which might force the driver to toggle the
hardware state and read the same input twice per scan.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510194947.31997-1-piyushpatle228@gmail.com?part=11