Message-ID: <ea72561a1ab953d3f2a99272c24cf5124c0c72ec.camel@microchip.com>
Date: Fri, 12 Jul 2024 14:41:51 +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 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.


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

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