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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The trigger handler incorrectly performs physical ADC reads for the software timestamp channel.
- [Medium] Missing `iio_device_claim_direct_mode()` protection in the newly added `hx710b_write_raw()`.
- [Low] Data race on `samp_freq` when handling `IIO_CHAN_INFO_SAMP_FREQ` reads.
--

commit 63c1473f6788341d410e6d9c03f632bdcaeaf6df
Author: Piyush Patle <piyushpatle228@gmail.com>

iio: adc: hx711: add support for HX710B

This commit adds support for the AVIA HX710B ADC to the existing hx711
driver. It handles the different trailing pulse requirements and fixed
gain scaling, while exposing both 10 and 40 SPS sample rates.

> diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
> index 8ec4dee1f451e..20ea667af7254 100644
> --- a/drivers/iio/adc/hx711.c
> +++ b/drivers/iio/adc/hx711.c
[ ... ]
> @@ -323,6 +378,9 @@ static int hx711_read_raw(struct iio_dev *indio_dev,
>  		if (*val < 0)
>  			return *val;
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = hx711_data->samp_freq;
> +		return IIO_VAL_INT;

Is there a potential data race here? 

In hx710b_write_raw(), hx711_data->samp_freq is updated while holding
hx711_data->lock. Should this read also be protected by the same mutex to
ensure state consistency?

[ ... ]
> @@ -400,20 +458,26 @@ static irqreturn_t hx711_trigger(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct hx711_data *hx711_data = iio_priv(indio_dev);
>  	int i, j = 0;
> +	int ret;
>  
>  	mutex_lock(&hx711_data->lock);
>  
> -	memset(&hx711_data->buffer, 0, sizeof(hx711_data->buffer));
> +	memset(hx711_data->buffer, 0, sizeof(hx711_data->buffer));
>  
>  	iio_for_each_active_channel(indio_dev, i) {
> -		hx711_data->buffer.channel[j] =
> -			hx711_reset_read(hx711_data, &indio_dev->channels[i]);
> -		j++;
> +		ret = hx711_reset_read(hx711_data, &indio_dev->channels[i]);

Does this loop incorrectly perform a physical ADC read for the software
timestamp channel when userspace enables it?

When the timestamp channel is enabled, its bit is set in active_scan_mask.
The iio_for_each_active_channel() macro will iterate over it, causing
hx711_reset_read() to execute for the timestamp channel.

For the HX710B variant, chan->differential evaluates to 0 for the timestamp
channel, which causes hx710b_trailing_pulses() to return 2 trailing pulses,
performing an unintentional read over the supply monitor channel.

For the HX711 variant, the timestamp channel sets chan->channel = -1. 
hx711_set_gain_for_channel() then falls into the else branch (since -1 != 0),
changing the hardware to channel B and performing a dummy physical read.

In both cases, this forces the driver to wait for the ADC to settle on every
trigger event, which can severely impact the sampling rate. Should this loop
skip the timestamp channel (for example, by checking if the channel type is
IIO_TIMESTAMP)?

[ ... ]
> +static int hx710b_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct hx711_data *hx711_data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (val != 10 && val != 40)
> +			return -EINVAL;
> +		mutex_lock(&hx711_data->lock);
> +		hx711_data->samp_freq = val;
> +		hx711_data->channel_set = 0;
> +		mutex_unlock(&hx711_data->lock);
> +		return 0;

Does this implementation need to use iio_device_claim_direct_mode() to
prevent concurrent hardware changes while a buffered capture is active?

If userspace modifies the sampling frequency during an active IIO triggered
buffer capture, it resets channel_set to 0. This could alter the hardware
configuration (changing the trailing pulses) out from under the IIO capture
thread, which violates IIO concurrency semantics.

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