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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7873 bytes --]

On Fri, Jul 12, 2024 at 02:41:51PM +0000, Marius.Cristea@microchip.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.
> 
> 
> > ...
> > > > > > ---
> > > > > >  .../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@vger.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.
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@vger.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]