Skip to contentSkip to conversation

adding support for Microchip PAC194X Power Monitor

PATCH Superseded [email protected] 5 participants 10 messages Mar 17, 2025 – Mar 24, 2025

Subsystems (2)

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v2 cover adding support for Microchip PAC194X Power Monitor

From: Marius Cristea <[email protected]>

Adding support for Microchip PAC194X and PAC195X series of Power Monitor with Accumulator chip family. This driver covers the following part numbers:
- PAC1941, PAC1941_2, PAC1942, PAC1942_2, PAC1943, PAC1944
- PAC1951, PAC1951_2, PAC1952, PAC1952_2, PAC1953, PAC1954

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.

Differences related to previous patch:
v2:
- fix review comments device tree binding:
remove underscore from names
add names to the interrupts and list them
add a better description for new properties
- fix review comments driver:
fix coding style issues
use bitmap for checking the active channels
keep the "pac1944_get_unaligned_be56" here because the change wasn't acceted
into the asm-generic.
document new added attributes
remove the "scan" part till we support buffered capture
remove "unlikely" marking
add masks up in some array of const structures to avoid some case statements remove pac1944_mutex_destroy function
replace some functions with a macro (just for testing)
replace dev_err with dev_err_probe in functions used in pac1944_probe

v1:
- first version committed to review

Marius Cristea (2):
dt-bindings: iio: adc: adding support for PAC194X
iio: adc: adding support for PAC194X

 .../ABI/testing/sysfs-bus-iio-adc-pac1944     |  118 +
 .../bindings/iio/adc/microchip,pac1944.yaml   |  195 +
 MAINTAINERS                                   |    7 +
 drivers/iio/adc/Kconfig                       |   12 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/pac1944.c                     | 3314 +++++++++++++++++
 6 files changed, 3647 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944
 create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml
 create mode 100644 drivers/iio/adc/pac1944.c

base-commit: 577a66e2e634f712384c57a98f504c44ea4b47da

Signature · 1 line
2.45.2
#1 lore raw

PATCH v2 1/2 dt-bindings: iio: adc: adding support for PAC194X

From: Marius Cristea <[email protected]>

This is the device tree schema for 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.

---
 .../bindings/iio/adc/microchip,pac1944.yaml   | 195 ++++++++++++++++++
 1 file changed, 195 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml
Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml new file +195 -0
new file mode 100644
index 000000000000..1997e889e3f6
@@ -0,0 +1,195 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/microchip,pac1944.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PAC1944 and PAC1954 Power Monitors with Accumulator
+
+maintainers:
+  - Marius Cristea <[email protected]>
+
+description: |
+  This device is part of the Microchip family of Power Monitors with
+  Accumulator. The datasheet for PAC1941-1, PAC1941-1, PAC1942-1, PAC1942-2,
+  PAC1943-1 and PAC1944-1 can be found here:
+    https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf
+  The datasheet for PAC1951-1, PAC1951-1, PAC1952-1, PAC1952-2, PAC1953-1 and
+  PAC1954-1 can be found here:
+    https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf
+
+properties:
+  compatible:
+    enum:
+      - microchip,pac1941
+      - microchip,pac19412
+      - microchip,pac1942
+      - microchip,pac19422
+      - microchip,pac1943
+      - microchip,pac1944
+      - microchip,pac1951
+      - microchip,pac19512
+      - microchip,pac1952
+      - microchip,pac19522
+      - microchip,pac1953
+      - microchip,pac1954
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  interrupts:
+    maxItems: 2
+
+  interrupt-names:
+    maxItems: 2
+    items:
+      enum:
+        - alert1
+        - alert2
+
+patternProperties:
+  "^channel@[1-4]+$":
+    type: object
+    $ref: adc.yaml
+    description:
+      Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        items:
+          minimum: 1
+          maximum: 4
+
+      shunt-resistor-micro-ohms:
+        description:
+          Value in micro Ohms of the shunt resistor connected between
+          the SENSE+ and SENSE- inputs, across which the current is measured.
+          Value is needed to compute the scaling of the measured current.
+
+      microchip,vbus-mode:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:
+          In order to increase measurement resolution and keeping the same
+          number the of bits the device has a configurable VBUS full range
+          scale (FSR). The range should be set by hardware design and it should
+          not be changed during runtime. The bipolar capability for VBUS enables
+          accurate offset measurement and correction.
+          The VBUS could be configured into the following full scale range
+            <0>  -  VBUS has unipolar +32V to 0V FSR (default) for PAC195X
+                    or +9V to 0V (default) for PAC194X
+            <1>  -  VBUS has bipolar +32V to -32V FSR for PAC195X
+                    or +9V to -9V for PAC194X. The actual range is limited to
+                    about -200 mV due to the impact of the ESD structures.
+            <2>  -  VBUS has bipolar +16V to -16V FSR for PAC195X
+                    or +4.5V to -4.5V for PAC194X. The actual range is limited
+                    to about -200 mV due to the impact of the ESD structures.
+        maximum: 2
+
+      microchip,vsense-mode:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:
+          In order to decrease the power dissipation on the shunt resistor and
+          in the same time to increase measurement resolution by keeping the
+          same number the of bits the device has a configurable VSENSE full
+          range scale (FSR). The range should be set by hardware design and it
+          should not be changed during runtime. 
+          The VSENSE could be configured into the following full scale range
+            <0>  -  VSENSE has unipolar +100 mV to 0V FSR (default)
+            <1>  -  VSENSE has bipolar +100 mV to -100 mV FSR
+            <2>  -  VSENSE has bipolar +50 mV to -50 mV FSR
+        maximum: 2
+
+      microchip,accumulation-mode:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:
+          The Hardware Accumulator may be used to accumulate VPOWER, VSENSE or
+          VBUS values for any channel. By setting the accumulator for a channel
+          to accumulate the VPOWER values gives a measure of accumulated power
+          into a time period, which is equivalent to energy. Setting the
+          accumulator for a channel to accumulate VSENSE values gives a measure
+          of accumulated current, which is equivalent to charge. This allows the
+          accumulator to be used as a coulomb counter. For either VSENSE or
+          VBUS, many samples may be accumulated on chip and the result collected
+          by the host and divided by the accumulator counter count value to
+          yield an average value with a very long integration time to reduce
+          noise. This feature is also very useful for system calibration,
+          allowing many averages to be accumulated for fast averaging/noise
+          reduction.
+          This functionality needs to be setup once and must not be changed
+          during the runtime, just in case the user wants to measure the charge
+          or the energy consumed from board power up till the user has control
+          or during a reboot of the system.          
+          The Hardware Accumulator could be configured to accumulate
+          VPOWER, VSENSE or VBUS
+            <0>  -  Accumulator accumulates VPOWER (default)
+            <1>  -  Accumulator accumulates VSENSE
+            <2>  -  Accumulator accumulates VBUS
+        maximum: 2
+
+    required:
+      - reg
+      - shunt-resistor-micro-ohms
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-monitor@10 {
+            compatible = "microchip,pac1954";
+            reg = <0x10>;
+            vdd-supply = <&vdd>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@1 {
+                reg = <0x1>;
+                shunt-resistor-micro-ohms = <24900>;
+                label = "CPU";
+                microchip,vbus-mode = <0>;
+                microchip,vsense-mode = <0>;
+                microchip,accumulation-mode = <0>;
+            };
+
+            channel@3 {
+                reg = <0x3>;
+                shunt-resistor-micro-ohms = <75000>;
+                label = "MEM";
+                microchip,vbus-mode = <0>;
+                microchip,vsense-mode = <0>;
+                microchip,accumulation-mode = <0>;
+            };
+
+            channel@4 {
+                reg = <0x4>;
+                shunt-resistor-micro-ohms = <100000>;
+                label = "NET";
+                microchip,vbus-mode = <0>;
+                microchip,vsense-mode = <0>;
+                microchip,accumulation-mode = <0>;
+            };
+        };
+    };
+
+...
Signature · 1 line
2.45.2
#2 lore raw

Re: [PATCH v2 1/2] dt-bindings: iio: adc: adding support for PAC194X

Quoted 11 lines · On Mon, 17 Mar 2025 11:08:02 +0200, [email protected] wrote:

From: Marius Cristea <[email protected]>

This is the device tree schema for 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.

Quoted 6 lines
---
 .../bindings/iio/adc/microchip,pac1944.yaml   | 195 ++++++++++++++++++
 1 file changed, 195 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml: properties:interrupt-names:items: {'enum': ['alert1', 'alert2']} is not of type 'array'
from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]

The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

maintainer
#3 lore raw

Re: [PATCH v2 1/2] dt-bindings: iio: adc: adding support for PAC194X

In reply to [email protected]
Quoted 4 lines · On Mon, 17 Mar 2025 11:08:02 +0200 <[email protected]> wrote:

From: Marius Cristea <[email protected]>

This is the device tree schema for iio driver for Microchip PAC194X and PAC195X series of Power Monitors with Accumulator.

Wrapping of commit message is a little messy. Either just have one paragraph or add some bland lines to make the paragraph breaks look more intentional.

Quoted 28 lines

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.

---
 .../bindings/iio/adc/microchip,pac1944.yaml   | 195 ++++++++++++++++++
 1 file changed, 195 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml
Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml new file +9 -0
new file mode 100644
index 000000000000..1997e889e3f6
 
+
+      microchip,vbus-mode:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:
+          In order to increase measurement resolution and keeping the same
+          number the of bits the device has a configurable VBUS full range
+          scale (FSR). The range should be set by hardware design and it should
+          not be changed during runtime. The bipolar capability for VBUS enables
+          accurate offset measurement and correction.

Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml line 94

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml new file +10 -0
+          The VBUS could be configured into the following full scale range
+            <0>  -  VBUS has unipolar +32V to 0V FSR (default) for PAC195X
+                    or +9V to 0V (default) for PAC194X
+            <1>  -  VBUS has bipolar +32V to -32V FSR for PAC195X
+                    or +9V to -9V for PAC194X. The actual range is limited to
+                    about -200 mV due to the impact of the ESD structures.
+            <2>  -  VBUS has bipolar +16V to -16V FSR for PAC195X
+                    or +4.5V to -4.5V for PAC194X. The actual range is limited
+                    to about -200 mV due to the impact of the ESD structures.
+        maximum: 2

There are examples in tree of multirange devices where we specify the pair of negative and positive limits. That makes for easy to read DT by avoiding the use of enums.

Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml new file +4 -0
+
+      microchip,vsense-mode:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:

Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml line 108

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml new file +10 -0
+          In order to decrease the power dissipation on the shunt resistor and
+          in the same time to increase measurement resolution by keeping the
+          same number the of bits the device has a configurable VSENSE full
+          range scale (FSR). The range should be set by hardware design and it
+          should not be changed during runtime. 
+          The VSENSE could be configured into the following full scale range
+            <0>  -  VSENSE has unipolar +100 mV to 0V FSR (default)
+            <1>  -  VSENSE has bipolar +100 mV to -100 mV FSR
+            <2>  -  VSENSE has bipolar +50 mV to -50 mV FSR
+        maximum: 2

Similar to above. Consider allowing <0, 100>, <-100, 100>. <-50, 50> I'm curious why you've documented them as positive to negative above.

Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml new file +11 -0
+
+      microchip,accumulation-mode:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:
+          The Hardware Accumulator may be used to accumulate VPOWER, VSENSE or
+          VBUS values for any channel. By setting the accumulator for a channel
+          to accumulate the VPOWER values gives a measure of accumulated power
+          into a time period, which is equivalent to energy. Setting the
+          accumulator for a channel to accumulate VSENSE values gives a measure
+          of accumulated current, which is equivalent to charge. This allows the
+          accumulator to be used as a coulomb counter. For either VSENSE or

Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml line 129

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml new file +10 -0
+          VBUS, many samples may be accumulated on chip and the result collected
+          by the host and divided by the accumulator counter count value to
+          yield an average value with a very long integration time to reduce
+          noise. This feature is also very useful for system calibration,
+          allowing many averages to be accumulated for fast averaging/noise
+          reduction.
+          This functionality needs to be setup once and must not be changed
+          during the runtime, just in case the user wants to measure the charge
+          or the energy consumed from board power up till the user has control
+          or during a reboot of the system.    

This one feels like it really isn't a one time thing.

For a few somewhat similarly behaving things (step counts on pedometers for example) we have explicit channel enabled attributes. Here you could just prevent any other actions that would break the accumulation after the software has opted in to enable a particular energy channel. If you need to fake a reset of similar just check the counter and subtract it in software after a channel change.

Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml line 135

Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml new file +6 -0
+          The Hardware Accumulator could be configured to accumulate
+          VPOWER, VSENSE or VBUS
+            <0>  -  Accumulator accumulates VPOWER (default)
+            <1>  -  Accumulator accumulates VSENSE
+            <2>  -  Accumulator accumulates VBUS
+        maximum: 2

If we do keep it in here then have
default: 0 and don't specify it in the examples below.

Jonathan

#4 lore raw

PATCH v2 2/2 iio: adc: adding support for PAC194X

In reply to [email protected]

From: Marius Cristea <[email protected]>

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.

---
 .../ABI/testing/sysfs-bus-iio-adc-pac1944     |  118 +
 MAINTAINERS                                   |    7 +
 drivers/iio/adc/Kconfig                       |   12 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/pac1944.c                     | 3314 +++++++++++++++++
 5 files changed, 3452 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944
 create mode 100644 drivers/iio/adc/pac1944.c
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +118 -0
new file mode 100644
index 000000000000..e4122f58fe39
@@ -0,0 +1,118 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_currentY_shunt_resistor
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		The value of the shunt resistor may be known only at runtime
+		and set by a client application. This attribute allows to
+		set its value in micro-ohms. X is the IIO index of the device.
+		Y is the channel number. The value is used to calculate
+		current, power and accumulated energy.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_ocY_limit_nsamples
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		Number of consecutive samples exceeding the overcurrent limit
+		that are required to trigger the ALERT function for channel Y.
+		Consecutive sample count to trigger could be:
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_ucY_limit_nsamples
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		Number of consecutive samples exceeding the undercurrent limit
+		that are required to trigger the ALERT function for
+		channel Y. Consecutive sample count to trigger could be:
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_opY_limit_nsamples
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		Number of consecutive samples exceeding the overpower limit that
+		are required to trigger the ALERT function for channel Y.
+		Consecutive sample count to trigger could be:
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_ovY_limit_nsamples
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		Number of consecutive samples exceeding the overvoltage limit
+		that are required to trigger the ALERT function for channel Y.
+		Consecutive sample count to trigger could be:
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_uvY_limit_nsamples
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		Number of consecutive samples exceeding the undervoltage limit
+		that are required to trigger the ALERT function for channel Y.
+		Consecutive sample count to trigger could be:
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_acc_fullness
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read/write property used to set a limit for how full the
+		Accumulators and Accumulator Count registers can be before the
+		Accumulator Full and Accumulator Count full limits are tripped.
+		This allows an ALERT to be registered when the Accumulator and
+		Accumulator Count are approaching 100% full.
+		The fullness limit could be set to:
+			- full,
+			- 15/16 full (default),
+			- 7/8 full,
+			- 3/4 full
+
+What:		/sys/bus/iio/devices/iio:deviceX/alert_enable
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read/write property used to enable/disable the limits events
+		and also controls the signals triggered when the Accumulator for
+		any channel overflows or exceeds its fullness or the Accumulator
+		Count overflows or exceeds its fullness limit.
+
+What:		/sys/bus/iio/devices/iio:deviceX/slow_alert1
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read/write property used to route 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
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read/write property used to route a specific ALERT signal to
+		to the GPIO/ALERT2 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:		/sys/bus/iio/devices/iio:deviceX/out_alert_status
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read only property used to determine the cause of ALERT/events
+		being tripped.
MAINTAINERS +7 -0
index 98a3c1e46311..afca53b4788b 100644
@@ -15409,6 +15409,13 @@ S:	Supported
 F:	Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
 F:	drivers/iio/adc/pac1934.c
 
+MICROCHIP PAC1944 ADC DRIVER
+M:	Marius Cristea <[email protected]>
+L:	[email protected]
+S:	Supported
+F:	Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml
+F:	drivers/iio/adc/pac1944.c
+
 MICROCHIP PCI1XXXX GP DRIVER
 M:	Vaibhaav Ram T.L <[email protected]>
 M:	Kumaravel Thiagarajan <[email protected]>
drivers/iio/adc/Kconfig +12 -0
index 849c90203071..66ecafa74da3 100644
@@ -1082,6 +1082,18 @@ config PAC1934
 	  This driver can also be built as a module. If so, the module
 	  will be called pac1934.
 
+config PAC1944
+	tristate "Microchip Technology PAC1944/PAC1954 driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Microchip Technology's PAC1941,
+	  PAC1941-2, PAC1942, PAC1942-2, PAC1943, PAC1944, PAC1951,
+	  PAC1951-2, PAC1952, PAC1952-2, PAC1953, PAC1954
+	  Single/Multi-Channel Power Monitor with Accumulator.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called pac1944.
+
 config PALMAS_GPADC
 	tristate "TI Palmas General Purpose ADC"
 	depends on MFD_PALMAS
drivers/iio/adc/Makefile +1 -0
index ee19afba62b7..311e504103de 100644
@@ -97,6 +97,7 @@ obj-$(CONFIG_NAU7802) += nau7802.o
 obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
 obj-$(CONFIG_PAC1921) += pac1921.o
 obj-$(CONFIG_PAC1934) += pac1934.o
+obj-$(CONFIG_PAC1944) += pac1944.o
 obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
 obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
 obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o
drivers/iio/adc/pac1944.c new file +3314 -0

2,844 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 1 line
2.45.2

Part of the diff is not shown here. View the raw message.

maintainer
#5 lore raw

Re: [PATCH v2 2/2] iio: adc: adding support for PAC194X

Quoted 10 lines · On Mon, 17 Mar 2025 11:08:03 +0200 <[email protected]> wrote:

From: Marius Cristea <[email protected]>

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.

This patch is too large. I'd suggest breaking it down into the core driver, no events, no custom ABI etc. Then propose the ABI stuff on top. That will let reviewers look at stuff that is much more standard before moving on to looking at the new stuff.

Main feedback here is don't invent new ABI without looking very closely at the existing ABI. Maybe I'm missing something, but superficially it feels like a lot of what we have here maps trivially to existing ABI.

There also seems to be quite a bit more ABI that is not in the docs you have added.

Jonathan

Quoted 18 lines
---
 .../ABI/testing/sysfs-bus-iio-adc-pac1944     |  118 +
 MAINTAINERS                                   |    7 +
 drivers/iio/adc/Kconfig                       |   12 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/pac1944.c                     | 3314 +++++++++++++++++
 5 files changed, 3452 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944
 create mode 100644 drivers/iio/adc/pac1944.c
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +0 -0
new file mode 100644
index 000000000000..e4122f58fe39

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 1

Quoted 1 line
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +1 -0
@@ -0,0 +1,118 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_currentY_shunt_resistor

This is a logical extension of in_current_shunt_resistor in the main ABI doc. Just add this indexed variant there instead of in here (which should just be ABI not in line with the main ABI).

Quoted 5 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +5 -0
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		The value of the shunt resistor may be known only at runtime
+		and set by a client application. This attribute allows to

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 16

Quoted 10 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +10 -0
+		set its value in micro-ohms. X is the IIO index of the device.
+		Y is the channel number. The value is used to calculate
+		current, power and accumulated energy.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_ocY_limit_nsamples
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		Number of consecutive samples exceeding the overcurrent limit
+		that are required to trigger the ALERT function for channel Y.

Whilst it will be more complex on the driver, please map this to the main _period event ABI that is time in seconds for which the condition should be true. I think this device has a sequencer that runs these monitoring conditions without us needing to trigger them individually. If so combing these with sampling_frequency value should allow you to use period.

Note that custom ABI is roughly speaking unused ABI, because no standard software will have any idea what it means. So where it is possible to map to ABI that is in existing use, we should do so rather than defining something new.

overcurrent in general usually maps to in_current_thresh_rising* with a specific current threshold value.

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 23

Quoted 7 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +7 -0
+		Consecutive sample count to trigger could be:
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_ucY_limit_nsamples

Undercurrent similarly normally maps to a falling threshold.

Quoted 6 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +6 -0
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		Number of consecutive samples exceeding the undercurrent limit
+		that are required to trigger the ALERT function for
+		channel Y. Consecutive sample count to trigger could be:

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 39

Quoted 10 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +10 -0
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_opY_limit_nsamples
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		Number of consecutive samples exceeding the overpower limit that

in_powerY_thresh_rising_period
I think maps to the equilvalent with units in seconds.

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 50

Quoted 11 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +11 -0
+		are required to trigger the ALERT function for channel Y.
+		Consecutive sample count to trigger could be:
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_ovY_limit_nsamples
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:

in_voltageY_thresh_rising_period.

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 59

Quoted 9 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +9 -0
+		Number of consecutive samples exceeding the overvoltage limit
+		that are required to trigger the ALERT function for channel Y.
+		Consecutive sample count to trigger could be:
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_uvY_limit_nsamples

in_voltageY_thresh_falling_period

Quoted 10 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +10 -0
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		Number of consecutive samples exceeding the undervoltage limit
+		that are required to trigger the ALERT function for channel Y.
+		Consecutive sample count to trigger could be:
+			- 1 sample (default),
+			- 4 samples,
+			- 8 samples,
+			- 16 samples.

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 79

Quoted 10 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +10 -0
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_acc_fullness
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read/write property used to set a limit for how full the
+		Accumulators and Accumulator Count registers can be before the
+		Accumulator Full and Accumulator Count full limits are tripped.
+		This allows an ALERT to be registered when the Accumulator and
+		Accumulator Count are approaching 100% full.

The aim here being to handle roll over? If so then just handle that in the driver by accumulating into a larger store and don't use an event at all.

Quoted 4 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +4 -0
+		The fullness limit could be set to:
+			- full,
+			- 15/16 full (default),
+			- 7/8 full,

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 93

Quoted 10 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +10 -0
+			- 3/4 full
+
+What:		/sys/bus/iio/devices/iio:deviceX/alert_enable
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read/write property used to enable/disable the limits events
+		and also controls the signals triggered when the Accumulator for
+		any channel overflows or exceeds its fullness or the Accumulator
+		Count overflows or exceeds its fullness limit.

You need to wrap this up to use standard ABI as well. Will be a bunch of separate event enables. Then probably filter in driver to only report the ones that are enabled.

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 102

Quoted 9 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +9 -0
+
+What:		/sys/bus/iio/devices/iio:deviceX/slow_alert1
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read/write property used to route 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).

So either:
a) This alert pin is not wired to the host. In which case this should probably be in DT. It reflects a signal used to cause something else to happen and those tend to be disabling power or something along those lines - not typically something we want userspace rewiring.

b) It is wired to the host, then it's not something userspace should play with. The driver should be able to figure out the right interrupts to map to this pin.

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 111

Quoted 9 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +9 -0
+
+What:		/sys/bus/iio/devices/iio:deviceX/gpio_alert2
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read/write property used to route a specific ALERT signal to
+		to the GPIO/ALERT2 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).

Similar to above.

Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 line 118

Quoted 7 lines
Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file +7 -0
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_alert_status
+KernelVersion:	6.13
+Contact:	[email protected]
+Description:
+		A read only property used to determine the cause of ALERT/events
+		being tripped.

Why not report them via the events interface?

Quoted 9 lines
drivers/iio/adc/pac1944.c new file +3 -0
new file mode 100644
index 000000000000..b6f93d21b86b
@@ -0,0 +1,3314 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IIO driver for PAC194X and PAC195X series chips

drivers/iio/adc/pac1944.c line 13

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+ *
+ * Copyright (C) 2022-2025 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Marius Cristea [email protected]
+ *
+ * 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
+ *

No blank line here.

drivers/iio/adc/pac1944.c line 286

Quoted 8 lines
drivers/iio/adc/pac1944.c new file +7 -0
+ */
 
+/*
+ * Universal Unique Identifier (UUID),
+ * 721F1534-5D27-4B60-9DF4-41A3C4B7DA3A,
+ * is reserved to Microchip for the PAC194x and PAC195x.
+ */
+#define PAC1944_DSM_UUID	"721F1534-5D27-4B60-9DF4-41A3C4B7DA3A"

Little advantage in having this up here. I'd push it down where it is used and put the comments there as well.

drivers/iio/adc/pac1944.c line 320

Quoted 8 lines
drivers/iio/adc/pac1944.c new file +8 -0
+
+/*
+ * [(100mV * 1000000) / (2^15)]*10^9  used to calculate the scale
+ * for accumulated current/Coulomb counter
+ */
+#define PAC1944_MAX_VSENSE_NANO			3051757812500UL
+
+#define TO_PAC1944_CHIP_INFO(d) container_of(d, struct pac1944_chip_info, work_chip_rfsh)

to_pac1944_chip_info() would be more in keeping with other container_of macros.

Quoted 19 lines
drivers/iio/adc/pac1944.c new file +19 -0
+
+static int pac1944_disable_alert_reg(struct device *dev, u32 mask, u8 *status)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1944_chip_info *info = iio_priv(indio_dev);
+	struct i2c_client *client = info->client;
+	int ret;
+	u32 val;
+	u8 buf[PAC1944_ALERT_ENABLE_REG_LEN];
+
+	ret = i2c_smbus_read_i2c_block_data(client,
+					    PAC1944_ALERT_ENABLE_REG_ADDR,
+					    PAC1944_ALERT_ENABLE_REG_LEN,
+					    status);
+	if (ret < 0) {
+		dev_err(dev, "failing %s\n", __func__);
+		return ret;
+	}
+

drivers/iio/adc/pac1944.c line 746

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+	val = get_unaligned_be24(status);
+	val = val & (~mask);
+	put_unaligned_be24(val, &buf[0]);
+
+	/* disable appropriate bit from the Alert enable register */
+	ret = i2c_smbus_write_block_data(client, PAC1944_ALERT_ENABLE_REG_ADDR,
+					 PAC1944_ALERT_ENABLE_REG_LEN,
+					 (u8 *)&buf[0]);
+
+	return ret;

return i2c_smbus_write_block_data()

Quoted 14 lines
drivers/iio/adc/pac1944.c new file +13 -0
+}
 
+
+static ssize_t pac1944_alert_status_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1944_chip_info *info = iio_priv(indio_dev);
+	struct i2c_client *client = info->client;
+	int ret;
+	u32 tmp;
+	u8 status[3];
+

drivers/iio/adc/pac1944.c line 1256

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1944_ALERT_STATUS_REG_ADDR,
+					    ARRAY_SIZE(status), (u8 *)status);
+	if (ret < 0) {
+		dev_err(dev, "%s - cannot read PAC1944 regs from 0x%02X\n",
+			__func__, PAC1944_ALERT_STATUS_REG_ADDR);
+		return ret;
+	}
+	tmp = get_unaligned_be24(&status[0]);
+
+	return sysfs_emit(buf, "%u\n", tmp);
	return sysfs_emit(buf, "%u\n", get_unaligned_be24(status));

drivers/iio/adc/pac1944.c line 1326

Quoted 4 lines
drivers/iio/adc/pac1944.c new file +3 -0
+}
 
+
+#define PAC1944_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)

I'd find it clearer if you just used the expanded form of this instead of the macro.

Quoted 11 lines
drivers/iio/adc/pac1944.c new file +11 -0
+static int pac1944_reg_snapshot(struct pac1944_chip_info *info,
+				bool do_refresh, u8 refresh_cmd, u32 wait_time)
+{
+	struct i2c_client *client = info->client;
+	u8 shift, idx;
+	u8 *offset_reg_data_p;
+	int cnt, ret;
+	u32 count, inc_count;
+	u32 fs = 0;
+	s64 stored_value, tmp_s64;
+	s64 inc = 0;

drivers/iio/adc/pac1944.c line 1483

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+	__be16 tmp_be16;
+	u16 smpl_mode;
+	bool is_unipolar;
+
+	guard(mutex)(&info->lock);
+
+	if (do_refresh) {
+		ret = pac1944_send_refresh(info, refresh_cmd, wait_time);
+		if (ret < 0) {
+			dev_err(&client->dev, "%s - cannot send refresh towards PAC1944\n",

Have a local struct device *dev = &client->dev;

drivers/iio/adc/pac1944.c line 1484

Quoted 1 line
drivers/iio/adc/pac1944.c new file +1 -0
+				__func__);

I'd drop the __func__ bit. The message should be enough info anyway.

Quoted 3 lines
drivers/iio/adc/pac1944.c new file +3 -0
+			return ret;
+		}
+	}

...

drivers/iio/adc/pac1944.c line 1632

Quoted 4 lines
drivers/iio/adc/pac1944.c new file +4 -0
+
+		if (info->chip_reg_data.vbus_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG)
+			info->chip_reg_data.vbus[cnt] =
+			sign_extend32(info->chip_reg_data.vbus[cnt], 15);

That indent is very much not style compliant. Add another tab. Check for similar cases (or get an editor that doesn't let you do this particular thing!)

Quoted 52 lines
drivers/iio/adc/pac1944.c new file +51 -0
+
+		offset_reg_data_p += PAC1944_VBUS_SENSE_REG_LEN;
+	}
+
+	for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) {
+		info->chip_reg_data.vsense[cnt] = get_unaligned_be16(offset_reg_data_p);
+
+		if (info->chip_reg_data.vsense_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG)
+			info->chip_reg_data.vsense[cnt] =
+			sign_extend32(info->chip_reg_data.vsense[cnt], 15);
+
+		offset_reg_data_p += PAC1944_VBUS_SENSE_REG_LEN;
+	}
+
+	for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) {
+		info->chip_reg_data.vbus_avg[cnt] = get_unaligned_be16(offset_reg_data_p);
+
+		if (info->chip_reg_data.vbus_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG)
+			info->chip_reg_data.vbus_avg[cnt] =
+			sign_extend32(info->chip_reg_data.vbus_avg[cnt], 15);
+
+		offset_reg_data_p += PAC1944_VBUS_SENSE_REG_LEN;
+	}
+
+	for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) {
+		info->chip_reg_data.vsense_avg[cnt] = get_unaligned_be16(offset_reg_data_p);
+
+		if (info->chip_reg_data.vsense_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG)
+			info->chip_reg_data.vsense_avg[cnt] =
+			sign_extend32(info->chip_reg_data.vsense_avg[cnt], 15);
+
+		offset_reg_data_p += PAC1944_VBUS_SENSE_REG_LEN;
+	}
+
+	for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) {
+		info->chip_reg_data.vpower[cnt] = get_unaligned_be32(offset_reg_data_p) >> 2;
+
+		if (info->chip_reg_data.vbus_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG ||
+		    info->chip_reg_data.vsense_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG)
+			info->chip_reg_data.vpower[cnt] =
+			sign_extend32(info->chip_reg_data.vpower[cnt], 29);
+
+		offset_reg_data_p += PAC1944_VPOWER_REG_LEN;
+	}
+
+	return 0;
+}
 
+static ssize_t pac1944_in_power_acc_scale_show(struct device *dev,
+					       struct device_attribute *attr,
+					       char *buf)
+{

drivers/iio/adc/pac1944.c line 1752

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1944_chip_info *info = iio_priv(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	unsigned int shunt, rem;
+	u64 tmp, ref;
+
+	if (info->is_pac195x_family)
+		ref = (u64)PAC195X_MAX_VPOWER_RSHIFTED_BY_29B;
+	else
+		ref = (u64)PAC194X_MAX_VPOWER_RSHIFTED_BY_29B;

why are the casts needed?

Quoted 9 lines
drivers/iio/adc/pac1944.c new file +9 -0
+
+static ssize_t pac1944_in_enable_acc_store(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1944_chip_info *info = iio_priv(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	int val;

drivers/iio/adc/pac1944.c line 1796

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+
+	if (kstrtouint(buf, 10, &val)) {
+		dev_err(dev, "Value is not valid\n");
+		return -EINVAL;
+	}
+
+	scoped_guard(mutex, &info->lock) {
+		info->enable_acc[this_attr->address] = val ? true : false;
+		if (!val) {
+			info->chip_reg_data.acc_val[this_attr->address] = 0;

Comment needed for this. Why is it reset on disable, rather just before enabling?

Quoted 25 lines
drivers/iio/adc/pac1944.c new file +24 -0
+			info->chip_reg_data.total_samples_nr[this_attr->address] = 0;
+		}
+	}
+
+	return count;
+}
 
+
+static ssize_t pac1944_in_current_acc_scale_show(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1944_chip_info *info = iio_priv(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	int shunt, rem;
+	u64 tmp_u64, ref;
+
+	/*
+	 * Currents - scale for mA - depends on the channel's shunt value
+	 * (100mV * 1000000) / (2^16 * shunt(uOhm))
+	 */
+	ref = (u64)PAC1944_MAX_VSENSE_NANO;
+
+	switch (info->chip_reg_data.vsense_mode[this_attr->address]) {

drivers/iio/adc/pac1944.c line 1846

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+	case PAC1944_UNIPOLAR_FSR_CFG:
+	case PAC1944_BIPOLAR_HALF_FSR_CFG:
+		shunt = info->shunts[this_attr->address];
+		break;
+	case PAC1944_BIPOLAR_FSR_CFG:
+		ref = ref << 1;
+		shunt = info->shunts[this_attr->address];
+		break;
+	default:
+		return 0;

Comment needed on why this is not returning an error.

Quoted 39 lines
drivers/iio/adc/pac1944.c new file +37 -0
+	}
+
+	/*
+	 * Increasing precision
+	 * (100mV * 1000000 * 1000000000) / 2^16 )
+	 */
+	tmp_u64 = div_u64(ref, shunt);
+	rem = do_div(tmp_u64, 1000000000LL);
+
+	return sysfs_emit(buf, "%lld.%09u\n", tmp_u64, rem);
+}
 
 
+
+static int pac1944_frequency_set(struct iio_dev *indio_dev,
+				 const struct iio_chan_spec *chan,
+				 unsigned int mode)
+{
+	struct pac1944_chip_info *info = iio_priv(indio_dev);
+	struct i2c_client *client = info->client;
+	int ret;
+	u16 tmp_u16;
+	__be16 tmp_be16;
+
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1944_CTRL_ACT_REG_ADDR,
+					    sizeof(tmp_u16), (u8 *)&tmp_be16);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "%s - cannot read PAC1944 regs from 0x%02X\n",
+			__func__, PAC1944_CTRL_ACT_REG_ADDR);
+		return ret;
+	}
+
+	tmp_u16 = be16_to_cpu(tmp_be16);
+	tmp_u16 &= ~PAC1944_CTRL_SAMPLE_MASK;
+	tmp_u16 |= FIELD_PREP(PAC1944_CTRL_SAMPLE_MASK, mode);
+	tmp_be16 = cpu_to_be16(tmp_u16);
+
+	scoped_guard(mutex, &info->lock) {
+		ret = i2c_smbus_write_word_data(client, PAC1944_CTRL_REG_ADDR, tmp_be16);

drivers/iio/adc/pac1944.c

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+		if (ret < 0) {
+			dev_err(&indio_dev->dev, "Failed to configure sampling mode\n");
+			return ret;
+		}
+
+		info->sampling_mode = mode;
+		info->chip_reg_data.ctrl_act_reg = tmp_u16
+	}
+
+	ret = pac1944_retrieve_data(info, PAC1944_MIN_UPDATE_WAIT_TIME_US);

Unless that returns positive just do return pac1944_...

drivers/iio/adc/pac1944.c line 2170

Quoted 12 lines
drivers/iio/adc/pac1944.c new file +12 -0
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int pac1944_frequency_get(struct iio_dev *indio_dev,
+				 const struct iio_chan_spec *chan)
+{
+	struct pac1944_chip_info *info;
+
+	info = iio_priv(indio_dev);
	struct pac1944_chip_info *info = iio_priv(indio_dev);
Quoted 3 lines
drivers/iio/adc/pac1944.c new file +3 -0
+
+	return info->sampling_mode;
+}

...

drivers/iio/adc/pac1944.c line 2183

Quoted 3 lines
drivers/iio/adc/pac1944.c new file +3 -0
+
+static const struct iio_chan_spec_ext_info pac1944_ext_info[] = {
+	IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, &sampling_mode_enum),

These are standard ABI, why are they in extinfo?

drivers/iio/adc/pac1944.c line 2190

Quoted 7 lines
drivers/iio/adc/pac1944.c new file +7 -0
+	{
+		.name = "sampling_frequency_available",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = iio_enum_available_read,
+		.private = (uintptr_t)&sampling_mode_enum,
+	},
+	{}
	{ }

would be my preference.

Quoted 6 lines
drivers/iio/adc/pac1944.c new file +4 -0
+};
+
 
 
+static int pac1944_read_thresh(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan, enum iio_event_type type,

drivers/iio/adc/pac1944.c line 2401

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+			       enum iio_event_direction dir, enum iio_event_info info,
+			       int *val, int *val2)
+{
+	struct pac1944_chip_info *chip_info = iio_priv(indio_dev);
+	int idx;
+
+	/* into the datasheet channels are noted from 1 to 4 */
+	idx = chan->channel - 1;
+
+	scoped_guard(mutex, &chip_info->lock) {

As below.

Quoted 27 lines
drivers/iio/adc/pac1944.c new file +27 -0
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				*val = chip_info->overvoltage[idx];
+				return IIO_VAL_INT;
+			case IIO_EV_DIR_FALLING:
+				*val = chip_info->undervoltage[idx];
+				return IIO_VAL_INT;
+			default:
+				return -EINVAL;
+			}
+		case IIO_CURRENT:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				*val = chip_info->overcurrent[idx];
+				return IIO_VAL_INT;
+			case IIO_EV_DIR_FALLING:
+				*val = chip_info->undercurrent[idx];
+				return IIO_VAL_INT;
+			default:
+				return -EINVAL;
+			}
+		case IIO_POWER:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				*val = chip_info->overpower[idx];

drivers/iio/adc/pac1944.c line 2438

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+				return IIO_VAL_INT;
+			default:
+				return -EINVAL;
+			}
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return -EINVAL;

Can't get here.

Quoted 4 lines
drivers/iio/adc/pac1944.c new file +4 -0
+}
+
+static int pac1944_write_thresh(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan, enum iio_event_type type,

drivers/iio/adc/pac1944.c line 2452

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+				enum iio_event_direction dir, enum iio_event_info info,
+				int val, int val2)
+{
+	struct pac1944_chip_info *chip_info = iio_priv(indio_dev);
+	int idx, ret;
+
+	/* into the datasheet channels are noted from 1 to 4 */
+	idx = chan->channel - 1;
+
+	scoped_guard(mutex, &chip_info->lock) {

Use a guard() here

scoped_guard() and unreachable tends to have odd effects on compilers that can get very confused.

Also, simple guard() reduces indent which is a nice to have here.

drivers/iio/adc/pac1944.c line 2462

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = pac1944_update_alert_16b(&indio_dev->dev,
+							       PAC1944_OV_LIMIT_REG_ADDR + idx,
+							       pac1944_overvoltage_mask_tbl[idx],
+							       val);
+				if (!ret)
+					chip_info->overvoltage[idx] = val;

Try to keep the error paths out of line, even if it adds a few lines to the code.

				if (ret)
					return ret;

				chip_info->overvoltage[idx] = val
				return 0;

drivers/iio/adc/pac1944.c

Quoted 1 line
drivers/iio/adc/pac1944.c new file +1 -0
+			return ret;

This is indented one tab too few.

Quoted 70 lines
drivers/iio/adc/pac1944.c new file +69 -0
+			case IIO_EV_DIR_FALLING:
+				ret = pac1944_update_alert_16b(&indio_dev->dev,
+							       PAC1944_UV_LIMIT_REG_ADDR + idx,
+							       pac1944_undervoltage_mask_tbl[idx],
+							       val);
+				if (!ret)
+					chip_info->undervoltage[idx] = val;
+				return ret;
+			default:
+				return -EINVAL;
+			}
+		case IIO_CURRENT:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = pac1944_update_alert_16b(&indio_dev->dev,
+							       PAC1944_OC_LIMIT_REG_ADDR + idx,
+							       pac1944_overcurrent_mask_tbl[idx],
+							       val);
+				if (!ret)
+					chip_info->overcurrent[idx] = val;
+				return ret;
+			case IIO_EV_DIR_FALLING:
+				ret = pac1944_update_alert_16b(&indio_dev->dev,
+							       PAC1944_UC_LIMIT_REG_ADDR + idx,
+							       pac1944_undercurrent_mask_tbl[idx],
+							       val);
+				if (!ret)
+					chip_info->undercurrent[idx] = val;
+				return ret;
+			default:
+				return -EINVAL;
+			}
+		case IIO_POWER:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = pac1944_update_alert_24b(&indio_dev->dev,
+							       PAC1944_OP_LIMIT_REG_ADDR + idx,
+							       pac1944_overpower_mask_tbl[idx],
+							       val);
+				if (!ret)
+					chip_info->overpower[idx] = val;
+				return ret;
+			default:
+				return -EINVAL;
+			}
+		default:
+			return -EINVAL;
+		}
+	}
+	unreachable();
+}
 
+static int pac1944_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      bool state)
+{
+	struct pac1944_chip_info *info = iio_priv(indio_dev);
+	struct i2c_client *client = info->client;
+	int idx, val, mask, ret;
+	bool update = false;
+	u8 tmp[PAC1944_ALERT_ENABLE_REG_LEN];
+
+	/* into the datasheet channels are noted from 1 to 4 */
+	idx = chan->channel - 1;
+
+	switch (chan->type) {
+	case IIO_VOLTAGE:
+		switch (dir) {

drivers/iio/adc/pac1944.c line 2597

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+		case IIO_EV_DIR_RISING:
+			mask = pac1944_overvoltage_mask_tbl[idx];
+			break;
+		case IIO_EV_DIR_FALLING:
+			mask = pac1944_undervoltage_mask_tbl[idx];
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;

Can't get here. So drop this break and similar ones.

Quoted 7 lines
drivers/iio/adc/pac1944.c new file +7 -0
+	case IIO_CURRENT:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			mask = pac1944_overcurrent_mask_tbl[idx];
+			break;
+		case IIO_EV_DIR_FALLING:
+			mask = pac1944_undercurrent_mask_tbl[idx];

drivers/iio/adc/pac1944.c line 2614

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_POWER:
+		if (dir == IIO_EV_DIR_RISING)
+			mask = pac1944_overpower_mask_tbl[idx];
+		else
+			return -EINVAL;

If you are going to check one specific thing, then check for error

		if (dir != IIO_EV_DIR_RISING)
			return -EINVAL;

		mask = pac1944_overpower_mask_tbl[idx];
Quoted 2 lines
drivers/iio/adc/pac1944.c new file +2 -0
+		break;
+	default:

...

drivers/iio/adc/pac1944.c line 2650

Quoted 4 lines
drivers/iio/adc/pac1944.c new file +4 -0
+
+static void pac1944_work_periodic_rfsh(struct work_struct *work)
+{
+	struct pac1944_chip_info *info = TO_PAC1944_CHIP_INFO((struct delayed_work *)work);

Should use to_delayed_work() to get from work to delayed work.

Quoted 45 lines
drivers/iio/adc/pac1944.c new file +45 -0
+	struct i2c_client *client = info->client;
+
+	dev_dbg(&client->dev, "%s - Periodic refresh\n", __func__);
+
+	pac1944_reg_snapshot(info, true, PAC1944_REFRESH_REG_ADDR,
+			     PAC1944_MIN_UPDATE_WAIT_TIME_US);
+
+	schedule_delayed_work(&info->work_chip_rfsh,
+			      msecs_to_jiffies(PAC1944_MAX_RFSH_LIMIT_MS));
+}
+
+/*
+ * documentation related to the ACPI device definition
+ * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/UserGuides/PAC194X_5X-UEFI-BIOS-Integration-and-Microsoft-Windows-10-and-Windows-11-Energy-Meter-Interface-Device-Driver-Users-Guide-DS50003155.pdf
+ */
+static int pac1944_acpi_parse_channel_config(struct i2c_client *client,
+					     struct pac1944_chip_info *info)
+{
+	acpi_handle handle;
+	union acpi_object *rez;
+	struct device *dev = &client->dev;
+	unsigned short bi_dir_mask;
+	int i;
+	guid_t guid;
+	const struct acpi_device_id *id;
+
+	handle = ACPI_HANDLE(dev);
+
+	id = acpi_match_device(dev->driver->acpi_match_table, dev);
+	if (!id)
+		return -ENODEV;
+
+	guid_parse(PAC1944_DSM_UUID, &guid);
+
+	rez = acpi_evaluate_dsm(handle, &guid, 0, PAC1944_ACPI_GET_NAMES, NULL);
+	if (!rez)
+		return -EINVAL;
+
+	for (i = 0; i < rez->package.count; i++) {
+		info->labels[i] = devm_kmemdup(dev, rez->package.elements[i].string.pointer,
+					       (size_t)rez->package.elements[i].string.length + 1,
+					       GFP_KERNEL);
+		info->labels[i][rez->package.elements[i].string.length] = '\0';
+	}
+

drivers/iio/adc/pac1944.c line 2705

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+	ACPI_FREE(rez);
+
+	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1944_ACPI_GET_UOHMS_VALS, NULL);
+	if (!rez)
+		return -EINVAL;
+
+	for (i = 0; i < rez->package.count; i++) {
+		info->shunts[i] = rez->package.elements[i].integer.value;
+		info->active_channels[i] = (info->shunts[i] != 0);
+		info->active_channels_mask |= 1 << i;

set_bit() is a little clearer.

Quoted 12 lines
drivers/iio/adc/pac1944.c new file +12 -0
+
+static int pac1944_of_parse_channel_config(struct i2c_client *client,
+					   struct pac1944_chip_info *info)
+{
+	unsigned int current_channel;
+	struct device *dev = &client->dev;
+	int idx, ret, temp;
+
+	current_channel = 1;
+
+	device_for_each_child_node_scoped(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &idx);

drivers/iio/adc/pac1944.c line 2772

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+		if (ret)
+			return dev_err_probe(dev, ret, "reading invalid channel index\n");
+
+		/* adjust idx to match channel index (1 to 4) from the datasheet */
+		idx--;
+
+		if (current_channel >= (info->phys_channels + 1) ||
+		    idx >= info->phys_channels || idx < 0)
+			return dev_err_probe(&client->dev, -EINVAL,
+					     "invalid channel_index %d value\n", (idx + 1));

Those inner () aren't adding anything, so drop them.

Quoted 3 lines
drivers/iio/adc/pac1944.c new file +2 -0
+
+		/* enable channel */

...

Quoted 16 lines
drivers/iio/adc/pac1944.c new file +16 -0
+
+static int pac1944_prep_iio_channels(struct pac1944_chip_info *info,
+				     struct iio_dev *indio_dev)
+{
+	struct device *dev = &info->client->dev;
+	struct iio_chan_spec *ch_sp;
+	int channel_size, attribute_count, cnt;
+	void *dyn_ch_struct;
+
+	/* Finding out dynamically how many IIO channels we need */
+	attribute_count = 0;
+	channel_size = 0;
+
+	for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) {
+		/* add the size of the properties of one chip physical channel */
+		channel_size += sizeof(pac1944_single_channel);

drivers/iio/adc/pac1944.c line 3094

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+		/* count how many enabled channels we have */
+		attribute_count += ARRAY_SIZE(pac1944_single_channel);
+		dev_dbg(dev, ":%s: Channel %d active\n", __func__, cnt + 1);
+	}
+
+	dyn_ch_struct = devm_kzalloc(dev, channel_size, GFP_KERNEL);
+	if (!dyn_ch_struct)
+		return -ENOMEM;
+
+	ch_sp = (struct iio_chan_spec *)dyn_ch_struct;

why jump through a void *? Just assign

	ch_sp = devm_kzalloc();
	if (!ch_sp)
		return -ENOMEM;
Quoted 16 lines
drivers/iio/adc/pac1944.c new file +16 -0
+	/* Populate the dynamic channels and make all the adjustments */
+	for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) {
+		memcpy(ch_sp, pac1944_single_channel, sizeof(pac1944_single_channel));
+		/*
+		 * Into the datasheet channels are noted from 1 to 4 so we will adjust
+		 * the channel to match channel index (1 to 4) from the datasheet
+		 */
+		ch_sp[PAC1944_CH_POWER].channel = cnt + 1;
+		ch_sp[PAC1944_CH_POWER].address = cnt + PAC1944_VPOWER_1_ADDR;
+		ch_sp[PAC1944_CH_VOLTAGE].channel = cnt + 1;
+		ch_sp[PAC1944_CH_VOLTAGE].address = cnt + PAC1944_VBUS_1_ADDR;
+		ch_sp[PAC1944_CH_CURRENT].channel = cnt + 1;
+		ch_sp[PAC1944_CH_CURRENT].address = cnt + PAC1944_VSENSE_1_ADDR;
+		/*
+		 * In order to be able to use labels for PAC1944_CH_VOLTAGE and
+		 * PAC1944_CH_VOLTAGE_AVERAGE, respectively PAC1944_CH_CURRENT

drivers/iio/adc/pac1944.c line 3120

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+		 * and PAC1944_CH_CURRENT_AVERAGE we need to use different channel numbers.
+		 * We will add  +5 (+1 to maximum PAC channels).
+		 */
+		ch_sp[PAC1944_CH_VOLTAGE_AVERAGE].channel = cnt + PAC1944_MAX_CH + 1;
+		ch_sp[PAC1944_CH_VOLTAGE_AVERAGE].address = cnt + PAC1944_VBUS_AVG_1_ADDR;
+		ch_sp[PAC1944_CH_CURRENT_AVERAGE].channel = cnt + PAC1944_MAX_CH + 1;
+		ch_sp[PAC1944_CH_CURRENT_AVERAGE].address = cnt + PAC1944_VSENSE_AVG_1_ADDR;
+
+		/* advance the pointer */
+		ch_sp = (void *)ch_sp + sizeof(pac1944_single_channel);

Keep the type as that ch_spec and do

		ch_sp += ARRAY_SIZE(pac1944_single_channel);

drivers/iio/adc/pac1944.c line 3128

Quoted 8 lines
drivers/iio/adc/pac1944.c new file +8 -0
+	}
+
+	/*
+	 * Send the updated dynamic channel structure information towards IIO
+	 * prepare the required field for IIO class registration
+	 */
+	indio_dev->num_channels = attribute_count;
+	indio_dev->channels = (const struct iio_chan_spec *)dyn_ch_struct;

That cast isn't needed. Not sure why there is one in the pac1921 driver in similar place.

Quoted 25 lines
drivers/iio/adc/pac1944.c new file +23 -0
+
+	return 0;
+}
 
 
+static int pac1944_probe(struct i2c_client *client)
+{
+	struct pac1944_chip_info *info;
+	struct iio_dev *indio_dev;
+	const struct pac1944_features *chip;
+	int cnt, ret;
+	struct device *dev = &client->dev;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+	info->client = client;
+
+	ret = pac1944_chip_identify(info);
+	if (ret < 0) {
+		/*
+		 * If failed to identify the hardware based on internal
+		 * registers, try using fallback compatible in device

drivers/iio/adc/pac1944.c line 3171

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+		 * tree to deal with some newer part number.
+		 */
+		chip = i2c_get_match_data(client);
+		if (!chip)
+			return -EINVAL;
+
+		info->chip_variant = chip->prod_id;
+		info->phys_channels = chip->phys_channels;
+		indio_dev->name = chip->name;
+	} else {

If the FW disagrees with what we found it is also nice to print a message to that effect. Not vital, but nice to have.

Quoted 3 lines
drivers/iio/adc/pac1944.c new file +3 -0
+		info->phys_channels = pac1944_chip_config[ret].phys_channels;
+		indio_dev->name = pac1944_chip_config[ret].name;
+	}

...

Quoted 6 lines
drivers/iio/adc/pac1944.c new file +6 -0
+}
+
+static const struct i2c_device_id pac1944_id[] = {
+	{ .name = "pac1941", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1941] },
+	{ .name = "pac19412", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1941_2] },
+	{ .name = "pac1942", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1942] },

drivers/iio/adc/pac1944.c line 3237

Quoted 10 lines
drivers/iio/adc/pac1944.c new file +10 -0
+	{ .name = "pac19422", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1942_2] },
+	{ .name = "pac1943", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1943] },
+	{ .name = "pac1944", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1944] },
+	{ .name = "pac1951", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1951] },
+	{ .name = "pac19512", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1951_2] },
+	{ .name = "pac1952", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1952] },
+	{ .name = "pac19522", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1952_2] },
+	{ .name = "pac1953", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1953] },
+	{ .name = "pac1954", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1954] },
+	{}

as below.

Quoted 8 lines
drivers/iio/adc/pac1944.c new file +8 -0
+};
+MODULE_DEVICE_TABLE(i2c, pac1944_id);
+
+static const struct of_device_id pac1944_of_match[] = {
+	{
+		.compatible = "microchip,pac1941",
+		.data = (void *)&pac1944_chip_config[PAC1941]
+	},

...

drivers/iio/adc/pac1944.c line 3290

Quoted 5 lines
drivers/iio/adc/pac1944.c new file +5 -0
+	{
+		.compatible = "microchip,pac1954",
+		.data = (void *)&pac1944_chip_config[PAC1954]
+	},
+	{}

As below. Totally trivial but nice to tidy up

drivers/iio/adc/pac1944.c line 3296

Quoted 6 lines
drivers/iio/adc/pac1944.c new file +6 -0
+};
+MODULE_DEVICE_TABLE(of, pac1944_of_match);
+
+static const struct acpi_device_id pac1944_acpi_match[] = {
+	{ "MCHP1940", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1944] },
+	{}

I'm slowly trying to standardize in IIO on one format for these and the random choice I made a while back was

	{ }

so with the space. It isn't really better than what you have here, but consistency is nice to have.

drivers/iio/adc/pac1944.c line 3309

Quoted 13 lines
drivers/iio/adc/pac1944.c new file +13 -0
+};
+MODULE_DEVICE_TABLE(acpi, pac1944_acpi_match);
+
+static struct i2c_driver pac1944_driver = {
+	.driver = {
+		.name = "pac1944",
+		.of_match_table = pac1944_of_match,
+		.acpi_match_table = pac1944_acpi_match
+	},
+	.probe = pac1944_probe,
+	.id_table = pac1944_id,
+};
+

Common practice is to tightly couple this with the struct i2c_driver definition above by not having a blank line here. (trivial!)

Quoted 1 line

+module_i2c_driver(pac1944_driver);

#6 lore raw

Re: [PATCH v2 2/2] iio: adc: adding support for PAC194X

In reply to [email protected]
kernel test robot noticed the following build warnings:

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 577a66e2e634f712384c57a98f504c44ea4b47da]

url:    https://github.com/intel-lab-lkp/linux/commits/marius-cristea-microchip-com/dt-bindings-iio-adc-adding-support-for-PAC194X/20250317-171150
base:   577a66e2e634f712384c57a98f504c44ea4b47da
patch link:    https://lore.kernel.org/r/20250317090803.30003-3-marius.cristea%40microchip.com
patch subject: [PATCH v2 2/2] iio: adc: adding support for PAC194X
config: sparc-randconfig-r073-20250318 (https://download.01.org/0day-ci/archive/20250318/[email protected]/config)
compiler: sparc64-linux-gcc (GCC) 12.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250318/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/pac1944.c:614: warning: Function parameter or struct member 'active_channels_mask' not described in 'pac1944_chip_info'

vim +614 drivers/iio/adc/pac1944.c

546
547 /**
548 * struct pac1944_chip_info - chip configuration
549 * @channels: array of values, true means that channel is active
550 * @iio_info: pointer to iio_info structure
551 * @client: a pointer to the i2c client associated with the device
552 * @lock: lock to prevent concurrent reads/writes
553 * @work_chip_rfsh: chip refresh workqueue implementation
554 * @phys_channels: number of physical channels for the device
555 * @active_channels: array of values, true means that channel is active
556 * @chip_variant: stores the type of the device
557 * @chip_revision: store the silicon revision version of the device
558 * @shunts: array of values, shunt resistor values
559 * @chip_reg_data: pointer to structure, containing data from the device registers
560 * @sample_rate_value: sampling frequency
561 * @labels: array of string, name of each channel
562 * @is_pac195x_family: true if device is part of the PAC195x family
563 * @sampling_mode: sampling mode used by the device
564 * @num_enabled_channels: count of how many chip channels are currently enabled
565 * @slow_alert1: snapshot of slow/alert register
566 * @gpio_alert2: snapshot of gpio/alert register
567 * @acc_fullness: snapshot of accumulator fullness limit register
568 * @overcurrent: array of values, overcurrent limit
569 * @undercurrent: array of values, undercurrent limit
570 * @overpower: array of values, overpower limit
571 * @overvoltage: array of values, overvoltage limit
572 * @undervoltage: array of values, undervoltage limit
573 * @oc_limit_nsamples: number of consecutive samples exceeding the overcurrent limit
574 * @uc_limit_nsamples: number of consecutive samples exceeding the undercurrent limit
575 * @op_limit_nsamples: number of consecutive samples exceeding the overpower limit
576 * @ov_limit_nsamples: number of consecutive samples exceeding the overvoltage limit
577 * @uv_limit_nsamples: number of consecutive samples exceeding the undervoltage limit
578 * @alert_enable: snapshot of alert enable register
579 * @enable_acc: array of values, true means that accumulation channel is measured
580 */
581 struct pac1944_chip_info {
582 const struct iio_chan_spec *channels;
583 struct iio_info iio_info;
584 struct i2c_client *client;
585 struct mutex lock; /* lock to prevent concurrent reads/writes */
586 struct delayed_work work_chip_rfsh;
587 u8 phys_channels;
588 bool active_channels[PAC1944_MAX_CH];
589 unsigned long active_channels_mask;
590 u8 chip_variant;
591 u8 chip_revision;
592 u32 shunts[PAC1944_MAX_CH];
593 struct reg_data chip_reg_data;
594 s32 sample_rate_value;
595 char *labels[PAC1944_MAX_CH];
596 bool is_pac195x_family;
597 u8 sampling_mode;
598 u8 num_enabled_channels;
599 u32 slow_alert1;
600 u32 gpio_alert2;
601 u16 acc_fullness;
602 u16 overcurrent[PAC1944_MAX_CH];
603 u16 undercurrent[PAC1944_MAX_CH];
604 u32 overpower[PAC1944_MAX_CH];
605 u16 overvoltage[PAC1944_MAX_CH];
606 u16 undervoltage[PAC1944_MAX_CH];
607 u8 oc_limit_nsamples[PAC1944_MAX_CH];
608 u8 uc_limit_nsamples[PAC1944_MAX_CH];
609 u8 op_limit_nsamples[PAC1944_MAX_CH];
610 u8 ov_limit_nsamples[PAC1944_MAX_CH];
611 u8 uv_limit_nsamples[PAC1944_MAX_CH];
612 u32 alert_enable;
613 bool enable_acc[PAC1944_MAX_CH];

 > 614	};

615

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
#7 lore raw

Re: [PATCH v2 2/2] iio: adc: adding support for PAC194X

In reply to [email protected]
kernel test robot noticed the following build warnings:

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 577a66e2e634f712384c57a98f504c44ea4b47da]

url:    https://github.com/intel-lab-lkp/linux/commits/marius-cristea-microchip-com/dt-bindings-iio-adc-adding-support-for-PAC194X/20250317-171150
base:   577a66e2e634f712384c57a98f504c44ea4b47da
patch link:    https://lore.kernel.org/r/20250317090803.30003-3-marius.cristea%40microchip.com
patch subject: [PATCH v2 2/2] iio: adc: adding support for PAC194X
config: riscv-randconfig-r072-20250318 (https://download.01.org/0day-ci/archive/20250318/[email protected]/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 87916f8c32ebd8e284091db9b70339df57fd1e90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250318/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from drivers/iio/adc/pac1944.c:20:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:22:
In file included from arch/riscv/include/asm/sections.h:9:
In file included from include/linux/mm.h:2223:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]

     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/pac1944.c:1689:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1689 |         if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1690 |                        msecs_to_jiffies(PAC1944_MIN_POLLING_TIME_MS))) {
         |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/jiffies.h:128:2: note: expanded from macro 'time_after'
     128 |         (typecheck(unsigned long, a) && \
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     129 |          typecheck(unsigned long, b) && \
         |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     130 |          ((long)((b) - (a)) < 0))
         |          ~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/adc/pac1944.c:1707:9: note: uninitialized use occurs here
    1707 |         return ret;
         |                ^~~
   drivers/iio/adc/pac1944.c:1689:2: note: remove the 'if' if its condition is always true
    1689 |         if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1690 |                        msecs_to_jiffies(PAC1944_MIN_POLLING_TIME_MS))) {
         |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/adc/pac1944.c:1683:9: note: initialize the variable 'ret' to silence this warning
    1683 |         int ret;
         |                ^
         |                 = 0

5 warnings generated.

vim +1689 drivers/iio/adc/pac1944.c

1680
1681 static int pac1944_retrieve_data(struct pac1944_chip_info *info, u32 wait_time)
1682 {
1683 int ret;
1684
1685 /*
1686 * Check if the minimal elapsed time has passed and if so,
1687 * re-read the chip, otherwise the cached info is just fine
1688 */

> 1689		if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +

1690 msecs_to_jiffies(PAC1944_MIN_POLLING_TIME_MS))) {
1691 /*
1692 * We need to re-read the chip values
1693 * call the pac1944_reg_snapshot
1694 */
1695 ret = pac1944_reg_snapshot(info, true,
1696 PAC1944_REFRESH_REG_ADDR,
1697 wait_time);
1698 /*
1699 * Re-schedule the work for the read registers timeout
1700 * (to prevent chip regs saturation)
1701 */
1702 cancel_delayed_work_sync(&info->work_chip_rfsh);
1703 schedule_delayed_work(&info->work_chip_rfsh,
1704 msecs_to_jiffies(PAC1944_MAX_RFSH_LIMIT_MS));
1705 }
1706
1707 return ret;
1708 }
1709

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
#8 lore raw

Re: [PATCH v2 2/2] iio: adc: adding support for PAC194X

In reply to [email protected]
kernel test robot noticed the following build warnings:

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 577a66e2e634f712384c57a98f504c44ea4b47da]

url:    https://github.com/intel-lab-lkp/linux/commits/marius-cristea-microchip-com/dt-bindings-iio-adc-adding-support-for-PAC194X/20250317-171150
base:   577a66e2e634f712384c57a98f504c44ea4b47da
patch link:    https://lore.kernel.org/r/20250317090803.30003-3-marius.cristea%40microchip.com
patch subject: [PATCH v2 2/2] iio: adc: adding support for PAC194X
config: microblaze-randconfig-r131-20250319 (https://download.01.org/0day-ci/archive/20250319/[email protected]/config)
compiler: microblaze-linux-gcc (GCC) 7.5.0
reproduce: (https://download.01.org/0day-ci/archive/20250319/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)

>> drivers/iio/adc/pac1944.c:813:55: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected unsigned short [usertype] value @@     got restricted __be16 [assigned] [usertype] tmp_be16 @@
   drivers/iio/adc/pac1944.c:813:55: sparse:     expected unsigned short [usertype] value
   drivers/iio/adc/pac1944.c:813:55: sparse:     got restricted __be16 [assigned] [usertype] tmp_be16
>> drivers/iio/adc/pac1944.c:2148:80: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected unsigned short [usertype] value @@     got restricted __be16 [addressable] [assigned] [usertype] tmp_be16 @@
   drivers/iio/adc/pac1944.c:2148:80: sparse:     expected unsigned short [usertype] value
   drivers/iio/adc/pac1944.c:2148:80: sparse:     got restricted __be16 [addressable] [assigned] [usertype] tmp_be16
>> drivers/iio/adc/pac1944.c:2936:79: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected unsigned short [usertype] value @@     got restricted __be16 [usertype] @@

drivers/iio/adc/pac1944.c:2936:79: sparse: expected unsigned short [usertype] value
drivers/iio/adc/pac1944.c:2936:79: sparse: got restricted __be16 [usertype]
drivers/iio/adc/pac1944.c:2953:72: sparse: sparse: incorrect type in argument 3 (different base types) @@ expected unsigned short [usertype] value @@ got restricted __be16 [usertype] @@
drivers/iio/adc/pac1944.c:2953:72: sparse: expected unsigned short [usertype] value
drivers/iio/adc/pac1944.c:2953:72: sparse: got restricted __be16 [usertype]

vim +813 drivers/iio/adc/pac1944.c

794
795 static int pac1944_update_alert_16b(struct device *dev, u8 addr,
796 u32 mask, u16 value)
797 {
798 struct iio_dev *indio_dev = dev_to_iio_dev(dev);
799 struct pac1944_chip_info *info = iio_priv(indio_dev);
800 struct i2c_client *client = info->client;
801 int ret;
802 __be16 tmp_be16;
803 u8 status[PAC1944_ALERT_ENABLE_REG_LEN];
804
805 ret = pac1944_disable_alert_reg(dev, mask, &status[0]);
806 if (ret) {
807 dev_err(dev, "failing to write %s\n", __func__);
808 return ret;
809 }
810
811 tmp_be16 = cpu_to_be16(value);
812

 > 813		ret = i2c_smbus_write_word_data(client, addr, tmp_be16);

814 if (ret) {
815 dev_err(dev, "failing to write %s\n", __func__);
816 return ret;
817 }
818
819 return pac1944_restore_alert_reg(indio_dev, &status[0]);
820 }
821

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
#9 lore raw

Re: [PATCH v2 2/2] iio: adc: adding support for PAC194X

In reply to [email protected]

Hi,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/marius-cristea-microchip-com/dt-bindings-iio-adc-adding-support-for-PAC194X/20250317-171150
base:   577a66e2e634f712384c57a98f504c44ea4b47da
patch link:    https://lore.kernel.org/r/20250317090803.30003-3-marius.cristea%40microchip.com
patch subject: [PATCH v2 2/2] iio: adc: adding support for PAC194X
config: arm64-randconfig-r071-20250322 (https://download.01.org/0day-ci/archive/20250323/[email protected]/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project c2692afc0a92cd5da140dfcdfff7818a5b8ce997)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags

| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/

smatch warnings:
drivers/iio/adc/pac1944.c:1707 pac1944_retrieve_data() error: uninitialized symbol 'ret'. drivers/iio/adc/pac1944.c:2463 pac1944_write_thresh() warn: inconsistent indenting

vim +/ret +1707 drivers/iio/adc/pac1944.c

e227d6e5b38646 Marius Cristea 2025-03-17  1681  static int pac1944_retrieve_data(struct pac1944_chip_info *info, u32 wait_time)
e227d6e5b38646 Marius Cristea 2025-03-17  1682  {
e227d6e5b38646 Marius Cristea 2025-03-17  1683  	int ret;
e227d6e5b38646 Marius Cristea 2025-03-17  1684  
e227d6e5b38646 Marius Cristea 2025-03-17  1685  	/*
e227d6e5b38646 Marius Cristea 2025-03-17  1686  	 * Check if the minimal elapsed time has passed and if so,
e227d6e5b38646 Marius Cristea 2025-03-17  1687  	 * re-read the chip, otherwise the cached info is just fine
e227d6e5b38646 Marius Cristea 2025-03-17  1688  	 */
e227d6e5b38646 Marius Cristea 2025-03-17  1689  	if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
e227d6e5b38646 Marius Cristea 2025-03-17  1690  		       msecs_to_jiffies(PAC1944_MIN_POLLING_TIME_MS))) {
e227d6e5b38646 Marius Cristea 2025-03-17  1691  		/*
e227d6e5b38646 Marius Cristea 2025-03-17  1692  		 * We need to re-read the chip values
e227d6e5b38646 Marius Cristea 2025-03-17  1693  		 * call the pac1944_reg_snapshot
e227d6e5b38646 Marius Cristea 2025-03-17  1694  		 */
e227d6e5b38646 Marius Cristea 2025-03-17  1695  		ret = pac1944_reg_snapshot(info, true,
e227d6e5b38646 Marius Cristea 2025-03-17  1696  					   PAC1944_REFRESH_REG_ADDR,
e227d6e5b38646 Marius Cristea 2025-03-17  1697  					   wait_time);
e227d6e5b38646 Marius Cristea 2025-03-17  1698  		/*
e227d6e5b38646 Marius Cristea 2025-03-17  1699  		 * Re-schedule the work for the read registers timeout
e227d6e5b38646 Marius Cristea 2025-03-17  1700  		 * (to prevent chip regs saturation)
e227d6e5b38646 Marius Cristea 2025-03-17  1701  		 */
e227d6e5b38646 Marius Cristea 2025-03-17  1702  		cancel_delayed_work_sync(&info->work_chip_rfsh);
e227d6e5b38646 Marius Cristea 2025-03-17  1703  		schedule_delayed_work(&info->work_chip_rfsh,
e227d6e5b38646 Marius Cristea 2025-03-17  1704  				      msecs_to_jiffies(PAC1944_MAX_RFSH_LIMIT_MS));
e227d6e5b38646 Marius Cristea 2025-03-17  1705  	}

ret isn't initialized on else path.

e227d6e5b38646 Marius Cristea 2025-03-17 1706
e227d6e5b38646 Marius Cristea 2025-03-17 @1707 return ret;
e227d6e5b38646 Marius Cristea 2025-03-17 1708 }

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki