Message-ID: <20250611171241.2c5c9319@jic23-huawei>
Date: Wed, 11 Jun 2025 17:12:41 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: <Marius.Cristea@...rochip.com>
Cc: <dlechner@...libre.com>, <nuno.sa@...log.com>, <broonie@...nel.org>,
 <linux-iio@...r.kernel.org>, <robh@...nel.org>,
 <linux-kernel@...r.kernel.org>, <andy@...nel.org>, <krzk+dt@...nel.org>,
 <conor+dt@...nel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X

On Tue, 10 Jun 2025 15:58:13 +0000
<Marius.Cristea@...rochip.com> wrote:

> On Sat, 2025-06-07 at 16:27 +0100, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Fri, 6 Jun 2025 12:39:29 +0300
> > <marius.cristea@...rochip.com> wrote:
> >   
> > > From: Marius Cristea <marius.cristea@...rochip.com>
> > > 
> > > This is the iio driver for Microchip PAC194X and PAC195X series of
> > > Power Monitors with Accumulator. The PAC194X family supports 9V
> > > Full-Scale Range and the PAC195X supports 32V Full-Scale Range.
> > > 
> > > There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices
> > > are for high-side current sensing and the PAC194X/5X-2 devices are
> > > for low-side current sensing or floating VBUS applications. The
> > > PAC194X/5X-1 is named shortly PAC194X/5X.
> > > 
> > > Signed-off-by: Marius Cristea <marius.cristea@...rochip.com>  
> > Hi Marius,
> > 
> > I entirely agree with David on this wanting splitting up into
> > a base driver + patches that add features to that in order to
> > make it easier to review.  That also potentially allows me to pick up
> > the basic support whilst any more controversial parts are still under
> > discussion.
> > 
> > Jonathan  
> 
> I will try to make it smaller and build on that.
> 
> >   
> > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > index 09ae6edb2650..ee47d880babf 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -103,6 +103,7 @@ obj-$(CONFIG_NCT7201) += nct7201.o
> > >  obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
> > >  obj-$(CONFIG_PAC1921) += pac1921.o
> > >  obj-$(CONFIG_PAC1934) += pac1934.o
> > > +obj-$(CONFIG_PAC1944) += pac1944.o
> > >  obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
> > >  obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
> > >  obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o
> > > diff --git a/drivers/iio/adc/pac1944.c b/drivers/iio/adc/pac1944.c
> > > new file mode 100644
> > > index 000000000000..ce09334b076a
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/pac1944.c
> > > @@ -0,0 +1,2841 @@  
> > 
> >   
> > > +/* Available Sample Modes */
> > > +static const char * const pac1944_frequency_avail[] = {
> > > +     "1024_ADAP",
> > > +     "256_ADAP",  
> > 
> > This adaptive mode shouldn't be controlled via this standard
> > ABI.  That needs to be considered separately.
> >   
> > > +     "64_ADAP",
> > > +     "8_ADAP",
> > > +     "1024",
> > > +     "256",
> > > +     "64",
> > > +     "8",  
> > This does not look even close to ABI complaint.
> > 
> > The numbers cases are fine. The others not really.  
> > > +     "single_shot_1x",  
> > 
> > That has nothing directly to do with the sampling frequency.
> > Some others look suspicious.  I'd stick to normal
> > sampling_frequency handling and have the discussion about the other
> > modes in here at a later date.
> >   
> > > +     "single_shot_8x",
> > > +     "fast",
> > > +     "burst",
> > > +};  
> 
> The adaptive mode is used to lower the power consumption of the device
> and keep the "same" calculation into the driver. I will try to look
> over an ABI to put the device into a low power mode and in this way I
> could change the frequency to ADAPTIVE.

We deliberately don't support 'mode' type switches for power saving in
the ABI. They are just too varied for userspace to 'generically'
know when to turn them on. Snag here is that it is a straight
trade off between a clever algorithm which will introduce some error
and straight forward sampling at high frequency. I don't currently
have a good idea on how we'd control that.  If we also had
a dataready trigger involved (rather than accumulating etc) then
maybe we'd do it via trigger selection, but that's not appropriate
here given this is about driving the accumulators, not a dataready
style readback of data (if we did that, we could just accumulate
in user space instead of the device - a very different thing!).


> 
> The "single_shot_1x" and "single_shot_8x" is a special case when the
> user could take only one measurement and that measurement could be
> triggered/synched.

8x seems to be about oversampling - but on a single trigger.
In that sense it is a bit unusual.  Looks like we could only do
it effectively on an external IIO trigger? e.g. hrtimer or something
like that?

single shot 1x looks like a similar mode to use only on an external
trigger (or a sysfs read).  Can we just enable that only if we aren't
using this devices own trigger?

fast vs burst is an open question but taking just say fast
that ends up being just another sampling frequency, be it one that
changes with number of enabled channels.



> 
> > > 
> > >   
> ...
> 
> > 
> > 
> > 
> > 
> >   
> > > +static struct attribute *pac1944_power_acc_attr[] = {
> > > +     &iio_dev_attr_in_energy1_raw.dev_attr.attr,
> > > +     &iio_dev_attr_in_energy2_raw.dev_attr.attr,
> > > +     &iio_dev_attr_in_energy3_raw.dev_attr.attr,
> > > +     &iio_dev_attr_in_energy4_raw.dev_attr.attr,
> > > +     &iio_dev_attr_in_energy1_scale.dev_attr.attr,
> > > +     &iio_dev_attr_in_energy2_scale.dev_attr.attr,
> > > +     &iio_dev_attr_in_energy3_scale.dev_attr.attr,
> > > +     &iio_dev_attr_in_energy4_scale.dev_attr.attr,  
> > 
> > These look like standard read_raw / info_mask based attributes will
> > work.
> > So do that, not custom attributes that are both harder to review and
> > don't work with in kernel consumers (which we may well see for a
> > power
> > monitoring chip).
> >   
> 
> Because the device could support different modes of operation for the
> same accumulator, but only one at a time. I was trying to add to the
> sysfs only the mode that was set from the device tree.

Create the iio_chan_spec dynamically instead of the attributes added.

Jonathan


> 
> > 
> >   

Powered by blists - more mailing lists