[PATCH 0/2] Adding support for Microchip PAC1711

[PATCH 0/2] Adding support for Microchip PAC1711

[Ariana Lazar]

The PAC1711 product is a single-channel power monitor with accumulator.
The device uses 12-bit resolution for voltage and current measurements and
24 bits power calculations. The accumulator register (56-bit) could
accumulate power (energy), current (Coulomb counter) or voltage.

PAC1711 measures up to 42V Full-Scale Range.

Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
---
Ariana Lazar (2):
      dt-bindings: iio: adc: adding support for PAC1711
      iio: adc: adding support for PAC1711

 .../ABI/testing/sysfs-bus-iio-adc-pac1711          |   57 +
 .../bindings/iio/adc/microchip,pac1711.yaml        |  195 +++
 MAINTAINERS                                        |    8 +
 drivers/iio/adc/Kconfig                            |   10 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/pac1711.c                          | 1448 ++++++++++++++++++++
 6 files changed, 1719 insertions(+)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250901-pac1711-d3bacda400fd

Best regards,
-- 
Ariana Lazar <ariana.lazar@microchip.com>

[PATCH 1/2] dt-bindings: iio: adc: adding support for PAC1711

[Ariana Lazar]

This is the device tree schema for Microchip PAC1711 single-channel power
monitor with accumulator. The device uses 12-bit resolution for voltage and
current measurements and 24 bits power calculations. The device supports
one 56-bit accumulator register.

PAC1711 measures up to 42V Full-Scale Range.

Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
---
 .../bindings/iio/adc/microchip,pac1711.yaml        | 195 +++++++++++++++++++++
 MAINTAINERS                                        |   6 +
 2 files changed, 201 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..67edd778981c2f0ed21dda02f14e383a153169b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
@@ -0,0 +1,195 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/microchip,pac1711.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PAC1711 Power Monitors with Accumulator
+
+maintainers:
+  - Ariana Lazar <ariana.lazar@microchip.com>
+
+description: |
+  This device is part of the Microchip family of Power Monitors with Accumulator.
+  The datasheet for PAC1711 can be found here:
+  https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/PAC1711-Data-Sheet-DS20007058.pdf
+
+properties:
+  compatible:
+    enum:
+      - microchip,pac1711
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  "#io-channel-cells":
+    const: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  gpio-controller:
+    description: Marks the device node as a GPIO controller.
+
+  "#gpio-cells":
+    const: 2
+    description:
+      The first cell is the GPIO number and the second cell specifies
+      GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
+
+  powerdown-gpios:
+    description:
+      Active low puts the device in power-down state. When the PWRDN pin is
+      pulled high, measurement and accumulation will resume using the default
+      register settings.
+    maxItems: 1
+
+  interrupts:
+    maxItems: 2
+
+  interrupt-names:
+    description:
+      Could be triggered by overvoltage, undervoltage, overcurrent, overpower,
+      undercurrent, step limit, accumulator overflow and accumulator count
+      overflow.
+    items:
+      - const: alert0
+      - const: alert1
+
+  shunt-resistor-micro-ohms:
+    description:
+      Value in micro Ohms of the shunt resistor connected between
+      the VSENSEP and VSENSEN inputs, across which the current is measured.
+      Value is needed to compute the scaling of the measured current.
+
+  label:
+    description: Unique name to identify which device this is.
+
+  microchip,gpio:
+    type: boolean
+    description:
+      In default mode, A0 pin is a GPIO 0 input pin, respectively A1 pin is
+      GPIO 1. The pins can be used for the SLOW function, the device will sample
+      at 8 samples/second if pulled high. A0 also function as the Alert0 and A1
+      as Alert1, but can no longer be used to control conversion rate or SLOW.
+
+  microchip,vbus-input-range-microvolt:
+    description: |
+      Specifies the voltage range in microvolts chosen for the voltage full
+      scale range (FSR). The range should be set as <minimum, maximum> by
+      hardware design and should not be changed during runtime.
+
+      The VBUS could be configured into the following full scale range:
+        -  VBUS has unipolar 0V to 42V FSR (default)
+        -  VBUS has bipolar -42V to 42V FSR
+        -  VBUS has bipolar -21V to 21V FSR
+    items:
+      - enum: [-42000000, -21000000, 0]
+      - enum: [21000000, 42000000]
+
+  microchip,vsense-input-range-microvolt:
+    description: |
+      Specifies the voltage range in microvolts chosen for the current full
+      scale range (FSR). The current is calculated by dividing the vsense
+      voltage by the value of the shunt resistor. The range should be set as
+      <minimum, maximum> by hardware design and it should not be changed during
+      runtime.
+
+      The VSENSE could be configured into the following full scale range:
+        -  VSENSE has unipolar 0 mV to 100V FSR (default)
+        -  VSENSE has bipolar -100 mV to 100 mV FSR
+        -  VSENSE has bipolar -50 mV to 50 mV FSR
+    items:
+      - enum: [-100000, -50000, 0]
+      - enum: [50000, 100000]
+
+  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.
+
+      The Hardware Accumulator could be configured as:
+       <0>  -  Accumulator accumulates VPOWER (default)
+       <1>  -  Accumulator accumulates VSENSE
+       <2>  -  Accumulator accumulates VBUS
+    maximum: 2
+    default: 0
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - shunt-resistor-micro-ohms
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - microchip,pac1711
+    then:
+      properties:
+        microchip,vbus-input-range-microvolt:
+          oneOf:
+            - items:
+                - const: 0
+                - const: 42000000
+            - items:
+                - const: -42000000
+                - const: 42000000
+            - items:
+                - const: -21000000
+                - const: 21000000
+          default:
+            items:
+              - const: 0
+              - const: 42000000
+
+        microchip,vsense-input-range-microvolt:
+          oneOf:
+            - items:
+                - const: 0
+                - const: 100000
+            - items:
+                - const: -100000
+                - const: 100000
+            - items:
+                - const: -50000
+                - const: 50000
+          default:
+            items:
+              - const: 0
+              - const: 100000
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pac1711@10 {
+            compatible = "microchip,pac1711";
+            reg = <0x10>;
+
+            shunt-resistor-micro-ohms = <10000>;
+            label = "VDD3V3";
+            vdd-supply = <&vdd>;
+            microchip,vbus-input-range-microvolt = <(-21000000) 21000000>;
+            microchip,vsense-input-range-microvolt = <(-50000) 50000>;
+            microchip,accumulation-mode = <0>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index a92290fffa163f9fe8fe3f04bf66426f9a894409..7686e2516c90442aa3e23d19cfb08e280a44ba76 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16337,6 +16337,12 @@ F:	Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
 F:	drivers/nvmem/microchip-otpc.c
 F:	include/dt-bindings/nvmem/microchip,sama7g5-otpc.h
 
+MICROCHIP PAC1711 DAC DRIVER
+M:	Ariana Lazar <ariana.lazar@microchip.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
+
 MICROCHIP PAC1921 POWER/CURRENT MONITOR DRIVER
 M:	Matteo Martelli <matteomartelli3@gmail.com>
 L:	linux-iio@vger.kernel.org

-- 
2.43.0

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

[Conor Dooley]

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

Hey,

On Wed, Oct 15, 2025 at 01:12:15PM +0300, Ariana Lazar wrote:
> This is the device tree schema for Microchip PAC1711 single-channel power
> monitor with accumulator. The device uses 12-bit resolution for voltage and
> current measurements and 24 bits power calculations. The device supports
> one 56-bit accumulator register.
> 
> PAC1711 measures up to 42V Full-Scale Range.
> 
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
> ---
>  .../bindings/iio/adc/microchip,pac1711.yaml        | 195 +++++++++++++++++++++
>  MAINTAINERS                                        |   6 +
>  2 files changed, 201 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..67edd778981c2f0ed21dda02f14e383a153169b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
> @@ -0,0 +1,195 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1711.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PAC1711 Power Monitors with Accumulator
> +
> +maintainers:
> +  - Ariana Lazar <ariana.lazar@microchip.com>
> +
> +description: |
> +  This device is part of the Microchip family of Power Monitors with Accumulator.
> +  The datasheet for PAC1711 can be found here:
> +  https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/PAC1711-Data-Sheet-DS20007058.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,pac1711
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0

There are no child nodes here, so I don't think size or address cells are
needed.

> +
> +  gpio-controller:
> +    description: Marks the device node as a GPIO controller.
> +
> +  "#gpio-cells":
> +    const: 2
> +    description:
> +      The first cell is the GPIO number and the second cell specifies
> +      GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
> +
> +  powerdown-gpios:
> +    description:
> +      Active low puts the device in power-down state. When the PWRDN pin is
> +      pulled high, measurement and accumulation will resume using the default
> +      register settings.
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 2
> +
> +  interrupt-names:
> +    description:
> +      Could be triggered by overvoltage, undervoltage, overcurrent, overpower,
> +      undercurrent, step limit, accumulator overflow and accumulator count
> +      overflow.
> +    items:
> +      - const: alert0
> +      - const: alert1

This won't allow using alert0 but not alert1, or vice versa. Is that
intended? I would imagine that only using one of the interrupt pins is a
valid behaviour.

> +
> +  shunt-resistor-micro-ohms:
> +    description:
> +      Value in micro Ohms of the shunt resistor connected between
> +      the VSENSEP and VSENSEN inputs, across which the current is measured.
> +      Value is needed to compute the scaling of the measured current.
> +
> +  label:
> +    description: Unique name to identify which device this is.
> +
> +  microchip,gpio:
> +    type: boolean
> +    description:
> +      In default mode, A0 pin is a GPIO 0 input pin, respectively A1 pin is
> +      GPIO 1. The pins can be used for the SLOW function, the device will sample
> +      at 8 samples/second if pulled high. A0 also function as the Alert0 and A1
> +      as Alert1, but can no longer be used to control conversion rate or SLOW.

This description provides zero detail about what the property actually
does. The driver doesn't appear to use it either, so no hints for me
there. If it is something to do with deciding if pins are interrupts or
are free for gpio use, can't that just be determined based on what
interrupts are in use?

> +
> +  microchip,vbus-input-range-microvolt:
> +    description: |
> +      Specifies the voltage range in microvolts chosen for the voltage full
> +      scale range (FSR). The range should be set as <minimum, maximum> by
> +      hardware design and should not be changed during runtime.
> +
> +      The VBUS could be configured into the following full scale range:
> +        -  VBUS has unipolar 0V to 42V FSR (default)
> +        -  VBUS has bipolar -42V to 42V FSR
> +        -  VBUS has bipolar -21V to 21V FSR
> +    items:
> +      - enum: [-42000000, -21000000, 0]
> +      - enum: [21000000, 42000000]
> +
> +  microchip,vsense-input-range-microvolt:
> +    description: |
> +      Specifies the voltage range in microvolts chosen for the current full
> +      scale range (FSR). The current is calculated by dividing the vsense
> +      voltage by the value of the shunt resistor. The range should be set as
> +      <minimum, maximum> by hardware design and it should not be changed during
> +      runtime.
> +
> +      The VSENSE could be configured into the following full scale range:
> +        -  VSENSE has unipolar 0 mV to 100V FSR (default)
> +        -  VSENSE has bipolar -100 mV to 100 mV FSR
> +        -  VSENSE has bipolar -50 mV to 50 mV FSR
> +    items:
> +      - enum: [-100000, -50000, 0]
> +      - enum: [50000, 100000]
> +
> +  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.
> +
> +      The Hardware Accumulator could be configured as:
> +       <0>  -  Accumulator accumulates VPOWER (default)
> +       <1>  -  Accumulator accumulates VSENSE
> +       <2>  -  Accumulator accumulates VBUS

Please make this a string, probably with values "vpower", "vsense" and
"vbus".

> +    maximum: 2
> +    default: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +  - shunt-resistor-micro-ohms
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - microchip,pac1711

This is the only compatible, using an if/then/else section here is not
required. This should be describable in the property itself.

> +    then:
> +      properties:
> +        microchip,vbus-input-range-microvolt:
> +          oneOf:
> +            - items:
> +                - const: 0
> +                - const: 42000000
> +            - items:
> +                - const: -42000000
> +                - const: 42000000
> +            - items:
> +                - const: -21000000
> +                - const: 21000000
> +          default:
> +            items:
> +              - const: 0
> +              - const: 42000000
> +
> +        microchip,vsense-input-range-microvolt:
> +          oneOf:
> +            - items:
> +                - const: 0
> +                - const: 100000
> +            - items:
> +                - const: -100000
> +                - const: 100000
> +            - items:
> +                - const: -50000
> +                - const: 50000
> +          default:
> +            items:
> +              - const: 0
> +              - const: 100000
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pac1711@10 {

"pac1711" is not a class of device, so probably dac@10?

pw-bot: changes-requested

> +            compatible = "microchip,pac1711";
> +            reg = <0x10>;
> +
> +            shunt-resistor-micro-ohms = <10000>;
> +            label = "VDD3V3";
> +            vdd-supply = <&vdd>;
> +            microchip,vbus-input-range-microvolt = <(-21000000) 21000000>;
> +            microchip,vsense-input-range-microvolt = <(-50000) 50000>;
> +            microchip,accumulation-mode = <0>;
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a92290fffa163f9fe8fe3f04bf66426f9a894409..7686e2516c90442aa3e23d19cfb08e280a44ba76 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16337,6 +16337,12 @@ F:	Documentation/devicetree/bindings/nvmem/microchip,sama7g5-otpc.yaml
>  F:	drivers/nvmem/microchip-otpc.c
>  F:	include/dt-bindings/nvmem/microchip,sama7g5-otpc.h
>  
> +MICROCHIP PAC1711 DAC DRIVER
> +M:	Ariana Lazar <ariana.lazar@microchip.com>

I also work for Microchip (in the FPGA BU), so whenever I see people
from Microchip whose name I don't recognise I look them up. I noticed
that Teams lists you as being an intern, so I have to ask what your plan
is for maintaining this after your internship has completed. Do you
intend passing it over to someone else from your team or continuing
yourself? If it's the former, it'd be good to CC them and/or add them
here as a co-maintainer.

Cheers,
Conor.

> +L:	linux-iio@vger.kernel.org
> +S:	Supported
> +F:	Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
> +
>  MICROCHIP PAC1921 POWER/CURRENT MONITOR DRIVER
>  M:	Matteo Martelli <matteomartelli3@gmail.com>
>  L:	linux-iio@vger.kernel.org
> 
> -- 
> 2.43.0
> 

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

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

[Jonathan Cameron]

On Wed, 15 Oct 2025 13:12:15 +0300
Ariana Lazar <ariana.lazar@microchip.com> wrote:

> This is the device tree schema for Microchip PAC1711 single-channel power
> monitor with accumulator. The device uses 12-bit resolution for voltage and
> current measurements and 24 bits power calculations. The device supports
> one 56-bit accumulator register.
> 
> PAC1711 measures up to 42V Full-Scale Range.
> 
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
> ---
>  .../bindings/iio/adc/microchip,pac1711.yaml        | 195 +++++++++++++++++++++
>  MAINTAINERS                                        |   6 +
>  2 files changed, 201 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..67edd778981c2f0ed21dda02f14e383a153169b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml

> +  microchip,vbus-input-range-microvolt:
> +    description: |
> +      Specifies the voltage range in microvolts chosen for the voltage full
> +      scale range (FSR). The range should be set as <minimum, maximum> by
> +      hardware design and should not be changed during runtime.
> +
> +      The VBUS could be configured into the following full scale range:
> +        -  VBUS has unipolar 0V to 42V FSR (default)
> +        -  VBUS has bipolar -42V to 42V FSR
> +        -  VBUS has bipolar -21V to 21V FSR
> +    items:
> +      - enum: [-42000000, -21000000, 0]
> +      - enum: [21000000, 42000000]
> +
> +  microchip,vsense-input-range-microvolt:
> +    description: |
> +      Specifies the voltage range in microvolts chosen for the current full
> +      scale range (FSR). The current is calculated by dividing the vsense
> +      voltage by the value of the shunt resistor. The range should be set as
> +      <minimum, maximum> by hardware design and it should not be changed during
> +      runtime.
> +
> +      The VSENSE could be configured into the following full scale range:
> +        -  VSENSE has unipolar 0 mV to 100V FSR (default)
> +        -  VSENSE has bipolar -100 mV to 100 mV FSR
> +        -  VSENSE has bipolar -50 mV to 50 mV FSR
> +    items:
> +      - enum: [-100000, -50000, 0]
> +      - enum: [50000, 100000]

These range setting things are common enough perhaps it's time to standardize
them as properties of channel sub nodes (the stuff in adc.yaml).

> +
> +  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.
> +
> +      The Hardware Accumulator could be configured as:
> +       <0>  -  Accumulator accumulates VPOWER (default)
> +       <1>  -  Accumulator accumulates VSENSE
> +       <2>  -  Accumulator accumulates VBUS

This feels like a runtime thing to control. To be in DT it should be related
to the board wiring rather than being a choice between multiple things
being measured.

> +    maximum: 2
> +    default: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +  - shunt-resistor-micro-ohms

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

[Ariana.Lazar]

Hi Jonathan,

Thanks for the review.

On Sun, 2025-10-19 at 10:04 +0100, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Wed, 15 Oct 2025 13:12:15 +0300
> Ariana Lazar <ariana.lazar@microchip.com> wrote:
> 
> > This is the device tree schema for Microchip PAC1711 single-channel
> > power
> > monitor with accumulator. The device uses 12-bit resolution for
> > voltage and
> > current measurements and 24 bits power calculations. The device
> > supports
> > one 56-bit accumulator register.
> > 
> > PAC1711 measures up to 42V Full-Scale Range.
> > 
> > Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
> > ---
> >  .../bindings/iio/adc/microchip,pac1711.yaml        | 195
> > +++++++++++++++++++++
> >  MAINTAINERS                                        |   6 +
> >  2 files changed, 201 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
> > b/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..67edd778981c2f0ed21dda02f
> > 14e383a153169b1
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
> 
> > +  microchip,vbus-input-range-microvolt:
> > +    description: |
> > +      Specifies the voltage range in microvolts chosen for the
> > voltage full
> > +      scale range (FSR). The range should be set as <minimum,
> > maximum> by
> > +      hardware design and should not be changed during runtime.
> > +
> > +      The VBUS could be configured into the following full scale
> > range:
> > +        -  VBUS has unipolar 0V to 42V FSR (default)
> > +        -  VBUS has bipolar -42V to 42V FSR
> > +        -  VBUS has bipolar -21V to 21V FSR
> > +    items:
> > +      - enum: [-42000000, -21000000, 0]
> > +      - enum: [21000000, 42000000]
> > +
> > +  microchip,vsense-input-range-microvolt:
> > +    description: |
> > +      Specifies the voltage range in microvolts chosen for the
> > current full
> > +      scale range (FSR). The current is calculated by dividing the
> > vsense
> > +      voltage by the value of the shunt resistor. The range should
> > be set as
> > +      <minimum, maximum> by hardware design and it should not be
> > changed during
> > +      runtime.
> > +
> > +      The VSENSE could be configured into the following full scale
> > range:
> > +        -  VSENSE has unipolar 0 mV to 100V FSR (default)
> > +        -  VSENSE has bipolar -100 mV to 100 mV FSR
> > +        -  VSENSE has bipolar -50 mV to 50 mV FSR
> > +    items:
> > +      - enum: [-100000, -50000, 0]
> > +      - enum: [50000, 100000]
> 
> These range setting things are common enough perhaps it's time to
> standardize
> them as properties of channel sub nodes (the stuff in adc.yaml).

Great idea. I will try to implement this into a separate patch.

> 
> > +
> > +  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.
> > +
> > +      The Hardware Accumulator could be configured as:
> > +       <0>  -  Accumulator accumulates VPOWER (default)
> > +       <1>  -  Accumulator accumulates VSENSE
> > +       <2>  -  Accumulator accumulates VBUS
> 
> This feels like a runtime thing to control. To be in DT it should be
> related
> to the board wiring rather than being a choice between multiple
> things
> being measured.
> > 
This property aims to specify what kind of hardware is intended to be
used/available for the user.

There are two main cases here:
- the user wants to measure also the current/power consumed before the
driver insertion (e.g. from the boot to user control) and if this is a
runtime setting, the hardware accumulator will be reset by the default
configuration the driver starts with.
- the driver doesn't know what type of hardware it's dealing with. In
case the part is monitoring the charge/discharge current it does not
make sense in user-space to change the accumulator to calculate energy.
Same if the hardware is intended to calculate energy it doesn't make
sense in user-space to change to Coulomb counter. Changing the setting
from one mode to another will reset the hardware accumulator inside the
chip.  

Best regards,
Ariana

> 

[PATCH 2/2] iio: adc: adding support for PAC1711

[Ariana Lazar]

This is the iio driver for Microchip PAC1711 single-channel power monitor
with accumulator. The device uses 12-bit resolution for voltage and current
measurements and 24 bits power calculations. The device supports one 56-bit
accumulator register.

PAC1711 measures up to 42V Full-Scale Range.

Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
---
 .../ABI/testing/sysfs-bus-iio-adc-pac1711          |   57 +
 MAINTAINERS                                        |    2 +
 drivers/iio/adc/Kconfig                            |   10 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/pac1711.c                          | 1448 ++++++++++++++++++++
 5 files changed, 1518 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1711 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1711
new file mode 100644
index 0000000000000000000000000000000000000000..7f6ab50d29ff064d57b80df0a0c162a4c98764f8
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1711
@@ -0,0 +1,57 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
+KernelVersion:	6.18
+Contact:	linux-iio@vger.kernel.org
+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. The value
+		is used to calculate current, power and accumulated energy.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_current_acc_raw
+KernelVersion:	6.18
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute is used to read the accumulated voltage measured
+		on the shunt resistor (Coulomb counter). Units after application
+		of scale are mA. X is the IIO index of the device.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_current_acc_scale
+KernelVersion:	6.18
+Contact:	linux-iio@vger.kernel.org
+Description:
+		If known for a device, scale to be applied to in_current_raw in
+		order to obtain the measured value in mA units. X is the IIO
+		index of the device.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_current_acc_en
+KernelVersion:	6.18
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute, if available, is used to enable digital
+		accumulation of VSENSE measurements. X is the IIO index of the
+		device.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_acc_raw
+KernelVersion:	6.18
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute is used to read the accumulated voltage on VBUS.
+		Units after application of scale are mV. X is the IIO index of
+		the device.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_acc_scale
+KernelVersion:	6.18
+Contact:	linux-iio@vger.kernel.org
+Description:
+		If known for a device, scale to be applied to in_voltage_raw
+		in order to obtain the measured value in mV units. X is the IIO
+		index of the device.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_acc_en
+KernelVersion:	6.18
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute, if available, is used to enable digital
+		accumulation of VBUS measurements. X is the IIO index of the
+		device.
+
diff --git a/MAINTAINERS b/MAINTAINERS
index 7686e2516c90442aa3e23d19cfb08e280a44ba76..d4d4e303ad278cbf8f628b52e3b5ebe60fb1bf09 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16341,7 +16341,9 @@ MICROCHIP PAC1711 DAC DRIVER
 M:	Ariana Lazar <ariana.lazar@microchip.com>
 L:	linux-iio@vger.kernel.org
 S:	Supported
+F:	Documentation/ABI/testing/sysfs-bus-iio-adc-pac1711
 F:	Documentation/devicetree/bindings/iio/adc/microchip,pac1711.yaml
+F:	drivers/iio/adc/pac1711.c
 
 MICROCHIP PAC1921 POWER/CURRENT MONITOR DRIVER
 M:	Matteo Martelli <matteomartelli3@gmail.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ea3ba139739281de82848e25fd2b6ca479a939dc..c91c66e583d1ce07aa1813a84d461ebd3fb6344d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1125,6 +1125,16 @@ config NPCM_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called npcm_adc.
 
+config PAC1711
+        tristate "Microchip Technology PAC1711 driver"
+        depends on I2C
+        help
+          Say yes here to build support for Microchip Technology's PAC1711
+          Single-Channel Power Monitor with Accumulator.
+
+          This driver can also be built as a module. If so, the module
+          will be called pac1711.
+
 config PAC1921
 	tristate "Microchip Technology PAC1921 driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 09ae6edb26504991f011def6618efc3f4cf4df4c..d039a23cde02d442b161730ad2c939c7d035a4c6 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o
 obj-$(CONFIG_NAU7802) += nau7802.o
 obj-$(CONFIG_NCT7201) += nct7201.o
 obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
+obj-$(CONFIG_PAC1711) += pac1711.o
 obj-$(CONFIG_PAC1921) += pac1921.o
 obj-$(CONFIG_PAC1934) += pac1934.o
 obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
diff --git a/drivers/iio/adc/pac1711.c b/drivers/iio/adc/pac1711.c
new file mode 100644
index 0000000000000000000000000000000000000000..30929af202e009c2d80ba91d5c137a26a03f09d7
--- /dev/null
+++ b/drivers/iio/adc/pac1711.c
@@ -0,0 +1,1448 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IIO driver for PAC1711 Multi-Channel DC Power/Energy Monitor
+ *
+ * Copyright (C) 2024 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Ariana Lazar <ariana.lazar@microchip.com>
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/unaligned.h>
+
+/*
+ * maximum accumulation time should be 1,165 hours at 1,024 sps
+ * till PAC1711 accumulation registers starts to saturate
+ */
+#define PAC1711_MAX_RFSH_LIMIT_MS		60000
+/* 50msec is the timeout for validity of the cached registers */
+#define PAC1711_MIN_POLLING_TIME_MS		50
+/*
+ * 1000usec is the minimum wait time for normal conversions when sample
+ * rate doesn't change
+ */
+#define PAC1711_MIN_UPDATE_WAIT_TIME_US		1000
+
+/* 42000mV */
+#define PAC1711_VOLTAGE_MILLIVOLTS_MAX	42000
+
+/* voltage bits resolution when set for unsigned values */
+#define PAC1711_VOLTAGE_12B_RES			12
+/* voltage bits resolution when set for signed values */
+#define PAC1711_VOLTAGE_11B_RES			11
+
+/* Power resolution is 24 bits when contains unsigned values */
+#define PAC1711_POWER_24B_RES			24
+/* Power resolution is 24 bits when contains signed values */
+#define PAC1711_POWER_23B_RES			23
+
+/* Maximum power-product value - 42 V * 0.1 V */
+#define PAC1711_PRODUCT_VOLTAGE_PV_FSR		4200000000000UL
+
+/* Scale constant = (10^3 * 4.2 * 10^9 / 2^24) for mili Watt-second */
+#define PAC1711_SCALE_CONSTANT			250340
+
+/* (100mV * 1000000) / (2^12)  used to calculate the scale for current */
+#define PAC1711_MAX_VSENSE_RSHIFTED_BY_12B	24414
+
+/*
+ * [(100mV * 1000000) / (2^12)]*10^9  used to calculate the scale
+ * for accumulated current/Coulomb counter
+ */
+#define PAC1711_MAX_VSENSE_NANO			24414062500000UL
+
+/* I2C address map */
+#define PAC1711_REFRESH_REG_ADDR		0x00
+#define PAC1711_CTRL_REG_ADDR			0x01
+#define PAC1711_ACC_COUNT_REG_ADDR		0x02
+#define PAC1711_VACC_ADDR			0x03
+#define PAC1711_VBUS_ADDR			0x04
+#define PAC1711_VSENSE_ADDR			0x05
+#define PAC1711_VBUS_AVG_ADDR			0x06
+#define PAC1711_VSENSE_AVG_ADDR			0x07
+#define PAC1711_VPOWER_ADDR			0x08
+
+/* Start of configurations registers */
+#define PAC1711_CTRL_LAT_REG_ADDR		0x0F
+#define PAC1711_NEG_PWR_FSR_REG_ADDR		0x13
+#define PAC1711_SLOW_REG_ADDR			0x16
+#define PAC1711_CTRL_ACT_REG_ADDR		0x17
+#define PAC1711_PID_REG_ADDR			0xFD
+#define PAC1711_REVISION_ID_REG_ADDR		0xFF
+
+#define PAC1711_ACC_DEVATTR			3
+#define PAC1711_COMMON_DEVATTR			1
+#define PAC1711_MEAS_REG_SNAPSHOT_LEN		23
+#define PAC1711_ACC_REG_LEN			4
+#define PAC1711_VACC_REG_LEN			7
+#define PAC1711_VBUS_SENSE_REG_LEN		2
+
+/* PRODUCT IDs */
+#define PAC_PRODUCT_ID_1711			0x80
+
+/* CTRL reg */
+#define PAC1711_CTRL_SAMPLE_MODE_MASK		GENMASK(15, 12)
+
+/* NEG_PWR_CFG reg */
+#define PAC1711_NEG_PWR_CFG_VS_MASK		GENMASK(3, 2)
+#define PAC1711_NEG_PWR_CFG_VB_MASK		GENMASK(1, 0)
+#define PAC1711_SAMPLING_MODE_MASK		GENMASK(11, 0)
+
+#define PAC1711_DEV_ATTR(name)			(&iio_dev_attr_##name.dev_attr.attr)
+
+enum pac1711_ch_idx {
+	PAC1711_CH_POWER,
+	PAC1711_CH_VOLTAGE,
+	PAC1711_CH_CURRENT,
+	PAC1711_CH_VOLTAGE_AVERAGE,
+	PAC1711_CH_CURRENT_AVERAGE,
+};
+
+enum pac1711_acc_mode {
+	PAC1711_ACCMODE_VPOWER,
+	PAC1711_ACCMODE_VSENSE,
+	PAC1711_ACCMODE_VBUS,
+};
+
+enum pac1711_fsr {
+	PAC1711_FULL_RANGE_UNIPOLAR,
+	PAC1711_FULL_RANGE_BIPOLAR,
+	PAC1711_HALF_RANGE_BIPOLAR,
+};
+
+static const int pac1711_vbus_range_tbl[][2] = {
+	[PAC1711_FULL_RANGE_UNIPOLAR] = {0, 42000000},
+	[PAC1711_FULL_RANGE_BIPOLAR] = {-42000000, 42000000},
+	[PAC1711_HALF_RANGE_BIPOLAR] = {-21000000, 21000000},
+};
+
+static const int pac1711_vsense_range_tbl[][2] = {
+	[PAC1711_FULL_RANGE_UNIPOLAR] = {0, 100000},
+	[PAC1711_FULL_RANGE_BIPOLAR] = {-100000, 100000},
+	[PAC1711_HALF_RANGE_BIPOLAR] = {-50000, 50000},
+};
+
+enum pac1711_samps {
+	PAC1711_SAMP_8192SPS,
+	PAC1711_SAMP_4096SPS,
+	PAC1711_SAMP_1024SPS,
+	PAC1711_SAMP_256SPS,
+	PAC1711_SAMP_64SPS,
+	PAC1711_SAMP_8SPS,
+};
+
+static const unsigned int pac1711_samp_rate_map_tbl[] = {
+	[PAC1711_SAMP_8192SPS] = 8192,
+	[PAC1711_SAMP_4096SPS] = 4096,
+	[PAC1711_SAMP_1024SPS] = 1024, // default
+	[PAC1711_SAMP_256SPS] = 256,
+	[PAC1711_SAMP_64SPS] = 64,
+	[PAC1711_SAMP_8SPS] = 8,
+};
+
+static const unsigned int pac1711_shift_map_tbl[] = {
+	[PAC1711_SAMP_8192SPS] = 13,
+	[PAC1711_SAMP_4096SPS] = 12,
+	[PAC1711_SAMP_1024SPS] = 10,
+	[PAC1711_SAMP_256SPS] = 8,
+	[PAC1711_SAMP_64SPS] = 6,
+	[PAC1711_SAMP_8SPS] = 3,
+};
+
+/* Available Sample Modes */
+static const char * const pac1711_frequency_avail[] = {
+	"8192",
+	"4096",
+	"1024",
+	"256",
+	"64",
+	"8",
+};
+
+/**
+ * struct pac1711_features - features of a pac1711 instance
+ * @prod_id: hardware ID
+ * @name: chip's name
+ */
+struct pac1711_features {
+	u8 prod_id;
+	const char *name;
+};
+
+static const struct pac1711_features pac1711_chip_features = {
+	.name = "pac1711",
+	.prod_id = PAC_PRODUCT_ID_1711,
+};
+
+/**
+ * struct reg_data - data from the registers
+ * @meas_regs:		snapshot of raw measurements registers
+ * @jiffies_tstamp:	timestamp
+ * @total_samples_nr:	total number of samples
+ * @ctrl_act_reg:	the ctrl_act register
+ * @ctrl_lat_reg:	the ctrl_lat register
+ * @vsense_avg:		averages of vsense registers
+ * @acc_count:		the acc_count register
+ * @vbus_avg:		averages of vbus registers
+ * @acc_val:		accumulated values per second
+ * @vpower:		vpower registers
+ * @vsense:		vsense registers
+ * @vacc:		accumulated vpower value
+ * @vbus:		vbus registers
+ */
+struct reg_data {
+	u8	meas_regs[PAC1711_MEAS_REG_SNAPSHOT_LEN];
+	unsigned long jiffies_tstamp;
+	u32	total_samples_nr;
+	u16	ctrl_act_reg;
+	u16	ctrl_lat_reg;
+	s32	vsense_avg;
+	u32	acc_count;
+	s32	vbus_avg;
+	s64	acc_val;
+	s32	vpower;
+	s32	vsense;
+	s32	vbus;
+	s64	vacc;
+};
+
+/**
+ * struct pac1711_chip_info - information about the chip
+ * @accumulation_mode:	accumulation mode for hardware accumulator
+ * @sample_rate_value:	sampling frequency
+ * @work_chip_rfsh:	work queue used for refresh commands
+ * @sampling_mode:	sampling mode used by the device
+ * @chip_reg_data:	chip reg data
+ * @chip_variant:	chip variant
+ * @vsense_mode:	Full Scale Range (FSR) mode for VSense
+ * @enable_acc:		true means that accumulation channel is measured
+ * @vbus_mode:		Full Scale Range (FSR) mode for VBus
+ * @iio_info:		iio_info
+ * @client:		the i2c-client attached to the device
+ * @label:		channel label
+ * @shunt:		shunt resistor value
+ * @lock:		synchronize access to driver's state members
+ */
+struct pac1711_chip_info {
+	u8			accumulation_mode;
+	s32			sample_rate_value;
+	struct delayed_work	work_chip_rfsh;
+	u8			sampling_mode;
+	struct reg_data		chip_reg_data;
+	u8			chip_variant;
+	u8			vsense_mode;
+	bool			enable_acc;
+	u8			vbus_mode;
+	struct iio_info		iio_info;
+	struct i2c_client	*client;
+	char			*label;
+	u32			shunt;
+	struct mutex		lock; /* synchronize access to driver's state members */
+};
+
+static inline u64 pac1711_get_unaligned_be56(u8 *p)
+{
+	return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 |
+		(u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6];
+}
+
+static int pac1711_send_refresh(struct pac1711_chip_info *info, u8 refresh_cmd,
+				u32 wait_time)
+{
+	struct i2c_client *client = info->client;
+	int ret;
+
+	/* Writing a REFRESH or a REFRESH_V command */
+	ret = i2c_smbus_write_byte(client, refresh_cmd);
+	if (ret) {
+		dev_err(&client->dev, "%s - cannot send Refresh cmd (0x%02X)\n",
+			__func__, refresh_cmd);
+		return ret;
+	}
+
+	/* Register data retrieval timestamp */
+	info->chip_reg_data.jiffies_tstamp = jiffies;
+	/* Wait till the data is available */
+	usleep_range(wait_time, wait_time + 100);
+
+	return 0;
+}
+
+static int pac1711_reg_snapshot(struct pac1711_chip_info *info,
+				bool do_refresh, u8 refresh_cmd, u32 wait_time)
+{
+	struct i2c_client *client = info->client;
+	struct device *dev = &client->dev;
+	s64 stored_value, tmp_s64;
+	u8 *offset_reg_data_p;
+	u32 count, inc_count;
+	bool is_bipolar;
+	s64 inc = 0;
+	u16 tmp_u16;
+	u8 shift;
+	int ret;
+
+	guard(mutex)(&info->lock);
+
+	if (do_refresh) {
+		ret = pac1711_send_refresh(info, refresh_cmd, wait_time);
+		if (ret < 0) {
+			dev_err(dev, "cannot send refresh\n");
+			return ret;
+		}
+	}
+
+	/* Read the ctrl/status registers for this snapshot */
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_CTRL_ACT_REG_ADDR,
+					    sizeof(tmp_u16), (u8 *)&tmp_u16);
+	if (ret < 0) {
+		dev_err(dev, "%s - cannot read regs from 0x%02X\n",
+			__func__, PAC1711_CTRL_ACT_REG_ADDR);
+		return ret;
+	}
+
+	be16_to_cpus(&tmp_u16);
+	info->chip_reg_data.ctrl_act_reg = tmp_u16;
+
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_CTRL_LAT_REG_ADDR,
+					    sizeof(tmp_u16), (u8 *)&tmp_u16);
+	if (ret < 0) {
+		dev_err(dev, "%s - cannot read regs from 0x%02X\n",
+			__func__, PAC1711_CTRL_LAT_REG_ADDR);
+		return ret;
+	}
+
+	be16_to_cpus(&tmp_u16);
+	info->chip_reg_data.ctrl_lat_reg = tmp_u16;
+
+	/* Read the data registers */
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_ACC_COUNT_REG_ADDR,
+					    PAC1711_MEAS_REG_SNAPSHOT_LEN,
+					    (u8 *)info->chip_reg_data.meas_regs);
+	if (ret < 0) {
+		dev_err(dev, "%s - cannot read regs from 0x%02X\n",
+			__func__, PAC1711_ACC_COUNT_REG_ADDR);
+		return ret;
+	}
+
+	offset_reg_data_p = &info->chip_reg_data.meas_regs[0];
+	info->chip_reg_data.acc_count = get_unaligned_be32(offset_reg_data_p);
+	offset_reg_data_p += PAC1711_ACC_REG_LEN;
+
+	/* skip if the energy accumulation is disabled */
+	if (info->enable_acc) {
+		stored_value = info->chip_reg_data.acc_val;
+
+		info->chip_reg_data.vacc = pac1711_get_unaligned_be56(offset_reg_data_p);
+		is_bipolar = false;
+
+		switch (info->accumulation_mode) {
+		case PAC1711_ACCMODE_VPOWER:
+			if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR ||
+			    info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
+				is_bipolar = true;
+			break;
+		case PAC1711_ACCMODE_VBUS:
+			if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR)
+				is_bipolar = true;
+			break;
+		case PAC1711_ACCMODE_VSENSE:
+			if (info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
+				is_bipolar = true;
+			break;
+		}
+
+		if (is_bipolar)
+			info->chip_reg_data.vacc = sign_extend64(info->chip_reg_data.vacc, 55);
+
+		if (info->accumulation_mode != PAC1711_ACCMODE_VBUS) {
+			/* VACC */
+			/*
+			 * Integrate the accumulated power or current over
+			 * the elapsed interval.
+			 */
+			tmp_u16 = FIELD_GET(PAC1711_CTRL_SAMPLE_MODE_MASK,
+					    info->chip_reg_data.ctrl_lat_reg);
+			tmp_s64 = info->chip_reg_data.vacc;
+
+			if (tmp_u16 <= PAC1711_SAMP_8SPS) {
+				shift = pac1711_shift_map_tbl[tmp_u16];
+				inc = tmp_s64 >> shift;
+			} else {
+				dev_err(dev, "Invalid sample rate index: %d!\n", tmp_u16);
+			}
+		} else {
+			count = info->chip_reg_data.total_samples_nr;
+			inc_count = info->chip_reg_data.acc_count;
+
+			/* Check if total number of samples will overflow */
+			if (check_add_overflow(count, inc_count, &count)) {
+				dev_err(dev, "Number of samples overflow!\n");
+				info->chip_reg_data.total_samples_nr = 0;
+				info->chip_reg_data.acc_val = 0;
+			}
+
+			info->chip_reg_data.total_samples_nr += inc_count;
+			inc = info->chip_reg_data.vacc;
+		}
+
+		if (check_add_overflow(stored_value, inc, &stored_value)) {
+			if (is_negative(stored_value))
+				info->chip_reg_data.acc_val = S64_MIN;
+			else
+				info->chip_reg_data.acc_val = S64_MAX;
+
+			dev_err(dev, "Overflow detected!\n");
+		} else {
+			info->chip_reg_data.acc_val += inc;
+		}
+	}
+
+	offset_reg_data_p += PAC1711_VACC_REG_LEN;
+
+	/* VBUS */
+	info->chip_reg_data.vbus = get_unaligned_be16(offset_reg_data_p) >> 4;
+
+	if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR)
+		info->chip_reg_data.vbus = sign_extend32(info->chip_reg_data.vbus, 11);
+
+	offset_reg_data_p += PAC1711_VBUS_SENSE_REG_LEN;
+
+	/* VSENSE */
+	info->chip_reg_data.vsense = get_unaligned_be16(offset_reg_data_p) >> 4;
+
+	if (info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
+		info->chip_reg_data.vsense = sign_extend32(info->chip_reg_data.vsense, 11);
+
+	offset_reg_data_p += PAC1711_VBUS_SENSE_REG_LEN;
+
+	/* VBUS_AVG */
+	info->chip_reg_data.vbus_avg = get_unaligned_be16(offset_reg_data_p) >> 4;
+
+	if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR)
+		info->chip_reg_data.vbus_avg = sign_extend32(info->chip_reg_data.vbus_avg, 11);
+
+	offset_reg_data_p += PAC1711_VBUS_SENSE_REG_LEN;
+
+	/* VSENSE_AVG */
+	info->chip_reg_data.vsense_avg = get_unaligned_be16(offset_reg_data_p) >> 4;
+
+	if (info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
+		info->chip_reg_data.vsense_avg = sign_extend32(info->chip_reg_data.vsense_avg, 11);
+
+	offset_reg_data_p += PAC1711_VBUS_SENSE_REG_LEN;
+
+	/* VPOWER */
+	info->chip_reg_data.vpower = get_unaligned_be32(offset_reg_data_p) >> 8;
+
+	if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR ||
+	    info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
+		info->chip_reg_data.vpower = sign_extend32(info->chip_reg_data.vpower, 23);
+
+	return 0;
+}
+
+static int pac1711_retrieve_data(struct pac1711_chip_info *info,
+				 u32 wait_time)
+{
+	int ret;
+
+	/*
+	 * check if the minimal elapsed time has passed and if so,
+	 * re-read the chip, otherwise the cached info is just fine
+	 */
+	if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
+			msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS))) {
+		ret = pac1711_reg_snapshot(info, true, PAC1711_REFRESH_REG_ADDR,
+					   wait_time);
+
+		/*
+		 * Re-schedule the work for the read registers timeout
+		 * (to prevent chip regs saturation)
+		 */
+		cancel_delayed_work_sync(&info->work_chip_rfsh);
+		schedule_delayed_work(&info->work_chip_rfsh,
+				      msecs_to_jiffies(PAC1711_MAX_RFSH_LIMIT_MS));
+	}
+
+	return ret;
+}
+
+static int pac1711_frequency_set(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
+				 unsigned int mode)
+{
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+	struct i2c_client *client = info->client;
+	u16 tmp_u16;
+	int ret;
+
+	guard(mutex)(&info->lock);
+
+	ret = i2c_smbus_read_word_swapped(client, PAC1711_CTRL_ACT_REG_ADDR);
+	if (ret < 0) {
+		dev_err(&client->dev, "cannot read regs from 0x%02X\n",
+			PAC1711_CTRL_ACT_REG_ADDR);
+		return ret;
+	}
+
+	tmp_u16 = ret;
+	tmp_u16 &= ~PAC1711_CTRL_SAMPLE_MODE_MASK;
+	tmp_u16 |= FIELD_PREP(PAC1711_CTRL_SAMPLE_MODE_MASK, mode);
+
+	ret = i2c_smbus_write_word_swapped(client, PAC1711_CTRL_REG_ADDR, tmp_u16);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to configure sampling mode\n");
+		return ret;
+	}
+
+	info->sampling_mode = mode;
+	info->chip_reg_data.ctrl_act_reg = tmp_u16;
+
+	return pac1711_retrieve_data(info, PAC1711_MIN_UPDATE_WAIT_TIME_US);
+}
+
+static int pac1711_frequency_get(struct iio_dev *indio_dev,
+				 const struct iio_chan_spec *chan)
+{
+	struct pac1711_chip_info *info;
+
+	info = iio_priv(indio_dev);
+
+	return info->sampling_mode;
+}
+
+static const struct iio_enum sampling_mode_enum = {
+	.items = pac1711_frequency_avail,
+	.num_items = ARRAY_SIZE(pac1711_frequency_avail),
+	.set = pac1711_frequency_set,
+	.get = pac1711_frequency_get,
+};
+
+static const struct iio_chan_spec_ext_info pac1711_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,
+	},
+	{ }
+};
+
+#define TO_PAC1711_CHIP_INFO(d) container_of(d, struct pac1711_chip_info, work_chip_rfsh)
+
+#define PAC1711_VBUS_CHANNEL(_index, _si, _address) {		\
+	.type = IIO_VOLTAGE,					\
+	.address = (_address),					\
+	.indexed = 1,						\
+	.channel = (_index),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = (_si),					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = 12,					\
+		.storagebits = 16,				\
+		.shift = 4,					\
+		.endianness = IIO_CPU,				\
+	},							\
+	.ext_info =  pac1711_ext_info				\
+}
+
+#define PAC1711_VBUS_AVG_CHANNEL(_index, _si, _address) {	\
+	.type = IIO_VOLTAGE,					\
+	.address = (_address),					\
+	.indexed = 1,						\
+	.channel = (_index) + 1,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_AVERAGE_RAW) |	\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = (_si),					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = 12,					\
+		.storagebits = 16,				\
+		.shift = 4,					\
+		.endianness = IIO_CPU,				\
+	},							\
+	.ext_info =  pac1711_ext_info				\
+}
+
+#define PAC1711_VSENSE_CHANNEL(_index, _si, _address) {		\
+	.type = IIO_CURRENT,					\
+	.address = (_address),					\
+	.indexed = 1,						\
+	.channel = (_index),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = (_si),					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = 12,					\
+		.storagebits = 16,				\
+		.shift = 4,					\
+		.endianness = IIO_CPU,				\
+	},							\
+	.ext_info =  pac1711_ext_info				\
+}
+
+#define PAC1711_VSENSE_AVG_CHANNEL(_index, _si, _address) {	\
+	.type = IIO_CURRENT,					\
+	.address = (_address),					\
+	.indexed = 1,						\
+	.channel = (_index) + 1,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_AVERAGE_RAW) |	\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = (_si),					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = 12,					\
+		.storagebits = 16,				\
+		.shift = 4,					\
+		.endianness = IIO_CPU,				\
+	},							\
+	.ext_info =  pac1711_ext_info				\
+}
+
+#define PAC1711_VPOWER_CHANNEL(_index, _si, _address) {		\
+	.type = IIO_POWER,					\
+	.address = (_address),					\
+	.indexed = 1,						\
+	.channel = (_index),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = (_si),					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = 24,					\
+		.storagebits = 32,				\
+		.shift = 8,					\
+		.endianness = IIO_CPU,				\
+	},							\
+	.ext_info =  pac1711_ext_info				\
+}
+
+static int pac1711_get_samp_rate_idx(struct pac1711_chip_info *info, u32 new_samp_rate)
+{
+	int cnt;
+
+	for (cnt = 0; cnt < ARRAY_SIZE(pac1711_samp_rate_map_tbl); cnt++)
+		if (new_samp_rate == pac1711_samp_rate_map_tbl[cnt])
+			return cnt;
+
+	/* not a valid sample rate value */
+	return -EINVAL;
+}
+
+static ssize_t pac1711_shunt_value_show(struct device *dev, struct device_attribute *attr,
+					char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+
+	return sysfs_emit(buf, "%u\n", info->shunt);
+}
+
+static ssize_t pac1711_shunt_value_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 pac1711_chip_info *info = iio_priv(indio_dev);
+	int sh_val;
+
+	if (kstrtouint(buf, 10, &sh_val)) {
+		dev_err(dev, "Shunt value is not valid\n");
+		return -EINVAL;
+	}
+
+	scoped_guard(mutex, &info->lock)
+		info->shunt = sh_val;
+
+	return count;
+}
+
+static ssize_t pac1711_in_power_acc_raw_show(struct device *dev, struct device_attribute *attr,
+					     char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+	s64 curr_energy, int_part;
+	int ret, rem;
+
+	ret = pac1711_retrieve_data(info, PAC1711_MIN_UPDATE_WAIT_TIME_US);
+	if (ret < 0)
+		return 0;
+
+	/*
+	 * Expresses the 64 bit energy value as a
+	 * 64 bit integer and a 32 bit nano value
+	 */
+	curr_energy = info->chip_reg_data.acc_val;
+	int_part = div_s64_rem(curr_energy, 1000000000, &rem);
+
+	if (rem < 0)
+		return sysfs_emit(buf, "-%lld.%09u\n", abs(int_part), -rem);
+	else
+		return sysfs_emit(buf, "%lld.%09u\n", int_part, abs(rem));
+}
+
+static ssize_t pac1711_in_power_acc_scale_show(struct device *dev, struct device_attribute *attr,
+					       char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+	unsigned int rem;
+	u64 tmp, ref;
+
+	ref = (u64)PAC1711_SCALE_CONSTANT;
+	tmp = div_u64(ref * 1000000000LL, info->shunt);
+	rem = do_div(tmp, 1000000000LL);
+
+	return sysfs_emit(buf, "%lld.%09u\n", tmp, rem);
+}
+
+static ssize_t pac1711_in_enable_acc_show(struct device *dev, struct device_attribute *attr,
+					  char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+
+	return sysfs_emit(buf, "%d\n", info->enable_acc);
+}
+
+static ssize_t pac1711_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 pac1711_chip_info *info = iio_priv(indio_dev);
+	int val;
+
+	if (kstrtouint(buf, 10, &val)) {
+		dev_err(dev, "Value is not valid\n");
+		return -EINVAL;
+	}
+
+	scoped_guard(mutex, &info->lock) {
+		info->enable_acc = val ? true : false;
+		if (!val) {
+			info->chip_reg_data.acc_val = 0;
+			info->chip_reg_data.total_samples_nr = 0;
+		}
+	}
+
+	return count;
+}
+
+static ssize_t pac1711_in_current_acc_raw_show(struct device *dev, struct device_attribute *attr,
+					       char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+	int ret;
+
+	ret = pac1711_retrieve_data(info, PAC1711_MIN_UPDATE_WAIT_TIME_US);
+	if (ret < 0)
+		return 0;
+
+	return sysfs_emit(buf, "%lld\n", info->chip_reg_data.acc_val);
+}
+
+static ssize_t pac1711_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 pac1711_chip_info *info = iio_priv(indio_dev);
+	u64 tmp_u64, ref;
+	int rem;
+
+	/*
+	 * Currents - scale for mA - depends on the channel's shunt value
+	 * (100mV * 1000000) / (2^12 * shunt(uOhm))
+	 */
+	ref = (u64)PAC1711_MAX_VSENSE_NANO;
+
+	if (info->vsense_mode == PAC1711_FULL_RANGE_BIPOLAR)
+		ref = ref << 1;
+
+	/*
+	 * Increasing precision
+	 * (100mV * 1000000 * 1000000000) / 2^12)
+	 */
+	tmp_u64 = div_u64(ref, info->shunt);
+	rem = do_div(tmp_u64, 1000000000LL);
+
+	return sysfs_emit(buf, "%lld.%09u\n", tmp_u64, rem);
+}
+
+static ssize_t pac1711_in_voltage_acc_raw_show(struct device *dev, struct device_attribute *attr,
+					       char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+	u32 samples_count;
+	s64 acc_voltage;
+	u64 tmp_u64;
+	int ret;
+
+	ret = pac1711_retrieve_data(info, PAC1711_MIN_UPDATE_WAIT_TIME_US);
+	if (ret < 0)
+		return 0;
+
+	acc_voltage = info->chip_reg_data.acc_val;
+	samples_count = info->chip_reg_data.total_samples_nr;
+
+	tmp_u64 = div_u64(abs(acc_voltage), samples_count);
+
+	if (is_negative(acc_voltage))
+		return sysfs_emit(buf, "-%lld\n", tmp_u64);
+	else
+		return sysfs_emit(buf, "%lld\n", tmp_u64);
+}
+
+static ssize_t pac1711_in_voltage_acc_scale_show(struct device *dev, struct device_attribute *attr,
+						 char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+	unsigned long long tmp;
+	int vals[2];
+
+	vals[0] = PAC1711_VOLTAGE_MILLIVOLTS_MAX;
+
+	if (info->vbus_mode == PAC1711_FULL_RANGE_BIPOLAR)
+		vals[1] = PAC1711_VOLTAGE_12B_RES;
+	else
+		/* PAC1711_FULL_RANGE_UNIPOLAR or PAC1711_HALF_RANGE_BIPOLAR */
+		vals[1] = PAC1711_VOLTAGE_11B_RES;
+
+	tmp = (s64)vals[0] * 1000000000LL >> vals[1];
+	vals[1] = do_div(tmp, 1000000000LL);
+	vals[0] = tmp;
+
+	return sysfs_emit(buf, "%d.%09u\n", vals[0], vals[1]);
+}
+
+static IIO_DEVICE_ATTR(in_shunt_resistor, 0644, pac1711_shunt_value_show,
+		       pac1711_shunt_value_store, 0);
+
+static struct attribute *pac1711_all_attrs[] = {
+	PAC1711_DEV_ATTR(in_shunt_resistor),
+	NULL
+};
+
+static int pac1711_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+	int ret;
+	u64 tmp;
+
+	ret = pac1711_retrieve_data(info, PAC1711_MIN_UPDATE_WAIT_TIME_US);
+	if (ret < 0)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = info->chip_reg_data.vbus;
+			return IIO_VAL_INT;
+		case IIO_CURRENT:
+			*val = info->chip_reg_data.vsense;
+			return IIO_VAL_INT;
+		case IIO_POWER:
+			*val = info->chip_reg_data.vpower;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_AVERAGE_RAW:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = info->chip_reg_data.vbus_avg;
+			return IIO_VAL_INT;
+		case IIO_CURRENT:
+			*val = info->chip_reg_data.vsense_avg;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->address) {
+		/* Voltages - scale for millivolts */
+		case PAC1711_VBUS_ADDR:
+		case PAC1711_VBUS_AVG_ADDR:
+			switch (info->chip_variant) {
+			case PAC_PRODUCT_ID_1711:
+				*val = PAC1711_VOLTAGE_MILLIVOLTS_MAX;
+				break;
+			default:
+				return -EINVAL;
+			}
+			switch (info->vbus_mode) {
+			case PAC1711_FULL_RANGE_UNIPOLAR:
+			case PAC1711_HALF_RANGE_BIPOLAR:
+				*val2 = PAC1711_VOLTAGE_12B_RES;
+				break;
+			case PAC1711_FULL_RANGE_BIPOLAR:
+				*val2 = PAC1711_VOLTAGE_11B_RES;
+				break;
+			default:
+				return -EINVAL;
+			}
+			return IIO_VAL_FRACTIONAL_LOG2;
+		/*
+		 * Currents - scale for mA - depends on the
+		 * channel's shunt value
+		 * (100mV * 1000000) / (2^16 * shunt(uohm))
+		 */
+		case PAC1711_VSENSE_ADDR:
+		case PAC1711_VSENSE_AVG_ADDR:
+			*val = PAC1711_MAX_VSENSE_RSHIFTED_BY_12B;
+			*val2 = info->shunt;
+
+			if (info->vsense_mode == PAC1711_FULL_RANGE_BIPOLAR)
+				*val = *val << 1;
+
+			return IIO_VAL_FRACTIONAL;
+		/*
+		 * Power - uW - it will use the combined scale
+		 * for current and voltage
+		 * current(mA) * voltage(mV) = power (uW)
+		 */
+		case PAC1711_VPOWER_ADDR:
+			switch (info->chip_variant) {
+			case PAC_PRODUCT_ID_1711:
+				tmp = PAC1711_PRODUCT_VOLTAGE_PV_FSR;
+				break;
+			}
+
+			do_div(tmp, info->shunt);
+			*val = (int)tmp;
+			if (info->vsense_mode == PAC1711_FULL_RANGE_UNIPOLAR &&
+			    info->vbus_mode == PAC1711_FULL_RANGE_UNIPOLAR)
+				*val2 = PAC1711_POWER_24B_RES;
+			else
+				*val2 = PAC1711_POWER_23B_RES;
+
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = info->sample_rate_value;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int pac1711_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+	struct i2c_client *client = info->client;
+	struct device *dev = &info->client->dev;
+	int ret = -EINVAL;
+	s32 old_samp_rate;
+	u8 ctrl_reg;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = pac1711_get_samp_rate_idx(info, val);
+		if (ret < 0)
+			return ret;
+
+		/* write the new sampling value and trigger a snapshot */
+		scoped_guard(mutex, &info->lock) {
+			ctrl_reg = FIELD_PREP(PAC1711_CTRL_SAMPLE_MODE_MASK, ret);
+			ret = i2c_smbus_write_byte_data(client, PAC1711_CTRL_REG_ADDR, ctrl_reg);
+			if (ret) {
+				dev_err(dev, "%s - can't update sample rate\n", __func__);
+				return ret;
+			}
+		}
+
+		old_samp_rate = info->sample_rate_value;
+		info->sample_rate_value = val;
+
+		/*
+		 * now, force a snapshot with refresh - call retrieve
+		 * data in order to update the refresh timer
+		 * alter the timestamp in order to force trigger a
+		 * register snapshot and a timestamp update
+		 */
+		info->chip_reg_data.jiffies_tstamp -= msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS);
+		ret = pac1711_retrieve_data(info, (1024 / old_samp_rate) * 1000);
+		if (ret < 0) {
+			dev_err(dev, "%s - cannot snapshot ctrl and measurement regs\n", __func__);
+			return ret;
+		}
+
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int pac1711_read_label(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, char *label)
+{
+	struct pac1711_chip_info *info = iio_priv(indio_dev);
+
+	switch (chan->address) {
+	case PAC1711_VBUS_ADDR:
+		return sysfs_emit(label, "%s_VBUS\n", info->label);
+	case PAC1711_VBUS_AVG_ADDR:
+		return sysfs_emit(label, "%s_VBUS_AVG\n", info->label);
+	case PAC1711_VSENSE_ADDR:
+		return sysfs_emit(label, "%s_IBUS\n", info->label);
+	case PAC1711_VSENSE_AVG_ADDR:
+		return sysfs_emit(label, "%s_IBUS_AVG\n", info->label);
+	case PAC1711_VPOWER_ADDR:
+		return sysfs_emit(label, "%s_POWER\n", info->label);
+	}
+
+	return 0;
+}
+
+static void pac1711_work_periodic_rfsh(struct work_struct *work)
+{
+	struct pac1711_chip_info *info = TO_PAC1711_CHIP_INFO((struct delayed_work *)work);
+	struct device *dev = &info->client->dev;
+
+	dev_dbg(dev, "%s - Periodic refresh\n", __func__);
+
+	/* do a REFRESH, then read */
+	pac1711_reg_snapshot(info, true, PAC1711_REFRESH_REG_ADDR,
+			     PAC1711_MIN_UPDATE_WAIT_TIME_US);
+
+	schedule_delayed_work(&info->work_chip_rfsh,
+			      msecs_to_jiffies(PAC1711_MAX_RFSH_LIMIT_MS));
+}
+
+static int pac1711_chip_identify(struct iio_dev *indio_dev, struct pac1711_chip_info *info)
+{
+	struct i2c_client *client = info->client;
+	struct device *dev = &client->dev;
+	u8 chip_rev_info[3];
+	int ret;
+
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_PID_REG_ADDR,
+					    sizeof(chip_rev_info), chip_rev_info);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret, "cannot read product ID reg\n");
+
+	info->chip_variant = chip_rev_info[0];
+	switch (info->chip_variant) {
+	case PAC_PRODUCT_ID_1711:
+		indio_dev->name = pac1711_chip_features.name;
+		break;
+	default:
+		return dev_err_probe(dev, -EINVAL,
+				     "product ID (0x%02X, 0x%02X, 0x%02X) not recognized\n",
+				     chip_rev_info[0], chip_rev_info[1], chip_rev_info[2]);
+	}
+
+	return 0;
+}
+
+static int pac1711_check_range(s32 *vals, const int ranges[][2], int num_ranges)
+{
+	int i;
+
+	for (i = 0; i < num_ranges; i++) {
+		if (vals[0] == ranges[i][0] && vals[1] == ranges[i][1])
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int pac1711_setup_vbus_range(struct pac1711_chip_info *info)
+{
+	const char *prop_name = "microchip,vbus-input-range-microvolt";
+	struct i2c_client *client = info->client;
+	struct device *dev = &client->dev;
+	unsigned int tbl_len, ret;
+	s32 vals[2];
+
+	/* default value is unipolar and Full Scale Range */
+	ret = device_property_read_u32_array(dev, prop_name, vals, 2);
+	if (!ret) {
+		tbl_len = ARRAY_SIZE(pac1711_vbus_range_tbl);
+		ret = pac1711_check_range(vals, pac1711_vbus_range_tbl, tbl_len);
+		if (ret < 0)
+			return dev_err_probe(dev, -EINVAL, "Invalid value %d, %d for prop %s\n",
+					     vals[0], vals[1], prop_name);
+
+		info->vbus_mode = ret;
+	}
+
+	return 0;
+}
+
+static int pac1711_setup_vsense_range(struct pac1711_chip_info *info)
+{
+	const char *prop_name = "microchip,vsense-input-range-microvolt";
+	struct i2c_client *client = info->client;
+	struct device *dev = &client->dev;
+	unsigned int tbl_len, ret;
+	s32 vals[2];
+
+	/* default value is unipolar and Full Scale Range */
+	ret = device_property_read_u32_array(dev, prop_name, vals, 2);
+	if (!ret) {
+		tbl_len = ARRAY_SIZE(pac1711_vsense_range_tbl);
+		ret = pac1711_check_range(vals, pac1711_vsense_range_tbl, tbl_len);
+		if (ret < 0)
+			return dev_err_probe(dev, -EINVAL, "Invalid value %d, %d for prop %s\n",
+					     vals[0], vals[1], prop_name);
+
+		info->vsense_mode = ret;
+	}
+
+	return 0;
+}
+
+static bool pac1711_of_parse_channel_config(struct i2c_client *client,
+					    struct pac1711_chip_info *info)
+{
+	struct device *dev = &client->dev;
+	int ret, temp;
+
+	if (device_property_present(dev, "shunt-resistor-micro-ohms")) {
+		ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms", &info->shunt);
+		if (ret)
+			return dev_err_probe(dev, ret, "Write default shunt-resistor value %d\n",
+					     info->shunt);
+	} else {
+		return dev_err_probe(dev, ret, "shunt-resistor property does not exist\n");
+	}
+
+	if (device_property_present(dev, "label")) {
+		ret = device_property_read_string(dev, "label", (const char **)&info->label);
+		if (ret)
+			return dev_err_probe(dev, ret, "invalid rail-name value\n");
+	}
+
+	ret = pac1711_setup_vbus_range(info);
+	if (ret)
+		return ret;
+
+	ret = pac1711_setup_vsense_range(info);
+	if (ret)
+		return ret;
+
+	ret = device_property_read_u32(dev, "microchip,accumulation-mode", &temp);
+	if (ret)
+		return dev_err_probe(dev, ret, "invalid accumulation-mode value %d\n", temp);
+
+	if (temp == PAC1711_ACCMODE_VPOWER || temp == PAC1711_ACCMODE_VSENSE ||
+	    temp == PAC1711_ACCMODE_VBUS) {
+		dev_dbg(dev, "Accumulation mode set to: %d\n", temp);
+		info->accumulation_mode = temp;
+	} else {
+		return dev_err_probe(dev, -EINVAL, "invalid accumulation-mode value %d\n", temp);
+	}
+
+	return 0;
+}
+
+static void pac1711_cancel_delayed_work(void *dwork)
+{
+	cancel_delayed_work_sync(dwork);
+}
+
+static int pac1711_chip_configure(struct pac1711_chip_info *info)
+{
+	struct i2c_client *client = info->client;
+	struct device *dev = &client->dev;
+	u32 wait_time;
+	u16 tmp_u16;
+	int ret = 0;
+	u8 tmp_u8;
+
+	/* get sampling rate from PAC */
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_CTRL_REG_ADDR,
+					    sizeof(tmp_u16), (u8 *)&tmp_u16);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "cannot read 0x%02X reg\n", PAC1711_CTRL_REG_ADDR);
+
+	be16_to_cpus(&tmp_u16);
+	info->sampling_mode = FIELD_GET(PAC1711_CTRL_SAMPLE_MODE_MASK, tmp_u16);
+
+	/*
+	 * The current/voltage can be measured unidirectional, bidirectional or half FSR
+	 * no SLOW triggered REFRESH, clear POR
+	 */
+	tmp_u8 = FIELD_PREP(PAC1711_NEG_PWR_CFG_VS_MASK, info->vsense_mode) |
+		FIELD_PREP(PAC1711_NEG_PWR_CFG_VB_MASK, info->vbus_mode);
+
+	ret = i2c_smbus_write_byte_data(client, PAC1711_NEG_PWR_FSR_REG_ADDR, tmp_u8);
+	if (ret)
+		return dev_err_probe(dev, ret, "cannot write 0x%02X reg\n",
+				     PAC1711_NEG_PWR_FSR_REG_ADDR);
+
+	tmp_u8 = 0;
+	ret = i2c_smbus_write_byte_data(client, PAC1711_SLOW_REG_ADDR, tmp_u8);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "cannot write 0x%02X reg\n", PAC1711_SLOW_REG_ADDR);
+
+	tmp_u16 = 0;
+	cpu_to_be16s(&tmp_u16);
+
+	ret = i2c_smbus_write_word_data(client, PAC1711_CTRL_REG_ADDR, tmp_u16);
+	if (ret)
+		return dev_err_probe(dev, ret, "cannot write 0x%02X reg\n", PAC1711_CTRL_REG_ADDR);
+
+	/*
+	 * Sending a REFRESH to the chip, so the new settings take place
+	 * as well as resetting the accumulators
+	 */
+	ret = i2c_smbus_write_byte(client, PAC1711_REFRESH_REG_ADDR);
+	if (ret)
+		return dev_err_probe(dev, ret, "cannot write 0x%02X reg\n",
+				     PAC1711_REFRESH_REG_ADDR);
+
+	usleep_range(125, 500);
+
+	/*
+	 * Get the current (in the chip) sampling speed and compute the
+	 * required timeout based on its value the timeout is 1/sampling_speed
+	 * wait the maximum amount of time to be on the safe side - the
+	 * maximum wait time is for 8sps
+	 */
+	wait_time = (1024 / pac1711_samp_rate_map_tbl[info->sampling_mode]) * 1000;
+	usleep_range(wait_time, wait_time + 100);
+
+	INIT_DELAYED_WORK(&info->work_chip_rfsh, pac1711_work_periodic_rfsh);
+	/* Setup the latest moment for reading the regs before saturation */
+	schedule_delayed_work(&info->work_chip_rfsh,
+			      msecs_to_jiffies(PAC1711_MAX_RFSH_LIMIT_MS));
+
+	return devm_add_action_or_reset(&client->dev, pac1711_cancel_delayed_work,
+					&info->work_chip_rfsh);
+}
+
+static struct iio_chan_spec pac1711_single_channel[] = {
+	PAC1711_VPOWER_CHANNEL(0, 0, PAC1711_VPOWER_ADDR),
+	PAC1711_VBUS_CHANNEL(0, 0, PAC1711_VBUS_ADDR),
+	PAC1711_VSENSE_CHANNEL(0, 0, PAC1711_VSENSE_ADDR),
+	PAC1711_VBUS_AVG_CHANNEL(0, 0, PAC1711_VBUS_AVG_ADDR),
+	PAC1711_VSENSE_AVG_CHANNEL(0, 0, PAC1711_VSENSE_AVG_ADDR),
+};
+
+static IIO_DEVICE_ATTR(in_energy_raw, 0444,
+		       pac1711_in_power_acc_raw_show, NULL, 0);
+
+static IIO_DEVICE_ATTR(in_energy_scale, 0444,
+		       pac1711_in_power_acc_scale_show, NULL, 0);
+
+static IIO_DEVICE_ATTR(in_energy_en, 0644,
+		       pac1711_in_enable_acc_show, pac1711_in_enable_acc_store, 0);
+
+static IIO_DEVICE_ATTR(in_current_acc_raw, 0444,
+		       pac1711_in_current_acc_raw_show, NULL, 0);
+
+static IIO_DEVICE_ATTR(in_current_acc_scale, 0444,
+		       pac1711_in_current_acc_scale_show, NULL, 0);
+
+static IIO_DEVICE_ATTR(in_current_acc_en, 0644,
+		       pac1711_in_enable_acc_show, pac1711_in_enable_acc_store, 0);
+
+static IIO_DEVICE_ATTR(in_voltage_acc_raw, 0444,
+		       pac1711_in_voltage_acc_raw_show, NULL, 0);
+
+static IIO_DEVICE_ATTR(in_voltage_acc_scale, 0444,
+		      pac1711_in_voltage_acc_scale_show, NULL, 0);
+
+static IIO_DEVICE_ATTR(in_voltage_acc_en, 0644,
+		       pac1711_in_enable_acc_show, pac1711_in_enable_acc_store, 0);
+
+static struct attribute *pac1711_power_acc_attr[] = {
+	PAC1711_DEV_ATTR(in_energy_raw),
+	PAC1711_DEV_ATTR(in_energy_scale),
+	PAC1711_DEV_ATTR(in_energy_en),
+	NULL
+};
+
+static struct attribute *pac1711_current_acc_attr[] = {
+	PAC1711_DEV_ATTR(in_current_acc_raw),
+	PAC1711_DEV_ATTR(in_current_acc_scale),
+	PAC1711_DEV_ATTR(in_current_acc_en),
+	NULL
+};
+
+static struct attribute *pac1711_voltage_acc_attr[] = {
+	PAC1711_DEV_ATTR(in_voltage_acc_raw),
+	PAC1711_DEV_ATTR(in_voltage_acc_scale),
+	PAC1711_DEV_ATTR(in_voltage_acc_en),
+	NULL
+};
+
+static int pac1711_prep_custom_attributes(struct pac1711_chip_info *info,
+					  struct iio_dev *indio_dev)
+{
+	struct attribute **pac1711_custom_attrs, **tmp_attr;
+	struct device *dev = &info->client->dev;
+	struct attribute_group *pac1711_group;
+	int custom_attr_cnt, j, i;
+
+	pac1711_group = devm_kzalloc(dev, sizeof(*pac1711_group), GFP_KERNEL);
+	if (!pac1711_group)
+		return -ENOMEM;
+
+	custom_attr_cnt = PAC1711_COMMON_DEVATTR + PAC1711_ACC_DEVATTR;
+
+	pac1711_custom_attrs = devm_kzalloc(dev, custom_attr_cnt * sizeof(*pac1711_group) + 1,
+					    GFP_KERNEL);
+
+	if (!pac1711_custom_attrs)
+		return -ENOMEM;
+
+	j = 0;
+
+	for (i = 0; i < PAC1711_COMMON_DEVATTR; i++)
+		pac1711_custom_attrs[j++] = pac1711_all_attrs[i];
+
+	switch (info->accumulation_mode) {
+	case PAC1711_ACCMODE_VPOWER:
+		tmp_attr = pac1711_power_acc_attr;
+		break;
+	case PAC1711_ACCMODE_VSENSE:
+		tmp_attr = pac1711_current_acc_attr;
+		break;
+	case PAC1711_ACCMODE_VBUS:
+		tmp_attr = pac1711_voltage_acc_attr;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	pac1711_custom_attrs[j++] = tmp_attr[0];
+	pac1711_custom_attrs[j++] = tmp_attr[1];
+	pac1711_custom_attrs[j++] = tmp_attr[2];
+
+	pac1711_group->attrs = pac1711_custom_attrs;
+	info->iio_info.attrs = pac1711_group;
+
+	return 0;
+}
+
+static const struct iio_info pac1711_info = {
+	.read_raw = pac1711_read_raw,
+	.write_raw = pac1711_write_raw,
+	.read_label = pac1711_read_label,
+};
+
+static int pac1711_probe(struct i2c_client *client)
+{
+	const struct pac1711_features *chip;
+	struct device *dev = &client->dev;
+	struct pac1711_chip_info *info;
+	struct iio_dev *indio_dev;
+	int ret, err;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+	info->client = client;
+
+	ret = pac1711_chip_identify(indio_dev, info);
+	if (ret < 0) {
+		/*
+		 * If failed to identify the hardware based on internal
+		 * registers, try using fallback compatible in device tree
+		 * to deal with some newer part number.
+		 */
+		chip = i2c_get_match_data(client);
+		if (!chip)
+			return -EINVAL;
+
+		info->chip_variant = chip->prod_id;
+		indio_dev->name = chip->name;
+	}
+
+	/* always start with accumulation channels enabled */
+	info->enable_acc = true;
+	err = pac1711_of_parse_channel_config(client, info);
+	if (err)
+		return dev_err_probe(dev, err, "Error parsing devicetree data\n");
+
+	ret = devm_mutex_init(dev, &info->lock);
+	if (ret)
+		return ret;
+
+	/*
+	 * do now any chip specific initialization (e.g. read/write
+	 * some registers), enable/disable certain channels, change the sampling
+	 * rate to the requested value
+	 */
+	ret = pac1711_chip_configure(info);
+	if (ret < 0)
+		return ret;
+
+	/* prepare the channel information */
+	indio_dev->num_channels = ARRAY_SIZE(pac1711_single_channel);
+	indio_dev->channels = pac1711_single_channel;
+	info->iio_info = pac1711_info;
+	indio_dev->info = &info->iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = pac1711_prep_custom_attributes(info, indio_dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Can't configure custom attributes for PAC1711 device\n");
+
+	/*
+	 * read whatever has been accumulated in the chip so far
+	 * and reset the accumulators
+	 */
+	ret = pac1711_reg_snapshot(info, true, PAC1711_REFRESH_REG_ADDR,
+				   PAC1711_MIN_UPDATE_WAIT_TIME_US);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Can't register IIO device\n");
+
+	return 0;
+}
+
+static const struct i2c_device_id pac1711_id[] = {
+	{ .name = "pac1711", .driver_data = (kernel_ulong_t)&pac1711_chip_features },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, pac1711_id);
+
+static const struct of_device_id pac1711_of_match[] = {
+	{
+		.compatible = "microchip,pac1711",
+		.data = &pac1711_chip_features
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, pac1711_of_match);
+
+static struct i2c_driver pac1711_driver = {
+	.driver	 = {
+		.name = "pac1711",
+		.of_match_table = pac1711_of_match,
+	},
+	.probe = pac1711_probe,
+	.id_table = pac1711_id,
+};
+
+module_i2c_driver(pac1711_driver);
+
+MODULE_AUTHOR("Ariana Lazar <ariana.lazar@microchip.com>");
+MODULE_DESCRIPTION("IIO driver for PAC1711 DC Power/Energy Monitor");
+MODULE_LICENSE("GPL");

-- 
2.43.0

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

[kernel test robot]

Hi Ariana,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 19272b37aa4f83ca52bdf9c16d5d81bdd1354494]

url:    https://github.com/intel-lab-lkp/linux/commits/Ariana-Lazar/dt-bindings-iio-adc-adding-support-for-PAC1711/20251016-002337
base:   19272b37aa4f83ca52bdf9c16d5d81bdd1354494
patch link:    https://lore.kernel.org/r/20251015-pac1711-v1-2-976949e36367%40microchip.com
patch subject: [PATCH 2/2] iio: adc: adding support for PAC1711
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20251016/202510161332.ntymBLFJ-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251016/202510161332.ntymBLFJ-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510161332.ntymBLFJ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/pac1711.c:459:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     459 |         if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     460 |                         msecs_to_jiffies(PAC1711_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/pac1711.c:473:9: note: uninitialized use occurs here
     473 |         return ret;
         |                ^~~
   drivers/iio/adc/pac1711.c:459:2: note: remove the 'if' if its condition is always true
     459 |         if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     460 |                         msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS))) {
         |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/adc/pac1711.c:453:9: note: initialize the variable 'ret' to silence this warning
     453 |         int ret;
         |                ^
         |                 = 0
>> drivers/iio/adc/pac1711.c:1124:29: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
    1124 |                 return dev_err_probe(dev, ret, "shunt-resistor property does not exist\n");
         |                                           ^~~
   drivers/iio/adc/pac1711.c:1116:9: note: initialize the variable 'ret' to silence this warning
    1116 |         int ret, temp;
         |                ^
         |                 = 0
   2 warnings generated.


vim +459 drivers/iio/adc/pac1711.c

   449	
   450	static int pac1711_retrieve_data(struct pac1711_chip_info *info,
   451					 u32 wait_time)
   452	{
   453		int ret;
   454	
   455		/*
   456		 * check if the minimal elapsed time has passed and if so,
   457		 * re-read the chip, otherwise the cached info is just fine
   458		 */
 > 459		if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
   460				msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS))) {
   461			ret = pac1711_reg_snapshot(info, true, PAC1711_REFRESH_REG_ADDR,
   462						   wait_time);
   463	
   464			/*
   465			 * Re-schedule the work for the read registers timeout
   466			 * (to prevent chip regs saturation)
   467			 */
   468			cancel_delayed_work_sync(&info->work_chip_rfsh);
   469			schedule_delayed_work(&info->work_chip_rfsh,
   470					      msecs_to_jiffies(PAC1711_MAX_RFSH_LIMIT_MS));
   471		}
   472	
   473		return ret;
   474	}
   475	

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

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

[Jonathan Cameron]

On Wed, 15 Oct 2025 13:12:16 +0300
Ariana Lazar <ariana.lazar@microchip.com> wrote:

> This is the iio driver for Microchip PAC1711 single-channel power monitor
> with accumulator. The device uses 12-bit resolution for voltage and current
> measurements and 24 bits power calculations. The device supports one 56-bit
> accumulator register.
> 
> PAC1711 measures up to 42V Full-Scale Range.
> 
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
See comments on hwmon / iio in reply to the cover letter. I'll review
this anyway as may save some time either way.

Biggest questions are around what ABI is actually useful to present to
userspace.   Often devices do things like providing voltage accumulation
but in practice that's only because it was easy to build the hardware,
not because anyone actually wants it.  We are safer to not introduce
ABI if unsure, and wait for the usecase to become clear.  New ABI has
a significant burden in terms of tooling updates and long term maintenance.

Thanks,

Jonathan

> ---
>  .../ABI/testing/sysfs-bus-iio-adc-pac1711          |   57 +
>  MAINTAINERS                                        |    2 +
>  drivers/iio/adc/Kconfig                            |   10 +
>  drivers/iio/adc/Makefile                           |    1 +
>  drivers/iio/adc/pac1711.c                          | 1448 ++++++++++++++++++++
>  5 files changed, 1518 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1711 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1711
> new file mode 100644
> index 0000000000000000000000000000000000000000..7f6ab50d29ff064d57b80df0a0c162a4c98764f8
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1711
> @@ -0,0 +1,57 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
> +KernelVersion:	6.18
> +Contact:	linux-iio@vger.kernel.org
> +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. The value
> +		is used to calculate current, power and accumulated energy.

This is standard IIO ABI. We don't duplicate documentation so no entry here.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_current_acc_raw
> +KernelVersion:	6.18
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute is used to read the accumulated voltage measured
> +		on the shunt resistor (Coulomb counter). Units after application
> +		of scale are mA. X is the IIO index of the device.

As you mention it is counting charge.  So not a current measurement at all.
We don't currently have a charge type so we should add one.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_current_acc_scale
> +KernelVersion:	6.18
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		If known for a device, scale to be applied to in_current_raw in
> +		order to obtain the measured value in mA units. X is the IIO
> +		index of the device.

It's not because it is accumulated so with approximation of 
charge = sumof delta t * instantaneous current 
it should be in coulombs.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_current_acc_en
> +KernelVersion:	6.18
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute, if available, is used to enable digital
> +		accumulation of VSENSE measurements. X is the IIO index of the
> +		device.

Other than the type this is standard ABI for channel types that count / accumulate.
So should be a new entry in the _en bit in sysfs-bus-ii

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_acc_raw
Voltage integrals are a weirder thing as they don't in of themselves represent
anything in particular.  I am a bit curious on why anyone wants these
except as part of computing something else.

Still we can define a new channel type for this if we have a bit more on why
it is useful.  It might be more useful presented as an average voltage by
dividing by number of samples.. (the datasheet calls this out as a possible
usecase)  The device also has short term averaging so I'm not sure how useful this
is.  It might not make sense to present this one at all.

> +KernelVersion:	6.18
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute is used to read the accumulated voltage on VBUS.
> +		Units after application of scale are mV. X is the IIO index of
> +		the device.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_acc_scale
> +KernelVersion:	6.18
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		If known for a device, scale to be applied to in_voltage_raw
> +		in order to obtain the measured value in mV units. X is the IIO
> +		index of the device.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_acc_en
> +KernelVersion:	6.18
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute, if available, is used to enable digital
> +		accumulation of VBUS measurements. X is the IIO index of the
> +		device.
> +


> diff --git a/drivers/iio/adc/pac1711.c b/drivers/iio/adc/pac1711.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..30929af202e009c2d80ba91d5c137a26a03f09d7
> --- /dev/null
> +++ b/drivers/iio/adc/pac1711.c
> @@ -0,0 +1,1448 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IIO driver for PAC1711 Multi-Channel DC Power/Energy Monitor
> + *
> + * Copyright (C) 2024 Microchip Technology Inc. and its subsidiaries
Should include 2025 given this is going to change.

> + *
> + * Author: Ariana Lazar <ariana.lazar@microchip.com>
> + *
> + */
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/unaligned.h>

There are definitely some headers missing here.  mutex.h for starters.

Aim to follow principles of include what you used. It is slightly fuzzy
for the kernel so do check that headers you add are commonly included
in drivers.  A few have standard 'grouping' headers that are always used
instead.

> +
> +/* voltage bits resolution when set for unsigned values */
> +#define PAC1711_VOLTAGE_12B_RES			12
> +/* voltage bits resolution when set for signed values */
> +#define PAC1711_VOLTAGE_11B_RES			11
> +
> +/* Power resolution is 24 bits when contains unsigned values */
> +#define PAC1711_POWER_24B_RES			24
> +/* Power resolution is 24 bits when contains signed values */
> +#define PAC1711_POWER_23B_RES			23
Defining something called 24 to the value 24 doesn't seem productive.
Just use 24 / 23 inline as their meaning should be clear.

> +
> +/* Maximum power-product value - 42 V * 0.1 V */
> +#define PAC1711_PRODUCT_VOLTAGE_PV_FSR		4200000000000UL
> +
> +/* Scale constant = (10^3 * 4.2 * 10^9 / 2^24) for mili Watt-second */
> +#define PAC1711_SCALE_CONSTANT			250340
> +
> +/* (100mV * 1000000) / (2^12)  used to calculate the scale for current */
> +#define PAC1711_MAX_VSENSE_RSHIFTED_BY_12B	24414
> +
> +/*
> + * [(100mV * 1000000) / (2^12)]*10^9  used to calculate the scale
> + * for accumulated current/Coulomb counter
I'd have this comment and the value down in the code. Not seeing huge value
from having it up here as it is only used once.
Check whehter the other defines benefit form up here.
> + */
> +#define PAC1711_MAX_VSENSE_NANO			24414062500000UL
> +
> +/* I2C address map */
> +#define PAC1711_REFRESH_REG_ADDR		0x00
> +#define PAC1711_CTRL_REG_ADDR			0x01
> +#define PAC1711_ACC_COUNT_REG_ADDR		0x02
> +#define PAC1711_VACC_ADDR			0x03
> +#define PAC1711_VBUS_ADDR			0x04
> +#define PAC1711_VSENSE_ADDR			0x05
> +#define PAC1711_VBUS_AVG_ADDR			0x06
> +#define PAC1711_VSENSE_AVG_ADDR			0x07
> +#define PAC1711_VPOWER_ADDR			0x08
> +
> +/* Start of configurations registers */
> +#define PAC1711_CTRL_LAT_REG_ADDR		0x0F
> +#define PAC1711_NEG_PWR_FSR_REG_ADDR		0x13
> +#define PAC1711_SLOW_REG_ADDR			0x16
> +#define PAC1711_CTRL_ACT_REG_ADDR		0x17
> +#define PAC1711_PID_REG_ADDR			0xFD
> +#define PAC1711_REVISION_ID_REG_ADDR		0xFF
> +
> +#define PAC1711_ACC_DEVATTR			3
Used in only a few places and better established via ARRAY_SIZE() or similar.

> +#define PAC1711_COMMON_DEVATTR			1
> +#define PAC1711_MEAS_REG_SNAPSHOT_LEN		23
Can we derive that from register addresses. If not add a comment on why it is this.
> +#define PAC1711_ACC_REG_LEN			4
Likewise. These look like values that should be derived not just encoded
as numbers here.
> +#define PAC1711_VACC_REG_LEN			7
> +#define PAC1711_VBUS_SENSE_REG_LEN		2
> +
> +/* PRODUCT IDs */
> +#define PAC_PRODUCT_ID_1711			0x80
> +
> +/* CTRL reg */
Which is this? There isn't a register simply called CTRL. I think
it is CTRL_LAT_REG


> +#define PAC1711_CTRL_SAMPLE_MODE_MASK		GENMASK(15, 12)
> +
> +/* NEG_PWR_CFG reg */

There isn't one called that above.

> +#define PAC1711_NEG_PWR_CFG_VS_MASK		GENMASK(3, 2)
> +#define PAC1711_NEG_PWR_CFG_VB_MASK		GENMASK(1, 0)
I'd group fields just under the register address definition. Makes it clear what
they are for and avoids need for comments.

> +#define PAC1711_SAMPLING_MODE_MASK		GENMASK(11, 0)
> +
> +#define PAC1711_DEV_ATTR(name)			(&iio_dev_attr_##name.dev_attr.attr)
> +
> +enum pac1711_ch_idx {
> +	PAC1711_CH_POWER,
> +	PAC1711_CH_VOLTAGE,
> +	PAC1711_CH_CURRENT,
> +	PAC1711_CH_VOLTAGE_AVERAGE,
> +	PAC1711_CH_CURRENT_AVERAGE,
> +};
> +
> +enum pac1711_acc_mode {
> +	PAC1711_ACCMODE_VPOWER,
> +	PAC1711_ACCMODE_VSENSE,
> +	PAC1711_ACCMODE_VBUS,
> +};
> +
> +enum pac1711_fsr {
> +	PAC1711_FULL_RANGE_UNIPOLAR,
> +	PAC1711_FULL_RANGE_BIPOLAR,
> +	PAC1711_HALF_RANGE_BIPOLAR,
> +};
> +
> +static const int pac1711_vbus_range_tbl[][2] = {
> +	[PAC1711_FULL_RANGE_UNIPOLAR] = {0, 42000000},
> +	[PAC1711_FULL_RANGE_BIPOLAR] = {-42000000, 42000000},
> +	[PAC1711_HALF_RANGE_BIPOLAR] = {-21000000, 21000000},
> +};
> +
> +static const int pac1711_vsense_range_tbl[][2] = {
> +	[PAC1711_FULL_RANGE_UNIPOLAR] = {0, 100000},
> +	[PAC1711_FULL_RANGE_BIPOLAR] = {-100000, 100000},
> +	[PAC1711_HALF_RANGE_BIPOLAR] = {-50000, 50000},
Spacing convention in IIO that we are slowly moving old code to as
well is space after { and before }

> +};
> +
> +enum pac1711_samps {
> +	PAC1711_SAMP_8192SPS,
Given I think these map to register values in the hardware,
I'd prefer to see them assigned values = 0, etc
Same for all similar cases.


> +	PAC1711_SAMP_4096SPS,
> +	PAC1711_SAMP_1024SPS,
> +	PAC1711_SAMP_256SPS,
> +	PAC1711_SAMP_64SPS,
> +	PAC1711_SAMP_8SPS,
> +};
> +
> +static const unsigned int pac1711_samp_rate_map_tbl[] = {
> +	[PAC1711_SAMP_8192SPS] = 8192,
> +	[PAC1711_SAMP_4096SPS] = 4096,
> +	[PAC1711_SAMP_1024SPS] = 1024, // default

With read_avail() callback you should be able to use this array
to provide the information.

> +	[PAC1711_SAMP_256SPS] = 256,
> +	[PAC1711_SAMP_64SPS] = 64,
> +	[PAC1711_SAMP_8SPS] = 8,
> +};
> +
> +static const unsigned int pac1711_shift_map_tbl[] = {

Add a comment on what this is.

> +	[PAC1711_SAMP_8192SPS] = 13,
> +	[PAC1711_SAMP_4096SPS] = 12,
> +	[PAC1711_SAMP_1024SPS] = 10,
> +	[PAC1711_SAMP_256SPS] = 8,
> +	[PAC1711_SAMP_64SPS] = 6,
> +	[PAC1711_SAMP_8SPS] = 3,
> +};
> +
> +/* Available Sample Modes */
> +static const char * const pac1711_frequency_avail[] = {
> +	"8192",
> +	"4096",
> +	"1024",
> +	"256",
> +	"64",
> +	"8",
> +};
> +
> +/**
> + * struct pac1711_features - features of a pac1711 instance
#
I mention it below. We don't normally do this until we have multiple
chips supported.  So I'd rip it out for now and put the values inline.

> + * @prod_id: hardware ID
> + * @name: chip's name
> + */
> +struct pac1711_features {
> +	u8 prod_id;
> +	const char *name;
> +};
> +
> +static const struct pac1711_features pac1711_chip_features = {
> +	.name = "pac1711",
> +	.prod_id = PAC_PRODUCT_ID_1711,
> +};
> +
> +/**
> + * struct reg_data - data from the registers
> + * @meas_regs:		snapshot of raw measurements registers
> + * @jiffies_tstamp:	timestamp
> + * @total_samples_nr:	total number of samples
> + * @ctrl_act_reg:	the ctrl_act register
> + * @ctrl_lat_reg:	the ctrl_lat register
> + * @vsense_avg:		averages of vsense registers
> + * @acc_count:		the acc_count register
> + * @vbus_avg:		averages of vbus registers
> + * @acc_val:		accumulated values per second
> + * @vpower:		vpower registers
> + * @vsense:		vsense registers
> + * @vacc:		accumulated vpower value
> + * @vbus:		vbus registers
> + */
> +struct reg_data {
> +	u8	meas_regs[PAC1711_MEAS_REG_SNAPSHOT_LEN];
> +	unsigned long jiffies_tstamp;
> +	u32	total_samples_nr;
> +	u16	ctrl_act_reg;
> +	u16	ctrl_lat_reg;
> +	s32	vsense_avg;
> +	u32	acc_count;
> +	s32	vbus_avg;
> +	s64	acc_val;
> +	s32	vpower;
> +	s32	vsense;
> +	s32	vbus;
> +	s64	vacc;
As below.  Reorder these to improve packing + group related entries.

> +};
> +
> +/**
> + * struct pac1711_chip_info - information about the chip
> + * @accumulation_mode:	accumulation mode for hardware accumulator
> + * @sample_rate_value:	sampling frequency
> + * @work_chip_rfsh:	work queue used for refresh commands
> + * @sampling_mode:	sampling mode used by the device
> + * @chip_reg_data:	chip reg data
> + * @chip_variant:	chip variant
> + * @vsense_mode:	Full Scale Range (FSR) mode for VSense
> + * @enable_acc:		true means that accumulation channel is measured
> + * @vbus_mode:		Full Scale Range (FSR) mode for VBus
> + * @iio_info:		iio_info
> + * @client:		the i2c-client attached to the device
> + * @label:		channel label
> + * @shunt:		shunt resistor value
> + * @lock:		synchronize access to driver's state members
> + */
> +struct pac1711_chip_info {
> +	u8			accumulation_mode;
Don't do reverse xmas tree for structure elements!
The packing looks terrible.
Order them first into logical related groups, then order such that packing
is efficient. Pahole or a glance at rules for packing in C will help with that.

> +	s32			sample_rate_value;
> +	struct delayed_work	work_chip_rfsh;
> +	u8			sampling_mode;
> +	struct reg_data		chip_reg_data;
> +	u8			chip_variant;
> +	u8			vsense_mode;
> +	bool			enable_acc;
> +	u8			vbus_mode;
> +	struct iio_info		iio_info;
> +	struct i2c_client	*client;
> +	char			*label;
> +	u32			shunt;
> +	struct mutex		lock; /* synchronize access to driver's state members */

Be more specific on this comment.  Which members and why.  Is it to keep
them inline with hardware settings, or are there complex things that
need to be done atomically?

> +};
> +
> +static inline u64 pac1711_get_unaligned_be56(u8 *p)
> +{
> +	return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 |
> +		(u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6];

Hmm.  Maybe this one is reasonable to add to the main unaligned function set.
Perhaps not yet as a grep didn't find multiple of this size.


> +}
> +
> +static int pac1711_send_refresh(struct pac1711_chip_info *info, u8 refresh_cmd,
> +				u32 wait_time)
> +{
> +	struct i2c_client *client = info->client;
> +	int ret;
> +
> +	/* Writing a REFRESH or a REFRESH_V command */
> +	ret = i2c_smbus_write_byte(client, refresh_cmd);
> +	if (ret) {
> +		dev_err(&client->dev, "%s - cannot send Refresh cmd (0x%02X)\n",
> +			__func__, refresh_cmd);
> +		return ret;
> +	}
> +
> +	/* Register data retrieval timestamp */
> +	info->chip_reg_data.jiffies_tstamp = jiffies;
> +	/* Wait till the data is available */

fsleep() for waits that are fuzzy like this one.
It adds standard level of fuzz and means we don't need to think about what
is appropriate.

> +	usleep_range(wait_time, wait_time + 100);
> +
> +	return 0;
> +}
> +
> +static int pac1711_reg_snapshot(struct pac1711_chip_info *info,
> +				bool do_refresh, u8 refresh_cmd, u32 wait_time)
> +{
> +	struct i2c_client *client = info->client;
> +	struct device *dev = &client->dev;
> +	s64 stored_value, tmp_s64;
> +	u8 *offset_reg_data_p;
> +	u32 count, inc_count;
> +	bool is_bipolar;
> +	s64 inc = 0;
> +	u16 tmp_u16;
> +	u8 shift;
> +	int ret;
> +
> +	guard(mutex)(&info->lock);
> +
> +	if (do_refresh) {
> +		ret = pac1711_send_refresh(info, refresh_cmd, wait_time);
> +		if (ret < 0) {
> +			dev_err(dev, "cannot send refresh\n");
> +			return ret;
> +		}
> +	}
> +
> +	/* Read the ctrl/status registers for this snapshot */
> +	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_CTRL_ACT_REG_ADDR,
> +					    sizeof(tmp_u16), (u8 *)&tmp_u16);
> +	if (ret < 0) {
> +		dev_err(dev, "%s - cannot read regs from 0x%02X\n",
> +			__func__, PAC1711_CTRL_ACT_REG_ADDR);
> +		return ret;
> +	}
> +
> +	be16_to_cpus(&tmp_u16);
As below. Use a local __be16 to keep the meaning of each local variable cleaner.

> +	info->chip_reg_data.ctrl_act_reg = tmp_u16;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_CTRL_LAT_REG_ADDR,
> +					    sizeof(tmp_u16), (u8 *)&tmp_u16);
> +	if (ret < 0) {
> +		dev_err(dev, "%s - cannot read regs from 0x%02X\n",
> +			__func__, PAC1711_CTRL_LAT_REG_ADDR);
> +		return ret;
> +	}
> +
> +	be16_to_cpus(&tmp_u16);
Same as above.
> +	info->chip_reg_data.ctrl_lat_reg = tmp_u16;
> +
> +	/* Read the data registers */
> +	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_ACC_COUNT_REG_ADDR,
> +					    PAC1711_MEAS_REG_SNAPSHOT_LEN,
> +					    (u8 *)info->chip_reg_data.meas_regs);
> +	if (ret < 0) {
> +		dev_err(dev, "%s - cannot read regs from 0x%02X\n",
> +			__func__, PAC1711_ACC_COUNT_REG_ADDR);
> +		return ret;
> +	}
> +
> +	offset_reg_data_p = &info->chip_reg_data.meas_regs[0];
> +	info->chip_reg_data.acc_count = get_unaligned_be32(offset_reg_data_p);
> +	offset_reg_data_p += PAC1711_ACC_REG_LEN;
> +
> +	/* skip if the energy accumulation is disabled */
> +	if (info->enable_acc) {
> +		stored_value = info->chip_reg_data.acc_val;
> +
> +		info->chip_reg_data.vacc = pac1711_get_unaligned_be56(offset_reg_data_p);
> +		is_bipolar = false;
> +
> +		switch (info->accumulation_mode) {
> +		case PAC1711_ACCMODE_VPOWER:
> +			if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR ||
> +			    info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
> +				is_bipolar = true;
> +			break;
> +		case PAC1711_ACCMODE_VBUS:
> +			if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR)
> +				is_bipolar = true;
> +			break;
> +		case PAC1711_ACCMODE_VSENSE:
> +			if (info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
> +				is_bipolar = true;
> +			break;
> +		}
> +
> +		if (is_bipolar)
> +			info->chip_reg_data.vacc = sign_extend64(info->chip_reg_data.vacc, 55);
> +
> +		if (info->accumulation_mode != PAC1711_ACCMODE_VBUS) {
> +			/* VACC */
> +			/*
> +			 * Integrate the accumulated power or current over
> +			 * the elapsed interval.
> +			 */
> +			tmp_u16 = FIELD_GET(PAC1711_CTRL_SAMPLE_MODE_MASK,
> +					    info->chip_reg_data.ctrl_lat_reg);
> +			tmp_s64 = info->chip_reg_data.vacc;
> +
> +			if (tmp_u16 <= PAC1711_SAMP_8SPS) {
> +				shift = pac1711_shift_map_tbl[tmp_u16];
> +				inc = tmp_s64 >> shift;
> +			} else {
> +				dev_err(dev, "Invalid sample rate index: %d!\n", tmp_u16);
return an error if this happens and give up.  I'm guessing it's not supposed to happen.

> +			}
> +		} else {
> +			count = info->chip_reg_data.total_samples_nr;
> +			inc_count = info->chip_reg_data.acc_count;
> +
> +			/* Check if total number of samples will overflow */
> +			if (check_add_overflow(count, inc_count, &count)) {
> +				dev_err(dev, "Number of samples overflow!\n");
As above. If we get an error condition I'd communicate it up with an error
return value, not just something stashed in the kernel log.
> +				info->chip_reg_data.total_samples_nr = 0;
> +				info->chip_reg_data.acc_val = 0;
> +			}
> +
> +			info->chip_reg_data.total_samples_nr += inc_count;
> +			inc = info->chip_reg_data.vacc;
> +		}
> +
> +		if (check_add_overflow(stored_value, inc, &stored_value)) {
> +			if (is_negative(stored_value))
> +				info->chip_reg_data.acc_val = S64_MIN;
> +			else
> +				info->chip_reg_data.acc_val = S64_MAX;
> +
> +			dev_err(dev, "Overflow detected!\n");

and again. These conditions are not supposed to happen.  Make them
really obvious.

> +		} else {
> +			info->chip_reg_data.acc_val += inc;
> +		}
> +	}
> +
> +	offset_reg_data_p += PAC1711_VACC_REG_LEN;
> +
> +	/* VBUS */
> +	info->chip_reg_data.vbus = get_unaligned_be16(offset_reg_data_p) >> 4;
> +
> +	if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR)
> +		info->chip_reg_data.vbus = sign_extend32(info->chip_reg_data.vbus, 11);
> +
> +	offset_reg_data_p += PAC1711_VBUS_SENSE_REG_LEN;
> +
> +	/* VSENSE */
> +	info->chip_reg_data.vsense = get_unaligned_be16(offset_reg_data_p) >> 4;
> +
> +	if (info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
> +		info->chip_reg_data.vsense = sign_extend32(info->chip_reg_data.vsense, 11);
> +
> +	offset_reg_data_p += PAC1711_VBUS_SENSE_REG_LEN;
> +
> +	/* VBUS_AVG */
> +	info->chip_reg_data.vbus_avg = get_unaligned_be16(offset_reg_data_p) >> 4;
> +
> +	if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR)
> +		info->chip_reg_data.vbus_avg = sign_extend32(info->chip_reg_data.vbus_avg, 11);
> +
> +	offset_reg_data_p += PAC1711_VBUS_SENSE_REG_LEN;
> +
> +	/* VSENSE_AVG */
> +	info->chip_reg_data.vsense_avg = get_unaligned_be16(offset_reg_data_p) >> 4;
> +
> +	if (info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
> +		info->chip_reg_data.vsense_avg = sign_extend32(info->chip_reg_data.vsense_avg, 11);
> +
> +	offset_reg_data_p += PAC1711_VBUS_SENSE_REG_LEN;
> +
> +	/* VPOWER */
> +	info->chip_reg_data.vpower = get_unaligned_be32(offset_reg_data_p) >> 8;
> +
> +	if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR ||
> +	    info->vsense_mode != PAC1711_FULL_RANGE_UNIPOLAR)
> +		info->chip_reg_data.vpower = sign_extend32(info->chip_reg_data.vpower, 23);
> +
> +	return 0;
> +}

> +
> +static const struct iio_chan_spec_ext_info pac1711_ext_info[] = {
> +	IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, &sampling_mode_enum),
Use the info_mask_shared_by_all for sampling frequency.

> +	{
> +		.name = "sampling_frequency_available",
read_avail() callback and appropriate mask for this.

we have had core support for generating these attributes for a while now.

> +		.shared = IIO_SHARED_BY_ALL,
> +		.read = iio_enum_available_read,
> +		.private = (uintptr_t)&sampling_mode_enum,
> +	},
> +	{ }
> +};


> +static int pac1711_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct pac1711_chip_info *info = iio_priv(indio_dev);
> +	int ret;
> +	u64 tmp;
> +
> +	ret = pac1711_retrieve_data(info, PAC1711_MIN_UPDATE_WAIT_TIME_US);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = info->chip_reg_data.vbus;
> +			return IIO_VAL_INT;
> +		case IIO_CURRENT:
> +			*val = info->chip_reg_data.vsense;
> +			return IIO_VAL_INT;
> +		case IIO_POWER:
> +			*val = info->chip_reg_data.vpower;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_AVERAGE_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = info->chip_reg_data.vbus_avg;
Mentioned elsewhere but from practical usecase point of view it is very
rare that rolling average and unaveraged at the same time is actually useful.
Providing the rolling average only  + description of filter parameters
that reflect the boxcar filter usually ends up being the cleaner interface.

If you have a usecase that needs both then we can figure out how to do it
cleanly.

> +			return IIO_VAL_INT;
> +		case IIO_CURRENT:
> +			*val = info->chip_reg_data.vsense_avg;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}

...

> +}
> +
> +static int pac1711_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct pac1711_chip_info *info = iio_priv(indio_dev);
> +	struct i2c_client *client = info->client;
> +	struct device *dev = &info->client->dev;
> +	int ret = -EINVAL;

set in all paths that use it so no need to initialize here.

> +	s32 old_samp_rate;
> +	u8 ctrl_reg;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = pac1711_get_samp_rate_idx(info, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* write the new sampling value and trigger a snapshot */
> +		scoped_guard(mutex, &info->lock) {
> +			ctrl_reg = FIELD_PREP(PAC1711_CTRL_SAMPLE_MODE_MASK, ret);
> +			ret = i2c_smbus_write_byte_data(client, PAC1711_CTRL_REG_ADDR, ctrl_reg);
> +			if (ret) {
> +				dev_err(dev, "%s - can't update sample rate\n", __func__);
> +				return ret;
> +			}
> +		}
> +
> +		old_samp_rate = info->sample_rate_value;
> +		info->sample_rate_value = val;
> +
> +		/*
> +		 * now, force a snapshot with refresh - call retrieve
> +		 * data in order to update the refresh timer
> +		 * alter the timestamp in order to force trigger a
> +		 * register snapshot and a timestamp update
> +		 */
> +		info->chip_reg_data.jiffies_tstamp -= msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS);
> +		ret = pac1711_retrieve_data(info, (1024 / old_samp_rate) * 1000);
> +		if (ret < 0) {
> +			dev_err(dev, "%s - cannot snapshot ctrl and measurement regs\n", __func__);
> +			return ret;
> +		}
> +
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int pac1711_read_label(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, char *label)
> +{
> +	struct pac1711_chip_info *info = iio_priv(indio_dev);
> +
> +	switch (chan->address) {
> +	case PAC1711_VBUS_ADDR:
> +		return sysfs_emit(label, "%s_VBUS\n", info->label);

I'm not sure I see the advantage in the postfixes.  This seems
to be building channel labels from the device label.  If you want
channel labels, provide the channel subnodes and put the label under that.

If you want a device label set indio_dev->label


> +	case PAC1711_VBUS_AVG_ADDR:
> +		return sysfs_emit(label, "%s_VBUS_AVG\n", info->label);

oh goody. Moving averages. These are a pain to support cleanly.  They
sound simple but they overlap too much with more complex forms of
data processing.

Typically a user either wants the smoothed version or the non smoothed
one so we don't necessarily need separate channels. An alternative is
to provide a filter control via the 3dB point of the boxcar filter
that an average is.

> +	case PAC1711_VSENSE_ADDR:
> +		return sysfs_emit(label, "%s_IBUS\n", info->label);
> +	case PAC1711_VSENSE_AVG_ADDR:
> +		return sysfs_emit(label, "%s_IBUS_AVG\n", info->label);
> +	case PAC1711_VPOWER_ADDR:
> +		return sysfs_emit(label, "%s_POWER\n", info->label);
> +	}
> +
> +	return 0;
> +}

> +
> +static int pac1711_chip_identify(struct iio_dev *indio_dev, struct pac1711_chip_info *info)
> +{
> +	struct i2c_client *client = info->client;
> +	struct device *dev = &client->dev;
> +	u8 chip_rev_info[3];
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_PID_REG_ADDR,
> +					    sizeof(chip_rev_info), chip_rev_info);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret, "cannot read product ID reg\n");
> +
> +	info->chip_variant = chip_rev_info[0];
> +	switch (info->chip_variant) {
> +	case PAC_PRODUCT_ID_1711:
> +		indio_dev->name = pac1711_chip_features.name;
> +		break;
return 0 here so we know nothing else to look for after thi.

> +	default:
> +		return dev_err_probe(dev, -EINVAL,
dev_info() / dev_warn() only as we are going to carry on anyway.

> +				     "product ID (0x%02X, 0x%02X, 0x%02X) not recognized\n",
> +				     chip_rev_info[0], chip_rev_info[1], chip_rev_info[2]);
> +	}
> +
> +	return 0;
> +}

> +static int pac1711_setup_vbus_range(struct pac1711_chip_info *info)
> +{
> +	const char *prop_name = "microchip,vbus-input-range-microvolt";
> +	struct i2c_client *client = info->client;
> +	struct device *dev = &client->dev;
> +	unsigned int tbl_len, ret;
> +	s32 vals[2];
> +
> +	/* default value is unipolar and Full Scale Range */
> +	ret = device_property_read_u32_array(dev, prop_name, vals, 2);
> +	if (!ret) {
> +		tbl_len = ARRAY_SIZE(pac1711_vbus_range_tbl);
> +		ret = pac1711_check_range(vals, pac1711_vbus_range_tbl, tbl_len);
> +		if (ret < 0)
> +			return dev_err_probe(dev, -EINVAL, "Invalid value %d, %d for prop %s\n",
> +					     vals[0], vals[1], prop_name);
> +
> +		info->vbus_mode = ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pac1711_setup_vsense_range(struct pac1711_chip_info *info)
> +{
> +	const char *prop_name = "microchip,vsense-input-range-microvolt";

Either figure out how to generalize this code to share with the function
above, or just put prop_name string inline.  It is a bit marginal
on whether it is worth generalizing.

> +	struct i2c_client *client = info->client;
> +	struct device *dev = &client->dev;
> +	unsigned int tbl_len, ret;
> +	s32 vals[2];
> +
> +	/* default value is unipolar and Full Scale Range */
> +	ret = device_property_read_u32_array(dev, prop_name, vals, 2);
> +	if (!ret) {
> +		tbl_len = ARRAY_SIZE(pac1711_vsense_range_tbl);
> +		ret = pac1711_check_range(vals, pac1711_vsense_range_tbl, tbl_len);
> +		if (ret < 0)
> +			return dev_err_probe(dev, -EINVAL, "Invalid value %d, %d for prop %s\n",
> +					     vals[0], vals[1], prop_name);
> +
> +		info->vsense_mode = ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static bool pac1711_of_parse_channel_config(struct i2c_client *client,
> +					    struct pac1711_chip_info *info)
> +{
> +	struct device *dev = &client->dev;
> +	int ret, temp;
> +
> +	if (device_property_present(dev, "shunt-resistor-micro-ohms")) {
> +		ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms", &info->shunt);
checking for presence before reading is pointless.
device_property_read_u32() will error out if it isn't there anyway.
The error code will indicate why.

> +		if (ret)
> +			return dev_err_probe(dev, ret, "Write default shunt-resistor value %d\n",
> +					     info->shunt);
> +	} else {
> +		return dev_err_probe(dev, ret, "shunt-resistor property does not exist\n");
> +	}
> +
> +	if (device_property_present(dev, "label")) {
> +		ret = device_property_read_string(dev, "label", (const char **)&info->label);

The cast seems odd here.  Why can't info->label be a const char * ?


> +		if (ret)
> +			return dev_err_probe(dev, ret, "invalid rail-name value\n");
> +	}
> +
> +	ret = pac1711_setup_vbus_range(info);
> +	if (ret)
> +		return ret;
> +
> +	ret = pac1711_setup_vsense_range(info);
> +	if (ret)
> +		return ret;
> +
> +	ret = device_property_read_u32(dev, "microchip,accumulation-mode", &temp);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "invalid accumulation-mode value %d\n", temp);

As in the dt-binding review. I'm not sure why this needs to be coming from DT.
As above, it's also not clear to me that it is worth supporting some of the modes.

> +
> +	if (temp == PAC1711_ACCMODE_VPOWER || temp == PAC1711_ACCMODE_VSENSE ||
> +	    temp == PAC1711_ACCMODE_VBUS) {
> +		dev_dbg(dev, "Accumulation mode set to: %d\n", temp);
> +		info->accumulation_mode = temp;
> +	} else {
> +		return dev_err_probe(dev, -EINVAL, "invalid accumulation-mode value %d\n", temp);
> +	}
> +
> +	return 0;
> +}
> +
> +static void pac1711_cancel_delayed_work(void *dwork)
> +{
> +	cancel_delayed_work_sync(dwork);
> +}
> +
> +static int pac1711_chip_configure(struct pac1711_chip_info *info)
> +{
> +	struct i2c_client *client = info->client;
> +	struct device *dev = &client->dev;
> +	u32 wait_time;
> +	u16 tmp_u16;
> +	int ret = 0;
> +	u8 tmp_u8;
> +
> +	/* get sampling rate from PAC */
> +	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_CTRL_REG_ADDR,
> +					    sizeof(tmp_u16), (u8 *)&tmp_u16);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "cannot read 0x%02X reg\n", PAC1711_CTRL_REG_ADDR);
> +
> +	be16_to_cpus(&tmp_u16);

Avoid messy endian confusion by reading into a __be16 and then
using tmp_u16 = be16_to_cpu(&tmp_be16);

> +	info->sampling_mode = FIELD_GET(PAC1711_CTRL_SAMPLE_MODE_MASK, tmp_u16);
> +
> +	/*
> +	 * The current/voltage can be measured unidirectional, bidirectional or half FSR
> +	 * no SLOW triggered REFRESH, clear POR
> +	 */
> +	tmp_u8 = FIELD_PREP(PAC1711_NEG_PWR_CFG_VS_MASK, info->vsense_mode) |
> +		FIELD_PREP(PAC1711_NEG_PWR_CFG_VB_MASK, info->vbus_mode);
> +
> +	ret = i2c_smbus_write_byte_data(client, PAC1711_NEG_PWR_FSR_REG_ADDR, tmp_u8);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "cannot write 0x%02X reg\n",
> +				     PAC1711_NEG_PWR_FSR_REG_ADDR);
> +
> +	tmp_u8 = 0;
Don't bother setting local variable, just pass 0 in directly to the write_byte_data()

> +	ret = i2c_smbus_write_byte_data(client, PAC1711_SLOW_REG_ADDR, tmp_u8);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "cannot write 0x%02X reg\n", PAC1711_SLOW_REG_ADDR);
> +
> +	tmp_u16 = 0;

As above. Avoid mixing what is stored in a variable by adding a __be16 local variable.

> +	cpu_to_be16s(&tmp_u16);
> +
> +	ret = i2c_smbus_write_word_data(client, PAC1711_CTRL_REG_ADDR, tmp_u16);
The documentation for what is supported is a bit vague, but I'm not seeing 
> +	if (ret)
> +		return dev_err_probe(dev, ret, "cannot write 0x%02X reg\n", PAC1711_CTRL_REG_ADDR);
> +
> +	/*
> +	 * Sending a REFRESH to the chip, so the new settings take place
> +	 * as well as resetting the accumulators
> +	 */
> +	ret = i2c_smbus_write_byte(client, PAC1711_REFRESH_REG_ADDR);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "cannot write 0x%02X reg\n",
> +				     PAC1711_REFRESH_REG_ADDR);
> +
> +	usleep_range(125, 500);
> +
> +	/*
> +	 * Get the current (in the chip) sampling speed and compute the
> +	 * required timeout based on its value the timeout is 1/sampling_speed
> +	 * wait the maximum amount of time to be on the safe side - the
> +	 * maximum wait time is for 8sps
> +	 */
> +	wait_time = (1024 / pac1711_samp_rate_map_tbl[info->sampling_mode]) * 1000;
> +	usleep_range(wait_time, wait_time + 100);
> +
> +	INIT_DELAYED_WORK(&info->work_chip_rfsh, pac1711_work_periodic_rfsh);
> +	/* Setup the latest moment for reading the regs before saturation */
> +	schedule_delayed_work(&info->work_chip_rfsh,
> +			      msecs_to_jiffies(PAC1711_MAX_RFSH_LIMIT_MS));
> +
> +	return devm_add_action_or_reset(&client->dev, pac1711_cancel_delayed_work,
> +					&info->work_chip_rfsh);
> +}
> +
> +static struct iio_chan_spec pac1711_single_channel[] = {

As below. Clearly not a single channel.

> +	PAC1711_VPOWER_CHANNEL(0, 0, PAC1711_VPOWER_ADDR),
> +	PAC1711_VBUS_CHANNEL(0, 0, PAC1711_VBUS_ADDR),
> +	PAC1711_VSENSE_CHANNEL(0, 0, PAC1711_VSENSE_ADDR),
> +	PAC1711_VBUS_AVG_CHANNEL(0, 0, PAC1711_VBUS_AVG_ADDR),
> +	PAC1711_VSENSE_AVG_CHANNEL(0, 0, PAC1711_VSENSE_AVG_ADDR),
> +};
> +
> +static IIO_DEVICE_ATTR(in_energy_raw, 0444,
> +		       pac1711_in_power_acc_raw_show, NULL, 0);
> +
> +static IIO_DEVICE_ATTR(in_energy_scale, 0444,
> +		       pac1711_in_power_acc_scale_show, NULL, 0);
> +
> +static IIO_DEVICE_ATTR(in_energy_en, 0644,
> +		       pac1711_in_enable_acc_show, pac1711_in_enable_acc_store, 0);
> +
> +static IIO_DEVICE_ATTR(in_current_acc_raw, 0444,
> +		       pac1711_in_current_acc_raw_show, NULL, 0);
> +
> +static IIO_DEVICE_ATTR(in_current_acc_scale, 0444,
> +		       pac1711_in_current_acc_scale_show, NULL, 0);
> +
> +static IIO_DEVICE_ATTR(in_current_acc_en, 0644,
> +		       pac1711_in_enable_acc_show, pac1711_in_enable_acc_store, 0);
> +
> +static IIO_DEVICE_ATTR(in_voltage_acc_raw, 0444,
> +		       pac1711_in_voltage_acc_raw_show, NULL, 0);
> +
> +static IIO_DEVICE_ATTR(in_voltage_acc_scale, 0444,
> +		      pac1711_in_voltage_acc_scale_show, NULL, 0);
> +
> +static IIO_DEVICE_ATTR(in_voltage_acc_en, 0644,
> +		       pac1711_in_enable_acc_show, pac1711_in_enable_acc_store, 0);
> +
> +static struct attribute *pac1711_power_acc_attr[] = {
> +	PAC1711_DEV_ATTR(in_energy_raw),
> +	PAC1711_DEV_ATTR(in_energy_scale),
> +	PAC1711_DEV_ATTR(in_energy_en),
> +	NULL
> +};
> +
> +static struct attribute *pac1711_current_acc_attr[] = {
> +	PAC1711_DEV_ATTR(in_current_acc_raw),
> +	PAC1711_DEV_ATTR(in_current_acc_scale),
> +	PAC1711_DEV_ATTR(in_current_acc_en),
> +	NULL
> +};
> +
> +static struct attribute *pac1711_voltage_acc_attr[] = {
> +	PAC1711_DEV_ATTR(in_voltage_acc_raw),
> +	PAC1711_DEV_ATTR(in_voltage_acc_scale),
> +	PAC1711_DEV_ATTR(in_voltage_acc_en),

If these did make sense (see other comments) then they should be
done as ext_info for the given channels rather than going though
the complexity of custom attributes.

Custom attributes make most sense when not associated with any channels.
A lot of drivers have them for historical reasons as well that we haven't
yet cleaned up.

Even in_shunt_resistor is can be a shared_by_all extended attribute
as it's still associated with a bunch of channels.

> +	NULL
> +};

> +
> +static int pac1711_probe(struct i2c_client *client)
> +{
> +	const struct pac1711_features *chip;
> +	struct device *dev = &client->dev;
> +	struct pac1711_chip_info *info;
> +	struct iio_dev *indio_dev;
> +	int ret, err;

> +
> +	/* always start with accumulation channels enabled */
> +	info->enable_acc = true;
> +	err = pac1711_of_parse_channel_config(client, info);
Should really using generic property.h accessors so name it after
fw rather than of as it applies to other firmware types.

> +	if (err)
> +		return dev_err_probe(dev, err, "Error parsing devicetree data\n");
> +
> +	ret = devm_mutex_init(dev, &info->lock);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * do now any chip specific initialization (e.g. read/write
> +	 * some registers), enable/disable certain channels, change the sampling
> +	 * rate to the requested value
I don't think this comment adds much.  The name makes it clear it's
configuring the chip and if anyone cares what that involves they can go look.
So I'd drop it.

> +	 */
> +	ret = pac1711_chip_configure(info);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* prepare the channel information */
Likewise. Comment is too obvious to be needed.  
> +	indio_dev->num_channels = ARRAY_SIZE(pac1711_single_channel);
> +	indio_dev->channels = pac1711_single_channel;
Single channel?  That seems unlikely naming for an array of several channels. Rename it.
pac1711_channels probably.

> +	info->iio_info = pac1711_info;
> +	indio_dev->info = &info->iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = pac1711_prep_custom_attributes(info, indio_dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Can't configure custom attributes for PAC1711 device\n");
> +
> +	/*
> +	 * read whatever has been accumulated in the chip so far
> +	 * and reset the accumulators
> +	 */
> +	ret = pac1711_reg_snapshot(info, true, PAC1711_REFRESH_REG_ADDR,
> +				   PAC1711_MIN_UPDATE_WAIT_TIME_US);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Can't register IIO device\n");
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id pac1711_id[] = {
> +	{ .name = "pac1711", .driver_data = (kernel_ulong_t)&pac1711_chip_features },

Normally we only introduce the use of driver data in a series that adds a second supported
part.  Otherwise we end up carrying complexity that may never be used.

If you already have code adding a second part then say that in the patch description
as a justification for having this flexibility from the first.

> +	{}
As below.

> +};
> +MODULE_DEVICE_TABLE(i2c, pac1711_id);
> +
> +static const struct of_device_id pac1711_of_match[] = {
> +	{
> +		.compatible = "microchip,pac1711",
> +		.data = &pac1711_chip_features
> +	},
> +	{}

For IIO at least I'm pushing to standardize on formatting these as
	{ }
It was a random choice a while back as we had about half each way.

> +};

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

[Ariana.Lazar]

Hi Jonathan,

Thank you for the feedback, please see my comments bellow:

> > Best regards,
Ariana
> > 
> 
> > +};
> > +
> > +static inline u64 pac1711_get_unaligned_be56(u8 *p)
> > +{
> > +     return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 |
> > +             (u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6];
> 
> Hmm.  Maybe this one is reasonable to add to the main unaligned
> function set.
> Perhaps not yet as a grep didn't find multiple of this size.
> 

There has been an unsuccessful attempt to submit this function to the
unaligned set. It was rejected because of the lack of current possible
shared uses. Please see the discussion thread below:

https://yhbt.net/lore/lkml/20240927083543.80275-1-
marius.cristea@microchip.com/T/#u

> 
> 
...
> 
> > +
> > +static struct iio_chan_spec pac1711_single_channel[] = {
> 
> As below. Clearly not a single channel.
> 
> > +     PAC1711_VPOWER_CHANNEL(0, 0, PAC1711_VPOWER_ADDR),
> > +     PAC1711_VBUS_CHANNEL(0, 0, PAC1711_VBUS_ADDR),
> > +     PAC1711_VSENSE_CHANNEL(0, 0, PAC1711_VSENSE_ADDR),
> > +     PAC1711_VBUS_AVG_CHANNEL(0, 0, PAC1711_VBUS_AVG_ADDR),
> > +     PAC1711_VSENSE_AVG_CHANNEL(0, 0, PAC1711_VSENSE_AVG_ADDR),
> > +};
> > +
> > +static IIO_DEVICE_ATTR(in_energy_raw, 0444,
> > +                    pac1711_in_power_acc_raw_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_energy_scale, 0444,
> > +                    pac1711_in_power_acc_scale_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_energy_en, 0644,
> > +                    pac1711_in_enable_acc_show,
> > pac1711_in_enable_acc_store, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_current_acc_raw, 0444,
> > +                    pac1711_in_current_acc_raw_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_current_acc_scale, 0444,
> > +                    pac1711_in_current_acc_scale_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_current_acc_en, 0644,
> > +                    pac1711_in_enable_acc_show,
> > pac1711_in_enable_acc_store, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_voltage_acc_raw, 0444,
> > +                    pac1711_in_voltage_acc_raw_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_voltage_acc_scale, 0444,
> > +                   pac1711_in_voltage_acc_scale_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_voltage_acc_en, 0644,
> > +                    pac1711_in_enable_acc_show,
> > pac1711_in_enable_acc_store, 0);
> > +
> > +static struct attribute *pac1711_power_acc_attr[] = {
> > +     PAC1711_DEV_ATTR(in_energy_raw),
> > +     PAC1711_DEV_ATTR(in_energy_scale),
> > +     PAC1711_DEV_ATTR(in_energy_en),
> > +     NULL
> > +};
> > +
> > +static struct attribute *pac1711_current_acc_attr[] = {
> > +     PAC1711_DEV_ATTR(in_current_acc_raw),
> > +     PAC1711_DEV_ATTR(in_current_acc_scale),
> > +     PAC1711_DEV_ATTR(in_current_acc_en),
> > +     NULL
> > +};
> > +
> > +static struct attribute *pac1711_voltage_acc_attr[] = {
> > +     PAC1711_DEV_ATTR(in_voltage_acc_raw),
> > +     PAC1711_DEV_ATTR(in_voltage_acc_scale),
> > +     PAC1711_DEV_ATTR(in_voltage_acc_en),
> 
> If these did make sense (see other comments) then they should be
> done as ext_info for the given channels rather than going though
> the complexity of custom attributes.
> 
> Custom attributes make most sense when not associated with any
> channels.
> A lot of drivers have them for historical reasons as well that we
> haven't
> yet cleaned up.
> 
> Even in_shunt_resistor is can be a shared_by_all extended attribute
> as it's still associated with a bunch of channels.
> 
> 

As for switching accumulator channels to ext_info instead of custom
attributes, only _en channel would be eligible. Both _raw and _scale do
not have constant values, therefore cannot be defined with consts at
startup. Scale can be modified during runtime by changing shunt value
and raw constantly accumulates the read values. It is not recommended
to switch accumulation mode during runtime, but that does not make
possible defining as consts the _raw or _scale attributes. If there
were an workaround which does not need the definition of items and
num_items fields, I would switch to using ext_info.

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

[Andy Shevchenko]

On Wed, Nov 19, 2025 at 03:31:24PM +0000, Ariana.Lazar@microchip.com wrote:
> 
> There has been an unsuccessful attempt to submit this function to the
> unaligned set. It was rejected because of the lack of current possible
> shared uses. Please see the discussion thread below:
> 
> https://yhbt.net/lore/lkml/20240927083543.80275-1-
> marius.cristea@microchip.com/T/#u

Please, make sure you provide working links which should be this one if I am
not mistaken:

https://lore.kernel.org/r/20240927083543.80275-1-marius.cristea@microchip.com/T/#u

Also put lore site directly. I don't want to make my browser crawling from
whatever possibly fishing and malware sites.

What you provided is two non-working links.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] Adding support for Microchip PAC1711

[Jonathan Cameron]

On Wed, 15 Oct 2025 13:12:14 +0300
Ariana Lazar <ariana.lazar@microchip.com> wrote:

> The PAC1711 product is a single-channel power monitor with accumulator.
> The device uses 12-bit resolution for voltage and current measurements and
> 24 bits power calculations. The accumulator register (56-bit) could
> accumulate power (energy), current (Coulomb counter) or voltage.
> 
> PAC1711 measures up to 42V Full-Scale Range.

Hi Ariana,

For devices like this where the datasheet explicitly calls out usecases in
power monitoring e.g. for "Portable and Embedded Computing" (amongst other
things) there is always a question to answer wrt to whether the correct
place to support them in Linux is in hwmon or IIO. Note that, whilst this
has long been an informal policy I've become more strict on this after some
concerns were raised in the last cycle - the presence of similar devices
in IIO isn't necessarily a sign that was the right choice, but it is worth
looking at the history of those divers as it may provide more insight into
why they are in IIO.

To address that we ask that:
1) Drivers for this sort of potentially borderline device are +CC to hwmon
   list and maintainers
2) A justification for IIO making more sense is included. That can be
   based on what cannot be supported in hwmon (high speed capture being
   a typical item - that doesn't seem to apply here as it's only 200 sample/sec)

Anyhow, I've +CC relevant folk so if you can reply with that info here then
that would be great.

Thanks,

Jonathan

> 
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
> ---
> Ariana Lazar (2):
>       dt-bindings: iio: adc: adding support for PAC1711
>       iio: adc: adding support for PAC1711
> 
>  .../ABI/testing/sysfs-bus-iio-adc-pac1711          |   57 +
>  .../bindings/iio/adc/microchip,pac1711.yaml        |  195 +++
>  MAINTAINERS                                        |    8 +
>  drivers/iio/adc/Kconfig                            |   10 +
>  drivers/iio/adc/Makefile                           |    1 +
>  drivers/iio/adc/pac1711.c                          | 1448 ++++++++++++++++++++
>  6 files changed, 1719 insertions(+)
> ---
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> change-id: 20250901-pac1711-d3bacda400fd
> 
> Best regards,

Re: [PATCH 0/2] Adding support for Microchip PAC1711

[Guenter Roeck]

On 10/19/25 03:31, Jonathan Cameron wrote:
> On Wed, 15 Oct 2025 13:12:14 +0300
> Ariana Lazar <ariana.lazar@microchip.com> wrote:
> 
>> The PAC1711 product is a single-channel power monitor with accumulator.
>> The device uses 12-bit resolution for voltage and current measurements and
>> 24 bits power calculations. The accumulator register (56-bit) could
>> accumulate power (energy), current (Coulomb counter) or voltage.
>>
>> PAC1711 measures up to 42V Full-Scale Range.
> 
> Hi Ariana,
> 
> For devices like this where the datasheet explicitly calls out usecases in
> power monitoring e.g. for "Portable and Embedded Computing" (amongst other
> things) there is always a question to answer wrt to whether the correct
> place to support them in Linux is in hwmon or IIO. Note that, whilst this
> has long been an informal policy I've become more strict on this after some
> concerns were raised in the last cycle - the presence of similar devices
> in IIO isn't necessarily a sign that was the right choice, but it is worth
> looking at the history of those divers as it may provide more insight into
> why they are in IIO.
> 
> To address that we ask that:
> 1) Drivers for this sort of potentially borderline device are +CC to hwmon
>     list and maintainers
> 2) A justification for IIO making more sense is included. That can be
>     based on what cannot be supported in hwmon (high speed capture being
>     a typical item - that doesn't seem to apply here as it's only 200 sample/sec)
> 
> Anyhow, I've +CC relevant folk so if you can reply with that info here then
> that would be great.
> 
This should really be a hardware monitoring driver.

Guenter

Re: [PATCH 0/2] Adding support for Microchip PAC1711

[Ariana.Lazar]

Hi all,
  
Thank you for the feedback, I will make sure that for border devices I
will add HWMON list.
    
  I was thinking that the PAC1711 device is more suitable for the
IIO subsystem for the following reasons:
  - first but not the most important is to have the same API for the
whole family (with similar functionalities) of the devices from
Microchip;
  - second and the most important: We are looking at PAC1711 as a
"special" ADC, that has some special capabilities like hardware limits.
The PAC1711 could have a sample rate of up to 16384 samples/second if
we are sampling only the voltage or only the current.
      In case of sampling only the current we have some request to
be able to profile the current consumption of the device under test.

  I have started with a simple driver (this one that is more
appropriate to be a HWMON) and willing to add more functionality
later (like data buffering that is quite important for example if
someone wants to profile current consumption of the processor itself,
a peripheral device, a battery or an industrial low voltage
automation, this kind of functionality that was requested by our
customers).

Best regards,
Ariana Lazar



On Sun, 2025-10-19 at 08:02 -0700, Guenter Roeck wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On 10/19/25 03:31, Jonathan Cameron wrote:
> > On Wed, 15 Oct 2025 13:12:14 +0300
> > Ariana Lazar <ariana.lazar@microchip.com> wrote:
> > 
> > > The PAC1711 product is a single-channel power monitor with
> > > accumulator.
> > > The device uses 12-bit resolution for voltage and current
> > > measurements and
> > > 24 bits power calculations. The accumulator register (56-bit)
> > > could
> > > accumulate power (energy), current (Coulomb counter) or voltage.
> > > 
> > > PAC1711 measures up to 42V Full-Scale Range.
> > 
> > Hi Ariana,
> > 
> > For devices like this where the datasheet explicitly calls out
> > usecases in
> > power monitoring e.g. for "Portable and Embedded Computing"
> > (amongst other
> > things) there is always a question to answer wrt to whether the
> > correct
> > place to support them in Linux is in hwmon or IIO. Note that,
> > whilst this
> > has long been an informal policy I've become more strict on this
> > after some
> > concerns were raised in the last cycle - the presence of similar
> > devices
> > in IIO isn't necessarily a sign that was the right choice, but it
> > is worth
> > looking at the history of those divers as it may provide more
> > insight into
> > why they are in IIO.
> > 
> > To address that we ask that:
> > 1) Drivers for this sort of potentially borderline device are +CC
> > to hwmon
> >     list and maintainers
> > 2) A justification for IIO making more sense is included. That can
> > be
> >     based on what cannot be supported in hwmon (high speed capture
> > being
> >     a typical item - that doesn't seem to apply here as it's only
> > 200 sample/sec)
> > 
> > Anyhow, I've +CC relevant folk so if you can reply with that info
> > here then
> > that would be great.
> > 
> This should really be a hardware monitoring driver.
> 
> Guenter
> 

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

[kernel test robot]

:::::: 
:::::: Manual check reason: "low confidence static check warning: drivers/iio/adc/pac1711.c:1079:27: sparse: sparse: unsigned value that used to be signed checked against zero?"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20251015-pac1711-v1-2-976949e36367@microchip.com>
References: <20251015-pac1711-v1-2-976949e36367@microchip.com>
TO: Ariana Lazar <ariana.lazar@microchip.com>
TO: Jonathan Cameron <jic23@kernel.org>
TO: David Lechner <dlechner@baylibre.com>
TO: "Nuno Sá" <nuno.sa@analog.com>
TO: Andy Shevchenko <andy@kernel.org>
TO: Rob Herring <robh@kernel.org>
TO: Krzysztof Kozlowski <krzk@kernel.org>
TO: Conor Dooley <conor+dt@kernel.org>
CC: linux-iio@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Ariana Lazar <ariana.lazar@microchip.com>

Hi Ariana,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 19272b37aa4f83ca52bdf9c16d5d81bdd1354494]

url:    https://github.com/intel-lab-lkp/linux/commits/Ariana-Lazar/dt-bindings-iio-adc-adding-support-for-PAC1711/20251016-002337
base:   19272b37aa4f83ca52bdf9c16d5d81bdd1354494
patch link:    https://lore.kernel.org/r/20251015-pac1711-v1-2-976949e36367%40microchip.com
patch subject: [PATCH 2/2] iio: adc: adding support for PAC1711
:::::: branch date: 5 days ago
:::::: commit date: 5 days ago
config: csky-randconfig-r112-20251021 (https://download.01.org/0day-ci/archive/20251021/202510210413.JRfDM9jF-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 13.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251021/202510210413.JRfDM9jF-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202510210413.JRfDM9jF-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/adc/pac1711.c:1079:27: sparse: sparse: unsigned value that used to be signed checked against zero?
>> drivers/iio/adc/pac1711.c:1078:42: sparse: signed value source
   drivers/iio/adc/pac1711.c:1102:27: sparse: sparse: unsigned value that used to be signed checked against zero?
   drivers/iio/adc/pac1711.c:1101:42: sparse: signed value source

vim +1079 drivers/iio/adc/pac1711.c

de2a3cdfd6e76b Ariana Lazar 2025-10-15  1065  
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1066  static int pac1711_setup_vbus_range(struct pac1711_chip_info *info)
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1067  {
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1068  	const char *prop_name = "microchip,vbus-input-range-microvolt";
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1069  	struct i2c_client *client = info->client;
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1070  	struct device *dev = &client->dev;
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1071  	unsigned int tbl_len, ret;
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1072  	s32 vals[2];
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1073  
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1074  	/* default value is unipolar and Full Scale Range */
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1075  	ret = device_property_read_u32_array(dev, prop_name, vals, 2);
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1076  	if (!ret) {
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1077  		tbl_len = ARRAY_SIZE(pac1711_vbus_range_tbl);
de2a3cdfd6e76b Ariana Lazar 2025-10-15 @1078  		ret = pac1711_check_range(vals, pac1711_vbus_range_tbl, tbl_len);
de2a3cdfd6e76b Ariana Lazar 2025-10-15 @1079  		if (ret < 0)
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1080  			return dev_err_probe(dev, -EINVAL, "Invalid value %d, %d for prop %s\n",
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1081  					     vals[0], vals[1], prop_name);
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1082  
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1083  		info->vbus_mode = ret;
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1084  	}
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1085  
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1086  	return 0;
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1087  }
de2a3cdfd6e76b Ariana Lazar 2025-10-15  1088  

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

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

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20251015-pac1711-v1-2-976949e36367@microchip.com>
References: <20251015-pac1711-v1-2-976949e36367@microchip.com>
TO: Ariana Lazar <ariana.lazar@microchip.com>
TO: Jonathan Cameron <jic23@kernel.org>
TO: David Lechner <dlechner@baylibre.com>
TO: "Nuno Sá" <nuno.sa@analog.com>
TO: Andy Shevchenko <andy@kernel.org>
TO: Rob Herring <robh@kernel.org>
TO: Krzysztof Kozlowski <krzk@kernel.org>
TO: Conor Dooley <conor+dt@kernel.org>
CC: linux-iio@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Ariana Lazar <ariana.lazar@microchip.com>

Hi Ariana,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 19272b37aa4f83ca52bdf9c16d5d81bdd1354494]

url:    https://github.com/intel-lab-lkp/linux/commits/Ariana-Lazar/dt-bindings-iio-adc-adding-support-for-PAC1711/20251016-002337
base:   19272b37aa4f83ca52bdf9c16d5d81bdd1354494
patch link:    https://lore.kernel.org/r/20251015-pac1711-v1-2-976949e36367%40microchip.com
patch subject: [PATCH 2/2] iio: adc: adding support for PAC1711
:::::: branch date: 7 days ago
:::::: commit date: 7 days ago
config: loongarch-randconfig-r072-20251019 (https://download.01.org/0day-ci/archive/20251023/202510230847.CEW0lvJl-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 15.1.0

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 <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202510230847.CEW0lvJl-lkp@intel.com/

New smatch warnings:
drivers/iio/adc/pac1711.c:473 pac1711_retrieve_data() error: uninitialized symbol 'ret'.
drivers/iio/adc/pac1711.c:924 pac1711_read_raw() error: uninitialized symbol 'tmp'.
drivers/iio/adc/pac1711.c:1124 pac1711_of_parse_channel_config() error: uninitialized symbol 'ret'.

Old smatch warnings:
drivers/iio/adc/pac1711.c:1079 pac1711_setup_vbus_range() warn: unsigned 'ret' is never less than zero.
drivers/iio/adc/pac1711.c:1079 pac1711_setup_vbus_range() warn: error code type promoted to positive: 'ret'
drivers/iio/adc/pac1711.c:1102 pac1711_setup_vsense_range() warn: unsigned 'ret' is never less than zero.
drivers/iio/adc/pac1711.c:1102 pac1711_setup_vsense_range() warn: error code type promoted to positive: 'ret'
drivers/iio/adc/pac1711.c:1377 pac1711_probe() warn: passing positive error code '1' to 'dev_err_probe'

vim +/ret +473 drivers/iio/adc/pac1711.c

de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  449  
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  450  static int pac1711_retrieve_data(struct pac1711_chip_info *info,
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  451  				 u32 wait_time)
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  452  {
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  453  	int ret;
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  454  
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  455  	/*
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  456  	 * check if the minimal elapsed time has passed and if so,
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  457  	 * re-read the chip, otherwise the cached info is just fine
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  458  	 */
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  459  	if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  460  			msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS))) {
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  461  		ret = pac1711_reg_snapshot(info, true, PAC1711_REFRESH_REG_ADDR,
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  462  					   wait_time);
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  463  
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  464  		/*
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  465  		 * Re-schedule the work for the read registers timeout
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  466  		 * (to prevent chip regs saturation)
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  467  		 */
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  468  		cancel_delayed_work_sync(&info->work_chip_rfsh);
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  469  		schedule_delayed_work(&info->work_chip_rfsh,
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  470  				      msecs_to_jiffies(PAC1711_MAX_RFSH_LIMIT_MS));
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  471  	}
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  472  
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15 @473  	return ret;
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  474  }
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  475  

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

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

[Dan Carpenter]

Hi Ariana,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Ariana-Lazar/dt-bindings-iio-adc-adding-support-for-PAC1711/20251016-002337
base:   19272b37aa4f83ca52bdf9c16d5d81bdd1354494
patch link:    https://lore.kernel.org/r/20251015-pac1711-v1-2-976949e36367%40microchip.com
patch subject: [PATCH 2/2] iio: adc: adding support for PAC1711
config: loongarch-randconfig-r072-20251019 (https://download.01.org/0day-ci/archive/20251023/202510230847.CEW0lvJl-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 15.1.0

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 <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202510230847.CEW0lvJl-lkp@intel.com/

New smatch warnings:
drivers/iio/adc/pac1711.c:473 pac1711_retrieve_data() error: uninitialized symbol 'ret'.
drivers/iio/adc/pac1711.c:924 pac1711_read_raw() error: uninitialized symbol 'tmp'.
drivers/iio/adc/pac1711.c:1124 pac1711_of_parse_channel_config() error: uninitialized symbol 'ret'.

Old smatch warnings:
drivers/iio/adc/pac1711.c:1079 pac1711_setup_vbus_range() warn: unsigned 'ret' is never less than zero.
drivers/iio/adc/pac1711.c:1079 pac1711_setup_vbus_range() warn: error code type promoted to positive: 'ret'
drivers/iio/adc/pac1711.c:1102 pac1711_setup_vsense_range() warn: unsigned 'ret' is never less than zero.
drivers/iio/adc/pac1711.c:1102 pac1711_setup_vsense_range() warn: error code type promoted to positive: 'ret'
drivers/iio/adc/pac1711.c:1377 pac1711_probe() warn: passing positive error code '1' to 'dev_err_probe'

vim +/ret +473 drivers/iio/adc/pac1711.c

de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  450  static int pac1711_retrieve_data(struct pac1711_chip_info *info,
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  451  				 u32 wait_time)
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  452  {
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  453  	int ret;
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  454  
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  455  	/*
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  456  	 * check if the minimal elapsed time has passed and if so,
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  457  	 * re-read the chip, otherwise the cached info is just fine
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  458  	 */
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  459  	if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  460  			msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS))) {
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  461  		ret = pac1711_reg_snapshot(info, true, PAC1711_REFRESH_REG_ADDR,
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  462  					   wait_time);
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  463  
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  464  		/*
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  465  		 * Re-schedule the work for the read registers timeout
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  466  		 * (to prevent chip regs saturation)
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  467  		 */
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  468  		cancel_delayed_work_sync(&info->work_chip_rfsh);
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  469  		schedule_delayed_work(&info->work_chip_rfsh,
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  470  				      msecs_to_jiffies(PAC1711_MAX_RFSH_LIMIT_MS));
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  471  	}

ret isn't initialized on the else path.

de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  472  
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15 @473  	return ret;
de2a3cdfd6e76b2 Ariana Lazar 2025-10-15  474  }

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