Re: [PATCH 2/2] iio: adc: add support for pac1921

[Jonathan Cameron]

On Mon, 08 Jul 2024 15:45:50 +0200
Matteo Martelli <matteomartelli3@xxxxxxxxx> wrote:

> Marius.Cristea@ wrote:
> > 
> > 
> >   Hi Matteo,
> > 
> >    Thank you very much for helping us adding PAC1921 support. I'm also
> > developing a similar driver and I could share the code with you to make
> > the driver better.
> > 
> >    Also I have a few review comments, please, see bellow:
> > 
> > Best regards,
> > Marius  
> 
> Hi Marius,
> 
> Thanks for your feedback, this is indeed very helpful, as it would be if you
> shared your code.
> 
> I addressed some of your comments below in patch v2 thread, replying to
> Jonathan [1]. If you have more comments about those points please reply on that
> thread.
> 
> [1]: https://lore.kernel.org/linux-iio/668bec2a8b23a_6e037017@njaxe.notmuch
> 
> See also my comments below.
> 
> > On Wed, Jul 03, 2024 at 03:34:34PM +0200, Matteo Martelli wrote:
> > 
> >   
> > > +
> > > +/* pac1921 regmap configuration */
> > > +static const struct regmap_range pac1921_regmap_wr_ranges[] = {
> > > +       regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
> > > +};
> > > +static const struct regmap_access_table pac1921_regmap_wr_table = {
> > > +       .yes_ranges = pac1921_regmap_wr_ranges,
> > > +       .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_wr_ranges),
> > > +};
> > > +static const struct regmap_range pac1921_regmap_rd_ranges[] = {
> > > +       regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
> > > +       regmap_reg_range(PAC1921_REG_VBUS, PAC1921_REG_VPOWER + 1),
> > > +};
> > > +static const struct regmap_access_table pac1921_regmap_rd_table = {
> > > +       .yes_ranges = pac1921_regmap_rd_ranges,
> > > +       .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_rd_ranges),
> > > +};  
> > I know that the regmap is the way forward, but the PAC devices are more
> > efficient if bulk read is done.
> >  
> I think performances depends on the use case, for example if the user is
> interested in only one type of measurement I don't think reading all registers
> result in a performance gain. Also consider that there are 8 registers of
> accumulators that are not currently exposed by the driver but that would be read
> anyway, so I am not sure it would result in a performance gain either. I would
> leave it as it is right now to keep the implementation simple and maybe extend
> it in the future in case performance gains become obvious and necessary for most
> use cases. Also consider that regmap supports bulk reads.

Usual trick with this stuff is to apply some heuristics ideally based on
what is reasonable on a test platform.  If we are going to read x% of registers
then bulk read them all, if it's less than %x do separate reads.

However, it does add maintenance burden, so there has to be a significant
advantage. Many userspace apps want all or almost all the data though
so we often optimise for that at the cost of reading single channels
(through the buffer interface anyway).

> >
> > Also when you are reading all values at
> > the same time, the Voltage, Current and the Power numbers will came
> > from the same sampling time and they will be correlated to each other.
> >   
> I think that there is no guarantee that the user would always request two
> successive raw readings, like Voltage and Current, within the same integration
> period. So for the second reading all registers may be read again and the two
> values might not be correlated as well. Also consider that the measurements are
> kept in the device registers until a new integration is complete, so if two
> registers are read consecutively within the same integration period they would
> be correlated.

We only usually care about correlation of channels when using the buffered
interface.  There are a few exceptions such as quaternions where they have
no meaning as separate values and where we are computing another value from
the read out of several physical channels (light sensors etc).

> > > +               case PAC1921_CHAN_POWER: {
> > > 
> > > +                       /* Power scale factor in mW:  
> > 
> > I think the scale here should be in microWatts. We have (mA)*(mV)
> >   
> Documentation/ABI/testing/sysfs-bus-iio states it should be in milliwatts:
> 
> What:		/sys/bus/iio/devices/iio:deviceX/in_powerY_raw
> KernelVersion:	4.5
> Contact:	linux-iio@xxxxxxxxxxxxxxx
> Description:
> 		Raw (unscaled no bias removal etc.) power measurement from
> 		channel Y. The number must always be specified and
> 		unique to allow association with event codes. Units after
> 		application of scale and offset are milliwatts.

These units tend to be copied from hwmon.  With hindsight we should just
have gone with volts / amps and watts but that's history now.

Jonathan