Message-ID: <88a54c736e0c39ead34dbde53c813526484d767d.camel@microchip.com>
Date: Wed, 10 Jul 2024 16:01:22 +0000
From: <Marius.Cristea@...rochip.com>
To: <jic23@...nel.org>, <matteomartelli3@...il.com>
CC: <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
	<devicetree@...r.kernel.org>, <lars@...afoo.de>, <linux-iio@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: adc: add support for pac1921

Hi Matteo,

On Mon, 2024-07-08 at 15:39 +0200, Matteo Martelli wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> > 
> 


> I think that the OUT pin might not be used at all in many use cases
> so I would
> leave the OUT pin setting as fixed for now and maybe extend it in the
> future
> when more use cases arise. I am open to reconsider this though.
> 

The OUT functionality could be set from the device tree.

> > 
> > > ---
> > >  .../ABI/testing/sysfs-bus-iio-adc-pac1921          |   45 +
> > >  MAINTAINERS                                        |    7 +
> > >  drivers/iio/adc/Kconfig                            |   10 +
> > >  drivers/iio/adc/Makefile                           |    1 +
> > >  drivers/iio/adc/pac1921.c                          | 1038
> > > ++++++++++++++++++++
> > >  5 files changed, 1101 insertions(+)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921
> > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921
> > > new file mode 100644
> > > index 000000000000..4a32e2d4207b
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921
> > Quite a bit of custom ABI in here.
> > 
> > Rule of thumb is that custom ABI is more or less pointless ABI for
> > 99% of users
> > because standard userspace won't use it.  So keep that in mind when
> > defining it.
> > 
> > > @@ -0,0 +1,45 @@
> > > +What:             
> > > /sys/bus/iio/devices/iio:deviceX/resolution_bits
> > > +KernelVersion:     6.10
> > > +Contact:   linux-iio@...r.kernel.org
> > > +Description:
> > > +           ADC measurement resolution. Can be either 11 bits or
> > > 14 bits
> > > +           (default). The driver sets the same resolution for
> > > both VBUS and
> > > +           VSENSE measurements even if the hardware could be
> > > configured to
> > > +           measure VBUS and VSENSE with different resolutions.
> > > +           This attribute affects the integration time: with 14
> > > bits
> > > +           resolution the integration time is increased by a
> > > factor of
> > > +           1.9 (the driver considers a factor of 2). See Table
> > > 4-5 in
> > > +           device datasheet for details.
> > 
> > Is the integration time ever high enough that it matters?
> > People tend not to do power measurement 'quickly'.
> > 
> > If we are doing it quickly then you'll probably want to be
> > providing buffered
> > support and that does allow you to 'read' the resolution for a part
> > where
> > it changes for some other reason.   I haven't yet understood this
> > case.
> 
> I will remove this control and fix the resolution bits to 14 (highest
> value),
> same as the HW default.

The resolution could be set from the device tree. As default it could
be 14 bits like into the hardware. The user could add
"microchip,low_resolution_voltage" into the device tree in order to use
only 11 bits for voltage samples.

> 
> > > +
> > > +What:             
> > > /sys/bus/iio/devices/iio:deviceX/resolution_bits_available
> > > +KernelVersion:     6.10
> > > +Contact:   linux-iio@...r.kernel.org
> > > +Description:
> > > +           List all possible ADC measurement resolutions: "11
> > > 14"
> > > +
> > > +What:             
> > > /sys/bus/iio/devices/iio:deviceX/integration_samples
> > > +KernelVersion:     6.10
> > > +Contact:   linux-iio@...r.kernel.org
> > > +Description:
> > > +           Number of samples taken during a full integration
> > > period. Can be
> > > +           set to any power of 2 value from 1 (default) to 2048.
> > > +           This attribute affects the integration time: higher
> > > the number
> > > +           of samples, longer the integration time. See Table 4-
> > > 5 in device
> > > +           datasheet for details.
> > 
> > Sounds like oversampling_ratio which is standards ABI. So use that
> > or explain
> > why you can't here.
> 
> I am not sure that this is an oversampling ratio but correct me if I
> am wrong:
> generally by increasing the oversampling you would have additional
> samples in a
> fixed time period, while in this case by increasing the number of
> samples you
> would still have the same number of samples in a fixed time period,
> but you
> would have a longer integration period. So maybe the comment is not
> very
> clear since this parameter actually means "the number of samples
> required to
> complete the integration period".
> 
> Initially I thought to let the user edit this by writing the
> integration_time
> control (which is currently read-only), but since the integration
> period
> depends also on the resolution and whether filters are enabled or
> not, it would
> have introduced some confusion: what parameter is being changed upon
> integretion_time write? Maybe after removing resolution and filter
> controls
> there would be no such confusion anymore.
> 
> > > +
> > > +What:             
> > > /sys/bus/iio/devices/iio:deviceX/integration_samples_available
> > > +KernelVersion:     6.10
> > > +Contact:   linux-iio@...r.kernel.org
> > > +Description:
> > > +           List all possible numbers of integration samples:
> > > +           "1 2 4 8 16 32 64 128 256 512 1024 2048"
> > > +
> > > +What:              /sys/bus/iio/devices/iio:devices/filters_en
> > > +KernelVersion:     6.10
> > > +Contact:   linux-iio@...r.kernel.org
> > > +Description:
> > > +           Attribute to enable/disable ADC post filters. Enabled
> > > by
> > > +           default.
> > > +           This attribute affects the integration time: with
> > > filters
> > > +           enabled the integration time is increased by 50%. See
> > > Table 4-5
> > > +           in device datasheet for details.
> > 
> > Do we have any idea what this filter is? Datasheet seems very vague
> > indeed and from
> > a control point of view that makes this largely useless. How does
> > userspace know
> > whether to turn it on?
> > 
> > We have an existing filter ABI but with so little information no
> > way to fit this in.
> > Gut feeling, leave it on all the time and drop the control
> > interface.
> 
> I will remove this control and leave it on all the time as the HW
> default.
> 

The filters could be enabled from the device tree. As default it could
be disabled.
As a small detail here this is a post processing digital filter that
could be enabled/disabled inside the PAC module.



> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index f60fe85a30d5..b56e494da970 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -991,6 +991,16 @@ config NPCM_ADC
> > >       This driver can also be built as a module. If so, the
> > > module
> > >       will be called npcm_adc.
> > > 
> > > +config PAC1921
> > > +   tristate "Microchip Technology PAC1921 driver"
> > > +   depends on I2C
> > 
> > 
> 

> Thanks again,
> Matteo


Thanks,
Marius

Powered by blists - more mailing lists