Message-ID: <3d95641d-c1c2-44bc-8478-c60734bcf420@baylibre.com>
Date: Tue, 10 Jun 2025 11:11:32 -0500
From: David Lechner <dlechner@...libre.com>
To: Marius.Cristea@...rochip.com, jic23@...nel.org, nuno.sa@...log.com,
 andy@...nel.org
Cc: 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 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.

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.

>>
>>> subsidiaries
>>> + *
>>> + * Author: Marius Cristea marius.cristea@...rochip.com
>>> + *
>>> + * Datasheet for PAC1941, PAC1942, PAC1943 and PAC1944 can be
>>> found here:
>>> + *
>>> https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf
>>> + * Datasheet for PAC1951, PAC1952, PAC1953 and PAC1954 can be
>>> found here:
>>> + *
>>> https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf
>>> + */
>>> +
>>> +#include <linux/acpi.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/err.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/events.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/unaligned.h>
>>
>> This seems incomplete. Expecting at least linux/module.h,
>> linux/property.h, etc.
> 
> I will double check. Most probably those module are already included by
> a header.
> 

Even if some things happens to be indirectly included by another
header, we still prefer to have the "primary" header included for
everything that is actually used in this file. E.g. since this
file uses the MODULE_DESCRIPTION() macro, we expect to see
linux/module.h even if another header already includes that
indirectly.

Usually the "primary" header is the one where a symbol/macro is
defined, but not always, e.g. if there is a linux/something.h
that incldues asm/something.h, we would prefer the linux/ one
rather than including the asm/ one directly.


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

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

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

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?

Powered by blists - more mailing lists