[PATCH 0/2] iio: adc: add support for pac1921

[PATCH 0/2] iio: adc: add support for pac1921

[Matteo Martelli]

Add iio driver and DT binding for the Microchip PAC1921 Current/Power
monitor.

Implemented most of the features with few limitations listed in the
driver commit message.

Tested with a Pine64 host board connected to a PAC1921 click board [1]
via I2C. The PAC1921 click board embeds the Microchip PAC1921 device
and a 10 mOhms shunt resistor. The PAC1921 datasheet is at [2].

[1]: https://www.mikroe.com/pac1921-click
[2]: https://ww1.microchip.com/downloads/en/DeviceDoc/PAC1921-Data-Sheet-DS20005293E.pdf

Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
Matteo Martelli (2):
      dt-bindings: iio: adc: add binding for pac1921
      iio: adc: add support for pac1921

 .../ABI/testing/sysfs-bus-iio-adc-pac1921          |   45 +
 .../bindings/iio/adc/microchip,pac1921.yaml        |   79 ++
 MAINTAINERS                                        |    7 +
 drivers/iio/adc/Kconfig                            |   10 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/pac1921.c                          | 1033 ++++++++++++++++++++
 6 files changed, 1175 insertions(+)
---
base-commit: 529d2e1900642eba6df28307e26e19793e227546
change-id: 20240606-iio-pac1921-77fa0fa3ac11

Best regards,
-- 
Matteo Martelli <matteomartelli3@gmail.com>

[PATCH 1/2] dt-bindings: iio: adc: add binding for pac1921

[Matteo Martelli]

Add binging for Microchip PAC1921 Power/Current monitor

Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
 .../bindings/iio/adc/microchip,pac1921.yaml        | 79 ++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml
new file mode 100644
index 000000000000..ec08228dcc16
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/microchip,pac1921.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PAC1921 High-Side Power/Current Monitor with Anaog Output
+
+maintainers:
+  - Matteo Martelli <matteomartelli3@gmail.com>
+
+description: |
+  The PAC1921 is a power/current monitoring device with an analog output
+  and I2C/SMBus interface.
+
+  Datasheet can be found here:
+  https://ww1.microchip.com/downloads/en/DeviceDoc/PAC1921-Data-Sheet-DS20005293E.pdf
+
+properties:
+  compatible:
+    const: microchip,pac1921
+
+  reg:
+    maxItems: 1
+
+  "#io-channel-cells":
+    const: 1
+
+  shunt-resistor-micro-ohms:
+    description:
+      Value in micro Ohms of the shunt resistor connected between
+      the SENSE+ and SENSE- inputs, across which the current is measured.
+      Value is needed to compute the scaling of the measured current.
+
+  label:
+    description: Unique name to identify which device this is.
+
+  microchip,dv-gain:
+    description:
+      Digital multiplier to control the effective bus voltage gain. The gain
+      value of 1 is the setting for the full-scale range and it can be increased
+      when the system is designed for a lower VBUS range.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 8, 16, 32]
+    default: 1
+
+  microchip,di-gain:
+    description:
+      Digital multiplier to control the effective current gain. The gain
+      value of 1 is the setting for the full-scale range and it can be
+      increased when the system is designed for a lower VSENSE range.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 8, 16, 32, 64, 128]
+    default: 1
+
+required:
+  - compatible
+  - reg
+  - shunt-resistor-micro-ohms
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@4c {
+            compatible = "microchip,pac1921";
+            #io-channel-cells = <1>;
+            label = "vbat";
+            reg = <0x4c>;
+            shunt-resistor-micro-ohms = <10000>;
+            microchip,dv-gain = <4>;
+            microchip,di-gain = <32>;
+        };
+    };
+...

-- 
2.45.1

Re: [PATCH 1/2] dt-bindings: iio: adc: add binding for pac1921

[Conor Dooley]

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

Hey,

On Wed, Jul 03, 2024 at 03:34:35PM +0200, Matteo Martelli wrote:
> Add binging for Microchip PAC1921 Power/Current monitor
> 
> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> ---
>  .../bindings/iio/adc/microchip,pac1921.yaml        | 79 ++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml
> new file mode 100644
> index 000000000000..ec08228dcc16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1921.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PAC1921 High-Side Power/Current Monitor with Anaog Output
> +
> +maintainers:
> +  - Matteo Martelli <matteomartelli3@gmail.com>
> +
> +description: |
> +  The PAC1921 is a power/current monitoring device with an analog output
> +  and I2C/SMBus interface.
> +
> +  Datasheet can be found here:
> +  https://ww1.microchip.com/downloads/en/DeviceDoc/PAC1921-Data-Sheet-DS20005293E.pdf
> +
> +properties:
> +  compatible:
> +    const: microchip,pac1921
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  shunt-resistor-micro-ohms:
> +    description:
> +      Value in micro Ohms of the shunt resistor connected between
> +      the SENSE+ and SENSE- inputs, across which the current is measured.
> +      Value is needed to compute the scaling of the measured current.
> +
> +  label:
> +    description: Unique name to identify which device this is.
> +
> +  microchip,dv-gain:
> +    description:
> +      Digital multiplier to control the effective bus voltage gain. The gain
> +      value of 1 is the setting for the full-scale range and it can be increased
> +      when the system is designed for a lower VBUS range.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 8, 16, 32]
> +    default: 1
> +
> +  microchip,di-gain:

Why is this gain a fixed property in the devicetree, rather than
something the user can control? Feels like it should be user
controllable.

> +    description:
> +      Digital multiplier to control the effective current gain. The gain
> +      value of 1 is the setting for the full-scale range and it can be
> +      increased when the system is designed for a lower VSENSE range.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 8, 16, 32, 64, 128]
> +    default: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - shunt-resistor-micro-ohms

You're missing a vdd-supply btw and the !read/int pin isn't described
here either. I think the latter needs a property to control it (probably
a GPIO since it is intended for host control) and a default value for if
the GPIO isn't provided?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@4c {
> +            compatible = "microchip,pac1921";
> +            #io-channel-cells = <1>;
> +            label = "vbat";
> +            reg = <0x4c>;

Order here should be compatible, reg, generic properties and then finally
vendor ones.

Thanks for your patch,
Conor.

> +            shunt-resistor-micro-ohms = <10000>;
> +            microchip,dv-gain = <4>;
> +            microchip,di-gain = <32>;
> +        };
> +    };
> +...
> 
> -- 
> 2.45.1
> 

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

Re: [PATCH 1/2] dt-bindings: iio: adc: add binding for pac1921

[Matteo Martelli]

Hi Conor, thanks for your feedback,

Conor Dooley wrote:
> > +
> > +  microchip,dv-gain:
> > +    description:
> > +      Digital multiplier to control the effective bus voltage gain. The gain
> > +      value of 1 is the setting for the full-scale range and it can be increased
> > +      when the system is designed for a lower VBUS range.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 2, 4, 8, 16, 32]
> > +    default: 1
> > +
> > +  microchip,di-gain:
> 
> Why is this gain a fixed property in the devicetree, rather than
> something the user can control? Feels like it should be user
> controllable.

Gains are user controllable via the IIO_CHAN_INFO_HARDWAREGAIN. I also added
them as DT properties thinking that they could be pre-set depending on hardware
specifications: for instance by board design the monitored section is already
known to be in a particular voltage/current range (datasheet specifies
gains-ranges mapping at table 4-6 and table 4-7). Then, even if gains are
pre-set, the user can change them at runtime for instance by scaling them down
upon an overflow event. However, I can get rid of those gain properties if they
are out of the DT scope.

> > +    description:
> > +      Digital multiplier to control the effective current gain. The gain
> > +      value of 1 is the setting for the full-scale range and it can be
> > +      increased when the system is designed for a lower VSENSE range.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 2, 4, 8, 16, 32, 64, 128]
> > +    default: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - shunt-resistor-micro-ohms
> 
> You're missing a vdd-supply btw and the !read/int pin isn't described
> here either. I think the latter needs a property to control it (probably
> a GPIO since it is intended for host control) and a default value for if
> the GPIO isn't provided?

The driver does not currently handle the vdd regulator nor the gpio for the
!read/int pin. Should they be added to the DT schema anyway?

I think I can add the vdd regulator handling with little effort, my guess is
that the "vdd-supply" property can be optional and defined as "vdd-supply: true"
in the DT schema. Then the driver, if the vdd-supply property is present in the
DT, would enable the regulator during device initialization and PM resume, and
disable it on driver removal and PM suspend.

Reguarding the !read/int pin, the current driver overrides it with a register
bit so it would not be considered at all by the device.

> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@4c {
> > +            compatible = "microchip,pac1921";
> > +            #io-channel-cells = <1>;
> > +            label = "vbat";
> > +            reg = <0x4c>;
> 
> Order here should be compatible, reg, generic properties and then finally
> vendor ones.

Thanks, I will fix this in next patch version.

> 
> Thanks for your patch,
> Conor.
> 

Thanks again for you feedback,
Matteo

Re: [PATCH 1/2] dt-bindings: iio: adc: add binding for pac1921

[Conor Dooley]

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

On Thu, Jul 04, 2024 at 12:06:31PM +0200, Matteo Martelli wrote:
> Conor Dooley wrote:
> > > +
> > > +  microchip,dv-gain:
> > > +    description:
> > > +      Digital multiplier to control the effective bus voltage gain. The gain
> > > +      value of 1 is the setting for the full-scale range and it can be increased
> > > +      when the system is designed for a lower VBUS range.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [1, 2, 4, 8, 16, 32]
> > > +    default: 1
> > > +
> > > +  microchip,di-gain:
> > 
> > Why is this gain a fixed property in the devicetree, rather than
> > something the user can control? Feels like it should be user
> > controllable.
> 
> Gains are user controllable via the IIO_CHAN_INFO_HARDWAREGAIN. I also added
> them as DT properties thinking that they could be pre-set depending on hardware
> specifications: for instance by board design the monitored section is already
> known to be in a particular voltage/current range (datasheet specifies
> gains-ranges mapping at table 4-6 and table 4-7). Then, even if gains are
> pre-set, the user can change them at runtime for instance by scaling them down
> upon an overflow event. However, I can get rid of those gain properties if they
> are out of the DT scope.

Usually gain values are left out of DT entirely, unless the gain is
something set by the board, for example, whether or not some input pins
are tied high or low.

> > > +    description:
> > > +      Digital multiplier to control the effective current gain. The gain
> > > +      value of 1 is the setting for the full-scale range and it can be
> > > +      increased when the system is designed for a lower VSENSE range.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [1, 2, 4, 8, 16, 32, 64, 128]
> > > +    default: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - shunt-resistor-micro-ohms
> > 
> > You're missing a vdd-supply btw and the !read/int pin isn't described
> > here either. I think the latter needs a property to control it (probably
> > a GPIO since it is intended for host control) and a default value for if
> > the GPIO isn't provided?
> 
> The driver does not currently handle the vdd regulator nor the gpio for the
> !read/int pin. Should they be added to the DT schema anyway?

Yes.

> I think I can add the vdd regulator handling with little effort, my guess is
> that the "vdd-supply" property can be optional and defined as "vdd-supply: true"
> in the DT schema. Then the driver, if the vdd-supply property is present in the
> DT, would enable the regulator during device initialization and PM resume, and
> disable it on driver removal and PM suspend.

Nah, the regulator should be marked required in the binding, since
without power the device cannot function, right? The regulator core will
create a dummy register if one is not provided in the device tree, so
you don't need to add any conditional logic around regulator actions.

> Reguarding the !read/int pin, the current driver overrides it with a register
> bit so it would not be considered at all by the device.

We should fully describe devices, where possible, even if the driver for
the device doesn't use it.

Cheers,
Conor.


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

Re: [PATCH 1/2] dt-bindings: iio: adc: add binding for pac1921

[Matteo Martelli]

Conor Dooley wrote:
> On Thu, Jul 04, 2024 at 12:06:31PM +0200, Matteo Martelli wrote:
> > Conor Dooley wrote:
> > > > +
> > > > +  microchip,dv-gain:
> > > > +    description:
> > > > +      Digital multiplier to control the effective bus voltage gain. The gain
> > > > +      value of 1 is the setting for the full-scale range and it can be increased
> > > > +      when the system is designed for a lower VBUS range.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [1, 2, 4, 8, 16, 32]
> > > > +    default: 1
> > > > +
> > > > +  microchip,di-gain:
> > > 
> > > Why is this gain a fixed property in the devicetree, rather than
> > > something the user can control? Feels like it should be user
> > > controllable.
> > 
> > Gains are user controllable via the IIO_CHAN_INFO_HARDWAREGAIN. I also added
> > them as DT properties thinking that they could be pre-set depending on hardware
> > specifications: for instance by board design the monitored section is already
> > known to be in a particular voltage/current range (datasheet specifies
> > gains-ranges mapping at table 4-6 and table 4-7). Then, even if gains are
> > pre-set, the user can change them at runtime for instance by scaling them down
> > upon an overflow event. However, I can get rid of those gain properties if they
> > are out of the DT scope.
> 
> Usually gain values are left out of DT entirely, unless the gain is
> something set by the board, for example, whether or not some input pins
> are tied high or low.
> 
> > > > +    description:
> > > > +      Digital multiplier to control the effective current gain. The gain
> > > > +      value of 1 is the setting for the full-scale range and it can be
> > > > +      increased when the system is designed for a lower VSENSE range.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [1, 2, 4, 8, 16, 32, 64, 128]
> > > > +    default: 1
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - shunt-resistor-micro-ohms
> > > 
> > > You're missing a vdd-supply btw and the !read/int pin isn't described
> > > here either. I think the latter needs a property to control it (probably
> > > a GPIO since it is intended for host control) and a default value for if
> > > the GPIO isn't provided?
> > 
> > The driver does not currently handle the vdd regulator nor the gpio for the
> > !read/int pin. Should they be added to the DT schema anyway?
> 
> Yes.
> 
> > I think I can add the vdd regulator handling with little effort, my guess is
> > that the "vdd-supply" property can be optional and defined as "vdd-supply: true"
> > in the DT schema. Then the driver, if the vdd-supply property is present in the
> > DT, would enable the regulator during device initialization and PM resume, and
> > disable it on driver removal and PM suspend.
> 
> Nah, the regulator should be marked required in the binding, since
> without power the device cannot function, right? The regulator core will
> create a dummy register if one is not provided in the device tree, so
> you don't need to add any conditional logic around regulator actions.
> 
> > Reguarding the !read/int pin, the current driver overrides it with a register
> > bit so it would not be considered at all by the device.
> 
> We should fully describe devices, where possible, even if the driver for
> the device doesn't use it.
> 
> Cheers,
> Conor.
> 

Thanks Conor, I sent a patch v2 addressing these points.

Link to v2: https://lore.kernel.org/linux-iio/20240704-iio-pac1921-v2-0-0deb95a48409@gmail.com

Matteo

[PATCH 2/2] iio: adc: add support for pac1921

[Matteo Martelli]

Add support for Microchip PAC1921 Power/Current monitor.

Implemented features:
* capture of bus voltage, sense voltage, current and power measurements
  in free-run integration mode
* support for both raw and triggered buffer reading
* support for overflow events
* userspace controls for voltage and current gains, measurement
  resolution, integration samples and filters enabling/disabling
* simple power management support

Limitations:
* operation mode fixed to free-run integration
* measurement resolution and filters controls are applied to both VSENSE
  and VBUS measurements

Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
 .../ABI/testing/sysfs-bus-iio-adc-pac1921          |   45 +
 MAINTAINERS                                        |    7 +
 drivers/iio/adc/Kconfig                            |   10 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/pac1921.c                          | 1033 ++++++++++++++++++++
 5 files changed, 1096 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921
new file mode 100644
index 000000000000..4a32e2d4207b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1921
@@ -0,0 +1,45 @@
+What:		/sys/bus/iio/devices/iio:deviceX/resolution_bits
+KernelVersion:	6.10
+Contact:	linux-iio@vger.kernel.org
+Description:
+		ADC measurement resolution. Can be either 11 bits or 14 bits
+		(default). The driver sets the same resolution for both VBUS and
+		VSENSE measurements even if the hardware could be configured to
+		measure VBUS and VSENSE with different resolutions.
+		This attribute affects the integration time: with 14 bits
+		resolution the integration time is increased by a factor of
+		1.9 (the driver considers a factor of 2). See Table 4-5 in
+		device datasheet for details.
+
+What:		/sys/bus/iio/devices/iio:deviceX/resolution_bits_available
+KernelVersion:	6.10
+Contact:	linux-iio@vger.kernel.org
+Description:
+		List all possible ADC measurement resolutions: "11 14"
+
+What:		/sys/bus/iio/devices/iio:deviceX/integration_samples
+KernelVersion:	6.10
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Number of samples taken during a full integration period. Can be
+		set to any power of 2 value from 1 (default) to 2048.
+		This attribute affects the integration time: higher the number
+		of samples, longer the integration time. See Table 4-5 in device
+		datasheet for details.
+
+What:		/sys/bus/iio/devices/iio:deviceX/integration_samples_available
+KernelVersion:	6.10
+Contact:	linux-iio@vger.kernel.org
+Description:
+		List all possible numbers of integration samples:
+		"1 2 4 8 16 32 64 128 256 512 1024 2048"
+
+What:		/sys/bus/iio/devices/iio:devices/filters_en
+KernelVersion:	6.10
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Attribute to enable/disable ADC post filters. Enabled by
+		default.
+		This attribute affects the integration time: with filters
+		enabled the integration time is increased by 50%. See Table 4-5
+		in device datasheet for details.
diff --git a/MAINTAINERS b/MAINTAINERS
index 0cbeb03847f5..a737dc00e29c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14793,6 +14793,13 @@ 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 PAC1921 POWER/CURRENT MONITOR DRIVER
+M:	Matteo Martelli <matteomartelli3@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml
+F:	drivers/iio/adc/pac1921.c
+
 MICROCHIP PAC1934 POWER/ENERGY MONITOR DRIVER
 M:	Marius Cristea <marius.cristea@microchip.com>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f60fe85a30d5..b56e494da970 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -991,6 +991,16 @@ config NPCM_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called npcm_adc.
 
+config PAC1921
+	tristate "Microchip Technology PAC1921 driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Microchip Technology's PAC1921
+	  High-Side Power/Current Monitor with Analog Output.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called pac1921.
+
 config PAC1934
 	tristate "Microchip Technology PAC1934 driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d370e066544e..5af30eeff262 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_MP2629_ADC) += mp2629_adc.o
 obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o
 obj-$(CONFIG_NAU7802) += nau7802.o
 obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
+obj-$(CONFIG_PAC1921) += pac1921.o
 obj-$(CONFIG_PAC1934) += pac1934.o
 obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
 obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
new file mode 100644
index 000000000000..2d11dcfe0e03
--- /dev/null
+++ b/drivers/iio/adc/pac1921.c
@@ -0,0 +1,1033 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IIO driver for PAC1921 High-Side Power/Current Monitor
+ *
+ * Copyright (C) 2024 Matteo Martelli <matteomartelli3@gmail.com>
+ */
+
+#include <linux/i2c.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/regmap.h>
+
+/* pac1921 registers */
+#define PAC1921_REG_GAIN_CFG		0x00
+#define PAC1921_REG_INT_CFG		0x01
+#define PAC1921_REG_CONTROL		0x02
+#define PAC1921_REG_VBUS		0x10
+#define PAC1921_REG_VSENSE		0x12
+#define PAC1921_REG_OVERFLOW_STS	0x1C
+#define PAC1921_REG_VPOWER		0x1D
+
+/* pac1921 gain configuration bits */
+#define PAC1921_GAIN_I_RES		BIT(7)
+#define PAC1921_GAIN_V_RES		BIT(6)
+#define PAC1921_GAIN_DI_GAIN_SHIFT	3
+#define PAC1921_GAIN_DI_GAIN_MASK	GENMASK(5, PAC1921_GAIN_DI_GAIN_SHIFT)
+#define PAC1921_GAIN_DI_GAIN_MAX	7
+#define PAC1921_GAIN_DV_GAIN_SHIFT	0
+#define PAC1921_GAIN_DV_GAIN_MASK	GENMASK(2, PAC1921_GAIN_DV_GAIN_SHIFT)
+#define PAC1921_GAIN_DV_GAIN_MAX	5
+
+/* pac1921 integration configuration bits */
+#define PAC1921_INT_CFG_SMPL_SHIFT	4
+#define PAC1921_INT_CFG_SMPL_MASK	GENMASK(7, PAC1921_INT_CFG_SMPL_SHIFT)
+#define PAC1921_INT_CFG_SMPL_MAX	11
+#define PAC1921_INT_CFG_VSFEN		BIT(3)
+#define PAC1921_INT_CFG_VBFEN		BIT(2)
+#define PAC1921_INT_CFG_RIOV		BIT(1)
+#define PAC1921_INT_CFG_INTEN		BIT(0)
+
+/* pac1921 control bits */
+#define PAC1921_CONTROL_MXSL_SHIFT	6
+enum pac1921_mxsl {
+	PAC1921_MXSL_VPOWER_PIN = 0,
+	PAC1921_MXSL_VSENSE_FREE_RUN = 1,
+	PAC1921_MXSL_VBUS_FREE_RUN = 2,
+	PAC1921_MXSL_VPOWER_FREE_RUN = 3,
+};
+#define PAC1921_CONTROL_SLEEP		BIT(2)
+
+/* pac1921 overflow status bits */
+#define PAC1921_OVERFLOW_VSOV		BIT(2)
+#define PAC1921_OVERFLOW_VBOV		BIT(1)
+#define PAC1921_OVERFLOW_VPOV		BIT(0)
+
+/* pac1921 constants */
+#define PAC1921_MAX_VSENSE_MV		100
+#define PAC1921_MAX_VBUS_V		32
+#define PAC1921_RES_RESOLUTION		1023 /* Result registers resolution */
+
+/* pac1921 defaults */
+#define PAC1921_DEFAULT_DV_GAIN		0 /* 2^(value): 1x gain */
+#define PAC1921_DEFAULT_DI_GAIN		0 /* 2^(value): 1x gain */
+#define PAC1921_DEFAULT_HIGH_RES	true /* 14-bit measurement resolution */
+#define PAC1921_DEFAULT_NUM_SAMPLES	0 /* 2^(value): 1 sample */
+#define PAC1921_DEFAULT_FILTERS_ENABLED true
+
+/* pac1921 tables to create iio available parameters */
+static const unsigned int pac1921_di_gains[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
+static const unsigned int pac1921_dv_gains[] = { 1, 2, 4, 8, 16, 32 };
+enum pac1921_meas_resolution_idx {
+	PAC1921_MEAS_RESOLUTION_IDX_LOW = 0,
+	PAC1921_MEAS_RESOLUTION_IDX_HIGH,
+};
+static const char *const pac1921_meas_resolutions[] = { "11", "14" };
+static const char *const pac1921_integr_num_samples[] = {
+	"1",  "2",   "4",   "8",   "16",   "32",
+	"64", "128", "256", "512", "1024", "2048"
+};
+
+/* pac1921 regmap configuration */
+static const struct regmap_range pac1921_regmap_wr_ranges[] = {
+	regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
+};
+static const struct regmap_access_table pac1921_regmap_wr_table = {
+	.yes_ranges = pac1921_regmap_wr_ranges,
+	.n_yes_ranges = ARRAY_SIZE(pac1921_regmap_wr_ranges),
+};
+static const struct regmap_range pac1921_regmap_rd_ranges[] = {
+	regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
+	regmap_reg_range(PAC1921_REG_VBUS, PAC1921_REG_VPOWER + 1),
+};
+static const struct regmap_access_table pac1921_regmap_rd_table = {
+	.yes_ranges = pac1921_regmap_rd_ranges,
+	.n_yes_ranges = ARRAY_SIZE(pac1921_regmap_rd_ranges),
+};
+static const struct regmap_config pac1921_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.rd_table = &pac1921_regmap_rd_table,
+	.wr_table = &pac1921_regmap_wr_table,
+};
+
+enum pac1921_channels {
+	PAC1921_CHAN_VBUS = 0,
+	PAC1921_CHAN_VSENSE = 1,
+	PAC1921_CHAN_CURRENT = 2,
+	PAC1921_CHAN_POWER = 3,
+};
+#define PAC1921_NUM_MEAS_CHANS 4
+
+struct pac1921_priv {
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct iio_info iio_info;
+
+	/* Synchronize access to private members, and ensure atomicity of
+	 * consecutive regmap operations.
+	 */
+	struct mutex lock;
+
+	u32 rshunt; /* uOhm */
+	bool high_res;
+	u8 dv_gain;
+	u8 di_gain;
+	u8 n_samples;
+	bool filters_en;
+	u8 prev_ovf_flags;
+
+	bool first_integr_started;
+	bool first_integr_done;
+	unsigned long integr_start_time; /* jiffies */
+	unsigned int integr_period; /* usecs */
+
+	struct {
+		u16 chan[PAC1921_NUM_MEAS_CHANS];
+		s64 timestamp __aligned(8);
+	} scan;
+};
+
+/* The integration period depends on the configuration of number of integration
+ * samples, measurement resolution and post filters. The following array
+ * contains integration periods, in microsecs unit, based on table 4-5 from
+ * datasheet considering power integration mode, 11-Bit resolution and post
+ * filters off. Each index corresponds to a specific number of samples from 1
+ * to 2048.
+ */
+static const unsigned int pac1921_integr_periods_us[] = {
+	930,   1460,  2410,   4320,   8050,   16100,
+	32100, 64200, 128300, 257000, 513000, 1026000
+};
+
+/* Calculate and update the integration period
+ *
+ * The base integration period is retrieved from pac1921_integr_periods_us
+ * array, then doubled if 14-Bit resolution is used (even if the actual
+ * increase factor is about 1.9) and increased by 50% if post filters are
+ * enabled.
+ *
+ * Must be called with lock held.
+ */
+static int pac1921_update_integr_period(struct pac1921_priv *priv)
+{
+	if (WARN_ON_ONCE(priv->n_samples >=
+			 ARRAY_SIZE(pac1921_integr_periods_us)))
+		return -EINVAL;
+
+	priv->integr_period = pac1921_integr_periods_us[priv->n_samples];
+	if (priv->high_res)
+		priv->integr_period *= 2;
+	if (priv->filters_en)
+		priv->integr_period += priv->integr_period / 2;
+
+	return 0;
+}
+
+/* Check if first integration after configuration update has completed.
+ *
+ * Must be called with lock held.
+ */
+static bool pac1921_data_ready(struct pac1921_priv *priv)
+{
+	if (!priv->first_integr_started)
+		return false;
+
+	if (!priv->first_integr_done) {
+		unsigned long t_ready = priv->integr_start_time +
+					usecs_to_jiffies(priv->integr_period);
+		if (time_before(jiffies, t_ready))
+			/* First integration not completed: data not ready */
+			return false;
+
+		priv->first_integr_done = true;
+	}
+
+	return true;
+}
+
+/* Check if overflow occurred and if so, push the corresponding events.
+ *
+ * Must be called with lock held.
+ */
+static int pac1921_check_push_overflow(struct iio_dev *indio_dev, s64 timestamp)
+{
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+	unsigned int flags;
+
+	int ret = regmap_read(priv->regmap, PAC1921_REG_OVERFLOW_STS, &flags);
+	if (ret)
+		return ret;
+
+	if (flags & PAC1921_OVERFLOW_VBOV &&
+	    !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VBOV)) {
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(
+				       IIO_VOLTAGE, PAC1921_CHAN_VBUS,
+				       IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING),
+			       timestamp);
+	}
+	if (flags & PAC1921_OVERFLOW_VSOV &&
+	    !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VSOV)) {
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(
+				       IIO_VOLTAGE, PAC1921_CHAN_VSENSE,
+				       IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING),
+			       timestamp);
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(
+				       IIO_CURRENT, PAC1921_CHAN_CURRENT,
+				       IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING),
+			       timestamp);
+	}
+	if (flags & PAC1921_OVERFLOW_VPOV &&
+	    !(priv->prev_ovf_flags & PAC1921_OVERFLOW_VPOV)) {
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(
+				       IIO_POWER, PAC1921_CHAN_POWER,
+				       IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING),
+			       timestamp);
+	}
+
+	priv->prev_ovf_flags = (u8)flags;
+
+	return 0;
+}
+
+/* Read the value from a result register
+ *
+ * Result registers contain the most recent averaged values of Vbus, Vsense and
+ * Vpower. Each value is 10 bits wide and spread across two consecutive 8 bit
+ * registers, with 6 bit LSB zero padding.
+ */
+static int pac1921_read_res(struct pac1921_priv *priv, unsigned long reg,
+			    int *val)
+{
+	u8 val_buf[2];
+
+	int ret = regmap_bulk_read(priv->regmap, (unsigned int)reg, &val_buf,
+				   sizeof(val_buf));
+	if (ret)
+		return ret;
+
+	*val = (val_buf[0] << 8 | val_buf[1]) >> 6;
+
+	return 0;
+}
+
+/* Get the Vsense LSB in nV
+ *
+ * Used to calculate scale factors for current and power measurements
+ */
+static inline int pac1921_vsense_lsb(u8 di_gain)
+{
+	int max = (PAC1921_MAX_VSENSE_MV * 1000000) >> (int)di_gain;
+
+	return DIV_ROUND_CLOSEST(max, PAC1921_RES_RESOLUTION);
+}
+
+static int pac1921_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		guard(mutex)(&priv->lock);
+
+		if (!pac1921_data_ready(priv))
+			return -EBUSY;
+
+		s64 ts = iio_get_time_ns(indio_dev);
+
+		int ret = pac1921_check_push_overflow(indio_dev, ts);
+		if (ret)
+			return ret;
+
+		ret = pac1921_read_res(priv, chan->address, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->channel) {
+		case PAC1921_CHAN_VBUS: {
+			/* Vbus scale factor in mV:
+			 * max_vbus (mV) / vgain / resolution
+			 */
+			guard(mutex)(&priv->lock);
+
+			*val = PAC1921_MAX_VBUS_V * 1000;
+			*val2 = PAC1921_RES_RESOLUTION << (int)priv->dv_gain;
+
+			return IIO_VAL_FRACTIONAL;
+		}
+		case PAC1921_CHAN_VSENSE: {
+			/* Vsense voltage scale factor in mV:
+			 * max_vsense (mV) / igain / resolution
+			 */
+			guard(mutex)(&priv->lock);
+
+			*val = PAC1921_MAX_VSENSE_MV;
+			*val2 = PAC1921_RES_RESOLUTION << (int)priv->di_gain;
+
+			return IIO_VAL_FRACTIONAL;
+		}
+		case PAC1921_CHAN_CURRENT: {
+			/* Current scale factor in mA:
+			 * Vsense LSB (nV) / shunt (uOhm)
+			 */
+			guard(mutex)(&priv->lock);
+
+			*val = pac1921_vsense_lsb(priv->di_gain);
+			*val2 = (int)priv->rshunt;
+
+			return IIO_VAL_FRACTIONAL;
+		}
+		case PAC1921_CHAN_POWER: {
+			/* Power scale factor in mW:
+			 * Vsense LSB (nV) * max_vbus (V) / vgain / shunt (uOhm)
+			 */
+			guard(mutex)(&priv->lock);
+
+			*val = pac1921_vsense_lsb(priv->di_gain) *
+			       (PAC1921_MAX_VBUS_V >> (int)priv->dv_gain);
+			*val2 = (int)priv->rshunt;
+
+			return IIO_VAL_FRACTIONAL;
+		}
+		default:
+			return -EINVAL;
+		}
+		break;
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		switch (chan->channel) {
+		case PAC1921_CHAN_VBUS: {
+			guard(mutex)(&priv->lock);
+			*val = 1 << (int)priv->dv_gain;
+			return IIO_VAL_INT;
+		}
+		case PAC1921_CHAN_VSENSE:
+		case PAC1921_CHAN_CURRENT: {
+			guard(mutex)(&priv->lock);
+			*val = 1 << (int)priv->di_gain;
+			return IIO_VAL_INT;
+		}
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_INT_TIME: {
+		/* Integration time is read only: it depends on the number of
+		 * integration samples, measurement resolution and post filters
+		 */
+		*val2 = 1000000; /* From usecs to fractional secs */
+		guard(mutex)(&priv->lock);
+		*val = (int)priv->integr_period;
+		return IIO_VAL_FRACTIONAL;
+	}
+	default:
+		return -EINVAL;
+	}
+}
+
+/* Perform configuration update sequence: set the device into read state, then
+ * write the config register and set the device back into integration state.
+ * Also reset integration start time and mark first integration to be yet
+ * completed.
+ *
+ * Must be called with lock held.
+ */
+static int pac1921_update_cfg_reg(struct pac1921_priv *priv, unsigned int reg,
+				  unsigned int mask, unsigned int val)
+{
+	/* Enter READ state before configuration */
+	int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
+				     PAC1921_INT_CFG_INTEN, 0);
+	if (ret)
+		return ret;
+
+	/* Update configuration value */
+	ret = regmap_update_bits(priv->regmap, reg, mask, val);
+	if (ret)
+		return ret;
+
+	/* Re-enable integration and reset start time */
+	ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
+				 PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN);
+	if (ret)
+		return ret;
+
+	priv->integr_start_time = jiffies;
+	priv->first_integr_done = false;
+
+	return 0;
+}
+
+static inline bool pac1921_gain_param_valid(unsigned int gain, unsigned int max)
+{
+	return (gain > 0 && gain <= (1U << max) && is_power_of_2(gain));
+}
+
+static int pac1921_update_gain(struct pac1921_priv *priv,
+			       struct iio_chan_spec const *chan, int val)
+{
+	unsigned int max;
+	unsigned int mask;
+	unsigned int shift;
+	u8 *priv_val;
+
+	switch (chan->channel) {
+	case PAC1921_CHAN_VBUS:
+		max = PAC1921_GAIN_DV_GAIN_MAX;
+		mask = PAC1921_GAIN_DV_GAIN_MASK;
+		shift = PAC1921_GAIN_DV_GAIN_SHIFT;
+		priv_val = &priv->dv_gain;
+		break;
+	case PAC1921_CHAN_VSENSE:
+	case PAC1921_CHAN_CURRENT:
+		max = PAC1921_GAIN_DI_GAIN_MAX;
+		mask = PAC1921_GAIN_DI_GAIN_MASK;
+		shift = PAC1921_GAIN_DI_GAIN_SHIFT;
+		priv_val = &priv->di_gain;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	unsigned int gain_param = (unsigned int)val;
+
+	if (!pac1921_gain_param_valid(gain_param, max))
+		return -EINVAL;
+
+	u8 gain = (u8)ilog2(gain_param);
+
+	guard(mutex)(&priv->lock);
+
+	if (*priv_val == gain)
+		return 0;
+
+	int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_GAIN_CFG, mask,
+					 gain << shift);
+	if (ret)
+		return ret;
+
+	*priv_val = gain;
+
+	return 0;
+}
+
+static int pac1921_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		return pac1921_update_gain(priv, chan, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int pac1921_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		switch (chan->channel) {
+		case PAC1921_CHAN_VBUS:
+			*length = ARRAY_SIZE(pac1921_dv_gains);
+			*vals = pac1921_dv_gains;
+			*type = IIO_VAL_INT;
+			return IIO_AVAIL_LIST;
+
+		case PAC1921_CHAN_VSENSE:
+		case PAC1921_CHAN_CURRENT:
+			*length = ARRAY_SIZE(pac1921_di_gains);
+			*vals = pac1921_di_gains;
+			*type = IIO_VAL_INT;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int pac1921_read_label(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, char *label)
+{
+	switch (chan->channel) {
+	case PAC1921_CHAN_VBUS:
+		return sprintf(label, "vbus\n");
+	case PAC1921_CHAN_VSENSE:
+		return sprintf(label, "vsense\n");
+	case PAC1921_CHAN_CURRENT:
+		return sprintf(label, "current\n");
+	case PAC1921_CHAN_POWER:
+		return sprintf(label, "power\n");
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info pac1921_iio = {
+	.read_raw = pac1921_read_raw,
+	.write_raw = pac1921_write_raw,
+	.read_avail = pac1921_read_avail,
+	.read_label = pac1921_read_label,
+};
+
+static int pac1921_get_resolution(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan)
+{
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+
+	guard(mutex)(&priv->lock);
+
+	if (priv->high_res)
+		return PAC1921_MEAS_RESOLUTION_IDX_HIGH;
+
+	return PAC1921_MEAS_RESOLUTION_IDX_LOW;
+}
+
+static int pac1921_set_resolution(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan,
+				  unsigned int val)
+{
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+	bool high_res;
+
+	switch (val) {
+	case PAC1921_MEAS_RESOLUTION_IDX_LOW:
+		high_res = false;
+		break;
+	case PAC1921_MEAS_RESOLUTION_IDX_HIGH:
+		high_res = true;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
+
+	guard(mutex)(&priv->lock);
+
+	if (priv->high_res == high_res)
+		return 0;
+
+	unsigned int mask = PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES;
+	unsigned int bits = high_res ?
+			    0 : PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES;
+
+	int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_GAIN_CFG,
+					 mask, bits);
+	if (ret)
+		return ret;
+
+	priv->high_res = high_res;
+
+	return pac1921_update_integr_period(priv);
+}
+
+static int pac1921_get_int_num_samples(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan)
+{
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+
+	guard(mutex)(&priv->lock);
+
+	return priv->n_samples;
+}
+
+static int pac1921_set_int_num_samples(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan,
+				       unsigned int val)
+{
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+
+	if (WARN_ON_ONCE(val > PAC1921_INT_CFG_SMPL_MAX))
+		return -EINVAL;
+
+	guard(mutex)(&priv->lock);
+
+	if (priv->n_samples == val)
+		return 0;
+
+	int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG,
+					 PAC1921_INT_CFG_SMPL_MASK,
+					 val << PAC1921_INT_CFG_SMPL_SHIFT);
+	if (ret)
+		return ret;
+
+	priv->n_samples = (u8)val;
+
+	return pac1921_update_integr_period(priv);
+}
+
+static ssize_t pac1921_read_filters_enabled(struct iio_dev *indio_dev,
+					    uintptr_t private,
+					    const struct iio_chan_spec *chan,
+					    char *buf)
+{
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+	bool enabled;
+
+	scoped_guard(mutex, &priv->lock) {
+		enabled = priv->filters_en;
+	}
+	return sysfs_emit(buf, "%d\n", enabled);
+}
+
+static ssize_t pac1921_write_filters_enabled(struct iio_dev *indio_dev,
+					     uintptr_t private,
+					     const struct iio_chan_spec *chan,
+					     const char *buf, size_t len)
+{
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+	bool enabled;
+
+	int ret = kstrtobool(buf, &enabled);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&priv->lock);
+
+	if (priv->filters_en == enabled)
+		return 0;
+
+	unsigned int mask = PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN;
+	unsigned int bits = enabled ?
+			    PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN : 0;
+
+
+	ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG, mask, bits);
+	if (ret)
+		return ret;
+
+	priv->filters_en = enabled;
+
+	ret = pac1921_update_integr_period(priv);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static const struct iio_enum pac1921_resolution_enum = {
+	.items = pac1921_meas_resolutions,
+	.num_items = ARRAY_SIZE(pac1921_meas_resolutions),
+	.get = pac1921_get_resolution,
+	.set = pac1921_set_resolution,
+};
+static const struct iio_enum pac1921_int_num_samples_enum = {
+	.items = pac1921_integr_num_samples,
+	.num_items = ARRAY_SIZE(pac1921_integr_num_samples),
+	.get = pac1921_get_int_num_samples,
+	.set = pac1921_set_int_num_samples,
+};
+static const struct iio_chan_spec_ext_info pac1921_ext_info[] = {
+	IIO_ENUM("resolution_bits", IIO_SHARED_BY_ALL,
+		 &pac1921_resolution_enum),
+	IIO_ENUM_AVAILABLE("resolution_bits", IIO_SHARED_BY_ALL,
+			   &pac1921_resolution_enum),
+	IIO_ENUM("integration_samples", IIO_SHARED_BY_ALL,
+		 &pac1921_int_num_samples_enum),
+	IIO_ENUM_AVAILABLE("integration_samples", IIO_SHARED_BY_ALL,
+			   &pac1921_int_num_samples_enum),
+	{
+		.name = "filters_en",
+		.read = pac1921_read_filters_enabled,
+		.write = pac1921_write_filters_enabled,
+		.shared = IIO_SHARED_BY_ALL,
+	},
+	{},
+};
+
+static const struct iio_event_spec pac1921_overflow_event[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+	},
+};
+
+static const struct iio_chan_spec pac1921_channels[] = {
+	{
+		.type = IIO_VOLTAGE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.channel = PAC1921_CHAN_VBUS,
+		.address = PAC1921_REG_VBUS,
+		.scan_index = PAC1921_CHAN_VBUS,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 10,
+			.storagebits = 16,
+			.endianness = IIO_CPU
+		},
+		.indexed = 1,
+		.event_spec = pac1921_overflow_event,
+		.num_event_specs = ARRAY_SIZE(pac1921_overflow_event),
+		.ext_info = pac1921_ext_info,
+	},
+	{
+		.type = IIO_VOLTAGE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.channel = PAC1921_CHAN_VSENSE,
+		.address = PAC1921_REG_VSENSE,
+		.scan_index = PAC1921_CHAN_VSENSE,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 10,
+			.storagebits = 16,
+			.endianness = IIO_CPU
+		},
+		.indexed = 1,
+		.event_spec = pac1921_overflow_event,
+		.num_event_specs = ARRAY_SIZE(pac1921_overflow_event),
+		.ext_info = pac1921_ext_info,
+	},
+	{
+		.type = IIO_CURRENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.channel = PAC1921_CHAN_CURRENT,
+		.address = PAC1921_REG_VSENSE,
+		.scan_index = PAC1921_CHAN_CURRENT,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 10,
+			.storagebits = 16,
+			.endianness = IIO_CPU
+		},
+		.event_spec = pac1921_overflow_event,
+		.num_event_specs = ARRAY_SIZE(pac1921_overflow_event),
+		.ext_info = pac1921_ext_info,
+	},
+	{
+		.type = IIO_POWER,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.channel = PAC1921_CHAN_POWER,
+		.address = PAC1921_REG_VPOWER,
+		.scan_index = PAC1921_CHAN_POWER,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 10,
+			.storagebits = 16,
+			.endianness = IIO_CPU
+		},
+		.event_spec = pac1921_overflow_event,
+		.num_event_specs = ARRAY_SIZE(pac1921_overflow_event),
+		.ext_info = pac1921_ext_info,
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(PAC1921_NUM_MEAS_CHANS),
+};
+
+static irqreturn_t pac1921_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct pac1921_priv *priv = iio_priv(idev);
+
+	guard(mutex)(&priv->lock);
+
+	if (!pac1921_data_ready(priv))
+		goto done;
+
+	int ret = pac1921_check_push_overflow(idev, pf->timestamp);
+	if (ret)
+		goto done;
+
+	memset(&priv->scan, 0, sizeof(priv->scan));
+
+	int bit, ch = 0;
+	for_each_set_bit(bit, idev->active_scan_mask, idev->masklength) {
+		int val;
+
+		ret = pac1921_read_res(priv, idev->channels[ch].address, &val);
+		if (ret)
+			goto done;
+
+		priv->scan.chan[ch++] = (u16)val;
+	}
+
+	iio_push_to_buffers_with_timestamp(idev, &priv->scan, pf->timestamp);
+
+done:
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int pac1921_init(struct pac1921_priv *priv)
+{
+	/* Enter READ state before configuration */
+	int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
+				 PAC1921_INT_CFG_INTEN, 0);
+	if (ret)
+		return ret;
+
+	/* Configure gains and measurements resolution */
+	unsigned int val = priv->di_gain << PAC1921_GAIN_DI_GAIN_SHIFT |
+			   priv->dv_gain << PAC1921_GAIN_DV_GAIN_SHIFT;
+	if (!priv->high_res)
+		val |= PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES;
+	ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val);
+	if (ret)
+		return ret;
+
+	/* Configure integration:
+	 * - num of integration samples, filters enabled/disabled
+	 * - set READ/INT pin override (RIOV) to control operation mode via
+	 *   register instead of pin
+	 */
+	val = priv->n_samples << PAC1921_INT_CFG_SMPL_SHIFT |
+	      PAC1921_INT_CFG_RIOV;
+	if (priv->filters_en)
+		val |= PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN;
+	ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val);
+	if (ret)
+		return ret;
+
+	/* Init control register:
+	 * - VPower free run integration mode
+	 * - OUT pin full scale range: 3V (HW detault)
+	 * - no timeout, no sleep, no sleep override, no recalc (HW defaults)
+	 */
+	val = PAC1921_MXSL_VPOWER_FREE_RUN << PAC1921_CONTROL_MXSL_SHIFT;
+	ret = regmap_write(priv->regmap, PAC1921_REG_CONTROL, val);
+	if (ret)
+		return ret;
+
+	/* Enable integration */
+	ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
+				 PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN);
+	if (ret)
+		return ret;
+
+	priv->first_integr_started = true;
+	priv->integr_start_time = jiffies;
+
+	return pac1921_update_integr_period(priv);
+}
+
+static int pac1921_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+
+	guard(mutex)(&priv->lock);
+
+	priv->first_integr_started = false;
+	priv->first_integr_done = false;
+
+	int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
+				     PAC1921_INT_CFG_INTEN, 0);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(priv->regmap, PAC1921_REG_CONTROL,
+				  PAC1921_CONTROL_SLEEP, PAC1921_CONTROL_SLEEP);
+}
+
+static int pac1921_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+
+	guard(mutex)(&priv->lock);
+
+	return pac1921_init(priv);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(pac1921_pm_ops, pac1921_suspend,
+				pac1921_resume);
+
+static int pac1921_parse_properties(struct pac1921_priv *priv)
+{
+	struct device *dev = &priv->client->dev;
+	u32 prop;
+
+	int ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
+					   &prop);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Cannot read shunt resistor property\n");
+	if (prop == 0 || prop > INT_MAX)
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid shunt resistor: %u\n", prop);
+	priv->rshunt = prop;
+
+	if (!device_property_read_u32(dev, "microchip,dv-gain", &prop)) {
+		if (!pac1921_gain_param_valid(prop, PAC1921_GAIN_DV_GAIN_MAX))
+			return dev_err_probe(dev, -EINVAL,
+					     "Invalid dv_gain: %u\n", prop);
+
+		priv->dv_gain = (u8)ilog2(prop);
+	}
+	if (!device_property_read_u32(dev, "microchip,di-gain", &prop)) {
+		if (!pac1921_gain_param_valid(prop, PAC1921_GAIN_DI_GAIN_MAX))
+			return dev_err_probe(dev, -EINVAL,
+					     "Invalid di_gain: %u\n", prop);
+
+		priv->di_gain = (u8)ilog2(prop);
+	}
+	return 0;
+}
+
+static int pac1921_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct pac1921_priv *priv;
+
+	struct iio_dev *indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	priv = iio_priv(indio_dev);
+	priv->client = client;
+	i2c_set_clientdata(client, indio_dev);
+
+	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
+	if (IS_ERR(priv->regmap))
+		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
+			      "Cannot initialize register map\n");
+
+	mutex_init(&priv->lock);
+
+	priv->dv_gain = PAC1921_DEFAULT_DV_GAIN;
+	priv->di_gain = PAC1921_DEFAULT_DI_GAIN;
+	priv->high_res = PAC1921_DEFAULT_HIGH_RES;
+	priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES;
+	priv->filters_en = PAC1921_DEFAULT_FILTERS_ENABLED;
+
+	int ret = pac1921_parse_properties(priv);
+	if (ret)
+		return ret;
+
+	ret = pac1921_init(priv);
+	if (ret)
+		return ret;
+
+	priv->iio_info = pac1921_iio;
+
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
+
+	indio_dev->name = id->name;
+	indio_dev->info = &priv->iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = pac1921_channels;
+	indio_dev->num_channels = ARRAY_SIZE(pac1921_channels);
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      &iio_pollfunc_store_time,
+					      &pac1921_trigger_handler, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Cannot setup IIO triggered buffer\n");
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot register IIO device\n");
+
+	return 0;
+}
+
+static const struct i2c_device_id pac1921_id[] = {
+	{ .name = "pac1921", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, pac1921_id);
+
+static const struct of_device_id pac1921_of_match[] = {
+	{ .compatible = "microchip,pac1921" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pac1921_of_match);
+
+static struct i2c_driver pac1921_driver = {
+	.driver	 = {
+		.name = "pac1921",
+		.pm = pm_sleep_ptr(&pac1921_pm_ops),
+		.of_match_table = pac1921_of_match,
+	},
+	.probe = pac1921_probe,
+	.id_table = pac1921_id,
+};
+
+module_i2c_driver(pac1921_driver);
+
+MODULE_AUTHOR("Matteo Martelli <matteomartelli3@gmail.com>");
+MODULE_DESCRIPTION("IIO driver for PAC1921 High-Side Power/Current Monitor");
+MODULE_LICENSE("GPL");

-- 
2.45.1

Re: [PATCH 2/2] iio: adc: add support for pac1921

[Marius.Cristea]

  Hi Matteo,

   Thank you very much for helping us adding PAC1921 support. I'm also
developing a similar driver and I could share the code with you to make
the driver better.

   Also I have a few review comments, please, see bellow:

Best regards,
Marius

On Wed, Jul 03, 2024 at 03:34:34PM +0200, Matteo Martelli wrote:


> +
> +/* pac1921 regmap configuration */
> +static const struct regmap_range pac1921_regmap_wr_ranges[] = {
> +       regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
> +};
> +static const struct regmap_access_table pac1921_regmap_wr_table = {
> +       .yes_ranges = pac1921_regmap_wr_ranges,
> +       .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_wr_ranges),
> +};
> +static const struct regmap_range pac1921_regmap_rd_ranges[] = {
> +       regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
> +       regmap_reg_range(PAC1921_REG_VBUS, PAC1921_REG_VPOWER + 1),
> +};
> +static const struct regmap_access_table pac1921_regmap_rd_table = {
> +       .yes_ranges = pac1921_regmap_rd_ranges,
> +       .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_rd_ranges),
> +};
I know that the regmap is the way forward, but the PAC devices are more
efficient if bulk read is done. Also when you are reading all values at
the same time, the Voltage, Current and the Power numbers will came
from the same sampling time and they will be correlated to each other.


> +static const struct regmap_config pac1921_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .rd_table = &pac1921_regmap_rd_table,
> +       .wr_table = &pac1921_regmap_wr_table,
> +};
> +
> +enum pac1921_channels {
> +       PAC1921_CHAN_VBUS = 0,
> +       PAC1921_CHAN_VSENSE = 1,

Not sure if this channel is useful. None of our clients ask us to
provide this information. This measurement could be calculated based on
the shunt and the current

> +       PAC1921_CHAN_CURRENT = 2,
> +       PAC1921_CHAN_POWER = 3,
> +};
> +#define PAC1921_NUM_MEAS_CHANS 4
> +
> +struct pac1921_priv {
> +       struct i2c_client *client;
> +       struct regmap *regmap;
> +       struct iio_info iio_info;
> +
> +       /* Synchronize access to private members, and ensure
> atomicity of
> +        * consecutive regmap operations.
> +        */
> +       struct mutex lock;
> +
> +       u32 rshunt; /* uOhm */

As in the case of PAC1934 I would like to change the rshunt value
during the runtime. This is useful during a calibration phase.

> +       bool high_res;
> +       u8 dv_gain;
> +       u8 di_gain;
> +       u8 n_samples;
> +       bool filters_en;
> +       u8 prev_ovf_flags;
> +
> +       bool first_integr_started;
> +       bool first_integr_done;
> +       unsigned long integr_start_time; /* jiffies */
> +       unsigned int integr_period; /* usecs */
> +
> +       struct {
> +               u16 chan[PAC1921_NUM_MEAS_CHANS];
> +               s64 timestamp __aligned(8);
> +       } scan;
> +};
> +
> +/* The integration period depends on the configuration of number of
> integration

I think it should be a multi line comment here. The same comment for
multiple places in the driver.

> + * samples, measurement resolution and post filters. The following
> array
> + * contains integration periods, in microsecs unit, based on table
> 4-5 from
> + * datasheet considering power integration mode, 11-Bit resolution
> and post
> + * filters off. Each index corresponds to a specific number of
> samples from 1
> + * to 2048.
> + */
> +static const unsigned int pac1921_integr_periods_us[] = {
> +       930,   1460,  2410,   4320,   8050,   16100,
> +       32100, 64200, 128300, 257000, 513000, 1026000
> +};
> 




> +       case IIO_CHAN_INFO_SCALE:
> +               switch (chan->channel) {
> +               case PAC1921_CHAN_VBUS: {
> +                       /* Vbus scale factor in mV:
> +                        * max_vbus (mV) / vgain / resolution
> +                        */
> +                       guard(mutex)(&priv->lock);
> +
> +                       *val = PAC1921_MAX_VBUS_V * 1000;
> +                       *val2 = PAC1921_RES_RESOLUTION << (int)priv-
> >dv_gain;
> +
> +                       return IIO_VAL_FRACTIONAL;
> +               }
> +               case PAC1921_CHAN_VSENSE: {
> +                       /* Vsense voltage scale factor in mV:
> +                        * max_vsense (mV) / igain / resolution
> +                        */
> +                       guard(mutex)(&priv->lock);
> +
> +                       *val = PAC1921_MAX_VSENSE_MV;
> +                       *val2 = PAC1921_RES_RESOLUTION << (int)priv-
> >di_gain;
> +
> +                       return IIO_VAL_FRACTIONAL;
> +               }
> +               case PAC1921_CHAN_CURRENT: {
> +                       /* Current scale factor in mA:
> +                        * Vsense LSB (nV) / shunt (uOhm)
> +                        */
> +                       guard(mutex)(&priv->lock);
> +
> +                       *val = pac1921_vsense_lsb(priv->di_gain);
> +                       *val2 = (int)priv->rshunt;
> +
> +                       return IIO_VAL_FRACTIONAL;
> +               }
> +               case PAC1921_CHAN_POWER: {
> 
> +                       /* Power scale factor in mW:

I think the scale here should be in microWatts. We have (mA)*(mV)

> +                        * Vsense LSB (nV) * max_vbus (V) / vgain /
> shunt (uOhm)
> +                        */
> +                       guard(mutex)(&priv->lock);
> +
> +                       *val = pac1921_vsense_lsb(priv->di_gain) *
> +                              (PAC1921_MAX_VBUS_V >> (int)priv-
> >dv_gain);
> +                       *val2 = (int)priv->rshunt;
> +
> +                       return IIO_VAL_FRACTIONAL;
> +               }
> +               default:
> +                       return -EINVAL;
> +               }
> +               break;
> +
> +       case IIO_CHAN_INFO_HARDWAREGAIN:

HARDWAREGAIN should not be used here. The gain (for voltage and for the
current) should be included into the appropriate scale. A property
named scale_available should be added and the used could change the
gain by setting the scale. Please see PAC1934 and few other drivers.



> +static int pac1921_write_raw(struct iio_dev *indio_dev,
> +                            struct iio_chan_spec const *chan, int
> val,
> +                            int val2, long mask)
> +{
> +       struct pac1921_priv *priv = iio_priv(indio_dev);
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_HARDWAREGAIN:
> +               return pac1921_update_gain(priv, chan, val);

This should not be needed...

> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +static int pac1921_read_avail(struct iio_dev *indio_dev,
> +                             struct iio_chan_spec const *chan,
> +                             const int **vals, int *type, int
> *length,
> +                             long mask)
> +{
> +       switch (mask) {
> +       case IIO_CHAN_INFO_HARDWAREGAIN:

this should be changed to use "scale_available"

> +               switch (chan->channel) {
> +               case PAC1921_CHAN_VBUS:
> +                       *length = ARRAY_SIZE(pac1921_dv_gains);
> +                       *vals = pac1921_dv_gains;
> +                       *type = IIO_VAL_INT;
> +                       return IIO_AVAIL_LIST;
> +
> +               case PAC1921_CHAN_VSENSE:
> +               case PAC1921_CHAN_CURRENT:
> +                       *length = ARRAY_SIZE(pac1921_di_gains);
> +                       *vals = pac1921_di_gains;
> +                       *type = IIO_VAL_INT;
> +                       return IIO_AVAIL_LIST;
> +               default:
> +                       return -EINVAL;
> +               }
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +static int pac1921_read_label(struct iio_dev *indio_dev,
> +                             struct iio_chan_spec const *chan, char
> *label)
> +{

there is a "label" into the device tree that should be used here in
order to identify the device and what the device measures, like
GPU_vbus or IO_vbus.

> +       switch (chan->channel) {
> +       case PAC1921_CHAN_VBUS:
> +               return sprintf(label, "vbus\n");
> +       case PAC1921_CHAN_VSENSE:
> +               return sprintf(label, "vsense\n");
> +       case PAC1921_CHAN_CURRENT:
> +               return sprintf(label, "current\n");
> +       case PAC1921_CHAN_POWER:
> +               return sprintf(label, "power\n");
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +static const struct iio_info pac1921_iio = {
> +       .read_raw = pac1921_read_raw,
> +       .write_raw = pac1921_write_raw,
> +       .read_avail = pac1921_read_avail,
> +       .read_label = pac1921_read_label,
> +};
> +
> +static int pac1921_get_resolution(struct iio_dev *indio_dev,
> +                                 const struct iio_chan_spec *chan)
> +{
> +       struct pac1921_priv *priv = iio_priv(indio_dev);
> +
> +       guard(mutex)(&priv->lock);
> +
> +       if (priv->high_res)
> +               return PAC1921_MEAS_RESOLUTION_IDX_HIGH;
> +
> +       return PAC1921_MEAS_RESOLUTION_IDX_LOW;
> +}
> +
> +static int pac1921_set_resolution(struct iio_dev *indio_dev,
> +                                 const struct iio_chan_spec *chan,
> +                                 unsigned int val)
> +{
> +       struct pac1921_priv *priv = iio_priv(indio_dev);
> +       bool high_res;
> +
> +       switch (val) {
> +       case PAC1921_MEAS_RESOLUTION_IDX_LOW:
> +               high_res = false;
> +               break;
> +       case PAC1921_MEAS_RESOLUTION_IDX_HIGH:
> +               high_res = true;
> +               break;
> +       default:
> +               WARN_ON_ONCE(1);
> +               return -EINVAL;
> +       }
> +
> +       guard(mutex)(&priv->lock);
> +
> +       if (priv->high_res == high_res)
> +               return 0;
> +
> +       unsigned int mask = PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES;
> +       unsigned int bits = high_res ?
> +                           0 : PAC1921_GAIN_I_RES |
> PAC1921_GAIN_V_RES;
> +
> +       int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_GAIN_CFG,
> +                                        mask, bits);
> +       if (ret)
> +               return ret;
> +
> +       priv->high_res = high_res;
> +
> +       return pac1921_update_integr_period(priv);
> +}
> +
> +static int pac1921_get_int_num_samples(struct iio_dev *indio_dev,
> +                                      const struct iio_chan_spec
> *chan)
> +{
> +       struct pac1921_priv *priv = iio_priv(indio_dev);
> +
> +       guard(mutex)(&priv->lock);
> +
> +       return priv->n_samples;
> +}
> +
> +static int pac1921_set_int_num_samples(struct iio_dev *indio_dev,
> +                                      const struct iio_chan_spec
> *chan,
> +                                      unsigned int val)
> +{
> +       struct pac1921_priv *priv = iio_priv(indio_dev);
> +
> +       if (WARN_ON_ONCE(val > PAC1921_INT_CFG_SMPL_MAX))
> +               return -EINVAL;
> +
> +       guard(mutex)(&priv->lock);
> +
> +       if (priv->n_samples == val)
> +               return 0;
> +
> +       int ret = pac1921_update_cfg_reg(priv, PAC1921_REG_INT_CFG,
> +                                        PAC1921_INT_CFG_SMPL_MASK,
> +                                        val <<
> PAC1921_INT_CFG_SMPL_SHIFT);
> +       if (ret)
> +               return ret;
> +
> +       priv->n_samples = (u8)val;
> +
> +       return pac1921_update_integr_period(priv);
> +}
> +
> +static ssize_t pac1921_read_filters_enabled(struct iio_dev
> *indio_dev,
> +                                           uintptr_t private,
> +                                           const struct
> iio_chan_spec *chan,
> +                                           char *buf)
> +{
> +       struct pac1921_priv *priv = iio_priv(indio_dev);
> +       bool enabled;
> +
> +       scoped_guard(mutex, &priv->lock) {
> +               enabled = priv->filters_en;
> +       }
> +       return sysfs_emit(buf, "%d\n", enabled);
> +}
> +
> +static ssize_t pac1921_write_filters_enabled(struct iio_dev
> *indio_dev,
> +                                            uintptr_t private,
> +                                            const struct
> iio_chan_spec *chan,
> +                                            const char *buf, size_t
> len)

I would like to set each filter separately and let the user to decide
how to use this functionality. Not always both filters are needed.






> +static int pac1921_init(struct pac1921_priv *priv)
> +{
> +       /* Enter READ state before configuration */
> +       int ret = regmap_update_bits(priv->regmap,
> PAC1921_REG_INT_CFG,
> +                                PAC1921_INT_CFG_INTEN, 0);
> +       if (ret)
> +               return ret;
> +
> +       /* Configure gains and measurements resolution */
> +       unsigned int val = priv->di_gain <<
> PAC1921_GAIN_DI_GAIN_SHIFT |
> +                          priv->dv_gain <<
> PAC1921_GAIN_DV_GAIN_SHIFT;
> +       if (!priv->high_res)
> +               val |= PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES;
> +       ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val);
> +       if (ret)
> +               return ret;
> +
> +       /* Configure integration:
> +        * - num of integration samples, filters enabled/disabled
> +        * - set READ/INT pin override (RIOV) to control operation
> mode via
> +        *   register instead of pin
> +        */
> +       val = priv->n_samples << PAC1921_INT_CFG_SMPL_SHIFT |
> +             PAC1921_INT_CFG_RIOV;
> +       if (priv->filters_en)
> +               val |= PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN;
> +       ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val);
> +       if (ret)
> +               return ret;
> +
> +       /* Init control register:
> +        * - VPower free run integration mode

User should control what to output on the "OUT DAC" pin.

> +        * - OUT pin full scale range: 3V (HW detault)

Full range for OUT should be configurable.

> +        * - no timeout, no sleep, no sleep override, no recalc (HW
> defaults)
> +        */
> +       val = PAC1921_MXSL_VPOWER_FREE_RUN <<
> PAC1921_CONTROL_MXSL_SHIFT;
> +       ret = regmap_write(priv->regmap, PAC1921_REG_CONTROL, val);
> +       if (ret)
> +               return ret;
> +
> +       /* Enable integration */
> +       ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
> +                                PAC1921_INT_CFG_INTEN,
> PAC1921_INT_CFG_INTEN);
> +       if (ret)
> +               return ret;
> +
> +       priv->first_integr_started = true;
> +       priv->integr_start_time = jiffies;
> +
> +       return pac1921_update_integr_period(priv);
> +}




> +static int pac1921_parse_properties(struct pac1921_priv *priv)
> +{
> +       struct device *dev = &priv->client->dev;
> +       u32 prop;
> +
> +       int ret = device_property_read_u32(dev, "shunt-resistor-
> micro-ohms",
> +                                          &prop);
> +       if (ret)
> +               return dev_err_probe(dev, ret,
> +                                    "Cannot read shunt resistor
> property\n");
> +       if (prop == 0 || prop > INT_MAX)
> +               return dev_err_probe(dev, -EINVAL,
> +                                    "Invalid shunt resistor: %u\n",
> prop);
> +       priv->rshunt = prop;
> +
> +       if (!device_property_read_u32(dev, "microchip,dv-gain",
> &prop)) {
> +               if (!pac1921_gain_param_valid(prop,
> PAC1921_GAIN_DV_GAIN_MAX))
> +                       return dev_err_probe(dev, -EINVAL,
> +                                            "Invalid dv_gain: %u\n",
> prop);
> +
> +               priv->dv_gain = (u8)ilog2(prop);
> +       }
> +       if (!device_property_read_u32(dev, "microchip,di-gain",
> &prop)) {
> +               if (!pac1921_gain_param_valid(prop,
> PAC1921_GAIN_DI_GAIN_MAX))
> +                       return dev_err_probe(dev, -EINVAL,
> +                                            "Invalid di_gain: %u\n",
> prop);
> +
> +               priv->di_gain = (u8)ilog2(prop);
> +       }
> +       return 0;
> +}
> +
> +static int pac1921_probe(struct i2c_client *client)
> +{
> +       struct device *dev = &client->dev;
> +       struct pac1921_priv *priv;
> +
> +       struct iio_dev *indio_dev = devm_iio_device_alloc(dev,
> sizeof(*priv));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       priv = iio_priv(indio_dev);
> +       priv->client = client;
> +       i2c_set_clientdata(client, indio_dev);
> +
> +       priv->regmap = devm_regmap_init_i2c(client,
> &pac1921_regmap_config);
> +       if (IS_ERR(priv->regmap))
> +               dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> +                             "Cannot initialize register map\n");
> +
> +       mutex_init(&priv->lock);
> +
> +       priv->dv_gain = PAC1921_DEFAULT_DV_GAIN;
> +       priv->di_gain = PAC1921_DEFAULT_DI_GAIN;
> +       priv->high_res = PAC1921_DEFAULT_HIGH_RES;
> +       priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES;
> +       priv->filters_en = PAC1921_DEFAULT_FILTERS_ENABLED;
> +
> +       int ret = pac1921_parse_properties(priv);

I would like to handle also the ACPI for x86 boards and not only the
device tree.


> +       if (ret)
> +               return ret;
> +
> +       ret = pac1921_init(priv);
> +       if (ret)
> +               return ret;
> +
> +       priv->iio_info = pac1921_iio;
> +
> +       const struct i2c_device_id *id =
> i2c_client_get_device_id(client);
> +
> +       indio_dev->name = id->name;
> +       indio_dev->info = &priv->iio_info;
> +       indio_dev->modes = INDIO_DIRECT_MODE;
> +       indio_dev->channels = pac1921_channels;
> +       indio_dev->num_channels = ARRAY_SIZE(pac1921_channels);
> +
> +       ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +                                            
> &iio_pollfunc_store_time,
> +                                            
> &pac1921_trigger_handler, NULL);
> +       if (ret)
> +               return dev_err_probe(dev, ret,
> +                                    "Cannot setup IIO triggered
> buffer\n");
> +
> +       ret = devm_iio_device_register(dev, indio_dev);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Cannot register IIO
> device\n");
> +
> +       return 0;
> +}
> +

Re: [PATCH 2/2] iio: adc: add support for pac1921

[Matteo Martelli]

Marius.Cristea@ wrote:
> 
> 
>   Hi Matteo,
> 
>    Thank you very much for helping us adding PAC1921 support. I'm also
> developing a similar driver and I could share the code with you to make
> the driver better.
> 
>    Also I have a few review comments, please, see bellow:
> 
> Best regards,
> Marius

Hi Marius,

Thanks for your feedback, this is indeed very helpful, as it would be if you
shared your code.

I addressed some of your comments below in patch v2 thread, replying to
Jonathan [1]. If you have more comments about those points please reply on that
thread.

[1]: https://lore.kernel.org/linux-iio/668bec2a8b23a_6e037017@njaxe.notmuch

See also my comments below.

> On Wed, Jul 03, 2024 at 03:34:34PM +0200, Matteo Martelli wrote:
> 
> 
> > +
> > +/* pac1921 regmap configuration */
> > +static const struct regmap_range pac1921_regmap_wr_ranges[] = {
> > +       regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
> > +};
> > +static const struct regmap_access_table pac1921_regmap_wr_table = {
> > +       .yes_ranges = pac1921_regmap_wr_ranges,
> > +       .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_wr_ranges),
> > +};
> > +static const struct regmap_range pac1921_regmap_rd_ranges[] = {
> > +       regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
> > +       regmap_reg_range(PAC1921_REG_VBUS, PAC1921_REG_VPOWER + 1),
> > +};
> > +static const struct regmap_access_table pac1921_regmap_rd_table = {
> > +       .yes_ranges = pac1921_regmap_rd_ranges,
> > +       .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_rd_ranges),
> > +};
> I know that the regmap is the way forward, but the PAC devices are more
> efficient if bulk read is done.
>
I think performances depends on the use case, for example if the user is
interested in only one type of measurement I don't think reading all registers
result in a performance gain. Also consider that there are 8 registers of
accumulators that are not currently exposed by the driver but that would be read
anyway, so I am not sure it would result in a performance gain either. I would
leave it as it is right now to keep the implementation simple and maybe extend
it in the future in case performance gains become obvious and necessary for most
use cases. Also consider that regmap supports bulk reads.
>
> Also when you are reading all values at
> the same time, the Voltage, Current and the Power numbers will came
> from the same sampling time and they will be correlated to each other.
> 
I think that there is no guarantee that the user would always request two
successive raw readings, like Voltage and Current, within the same integration
period. So for the second reading all registers may be read again and the two
values might not be correlated as well. Also consider that the measurements are
kept in the device registers until a new integration is complete, so if two
registers are read consecutively within the same integration period they would
be correlated.

> > +static const struct regmap_config pac1921_regmap_config = {
> > +       .reg_bits = 8,
> > +       .val_bits = 8,
> > +       .rd_table = &pac1921_regmap_rd_table,
> > +       .wr_table = &pac1921_regmap_wr_table,
> > +};
> > +
> > +enum pac1921_channels {
> > +       PAC1921_CHAN_VBUS = 0,
> > +       PAC1921_CHAN_VSENSE = 1,
> 
> Not sure if this channel is useful. None of our clients ask us to
> provide this information. This measurement could be calculated based on
> the shunt and the current
> 
I would not mind removing the VSENSE channel, but there are other drivers
exposing it (some of them calls it vshunt or shunt_voltage, e.g. ina2xx). Are we
sure no one is interested in it?

> > +       PAC1921_CHAN_CURRENT = 2,
> > +       PAC1921_CHAN_POWER = 3,
> > +};
> > +#define PAC1921_NUM_MEAS_CHANS 4
> > +
> > +struct pac1921_priv {
> > +       struct i2c_client *client;
> > +       struct regmap *regmap;
> > +       struct iio_info iio_info;
> > +
> > +       /* Synchronize access to private members, and ensure
> > atomicity of
> > +        * consecutive regmap operations.
> > +        */
> > +       struct mutex lock;
> > +
> > +       u32 rshunt; /* uOhm */
> 
> As in the case of PAC1934 I would like to change the rshunt value
> during the runtime. This is useful during a calibration phase.
> 
All right, I would add this as an iio_chan_spec_ext_info. I mentioned this in
patch v2 thread [1], please reply there if you have more comments.

> > +       bool high_res;
> > +       u8 dv_gain;
> > +       u8 di_gain;
> > +       u8 n_samples;
> > +       bool filters_en;
> > +       u8 prev_ovf_flags;
> > +
> > +       bool first_integr_started;
> > +       bool first_integr_done;
> > +       unsigned long integr_start_time; /* jiffies */
> > +       unsigned int integr_period; /* usecs */
> > +
> > +       struct {
> > +               u16 chan[PAC1921_NUM_MEAS_CHANS];
> > +               s64 timestamp __aligned(8);
> > +       } scan;
> > +};
> > +
> > +/* The integration period depends on the configuration of number of
> > integration
> 
> I think it should be a multi line comment here. The same comment for
> multiple places in the driver.
> 
Thanks for catching this, I will fix it in next patch version.

> > +               case PAC1921_CHAN_POWER: {
> > 
> > +                       /* Power scale factor in mW:
> 
> I think the scale here should be in microWatts. We have (mA)*(mV)
> 
Documentation/ABI/testing/sysfs-bus-iio states it should be in milliwatts:

What:		/sys/bus/iio/devices/iio:deviceX/in_powerY_raw
KernelVersion:	4.5
Contact:	linux-iio@vger.kernel.org
Description:
		Raw (unscaled no bias removal etc.) power measurement from
		channel Y. The number must always be specified and
		unique to allow association with event codes. Units after
		application of scale and offset are milliwatts.

> > +                        * Vsense LSB (nV) * max_vbus (V) / vgain /
> > shunt (uOhm)
> > +                        */
> > +                       guard(mutex)(&priv->lock);
> > +
> > +                       *val = pac1921_vsense_lsb(priv->di_gain) *
> > +                              (PAC1921_MAX_VBUS_V >> (int)priv-
> > >dv_gain);
> > +                       *val2 = (int)priv->rshunt;
> > +
> > +                       return IIO_VAL_FRACTIONAL;
> > +               }
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +               break;
> > +
> > +       case IIO_CHAN_INFO_HARDWAREGAIN:
> 
> HARDWAREGAIN should not be used here. The gain (for voltage and for the
> current) should be included into the appropriate scale. A property
> named scale_available should be added and the used could change the
> gain by setting the scale. Please see PAC1934 and few other drivers.
> 
I will include gains into the scales as suggested. I addressed this in
patch v2 thread [1]. Please reply there if you have additional comments.
> 
> 
> > +
> > +static int pac1921_read_label(struct iio_dev *indio_dev,
> > +                             struct iio_chan_spec const *chan, char
> > *label)
> > +{
> 
> there is a "label" into the device tree that should be used here in
> order to identify the device and what the device measures, like
> GPU_vbus or IO_vbus.
> 
The device label from DT is already exposed into its specific sysfs attribute
and so available to the users. I think using it as a prefix for the channel
would just be a duplication. The pac1934 driver in fact prepends the channel
label here not the device label. I think the same does not apply for the pac1921
being a single channel monitor.

> > +       switch (chan->channel) {
> > +       case PAC1921_CHAN_VBUS:
> > +               return sprintf(label, "vbus\n");
> > +       case PAC1921_CHAN_VSENSE:
> > +               return sprintf(label, "vsense\n");
> > +       case PAC1921_CHAN_CURRENT:
> > +               return sprintf(label, "current\n");
> > +       case PAC1921_CHAN_POWER:
> > +               return sprintf(label, "power\n");
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +}
>
>
> > +static ssize_t pac1921_read_filters_enabled(struct iio_dev
> > *indio_dev,
> > +                                           uintptr_t private,
> > +                                           const struct
> > iio_chan_spec *chan,
> > +                                           char *buf)
> > +{
> > +       struct pac1921_priv *priv = iio_priv(indio_dev);
> > +       bool enabled;
> > +
> > +       scoped_guard(mutex, &priv->lock) {
> > +               enabled = priv->filters_en;
> > +       }
> > +       return sysfs_emit(buf, "%d\n", enabled);
> > +}
> > +
> > +static ssize_t pac1921_write_filters_enabled(struct iio_dev
> > *indio_dev,
> > +                                            uintptr_t private,
> > +                                            const struct
> > iio_chan_spec *chan,
> > +                                            const char *buf, size_t
> > len)
> 
> I would like to set each filter separately and let the user to decide
> how to use this functionality. Not always both filters are needed.
> 
I would remove filters control entirely based on Jonathan comments in patch v2
thread [1]. Please reply in that thread if you have more comments.
> 
> 
> > +static int pac1921_init(struct pac1921_priv *priv)
> > +{
> > +       /* Enter READ state before configuration */
> > +       int ret = regmap_update_bits(priv->regmap,
> > PAC1921_REG_INT_CFG,
> > +                                PAC1921_INT_CFG_INTEN, 0);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Configure gains and measurements resolution */
> > +       unsigned int val = priv->di_gain <<
> > PAC1921_GAIN_DI_GAIN_SHIFT |
> > +                          priv->dv_gain <<
> > PAC1921_GAIN_DV_GAIN_SHIFT;
> > +       if (!priv->high_res)
> > +               val |= PAC1921_GAIN_I_RES | PAC1921_GAIN_V_RES;
> > +       ret = regmap_write(priv->regmap, PAC1921_REG_GAIN_CFG, val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Configure integration:
> > +        * - num of integration samples, filters enabled/disabled
> > +        * - set READ/INT pin override (RIOV) to control operation
> > mode via
> > +        *   register instead of pin
> > +        */
> > +       val = priv->n_samples << PAC1921_INT_CFG_SMPL_SHIFT |
> > +             PAC1921_INT_CFG_RIOV;
> > +       if (priv->filters_en)
> > +               val |= PAC1921_INT_CFG_VSFEN | PAC1921_INT_CFG_VBFEN;
> > +       ret = regmap_write(priv->regmap, PAC1921_REG_INT_CFG, val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Init control register:
> > +        * - VPower free run integration mode
> 
> User should control what to output on the "OUT DAC" pin.
> 
> > +        * - OUT pin full scale range: 3V (HW detault)
> 
> Full range for OUT should be configurable.
> 
I would leave this for future extensions. I addressed this in patch v2 thread
[1]. Please reply there if you have more comments.

>
>
> > +
> > +static int pac1921_probe(struct i2c_client *client)
> > +{
> > +       struct device *dev = &client->dev;
> > +       struct pac1921_priv *priv;
> > +
> > +       struct iio_dev *indio_dev = devm_iio_device_alloc(dev,
> > sizeof(*priv));
> > +       if (!indio_dev)
> > +               return -ENOMEM;
> > +
> > +       priv = iio_priv(indio_dev);
> > +       priv->client = client;
> > +       i2c_set_clientdata(client, indio_dev);
> > +
> > +       priv->regmap = devm_regmap_init_i2c(client,
> > &pac1921_regmap_config);
> > +       if (IS_ERR(priv->regmap))
> > +               dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > +                             "Cannot initialize register map\n");
> > +
> > +       mutex_init(&priv->lock);
> > +
> > +       priv->dv_gain = PAC1921_DEFAULT_DV_GAIN;
> > +       priv->di_gain = PAC1921_DEFAULT_DI_GAIN;
> > +       priv->high_res = PAC1921_DEFAULT_HIGH_RES;
> > +       priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES;
> > +       priv->filters_en = PAC1921_DEFAULT_FILTERS_ENABLED;
> > +
> > +       int ret = pac1921_parse_properties(priv);
> 
> I would like to handle also the ACPI for x86 boards and not only the
> device tree.
> 
I would leave this for future extensions since I cannot test it against ACPI.

Thanks again for your feedback.

Best regards,
Matteo

Re: [PATCH 2/2] iio: adc: add support for pac1921

[Jonathan Cameron]

On Mon, 08 Jul 2024 15:45:50 +0200
Matteo Martelli <matteomartelli3@gmail.com> wrote:

> Marius.Cristea@ wrote:
> > 
> > 
> >   Hi Matteo,
> > 
> >    Thank you very much for helping us adding PAC1921 support. I'm also
> > developing a similar driver and I could share the code with you to make
> > the driver better.
> > 
> >    Also I have a few review comments, please, see bellow:
> > 
> > Best regards,
> > Marius  
> 
> Hi Marius,
> 
> Thanks for your feedback, this is indeed very helpful, as it would be if you
> shared your code.
> 
> I addressed some of your comments below in patch v2 thread, replying to
> Jonathan [1]. If you have more comments about those points please reply on that
> thread.
> 
> [1]: https://lore.kernel.org/linux-iio/668bec2a8b23a_6e037017@njaxe.notmuch
> 
> See also my comments below.
> 
> > On Wed, Jul 03, 2024 at 03:34:34PM +0200, Matteo Martelli wrote:
> > 
> >   
> > > +
> > > +/* pac1921 regmap configuration */
> > > +static const struct regmap_range pac1921_regmap_wr_ranges[] = {
> > > +       regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
> > > +};
> > > +static const struct regmap_access_table pac1921_regmap_wr_table = {
> > > +       .yes_ranges = pac1921_regmap_wr_ranges,
> > > +       .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_wr_ranges),
> > > +};
> > > +static const struct regmap_range pac1921_regmap_rd_ranges[] = {
> > > +       regmap_reg_range(PAC1921_REG_GAIN_CFG, PAC1921_REG_CONTROL),
> > > +       regmap_reg_range(PAC1921_REG_VBUS, PAC1921_REG_VPOWER + 1),
> > > +};
> > > +static const struct regmap_access_table pac1921_regmap_rd_table = {
> > > +       .yes_ranges = pac1921_regmap_rd_ranges,
> > > +       .n_yes_ranges = ARRAY_SIZE(pac1921_regmap_rd_ranges),
> > > +};  
> > I know that the regmap is the way forward, but the PAC devices are more
> > efficient if bulk read is done.
> >  
> I think performances depends on the use case, for example if the user is
> interested in only one type of measurement I don't think reading all registers
> result in a performance gain. Also consider that there are 8 registers of
> accumulators that are not currently exposed by the driver but that would be read
> anyway, so I am not sure it would result in a performance gain either. I would
> leave it as it is right now to keep the implementation simple and maybe extend
> it in the future in case performance gains become obvious and necessary for most
> use cases. Also consider that regmap supports bulk reads.

Usual trick with this stuff is to apply some heuristics ideally based on
what is reasonable on a test platform.  If we are going to read x% of registers
then bulk read them all, if it's less than %x do separate reads.

However, it does add maintenance burden, so there has to be a significant
advantage. Many userspace apps want all or almost all the data though
so we often optimise for that at the cost of reading single channels
(through the buffer interface anyway).

> >
> > Also when you are reading all values at
> > the same time, the Voltage, Current and the Power numbers will came
> > from the same sampling time and they will be correlated to each other.
> >   
> I think that there is no guarantee that the user would always request two
> successive raw readings, like Voltage and Current, within the same integration
> period. So for the second reading all registers may be read again and the two
> values might not be correlated as well. Also consider that the measurements are
> kept in the device registers until a new integration is complete, so if two
> registers are read consecutively within the same integration period they would
> be correlated.

We only usually care about correlation of channels when using the buffered
interface.  There are a few exceptions such as quaternions where they have
no meaning as separate values and where we are computing another value from
the read out of several physical channels (light sensors etc).

> > > +               case PAC1921_CHAN_POWER: {
> > > 
> > > +                       /* Power scale factor in mW:  
> > 
> > I think the scale here should be in microWatts. We have (mA)*(mV)
> >   
> Documentation/ABI/testing/sysfs-bus-iio states it should be in milliwatts:
> 
> What:		/sys/bus/iio/devices/iio:deviceX/in_powerY_raw
> KernelVersion:	4.5
> Contact:	linux-iio@vger.kernel.org
> Description:
> 		Raw (unscaled no bias removal etc.) power measurement from
> 		channel Y. The number must always be specified and
> 		unique to allow association with event codes. Units after
> 		application of scale and offset are milliwatts.

These units tend to be copied from hwmon.  With hindsight we should just
have gone with volts / amps and watts but that's history now.

Jonathan

Re: [PATCH 0/2] iio: adc: add support for pac1921

[Conor Dooley]

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

On Wed, Jul 03, 2024 at 03:34:34PM +0200, Matteo Martelli wrote:
> Add iio driver and DT binding for the Microchip PAC1921 Current/Power
> monitor.
> 
> Implemented most of the features with few limitations listed in the
> driver commit message.
> 
> Tested with a Pine64 host board connected to a PAC1921 click board [1]
> via I2C. The PAC1921 click board embeds the Microchip PAC1921 device
> and a 10 mOhms shunt resistor. The PAC1921 datasheet is at [2].
> 
> [1]: https://www.mikroe.com/pac1921-click
> [2]: https://ww1.microchip.com/downloads/en/DeviceDoc/PAC1921-Data-Sheet-DS20005293E.pdf
> 
> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>

Marius, could you take a look at this driver/binding?

> ---
> Matteo Martelli (2):
>       dt-bindings: iio: adc: add binding for pac1921
>       iio: adc: add support for pac1921
> 
>  .../ABI/testing/sysfs-bus-iio-adc-pac1921          |   45 +
>  .../bindings/iio/adc/microchip,pac1921.yaml        |   79 ++
>  MAINTAINERS                                        |    7 +
>  drivers/iio/adc/Kconfig                            |   10 +
>  drivers/iio/adc/Makefile                           |    1 +
>  drivers/iio/adc/pac1921.c                          | 1033 ++++++++++++++++++++
>  6 files changed, 1175 insertions(+)
> ---
> base-commit: 529d2e1900642eba6df28307e26e19793e227546
> change-id: 20240606-iio-pac1921-77fa0fa3ac11
> 
> Best regards,
> -- 
> Matteo Martelli <matteomartelli3@gmail.com>
> 

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