linux-kernel - Re: [PATCH v2 2/2] iio: adc: add support for pac1921

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <668f84e2f3e10_2b423707a@njaxe.notmuch>
Date: Thu, 11 Jul 2024 09:08:18 +0200
From: Matteo Martelli <matteomartelli3@...il.com>
To: Marius.Cristea@...rochip.com, 
 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 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.

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

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

...
> Thanks,
> Marius

Thanks,
Matteo