Re: [PATCH v4 7/7] iio: adc: hx711: add support for HX710B

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Apr 27, 2026 at 03:39:38PM +0530, Piyush Patle wrote:
> Add support for the AVIA HX710B ADC, which shares the HX711 GPIO
> interface but has a fixed gain of 128 and uses trailing PD_SCK pulses
> to select the active channel rather than the gain.
> 
> The HX710B has three operating modes controlled by the trailing pulse
> count after the 24 data bits (Table 3 in the HX710B datasheet):
>   25 pulses (1 trailing): differential input at 10 SPS
>   26 pulses (2 trailing): DVDD-AVDD supply monitor at 40 SPS
>   27 pulses (3 trailing): differential input at 40 SPS
> 
> Model the HX710B with its own hx710b_chan_spec[] and hx710b_iio_info.
> Store the trailing pulse count in chan->address so hx710b_set_channel()
> can switch channels without a separate lookup table. The supply monitor
> uses .channel = 2 to avoid aliasing the channel2 terminal (index 1) of
> the differential pair.
> 
> The HX710B has a dedicated VREF pin for the ADC reference voltage. The
> driver tries vref-supply first; if absent it falls back to avdd-supply
> (for boards where VREF is tied to AVDD). The HX711 uses AVDD as its
> reference and is unaffected.
> 
> Add fixed_gain and fixed_gain_val fields to hx711_chip_info to carry the
> gain into the scale calculation. Store a per-instance scale in
> hx711_data for HX710B and use it in hx711_read_raw() when fixed_gain is
> set. Update hx711_reset() to reset channel_set on HX710B after a
> power-down cycle.
> 
> Enlarge the trigger buffer from 2 to 3 channels plus a pad word to keep
> the timestamp naturally aligned; HX711 continues to use only the first
> two slots.

So, missed types.h should be in this patch.

...

>  /**
>   * struct hx711_chip_info - per-variant static configuration
> - * @name:         IIO device name
> - * @channels:     channel specification array
> - * @iio_info:     IIO info ops for this variant
> - * @num_channels: number of entries in @channels
> + * @name:          IIO device name
> + * @channels:      channel specification array
> + * @iio_info:      IIO info ops for this variant
> + * @num_channels:  number of entries in @channels
> + * @fixed_gain_val: fixed PGA gain (used when @fixed_gain is true)
> + * @fixed_gain:    true if the variant has a fixed ADC gain (e.g. HX710B)

No, make sure there will be no '-' lines here.

>   */

...

>  struct hx711_data {

>  	int			gain_set;	/* gain set on device */
>  	int			gain_chan_a;	/* gain for channel A */
>  	int			gain_scale[HX711_GAIN_MAX];

> +	unsigned int		channel_set;	/* HX710B active channel */
>  	const struct hx711_chip_info	*chip_info;
> +	unsigned int		scale;		/* HX710B fixed-gain scale */

Semantically the scale is closer to the above gain_* ones AFAICS.

>  	struct mutex		lock;
>  	/*
>  	 * triggered buffer
> -	 * 2x32-bit channel + 64-bit naturally aligned timestamp
> +	 * up to 3x32-bit channels + pad + 64-bit naturally aligned timestamp
>  	 */
>  	struct {
> -		u32 channel[2];
> +		u32 channel[3];
> +		u32 pad;
>  		aligned_s64 timestamp;
>  	} buffer;
>  	/*

...

>  static int hx711_reset(struct hx711_data *hx711_data)
>  {
> +	const struct hx711_chip_info *info = hx711_data->chip_info;
>  	int val = hx711_wait_for_ready(hx711_data);
>  
>  	if (val) {

The rule of thumb is to split definition and assignment for the variables that
are going to be validated.

Here I expect to see

	int val;

	val = hx711_wait_for_ready(hx711_data);
	if (val) {

But since it's not related directly to this change, make a preparatory patch.
Because now with two variables this looks worse and adds more potential to be
mistaken in the future.


> +static int hx710b_set_channel(struct hx711_data *hx711_data,
> +			      const struct iio_chan_spec *chan)
> +{
> +	int ret;
> +
> +	if (hx711_data->channel_set == (unsigned int)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;
> +}

...

> -	ret = hx711_set_gain_for_channel(hx711_data, chan->channel);
> -	if (ret < 0)
> -		return ret;

> +		ret = hx711_set_gain_for_channel(hx711_data, chan->channel);
> +		if (ret < 0)
> +			return ret;
> +		trailing_pulses = hx711_get_gain_to_pulse(hx711_data->gain_set);
> +	}
>  
> -	trailing_pulses = hx711_get_gain_to_pulse(hx711_data->gain_set);
>  	return hx711_read(hx711_data, trailing_pulses);

Instead of the above, first split these lines to a helper

static int hx711_set_hx711_channel(..., *tp)
{
	int ret;

	ret = hx711_set_gain_for_channel(hx711_data, chan->channel);
	if (ret < 0)
		return ret;

	*tp = hx711_get_gain_to_pulse(hx711_data->gain_set);
	return 0;
}

And name your new function something like

hx711_set_hx710b_channel()

...

> +	/*
> +	 * The HX710B uses the VREF pin as the ADC reference; try vref-supply
> +	 * first and fall back to avdd-supply when VREF is tied to AVDD on the
> +	 * board.  The HX711 uses AVDD as its reference.

I think we use a single space in the comment, but double check that.
The rule of thumb is to keep the original driver style and change it
separately if required.

> +	 */

...

>  	/*
>  	 * with

While at it, with --> With.

> -	 * full scale differential input range: AVDD / GAIN
> +	 * full scale differential input range: VREF / GAIN
>  	 * full scale output data: 2^24
>  	 * we can say:
> -	 *     AVDD / GAIN = 2^24
> +	 *     VREF / GAIN = 2^24
>  	 * therefore:
> -	 *     1 LSB = AVDD / GAIN / 2^24
> -	 * AVDD is in uV, but we need 10^-9 mV
> +	 *     1 LSB = VREF / GAIN / 2^24
> +	 * VREF is in uV, but we need 10^-9 mV
>  	 * approximately to fit into a 32 bit number:
> -	 * 1 LSB = (AVDD * 100) / GAIN / 1678 [10^-9 mV]
> +	 * 1 LSB = (VREF * 100) / GAIN / 1678 [10^-9 mV]
>  	 */

...

> -	for (i = 0; i < HX711_GAIN_MAX; i++)
> -		hx711_data->gain_scale[i] =
> -			ret / hx711_gain_to_scale[i].gain / 1678;
> +	if (chip_info->fixed_gain) {
> +		hx711_data->scale = ret / chip_info->fixed_gain_val / 1678;
> +	} else {
> +		for (i = 0; i < HX711_GAIN_MAX; i++)
> +			hx711_data->gain_scale[i] =
> +				ret / hx711_gain_to_scale[i].gain / 1678;
>  
> -	hx711_data->gain_set = 128;
> -	hx711_data->gain_chan_a = 128;
> +		hx711_data->gain_set = 128;
> +		hx711_data->gain_chan_a = 128;
> +	}

Looking at this I'm wondering wouldn't be easier just to have the array, but
feel it with the same data.

...

> -	{ .compatible = "avia,hx711", .data = &hx711_chip },

> +	{ .compatible = "avia,hx711",  .data = &hx711_chip  },

Why?! This is stray change.


-- 
With Best Regards,
Andy Shevchenko