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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 19 May 2026 03:32:27 +0530
Piyush Patle <piyushpatle228@gmail.com> wrote:

> Add support for the AVIA HX710B ADC, which shares the HX711 GPIO
> interface but uses trailing PD_SCK pulses to select the active mode.
> 
> Model the HX710B with variant-specific channel tables and IIO info,
> track the active channel across conversions, and use the fixed gain
> value when computing scale.
> 
> Also update the adjacent Kconfig text, file header, and module
> description so the driver text matches the newly supported variant.
> 
> Signed-off-by: Piyush Patle <piyushpatle228@gmail.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

The vast majority of sashiko feedback on this version is incorrect
or already something we've ruled out as needing handling.

However, very last point looks valid to me so I've highlighted that.

Otherwise, the main thing is it is better to keep the structure
for the buffer now we don't have variable numbers of channels between
the two devices.  That is the preferred route except when it becomes
misleading (which it did with 2 vs 3 channels).


> diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
> index 183568196d52..d5c977b4669b 100644
> --- a/drivers/iio/adc/hx711.c
> +++ b/drivers/iio/adc/hx711.c

>  
>  struct hx711_data {
> @@ -99,16 +105,12 @@ 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 */
> +	unsigned int		samp_freq;	/* HX710B differential channel sample rate */
>  	const struct hx711_chip_info	*chip_info;
>  	struct mutex		lock;
> -	/*
> -	 * triggered buffer
> -	 * 2x32-bit channel + 64-bit naturally aligned timestamp
> -	 */
> -	struct {
> -		u32 channel[2];
> -		aligned_s64 timestamp;
> -	} buffer;
> +	/* 2x32-bit channels + 64-bit naturally aligned timestamp */
> +	IIO_DECLARE_BUFFER_WITH_TS(u32, buffer, 2);

Now we are back to fixed 2 channels, don't need this change. The structure
is easier to interpret so please go back to that.

>  	/*
>  	 * delay after a rising edge on SCK until the data is ready DOUT
>  	 * this is dependent on the hx711 where the datasheet tells a


> @@ -463,6 +527,50 @@ static const struct iio_info hx711_iio_info = {
>  	.attrs			= &hx711_attribute_group,
>  };
>  
> +static const int hx710b_samp_freq_avail[] = { 10, 40 };
> +
> +static int hx710b_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = hx710b_samp_freq_avail;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(hx710b_samp_freq_avail);
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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);
From Sahiko:

  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.

Two possible fixes:
- The one sashiko suggests around claiming direct mode.
- Maybe not set channel_set = 0?  Then it becomes a simple race for
  whether the value of samp_freq is updated or not.  Either is harmless.

I'd be tempted to go with direct mode claiming but also consider if you can
drop that channel_set = 0 - or add a comment on why it's there perhaps.


> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info hx710b_iio_info = {
> +	.read_raw		= hx711_read_raw,
> +	.write_raw		= hx710b_write_raw,
> +	.read_avail		= hx710b_read_avail,
> +};

>  static const struct hx711_chip_info hx711_chip = {
>  	.name		= "hx711",
>  	.channels	= hx711_chan_spec,
> @@ -502,6 +655,15 @@ static const struct hx711_chip_info hx711_chip = {
>  	.num_channels	= ARRAY_SIZE(hx711_chan_spec),
>  };

> @@ -608,6 +781,7 @@ static int hx711_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id of_hx711_match[] = {
> +	{ .compatible = "avia,hx710b", .data = &hx710b_chip },
>  	{ .compatible = "avia,hx711", .data = &hx711_chip },
>  	{ }
>  };
> @@ -625,7 +799,7 @@ static struct platform_driver hx711_driver = {
>  module_platform_driver(hx711_driver);
>  
>  MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
> -MODULE_DESCRIPTION("HX711 bitbanging driver - ADC for weight cells");
> +MODULE_DESCRIPTION("HX711 and compatible bitbanging ADC driver");

Trivial but switch that to 'and similar' as they aren't quite compatible.
Sometimes vagueness is helpful :)

Anyhow, looking in pretty good shape so hopefully v10 is the lucky version.

If you have time, it would be nice to get rid of the custom available attributes
for the hx711 as well - similar approach to you have done for the new part.


Jonathan


>  MODULE_LICENSE("GPL");
>  MODULE_ALIAS("platform:hx711-gpio");
>