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

[Jonathan Cameron]

On Wed, 29 Apr 2026 22:06:34 +0300
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:

> On Wed, Apr 29, 2026 at 11:15:44AM +0530, Piyush Patle 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.  
> 
> ...
A few follow up responses inline.

> 
> >  /*
> > - * HX711: analog to digital converter for weight sensor module
> > + * HX711 and compatible ADCs driver for weight sensor modules  
> 
> As noticed in previous round, shouldn't we fix weight --> weigh?
Just for clarity on this following are correct (I think)

Weight sensor
Weigh cell

Not that I care that much give the meaning is clear for all combinations.

> 

> In accordance with the kernel-doc, name the variable "has_fixed_gain".
> 
> ...
> 
> >  	/*
> >  	 * 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;  
> 
> I think I still doesn't follow if this kind of changes affects ABI or not.
> Jonathan, is it usually okay to inject more entries in this structure?

Good point.
Using an explicit structure here is now misleading.  The timestamp
will move if only two channels are enabled vs all 3 enabled.

So this should change to instead use IIO_DECLARE_BUFFER_WITH_TS()


Also unless this driver explicitly writes to pad that shouldn't be there. The
aligned_s64 will ensure the padding is there without it and generally in 
IIO we rely on that rather that adding padding by hand.


> 
> ...
> 
> > +static int hx711_set_hx710b_channel(struct hx711_data *hx711_data,
> > +				    const struct iio_chan_spec *chan)
> > +{
> > +	int ret;
> > +
> > +	if (hx711_data->channel_set == (unsigned int)chan->channel)  
> 
> Why do we need explicit casting?
> 
> > +		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;
> > +}  
>