Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X

[<Marius.Cristea@microchip.com>]

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@microchip.com> wrote:
> 
> > From: Marius Cristea <marius.cristea@microchip.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@microchip.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.

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.

> > 
> > 
...

> 
> 
> 
> 
> 
> > +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.

> 
>