Message-ID: <20240713113644.464aa6d0@jic23-huawei>
Date: Sat, 13 Jul 2024 11:36:44 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Conor Dooley <conor@...nel.org>
Cc: Marius.Cristea@...rochip.com, matteomartelli3@...il.com,
 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

On Fri, 12 Jul 2024 18:02:54 +0100
Conor Dooley <conor@...nel.org> wrote:

> On Fri, Jul 12, 2024 at 02:41:51PM +0000, Marius.Cristea@...rochip.com wrote:
> > Hi Matteo,
> > 
> > 
> > On Thu, 2024-07-11 at 09:08 +0200, Matteo Martelli wrote:  
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > Hi Marius,
> > > 
> > > Marius.Cristea@ wrote:  
> > > > > 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.  
> > > 
> > > I think this should to be controlled during runtime since it's a
> > > configuration
> > > that changes the device operation mode and so also what measurements
> > > are
> > > exposed to the user. An additional DT property could be useful but I
> > > am not
> > > sure it would fit in the DT scope.
> > > Anyway I will leave this for future extensions.
> > >   
> > 
> > I think there are 2 different things here. Setting the configuration at
> > startup by hard-coding things at probe time or taken those from device
> > tree (we can add multiple properties here, as long those properties are
> > documented into the dt-binding file) and the user controlled part at
> > runtime.
> > Because there is no standard interface to change the functionality, it
> > will be easy to startup from the device tree and let the user to do
> > some minor adjustments and not hardcode configuration.

There is a quirk here.  If out is wired to an ADC on the SoC for some reason
then indeed it should be runtime configurable.  If it's wired to some
types of analog circuitry or a separate thermal monitoring micro controller
then it 'might' belong in DT because that 'wiring' is not discoverable.
However, this usecase isn't one anyone has 'yet' asked for so for now
we have no reason to provide a binding for it.  Also if this wiring
is the case, we would probably not provide a userspace interface to control
the pin (smoke might be the result).

If it's wired to an ADC on the linux running SoC then this is definitely
a userspace control thing and we've lots of examples on how to do
that in 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.  
> > > 
> > > I think this should be controlled during runtime since it does not
> > > depend on
> > > the HW design but more on the user preferences about measurements
> > > precision.
> > > As Jonathan pointed out, since custom ABIs should be avoided when
> > > possible, I
> > > will leave it out from now until it becomes necessary and fix the
> > > resolution to
> > > 14 bits, as the HW default.
> > >   
> > 
> > Set the configuration from the device tree, will avoid custom ABI. The
> > device tree could be changed also at runtime.  
> 
> Custom ABI in devicetree is not a replacement for custom ABI in userspace.
> If things are fixed by the hardware and non-discoverable, then sure add
> devicetree properties - but if it is things like "the user wants 11-bit
> mode", then that does not sound suitable for a devicetree property at
> all.

Indeed, resolution doesn't belong in device tree as it has nothing to do
with physical wiring, but is a policy control.  I have no problem with
providing a userspace ABI, but so far I've not heard a usecase for
enabling it at all on this device.  Who runs power measurement that
needs to be a little bit faster than can be done with 14bit all the time?

We have only done resolution control on devices where that resolution is really
changing as a result of some other factor (oversampling ratio or similar)
so it's been a read only aspect of the ABI.

Note we've been round this resolution question many times and so far
the number of actual usecases that have materialised is very very small.

Jonathan


> And no, you can't just change the devicetree at runtime like that either
> as far as I understand - that's gonna cause memory leaks etc and I don't
> think can be done from userspace without out-of-tree patches anyway.
> 
> Cheers,
> Conor.
> 
> >   
> > > ...  
> > > > > > > +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.
> > > >   
> > > 
> > > Same reasoning of the resolution_bits parameter applies here. I will
> > > fix the
> > > filters to enabled, as the HW default. If there is any particular
> > > reason to
> > > prefer the filters fixed as disabled I will change that.
> > >   
> > If the user can change the on/off for the filters it doesn't matter
> > what will be the default behavior. Being a single channel device, the
> > probability for the user to change the filter behavior during runtime
> > is minimal, that was the main reason for letting the user to change the
> > configuration from the device tree and not hardcode it.
> >   
> > > ...  
> > > > Thanks,
> > > > Marius  
> > > 
> > > Thanks,
> > > Matteo  
> > 
> > Thanks,
> > Marius  

Powered by blists - more mailing lists