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

[Piyush Patle]

On Wed, May 20, 2026 at 4:01 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue, 19 May 2026 03:32:27 +0530
> Piyush Patle <piyushpatle228@xxxxxxxxx> 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@xxxxxxxxx>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
>
> 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).
>
Yeah! Since both variants are now fixed at 2 physical channels, IIO_DECLARE_
BUFFER_WITH_TS() adds nothing over the explicit struct.  Will revert:

      struct {
          u32 channel[2];
          aligned_s64 timestamp;
      } buffer;

The hx711_trigger() memset and iio_push_to_buffers_with_timestamp()
call sites will be updated accordingly.
>
> > 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.
>

Agreed the last of Sashiko's points is valid.  In v10, I will update
these 2 thing:
 - Guard the samp_freq update with iio_device_claim_direct_mode() /
    iio_device_release_direct_mode().  This causes write_raw to return
    -EBUSY when a triggered buffer capture is active, which is
    standard IIO semantics for writes that change hardware channel
    configuration

 - Drop the "channel_set = 0" reset.  It is not needed for
   correctness: hx711_set_hx710b_channel() always compares
   extra pulses only when they differ, so the chip will be
   re-configured automatically on the next hx711_reset_read() call.
   dropping it also eliminates the race entirely.

The resulting write_raw case will be:

      case IIO_CHAN_INFO_SAMP_FREQ:
          if (val != 10 && val != 40)
              return -EINVAL;
          ret = iio_device_claim_direct_mode(indio_dev);
          if (ret)
              return ret;
          mutex_lock(&hx711_data->lock);
          hx711_data->samp_freq = val;
          mutex_unlock(&hx711_data->lock);
          iio_device_release_direct_mode(indio_dev);
          return 0;
>
> > +             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@xxxxxxxxxxxxx>");
> > -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");
> >
>