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

[Piyush Patle]

On Mon, Apr 27, 2026 at 8:04 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
>
> 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.
>
Will add a preparatory patch before the HX710B patch that splits
int val = hx711_wait_for_ready(hx711_data) into a plain declaration
followed by a separate assignment.

>
> > +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()
>
Good idea. Will extract the existing gain-switching logic from
hx711_reset_read()
into a hx711_set_hx711_channel() helper in the iio_chan_spec refactor
patch (patch 6), then add hx711_set_hx710b_channel() symmetrically in patch 7.
The current hx710b_set_channel() name will change to hx711_set_hx710b_channel()
to match the naming convention.
> ...
>
> > +     /*
> > +      * 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.
The rest of the driver uses single spaces. Will fix
>
> > +      */
>
> ...
>
> >       /*
> >        * with
>
> While at it, with --> With.
Will fix.
>
> > -      * 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.
>
Yes, agreed.
In v5 I will drop the separate scale field and the
fixed_gain special-casing in hx711_read_raw()
For HX710B, probe will populate gain_scale[] with the same
scale value for all entries based on the fixed gain, and
set gain_set to fixed_gain_val. reusing the existing
hx711_get_gain_to_scale() path without any variant-specific handling.

> ...
>
> > -     { .compatible = "avia,hx711", .data = &hx711_chip },
>
> > +     { .compatible = "avia,hx711",  .data = &hx711_chip  },
>
> Why?! This is stray change.
>
Will drop stray alignment spaces.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>