Message-ID: <20250611165939.45bb50ad@jic23-huawei>
Date: Wed, 11 Jun 2025 16:59:39 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Marius.Cristea@...rochip.com, nuno.sa@...log.com, andy@...nel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 broonie@...nel.org, devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X

On Tue, 10 Jun 2025 11:11:32 -0500
David Lechner <dlechner@...libre.com> wrote:

> On 6/10/25 10:07 AM, Marius.Cristea@...rochip.com wrote:
> > On Fri, 2025-06-06 at 12:02 -0500, David Lechner wrote:  
> 
> ...
> 
> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944
> >>> b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944
> >>> new file mode 100644
> >>> index 000000000000..ae88eac354a4
> >>> --- /dev/null
> >>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944
> >>> @@ -0,0 +1,17 @@
> >>> +What:               
> >>> /sys/bus/iio/devices/iio:deviceX/slow_alert1_cfg
> >>> +KernelVersion:       6.16
> >>> +Contact:     linux-iio@...r.kernel.org
> >>> +Description:
> >>> +             A read/write property used to route, inside the PAC
> >>> device, a specific ALERT
> >>> +             signal to the SLOW/ALERT1 pin. The SLOW/ALERT1 pin
> >>> must be configured for the
> >>> +             ALERT function in order to control the device
> >>> hardware pin (this is the default
> >>> +             functionality of the device hardware pin).
> >>> +
> >>> +What:               
> >>> /sys/bus/iio/devices/iio:deviceX/gpio_alert2_cfg
> >>> +KernelVersion:       6.16
> >>> +Contact:     linux-iio@...r.kernel.org
> >>> +Description:
> >>> +             A read/write property used to route, inside the PAC
> >>> device, a specific ALERT
> >>> +             signal to the GPIO/ALERT2 hardware pin. The
> >>> GPIO/ALERT2 pin must be configured
> >>> +             for ALERT function in order to control the device
> >>> hardware pin (this is the
> >>> +             default functionality of the device hardware pin).  
> >>
> >>
> >> What is the use case for needing these? In otherwords, why can't the
> >> driver just
> >> make best use of available resources as it sees fit?
> >>  
> > 
> > Here inside the PAC the user could choose what limit to be routeed
> > outside the chip. For sure, all of the limits could be routed to the
> > same hardware pin, but there are some use cases where the user will
> > want to connect that output pin to a safety hardware (e.g. over-current
> > protection or over-voltage and over-current) and in this case we need a
> > way to allow the user to do the setup.
> >   
> 
> This sounds like it depends on what is wired to the alert pin, so sounds
> like something that should be specified in the devicetree.

Absolutely agree.

These corners tend to be tricky to handle cleanly
in either DT or userspace interfaces though and sometimes we've
just decided not to be fully flexible to keep that sane.

> 
> I.e. in the devicetree, have a bool property microchip,alert1-is-safety
> to indicate the ALERT1 pin is wired to the safety hardware. (It could
> still be also wired as an interrupt input at the same time - or not,
> doesn't really matter.)
> 
> Then, on the event attributes add a boolean "safety" attribute to allow
> routing the signal to either the pin that was flagged as the safety pin
> or not. This would allow the user to chose which signals control the
> safety hardware at runtime without them having to know how the hardware
> is actually wired up.

I'd question if the use cases are such that it makes sense to expose the
option to user space. Generally if you have a way to trigger a safety circuit
it's there for a reason and you want it always on for that reason (i.e.
you might tweak exactly when a cut off on power usage fires, but you always
do it on power usage).

We probably want to propose a binding to he DT folk + take a look at
what the ABI David suggests looks like in the driver. That's
messy if it isn't double wired to the interrupt controller though as
we then have events that never fire.


> >>
> >> ...
> >>  
> >>> +static IIO_DEVICE_ATTR(in_current1_shunt_resistor, 0644,
> >>> +                    pac1944_shunt_value_show,
> >>> pac1944_shunt_value_store, 0);
> >>> +static IIO_DEVICE_ATTR(in_current2_shunt_resistor, 0644,
> >>> +                    pac1944_shunt_value_show,
> >>> pac1944_shunt_value_store, 1);
> >>> +static IIO_DEVICE_ATTR(in_current3_shunt_resistor, 0644,
> >>> +                    pac1944_shunt_value_show,
> >>> pac1944_shunt_value_store, 2);
> >>> +static IIO_DEVICE_ATTR(in_current4_shunt_resistor, 0644,
> >>> +                    pac1944_shunt_value_show,
> >>> pac1944_shunt_value_store, 3);  
> >>
> >> These are specified in the devicetree. Why are there also sysfs
> >> attribtes?  
> > 
> > Yes, you could put a generic shunt resistor into the device tree but
> > this resistor will have a tolerance. Because the end user could
> > calibrate the system, it could also save the calculated/calibrated
> > shunt resistor somewhere and restore that calibrated value each time
> > the driver is loaded.
> >   
> 
> If changing the resistor value changes the measured raw value, we
> could probably use one of the existing standard calibration attributes
> instead, like calibbias or calibscale.

We have precedence for shunt resistor ABI for this reason, so I agree with
David if we were starting from scratch (and it's a viable option to
use the calibration attributes) but we can't drop the existing ABI
and it does limited harm to carry on using it.

> 
> >   
> >>
> >>  
> >>> +/* Available Sample Modes */
> >>> +static const char * const pac1944_frequency_avail[] = {
> >>> +     "1024_ADAP",
> >>> +     "256_ADAP",
> >>> +     "64_ADAP",
> >>> +     "8_ADAP",
> >>> +     "1024",
> >>> +     "256",
> >>> +     "64",
> >>> +     "8",
> >>> +     "single_shot_1x",
> >>> +     "single_shot_8x",
> >>> +     "fast",
> >>> +     "burst",
> >>> +};  
> >>  
> >>>  
> > ...  
> >>> +
> >>> +static const struct iio_chan_spec_ext_info pac1944_ext_info[] = {
> >>> +     IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL,
> >>> &sampling_mode_enum),
> >>> +     {
> >>> +             .name = "sampling_frequency_available",
> >>> +             .shared = IIO_SHARED_BY_ALL,
> >>> +             .read = iio_enum_available_read,
> >>> +             .private = (uintptr_t)&sampling_mode_enum,
> >>> +     },
> >>> +     { }
> >>> +};  
> >>
> >> sampling_frequency{_avialable} are already standard attributes in IIO
> >> and is
> >> defined to be a number in Hz. So we will need to find a way to make
> >> this
> >> work with the standard attribute (can use IIO_CHAN_INFO_SAMPLE_FREQ,
> >> by the way).
> >> And figure out how the other parts fit into other existing IIO
> >> features.  
> > 
> > I can change to the standard attributes but I still have some question
> > related to how to handle the ADAPTIVE sampling frequency that the chip
> > supports and that it could be used to lower the power consumption of
> > the chip.

Gah.  Second device to do something like this that we've seen this month.
(ADXL313 does something similar when enough activity detection modes are
turned on).  We don't have a good answer yet :(

> >   
> >>  
> 
> From a quick look at one of the datasheets, it sounds like this
> "adaptive" mode only applies when using an accumulator. And it doesn't
> actually change the sample rate, but rather other factors, like scale
> and the accumulator counter incitement. So it seems like it would be
> a separate custom boolean attribute.

Generally when I see this sort of thing my first instinct is hide it.
What use is it for userspace to see that the frequency is changing?
Maybe we just report the highest (or lowest?) frequency that might
be going on?  Here it doesn't seem to be coupled to anything else
though (unlikely the accelerometer) so I'm not sure how the driver
would guess it makes sense to enable it?

> 
> Also, I noticed that the fast mode and burst mode make the sampling
> frequency dependent on the number of enabled channels. So to handle
> this, normally, that would mean that IIO_CHAN_INFO_SAMP_FREQ would
> need to be IIO_SEPARATE rather than IIO_SHARED_BY_ALL.
> 
> But since these chips support can work both ways (there are modes
> where sample rate doesn't depend on the number of channels enabled
> and there are modes where it does), I'm not sure what the right way
> to handle that would be here. Maybe Jonathan will have some suggestion?

Oh goody a new way to stretch that already much stretched interface.
It's actually more fun because it's not the sum of the number of channels
for fast mode but 1 more than that.

Given we can't do both ways of reporting it at the same time we will
probably have to just do IIO_SHARED_BY_ALL.  We can only then report
the effective sampling frequency as it is fixed in these modes (so
we can't attempt to match a userspace request by cranking it down
if there are fewer channels enabled or similar).

So we need
sampling_frequency_available where the fastest frequency presented
changes depending on the enabled channels.  Lower values match
the 8/64/256/1024 values however many are enabled.

if sampling_frequency == top value then we just change it if more
channels are enabled.  ABI always allows for other attributes to
change 'randomly' when you touch a different one.  That flexibility
is a pain for users but necessary for cases like this :(

I'm not sure how we pick between burst and fast mode though
as seems to be about auto offset stuff.

Jonathan

Powered by blists - more mailing lists