| 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
| ||
|
Message-ID: <1b8b10816d1f2f34724e77c68de869422d6c84b6.camel@microchip.com>
Date: Tue, 10 Jun 2025 15:07:34 +0000
From: <Marius.Cristea@...rochip.com>
To: <jic23@...nel.org>, <nuno.sa@...log.com>, <dlechner@...libre.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 Fri, 2025-06-06 at 12:02 -0500, David Lechner wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On 6/6/25 4:39 AM, marius.cristea@...rochip.com wrote:
> > From: Marius Cristea <marius.cristea@...rochip.com>
> >
> > This is the iio driver for Microchip PAC194X and PAC195X series of
> > Power Monitors with Accumulator. The PAC194X family supports 9V
> > Full-Scale Range and the PAC195X supports 32V Full-Scale Range.
> >
> > There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices
> > are for high-side current sensing and the PAC194X/5X-2 devices are
> > for low-side current sensing or floating VBUS applications. The
> > PAC194X/5X-1 is named shortly PAC194X/5X.
> >
> > Signed-off-by: Marius Cristea <marius.cristea@...rochip.com>
> > ---
> > .../ABI/testing/sysfs-bus-iio-adc-pac1944 | 17 +
> > MAINTAINERS | 7 +
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/pac1944.c | 2841
> > +++++++++++++++++
>
> Oof. Very hard to make a review of something this big. Is it possible
> to break it down into more patches? e.g. start with just the basic
> functionality, then add the accumulator in a separate patch, add
> events
> in a separate patch, etc. It's not always possible, but 500 lines per
> patch is a nice number to aim for.
>
The first 2 version of the driver was much bigger. I will try make it
smaller and build on top of that.
>
> > 5 files changed, 2878 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-
> > pac1944
> > create mode 100644 drivers/iio/adc/pac1944.c
> >
> > 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.
>
> > 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.
>
> ...
>
> > +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.
>
>
> > +/* 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.
>
Powered by blists - more mailing lists