[PATCH v3 0/2] adding support for Microchip PAC193X Power Monitor

[PATCH v3 0/2] adding support for Microchip PAC193X Power Monitor

[marius.cristea]

From: Marius Cristea <marius.cristea@microchip.com>

Adding support for Microchip PAC193X series of Power Monitor with
Accumulator chip family. This driver covers the following part numbers:
 - PAC1931, PAC1932, PAC1933 and PAC1934

  This device is at the boundary between IIO and HWMON (if you are
looking just at the "shunt resistors, vsense, power, energy"). The
device also has ADC internally that can measure voltages (up to 4
channels) and also currents (up to 4 channels). The current is measured as
voltage across the shunt_resistor.

  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 power consumption of the
processor itself, or a peripheral device, or a battery, this kind of functionality
was requested by our customers).


Differences related to previous patch:

v3:
- this version was sent also to HWMON list
- fix review comments:
  - drop redundant description from device tree bindings
  - reorder "patternProperties:" to follow "properties:" in device tree bindings
  - update comments to proper describe code
  - use numbers instead of defines for clarity in some part of the code
  - use the new "guard(mutex)"
  - use "clamp()" instead of duplicating code
  - remove extra layer of checking in some switch cases
  - use "i2c_get_match_data()"
  - replace while with for loops for the code to look cleaner
  - reverse the logic to reduce indent.
  - add comment related to channels numbering
  - remove memory duplicate when creating dynamic channels
  - add "devm_add_action_or_reset" to handle the "cancel_delayed_work_sync"
  - remove "pac1934_remove()" function

v2:
- fix review comments:
  - change the device tree bindings
  - use label property
  - fix coding style issues
  - remove unused headers
  - use get_unaligned_bexx instead of own functions
  - change to use a system work queue
  - use probe_new instead of old probe

v1:
- first version committed to review

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

 .../ABI/testing/sysfs-bus-iio-adc-pac1934     |   15 +
 .../bindings/iio/adc/microchip,pac1934.yaml   |  137 ++
 MAINTAINERS                                   |    7 +
 drivers/iio/adc/Kconfig                       |   12 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/pac1934.c                     | 1673 +++++++++++++++++
 6 files changed, 1845 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
 create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
 create mode 100644 drivers/iio/adc/pac1934.c


base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec
-- 
2.34.1

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

[marius.cristea]

From: Marius Cristea <marius.cristea@microchip.com>

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

Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
---
 .../bindings/iio/adc/microchip,pac1934.yaml   | 137 ++++++++++++++++++
 1 file changed, 137 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
new file mode 100644
index 000000000000..2609cb19c377
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
@@ -0,0 +1,137 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PAC1934 Power Monitors with Accumulator
+
+maintainers:
+  - Marius Cristea <marius.cristea@microchip.com>
+
+description: |
+  This device is part of the Microchip family of Power Monitors with Accumulator.
+  The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here:
+    https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf
+
+properties:
+  compatible:
+    enum:
+      - microchip,pac1931
+      - microchip,pac1932
+      - microchip,pac1933
+      - microchip,pac1934
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  interrupts:
+    maxItems: 1
+
+  microchip,slow-io:
+    type: boolean
+    description: |
+      A GPIO used to trigger a change is sampling rate (lowering the chip power consumption).
+      If configured in SLOW mode, if this pin is forced high, sampling rate is forced to eight
+      samples/second. When it is forced low, the sampling rate is 1024 samples/second unless
+      a different sample rate has been programmed.
+
+patternProperties:
+  "^channel@[1-4]+$":
+    type: object
+    $ref: adc.yaml
+    description: Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        items:
+          minimum: 1
+          maximum: 4
+
+      shunt-resistor-micro-ohms:
+        description: |
+          Value in micro Ohms of the shunt resistor connected between
+          the SENSE+ and SENSE- inputs, across which the current is measured. Value
+          is needed to compute the scaling of the measured current.
+
+    required:
+      - reg
+      - shunt-resistor-micro-ohms
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: interrupts
+    then:
+      properties:
+        microchip,slow-io: false
+    else:
+      if:
+        properties:
+          compatible:
+            contains:
+              const: microchip,slow-io
+      then:
+        properties:
+          interrupts: false
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pac193x: power-monitor@10 {
+            compatible = "microchip,pac1934";
+            reg = <0x10>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@1 {
+                reg = <0x1>;
+                shunt-resistor-micro-ohms = <24900000>;
+                label = "CPU";
+            };
+
+            channel@2 {
+                reg = <0x2>;
+                shunt-resistor-micro-ohms = <49900000>;
+                label = "GPU";
+            };
+
+            channel@3 {
+                reg = <0x3>;
+                shunt-resistor-micro-ohms = <75000000>;
+                label = "MEM";
+                bipolar;
+            };
+
+            channel@4 {
+                reg = <0x4>;
+                shunt-resistor-micro-ohms = <100000000>;
+                label = "NET";
+                bipolar;
+            };
+        };
+    };
+
+...
-- 
2.34.1

[PATCH v3 2/2] iio: adc: adding support for PAC193x

[marius.cristea]

From: Marius Cristea <marius.cristea@microchip.com>

This is the iio driver for Microchip
PAC193X series of Power Monitor with Accumulator chip family.

Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
---
 .../ABI/testing/sysfs-bus-iio-adc-pac1934     |   15 +
 MAINTAINERS                                   |    7 +
 drivers/iio/adc/Kconfig                       |   12 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/pac1934.c                     | 1673 +++++++++++++++++
 5 files changed, 1708 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
 create mode 100644 drivers/iio/adc/pac1934.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
new file mode 100644
index 000000000000..533429eaac6a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
@@ -0,0 +1,15 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor_X
+KernelVersion:	6.7
+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/reset_accumulators
+KernelVersion:	6.7
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Writing any value resets the accumulated power device's statistics
+		for all enabled channels.
diff --git a/MAINTAINERS b/MAINTAINERS
index 337a4244ee30..c966551604a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14054,6 +14054,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 PAC1934 POWER/ENERGY MONITOR DRIVER
+M:	Marius Cristea <marius.cristea@microchip.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
+F:	drivers/iio/adc/pac1934.c
+
 MICROCHIP PCI1XXXX GP DRIVER
 M:	Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
 M:	Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 35f9867da12c..07faf793795d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -903,6 +903,18 @@ config NPCM_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called npcm_adc.
 
+config PAC1934
+	tristate "Microchip Technology PAC1934 driver"
+	depends on I2C
+	depends on IIO
+	help
+	  Say yes here to build support for Microchip Technology's PAC1931,
+	  PAC1932, PAC1933, PAC1934 Single/Multi-Channel Power Monitor with
+	  Accumulator.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called pac1934.
+
 config PALMAS_GPADC
 	tristate "TI Palmas General Purpose ADC"
 	depends on MFD_PALMAS
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index bee11d442af4..bb066c827e78 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -83,6 +83,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_PAC1934) += pac1934.o
 obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
 obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o
 obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
new file mode 100644
index 000000000000..d2be205c3cb4
--- /dev/null
+++ b/drivers/iio/adc/pac1934.c
@@ -0,0 +1,1673 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IIO driver for PAC1934 Multi-Channel DC Power/Energy Monitor
+ *
+ * Copyright (C) 2017-2023 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Bogdan Bolocan <bogdan.bolocan@microchip.com>
+ * Author: Victor Tudose
+ * Author: Marius Cristea <marius.cristea@microchip.com>
+ *
+ * Datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here:
+ * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf
+ */
+
+#include <linux/acpi.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <asm/unaligned.h>
+
+/*
+ * maximum accumulation time should be (17 * 60 * 1000) around 17 minutes@1024 sps
+ * till PAC1934 accumulation registers starts to saturate
+ */
+#define PAC1934_MAX_RFSH_LIMIT_MS		60000
+/* 50msec is the timeout for validity of the cached registers */
+#define PAC1934_MIN_POLLING_TIME_MS		50
+/*
+ * 1000usec is the minimum wait time for normal conversions when sample
+ * rate doesn't change
+ */
+#define PAC1934_MIN_UPDATE_WAIT_TIME_US		1000
+
+/* 32000mV */
+#define PAC1934_VOLTAGE_MILLIVOLTS_MAX		32000
+/* voltage bits resolution when set for unsigned values */
+#define PAC1934_VOLTAGE_U_RES			16
+/* voltage bits resolution when set for signed values */
+#define PAC1934_VOLTAGE_S_RES			15
+
+/*
+ * max signed value that can be stored on 32 bits and 8 digits fractional value
+ * (2^31 - 1) * 10^8 + 99999999
+ */
+#define PAC_193X_MAX_POWER_ACC			214748364799999999LL
+/*
+ * min signed value that can be stored on 32 bits and 8 digits fractional value
+ * -(2^31) * 10^8 - 99999999
+ */
+#define PAC_193X_MIN_POWER_ACC			-214748364899999999LL
+
+#define PAC1934_MAX_NUM_CHANNELS		4
+#define PAC1934_CH_1				0
+#define PAC1934_CH_2				1
+#define PAC1934_CH_3				2
+#define PAC1934_CH_4				3
+#define PAC1934_MEAS_REG_LEN			76
+#define PAC1934_CTRL_REG_LEN			12
+
+#define PAC1934_DEFAULT_CHIP_SAMP_SPEED		1024
+
+/* I2C address map */
+#define PAC1934_REFRESH_REG_ADDR		0x00
+#define PAC1934_CTRL_REG_ADDR			0x01
+#define PAC1934_ACC_COUNT_REG_ADDR		0x02
+#define PAC1934_VPOWER_ACC_1_ADDR		0x03
+#define PAC1934_VPOWER_ACC_2_ADDR		0x04
+#define PAC1934_VPOWER_ACC_3_ADDR		0x05
+#define PAC1934_VPOWER_ACC_4_ADDR		0x06
+#define PAC1934_VBUS_1_ADDR			0x07
+#define PAC1934_VBUS_2_ADDR			0x08
+#define PAC1934_VBUS_3_ADDR			0x09
+#define PAC1934_VBUS_4_ADDR			0x0A
+#define PAC1934_VSENSE_1_ADDR			0x0B
+#define PAC1934_VSENSE_2_ADDR			0x0C
+#define PAC1934_VSENSE_3_ADDR			0x0D
+#define PAC1934_VSENSE_4_ADDR			0x0E
+#define PAC1934_VBUS_AVG_1_ADDR			0x0F
+#define PAC1934_VBUS_AVG_2_ADDR			0x10
+#define PAC1934_VBUS_AVG_3_ADDR			0x11
+#define PAC1934_VBUS_AVG_4_ADDR			0x12
+#define PAC1934_VSENSE_AVG_1_ADDR		0x13
+#define PAC1934_VSENSE_AVG_2_ADDR		0x14
+#define PAC1934_VSENSE_AVG_3_ADDR		0x15
+#define PAC1934_VSENSE_AVG_4_ADDR		0x16
+#define PAC1934_VPOWER_1_ADDR			0x17
+#define PAC1934_VPOWER_2_ADDR			0x18
+#define PAC1934_VPOWER_3_ADDR			0x19
+#define PAC1934_VPOWER_4_ADDR			0x1A
+#define PAC1934_REFRESH_V_REG_ADDR		0x1F
+#define PAC1934_CTRL_STAT_REGS_ADDR		0x1C
+#define PAC1934_PID_REG_ADDR			0xFD
+#define PAC1934_MID_REG_ADDR			0xFE
+#define PAC1934_RID_REG_ADDR			0xFF
+
+/* PRODUCT ID REGISTER + MANUFACTURER ID REGISTER + REVISION ID REGISTER */
+#define PAC1934_ID_REG_LEN			3
+#define PAC1934_PID_IDX				0
+#define PAC1934_MID_IDX				1
+#define PAC1934_RID_IDX				2
+
+#define PAC1934_ACPI_GET_NAMES_AND_MOHMS_VALS	1
+#define PAC1934_ACPI_GET_UOHMS_VALS		2
+#define PAC1934_ACPI_GET_BIPOLAR_SETTINGS	4
+#define PAC1934_ACPI_GET_SAMP			5
+
+#define PAC1934_SAMPLE_RATE_SHIFT		6
+
+#define PAC1934_VBUS_SENSE_REG_LEN		2
+#define PAC1934_ACC_REG_LEN			3
+#define PAC1934_VPOWER_REG_LEN			4
+#define PAC1934_VPOWER_ACC_REG_LEN		6
+#define PAC1934_MAX_REGISTER_LENGTH		6
+
+#define PAC1934_CUSTOM_ATTR_FOR_CHANNEL		1
+#define PAC1934_SHARED_DEVATTRS_COUNT		1
+
+/*
+ * relative offsets when using multi-byte reads/writes even though these
+ * bytes are read one after the other, they are not at adjacent memory
+ * locations within the I2C memory map. The chip can skip some addresses
+ */
+#define PAC1934_CHANNEL_DIS_REG_OFF		0
+#define PAC1934_NEG_PWR_REG_OFF			1
+
+/*
+ * when reading/writing multiple bytes from offset PAC1934_CHANNEL_DIS_REG_OFF,
+ * the chip jumps over the 0x1E (REFRESH_G) and 0x1F (REFRESH_V) offsets
+ */
+#define PAC1934_SLOW_REG_OFF			2
+#define PAC1934_CTRL_ACT_REG_OFF		3
+#define PAC1934_CHANNEL_DIS_ACT_REG_OFF		4
+#define PAC1934_NEG_PWR_ACT_REG_OFF		5
+#define PAC1934_CTRL_LAT_REG_OFF		6
+#define PAC1934_CHANNEL_DIS_LAT_REG_OFF		7
+#define PAC1934_NEG_PWR_LAT_REG_OFF		8
+#define PAC1934_PID_REG_OFF			9
+#define PAC1934_MID_REG_OFF			10
+#define PAC1934_REV_REG_OFF			11
+#define PAC1934_CTRL_STATUS_INFO_LEN		12
+
+#define PAC1934_MID				0x5D
+#define PAC1931_PID				0x58
+#define PAC1932_PID				0x59
+#define PAC1933_PID				0x5A
+#define PAC1934_PID				0x5B
+
+/* Scale constant = (10^3 * 3.2 * 10^9 / 2^28) for mili Watt-second */
+#define PAC1934_SCALE_CONSTANT			11921
+
+#define PAC1934_MAX_VPOWER_RSHIFTED_BY_28B	11921
+#define PAC1934_MAX_VSENSE_RSHIFTED_BY_16B	1525
+
+#define PAC1934_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
+
+#define PAC1934_CRTL_SAMPLE_RATE_MASK	GENMASK(7, 6)
+#define PAC1934_CHAN_SLEEP_MASK		BIT(5)
+#define PAC1934_CHAN_SLEEP_SET		BIT(5)
+#define PAC1934_CHAN_SINGLE_MASK	BIT(4)
+#define PAC1934_CHAN_SINGLE_SHOT_SET	BIT(4)
+#define PAC1934_CHAN_ALERT_MASK		BIT(3)
+#define PAC1934_CHAN_ALERT_EN		BIT(3)
+#define PAC1934_CHAN_ALERT_CC_MASK	BIT(2)
+#define PAC1934_CHAN_ALERT_CC_EN	BIT(2)
+#define PAC1934_CHAN_OVF_ALERT_MASK	BIT(1)
+#define PAC1934_CHAN_OVF_ALERT_EN	BIT(1)
+#define PAC1934_CHAN_OVF_MASK		BIT(0)
+
+#define PAC1934_CHAN_DIS_CH1_OFF_MASK	BIT(7)
+#define PAC1934_CHAN_DIS_CH2_OFF_MASK	BIT(6)
+#define PAC1934_CHAN_DIS_CH3_OFF_MASK	BIT(5)
+#define PAC1934_CHAN_DIS_CH4_OFF_MASK	BIT(4)
+#define PAC1934_SMBUS_TIMEOUT_MASK	BIT(3)
+#define PAC1934_SMBUS_BYTECOUNT_MASK	BIT(2)
+#define PAC1934_SMBUS_NO_SKIP_MASK	BIT(1)
+
+#define PAC1934_NEG_PWR_CH1_BIDI_MASK	BIT(7)
+#define PAC1934_NEG_PWR_CH2_BIDI_MASK	BIT(6)
+#define PAC1934_NEG_PWR_CH3_BIDI_MASK	BIT(5)
+#define PAC1934_NEG_PWR_CH4_BIDI_MASK	BIT(4)
+#define PAC1934_NEG_PWR_CH1_BIDV_MASK	BIT(3)
+#define PAC1934_NEG_PWR_CH2_BIDV_MASK	BIT(2)
+#define PAC1934_NEG_PWR_CH3_BIDV_MASK	BIT(1)
+#define PAC1934_NEG_PWR_CH4_BIDV_MASK	BIT(0)
+
+/*
+ * Universal Unique Identifier (UUID),
+ * 033771E0-1705-47B4-9535-D1BBE14D9A09, is
+ * reserved to Microchip for the PAC1934 and must not be changed
+ */
+#define PAC1934_DSM_UUID		"033771E0-1705-47B4-9535-D1BBE14D9A09"
+
+enum pac1934_ids {
+	PAC1931,
+	PAC1932,
+	PAC1933,
+	PAC1934
+};
+
+enum pac1934_samps {
+	PAC1934_SAMP_1024SPS,
+	PAC1934_SAMP_256SPS,
+	PAC1934_SAMP_64SPS,
+	PAC1934_SAMP_8SPS
+};
+
+/*
+ * these indexes are exactly describing the element order within a single
+ * PAC1934 phys channel IIO channel descriptor; see the static const struct
+ * iio_chan_spec pac1934_single_channel[] declaration
+ */
+enum pac1934_ch_idx {
+	IIO_EN,
+	IIO_POW,
+	IIO_VOLT,
+	IIO_CRT,
+	IIO_VOLTAVG,
+	IIO_CRTAVG
+};
+
+/**
+ * struct pac1934_features - features of a pac1934 instance
+ * @phys_channels:	number of physical channels supported by the chip
+ * @name:		chip's name
+ */
+struct pac1934_features {
+	u8		phys_channels;
+	const char	*name;
+};
+
+struct samp_rate_mapping {
+	u16 samp_rate;
+	u8 shift2value;
+};
+
+static const unsigned int samp_rate_map_tbl[] = {
+	[PAC1934_SAMP_1024SPS] = 1024,
+	[PAC1934_SAMP_256SPS] = 256,
+	[PAC1934_SAMP_64SPS] = 64,
+	[PAC1934_SAMP_8SPS] = 8,
+};
+
+static const struct pac1934_features pac1934_chip_config[] = {
+	[PAC1931] = {
+	    .phys_channels = 1,
+	    .name = "pac1931",
+	},
+	[PAC1932] = {
+	    .phys_channels = 2,
+	    .name = "pac1932",
+	},
+	[PAC1933] = {
+	    .phys_channels = 3,
+	    .name = "pac1933",
+	},
+	[PAC1934] = {
+	    .phys_channels = 4,
+	    .name = "pac1934",
+	},
+};
+
+/**
+ * struct reg_data - data from the registers
+ * @meas_regs:			snapshot of raw measurements registers
+ * @ctrl_regs:			snapshot of control registers
+ * @energy_sec_acc:		snapshot of energy values
+ * @vpower_acc:			accumulated vpower values
+ * @vpower:			snapshot of vpower registers
+ * @vbus:			snapshot of vbus registers
+ * @vbus_avg:			averages of vbus registers
+ * @vsense:			snapshot of vsense registers
+ * @vsense_avg:			averages of vsense registers
+ * @num_enabled_channels:	count of how many chip channels are currently enabled
+ */
+struct reg_data {
+	u8	meas_regs[PAC1934_MEAS_REG_LEN];
+	u8	ctrl_regs[PAC1934_CTRL_REG_LEN];
+	s64	energy_sec_acc[PAC1934_MAX_NUM_CHANNELS];
+	s64	vpower_acc[PAC1934_MAX_NUM_CHANNELS];
+	s32	vpower[PAC1934_MAX_NUM_CHANNELS];
+	s32	vbus[PAC1934_MAX_NUM_CHANNELS];
+	s32	vbus_avg[PAC1934_MAX_NUM_CHANNELS];
+	s32	vsense[PAC1934_MAX_NUM_CHANNELS];
+	s32	vsense_avg[PAC1934_MAX_NUM_CHANNELS];
+	u8	num_enabled_channels;
+};
+
+/**
+ * struct pac1934_chip_info - information about the chip
+ * @client:			the i2c-client attached to the device
+ * @lock:			synchronize access to driver's state members
+ * @work_chip_rfsh:		work queue used for refresh commands
+ * @phys_channels:		phys channels count
+ * @active_channels:		array of values, true means that channel is active
+ * @bi_dir:			array of bools, true means that channel is bidirectional
+ * @chip_variant:		chip variant
+ * @chip_revision:		chip revision
+ * @shunts:			shunts
+ * @chip_reg_data:		chip reg data
+ * @sample_rate_value:		sampling frequency
+ * @labels:			table with channels labels
+ * @pac1934_info:		pac1934_info
+ * @crt_samp_spd_bitfield:	the current sampling speed
+ * @jiffies_tstamp:		chip's uptime
+ */
+struct pac1934_chip_info {
+	struct i2c_client	*client;
+	struct mutex		lock; /* synchronize access to driver's state members */
+	struct delayed_work	work_chip_rfsh;
+	u8			phys_channels;
+	bool			active_channels[PAC1934_MAX_NUM_CHANNELS];
+	bool			bi_dir[PAC1934_MAX_NUM_CHANNELS];
+	u8			chip_variant;
+	u8			chip_revision;
+	u32			shunts[PAC1934_MAX_NUM_CHANNELS];
+	struct reg_data		chip_reg_data;
+	s32			sample_rate_value;
+	char			*labels[PAC1934_MAX_NUM_CHANNELS];
+	struct iio_info		pac1934_info;
+	u8			crt_samp_spd_bitfield;
+	unsigned long		jiffies_tstamp;
+};
+
+#define TO_PAC1934_CHIP_INFO(d) container_of(d, struct pac1934_chip_info, work_chip_rfsh)
+
+#define PAC1934_VPOWER_ACC_CHANNEL(_index, _si, _address) {			\
+	.type = IIO_ENERGY,							\
+	.address = (_address),							\
+	.indexed = 1,								\
+	.channel = (_index),							\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)	|			\
+			      BIT(IIO_CHAN_INFO_SCALE),				\
+	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = (_si),							\
+	.scan_type = {								\
+		.sign = 'u',							\
+		.realbits = 48,							\
+		.storagebits = 48,						\
+		.endianness = IIO_CPU,						\
+	}									\
+}
+
+#define PAC1934_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),				\
+	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = (_si),							\
+	.scan_type = {								\
+		.sign = 'u',							\
+		.realbits = 16,							\
+		.storagebits = 16,						\
+		.endianness = IIO_CPU,						\
+	}									\
+}
+
+#define PAC1934_VBUS_AVG_CHANNEL(_index, _si, _address) {			\
+	.type = IIO_VOLTAGE,							\
+	.address = (_address),							\
+	.indexed = 1,								\
+	.channel = (_index),							\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_AVERAGE_RAW)	|		\
+			      BIT(IIO_CHAN_INFO_SCALE),				\
+	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = (_si),							\
+	.scan_type = {								\
+		.sign = 'u',							\
+		.realbits = 16,							\
+		.storagebits = 16,						\
+		.endianness = IIO_CPU,						\
+	}									\
+}
+
+#define PAC1934_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),				\
+	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = (_si),							\
+	.scan_type = {								\
+		.sign = 'u',							\
+		.realbits = 16,							\
+		.storagebits = 16,						\
+		.endianness = IIO_CPU,						\
+	}									\
+}
+
+#define PAC1934_VSENSE_AVG_CHANNEL(_index, _si, _address) {			\
+	.type = IIO_CURRENT,							\
+	.address = (_address),							\
+	.indexed = 1,								\
+	.channel = (_index),							\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_AVERAGE_RAW)	|		\
+			      BIT(IIO_CHAN_INFO_SCALE),				\
+	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = (_si),							\
+	.scan_type = {								\
+		.sign = 'u',							\
+		.realbits = 16,							\
+		.storagebits = 16,						\
+		.endianness = IIO_CPU,						\
+	}									\
+}
+
+#define PAC1934_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),				\
+	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = (_si),							\
+	.scan_type = {								\
+		.sign = 'u',							\
+		.realbits = 28,							\
+		.storagebits = 32,						\
+		.shift = 4,							\
+		.endianness = IIO_CPU,						\
+	}									\
+}
+
+static const struct iio_chan_spec pac1934_single_channel[] = {
+	PAC1934_VPOWER_ACC_CHANNEL(0, 0, PAC1934_VPOWER_ACC_1_ADDR),
+	PAC1934_VPOWER_CHANNEL(0, 0, PAC1934_VPOWER_1_ADDR),
+	PAC1934_VBUS_CHANNEL(0, 0, PAC1934_VBUS_1_ADDR),
+	PAC1934_VSENSE_CHANNEL(0, 0, PAC1934_VSENSE_1_ADDR),
+	PAC1934_VBUS_AVG_CHANNEL(0, 0, PAC1934_VBUS_AVG_1_ADDR),
+	PAC1934_VSENSE_AVG_CHANNEL(0, 0, PAC1934_VSENSE_AVG_1_ADDR),
+};
+
+/* Low-level I2c functions used to transfer up to 76 bytes at once */
+static int pac1934_i2c_read(struct i2c_client *client, u8 reg_addr, void *databuf, u8 len)
+{
+	int ret;
+	struct i2c_msg msgs[2] = {
+		{ .addr = client->addr,
+		 .len = 1,
+		 .buf = (u8 *)&reg_addr,
+		},
+		{ .addr = client->addr,
+		 .len = len,
+		 .buf = databuf,
+		 .flags = I2C_M_RD
+		}
+	};
+
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int pac1934_match_samp_rate(struct pac1934_chip_info *info, u32 new_samp_rate)
+{
+	int cnt;
+
+	for (cnt = 0; cnt < ARRAY_SIZE(samp_rate_map_tbl); cnt++) {
+		if (new_samp_rate == samp_rate_map_tbl[cnt]) {
+			info->crt_samp_spd_bitfield = cnt;
+			return 0;
+		}
+	}
+
+	/* not a valid sample rate value */
+	return -EINVAL;
+}
+
+static ssize_t shunt_value_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct pac1934_chip_info *info = iio_priv(indio_dev);
+	int len = 0;
+	int target = (int)(attr->attr.name[strlen(attr->attr.name) - 1] - '0') - 1;
+
+	len += sprintf(buf, "%u\n", info->shunts[target]);
+
+	return len;
+}
+
+static ssize_t 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 pac1934_chip_info *info = iio_priv(indio_dev);
+	int sh_val, target;
+
+	target = (int)(attr->attr.name[strlen(attr->attr.name) - 1] - '0') - 1;
+	if (kstrtouint(buf, 10, &sh_val)) {
+		dev_err(dev, "Shunt value is not valid\n");
+		return -EINVAL;
+	}
+
+	scoped_guard(mutex, &info->lock)
+		info->shunts[target] = sh_val;
+
+	return count;
+}
+
+static int pac1934_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *channel,
+			      const int **vals, int *type, int *length, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*type = IIO_VAL_INT;
+		*vals = samp_rate_map_tbl;
+		*length = ARRAY_SIZE(samp_rate_map_tbl);
+		return IIO_AVAIL_LIST;
+	}
+
+	return -EINVAL;
+}
+
+static int pac1934_send_refresh(struct pac1934_chip_info *info,
+				u8 refresh_cmd, u32 wait_time)
+{
+	/* this function only sends REFRESH or REFRESH_V */
+	struct i2c_client *client = info->client;
+	int ret;
+	u8 bidir_reg;
+	bool revision_bug = false;
+
+	if (info->chip_revision == 2 || info->chip_revision == 3) {
+		/*
+		 * chip rev 2 and 3 bug workaround
+		 * see: PAC1934 Family Data Sheet Errata DS80000836A.pdf
+		 */
+		revision_bug = true;
+
+		bidir_reg =
+			FIELD_PREP(PAC1934_NEG_PWR_CH1_BIDI_MASK, info->bi_dir[PAC1934_CH_1]) |
+			FIELD_PREP(PAC1934_NEG_PWR_CH2_BIDI_MASK, info->bi_dir[PAC1934_CH_2]) |
+			FIELD_PREP(PAC1934_NEG_PWR_CH3_BIDI_MASK, info->bi_dir[PAC1934_CH_3]) |
+			FIELD_PREP(PAC1934_NEG_PWR_CH4_BIDI_MASK, info->bi_dir[PAC1934_CH_4]) |
+			FIELD_PREP(PAC1934_NEG_PWR_CH1_BIDV_MASK, info->bi_dir[PAC1934_CH_1]) |
+			FIELD_PREP(PAC1934_NEG_PWR_CH2_BIDV_MASK, info->bi_dir[PAC1934_CH_2]) |
+			FIELD_PREP(PAC1934_NEG_PWR_CH3_BIDV_MASK, info->bi_dir[PAC1934_CH_3]) |
+			FIELD_PREP(PAC1934_NEG_PWR_CH4_BIDV_MASK, info->bi_dir[PAC1934_CH_4]);
+
+		ret = i2c_smbus_write_byte_data(client,
+						PAC1934_CTRL_STAT_REGS_ADDR +
+						PAC1934_NEG_PWR_REG_OFF,
+						bidir_reg);
+		if (ret)
+			return ret;
+	}
+
+	ret = i2c_smbus_write_byte(client, refresh_cmd);
+	if (ret) {
+		dev_err(&client->dev, "%s - cannot send 0x%02X\n",
+			__func__, refresh_cmd);
+		return ret;
+	}
+
+	if (revision_bug) {
+		/*
+		 * chip rev 2 and 3 bug workaround - write again the same register
+		 * write the updated registers back
+		 */
+		ret = i2c_smbus_write_byte_data(client,
+						PAC1934_CTRL_STAT_REGS_ADDR +
+						PAC1934_NEG_PWR_REG_OFF, bidir_reg);
+		if (ret)
+			return ret;
+	}
+
+	/* register data retrieval timestamp */
+	info->jiffies_tstamp = jiffies;
+
+	/* wait till the data is available */
+	usleep_range(wait_time, wait_time + 100);
+
+	return ret;
+}
+
+static int pac1934_reg_snapshot(struct pac1934_chip_info *info,
+				bool do_refresh, u8 refresh_cmd, u32 wait_time)
+{
+	int ret;
+	struct i2c_client *client = info->client;
+	u8 samp_shift, ctrl_regs_tmp;
+	u8 *offset_reg_data_p;
+	u16 tmp_value;
+	u32 samp_rate, cnt, tmp;
+	s64 curr_energy, inc;
+	u64 tmp_energy;
+	struct reg_data *reg_data;
+
+	guard(mutex)(&info->lock);
+
+	if (do_refresh) {
+		ret = pac1934_send_refresh(info, refresh_cmd, wait_time);
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"%s - cannot send refresh\n",
+				__func__);
+			return ret;
+		}
+	}
+
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
+					    PAC1934_CTRL_REG_LEN,
+					    (u8 *)info->chip_reg_data.ctrl_regs);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"%s - cannot read ctrl/status registers\n",
+			__func__);
+		return ret;
+	}
+
+	reg_data = &info->chip_reg_data;
+
+	/* read the data registers */
+	ret = pac1934_i2c_read(client, PAC1934_ACC_COUNT_REG_ADDR,
+			       (u8 *)reg_data->meas_regs, PAC1934_MEAS_REG_LEN);
+	if (ret) {
+		dev_err(&client->dev,
+			"%s - cannot read ACC_COUNT register: %d:%d\n",
+			__func__, ret, PAC1934_MEAS_REG_LEN);
+		return ret;
+	}
+
+	/* see how much shift is required by the sample rate */
+	samp_rate = samp_rate_map_tbl[((reg_data->ctrl_regs[PAC1934_CTRL_LAT_REG_OFF]) >> 6)];
+	samp_shift = get_count_order(samp_rate);
+
+	ctrl_regs_tmp = reg_data->ctrl_regs[PAC1934_CHANNEL_DIS_LAT_REG_OFF];
+	offset_reg_data_p = &reg_data->meas_regs[PAC1934_ACC_REG_LEN];
+
+	/* start with VPOWER_ACC */
+	for (cnt = 0; cnt < info->phys_channels; cnt++) {
+		/* check if the channel is active, skip all fields if disabled */
+		if ((ctrl_regs_tmp << cnt) & 0x80)
+			continue;
+
+		curr_energy = info->chip_reg_data.energy_sec_acc[cnt];
+
+		tmp_energy = get_unaligned_be48(offset_reg_data_p);
+
+		if (info->bi_dir[cnt])
+			reg_data->vpower_acc[cnt] = sign_extend64(tmp_energy, 47);
+		else
+			reg_data->vpower_acc[cnt] = tmp_energy;
+
+		/*
+		 * compute the scaled to 1 second accumulated energy value;
+		 * energy accumulator scaled to 1sec = VPOWER_ACC/2^samp_shift
+		 * the chip's sampling rate is 2^samp_shift samples/sec
+		 */
+		inc = (reg_data->vpower_acc[cnt] >> samp_shift);
+
+		/* add the power_acc field */
+		curr_energy += inc;
+
+		clamp(curr_energy, PAC_193X_MIN_POWER_ACC, PAC_193X_MAX_POWER_ACC);
+
+		reg_data->energy_sec_acc[cnt] = curr_energy;
+
+		offset_reg_data_p += PAC1934_VPOWER_ACC_REG_LEN;
+	}
+
+	/* continue with VBUS */
+	for (cnt = 0; cnt < info->phys_channels; cnt++) {
+		if ((ctrl_regs_tmp << cnt) & 0x80)
+			continue;
+
+		tmp_value = get_unaligned_be16(offset_reg_data_p);
+
+		if (info->bi_dir[cnt])
+			reg_data->vbus[cnt] = sign_extend32((u32)(tmp_value), 15);
+		else
+			reg_data->vbus[cnt] = tmp_value;
+
+		offset_reg_data_p += PAC1934_VBUS_SENSE_REG_LEN;
+	}
+
+	/* VSENSE */
+	for (cnt = 0; cnt < info->phys_channels; cnt++) {
+		if ((ctrl_regs_tmp << cnt) & 0x80)
+			continue;
+
+		tmp_value = get_unaligned_be16(offset_reg_data_p);
+
+		if (info->bi_dir[cnt])
+			reg_data->vsense[cnt] = sign_extend32((u32)(tmp_value), 15);
+		else
+			reg_data->vsense[cnt] = tmp_value;
+
+		offset_reg_data_p += PAC1934_VBUS_SENSE_REG_LEN;
+	}
+
+	/* VBUS_AVG */
+	for (cnt = 0; cnt < info->phys_channels; cnt++) {
+		if ((ctrl_regs_tmp << cnt) & 0x80)
+			continue;
+
+		tmp_value = get_unaligned_be16(offset_reg_data_p);
+
+		if (info->bi_dir[cnt])
+			reg_data->vbus_avg[cnt] = sign_extend32((u32)(tmp_value), 15);
+		else
+			reg_data->vbus_avg[cnt] = tmp_value;
+
+		offset_reg_data_p += PAC1934_VBUS_SENSE_REG_LEN;
+	}
+
+	/* VSENSE_AVG */
+	for (cnt = 0; cnt < info->phys_channels; cnt++) {
+		if ((ctrl_regs_tmp << cnt) & 0x80)
+			continue;
+
+		tmp_value = get_unaligned_be16(offset_reg_data_p);
+
+		if (info->bi_dir[cnt])
+			reg_data->vsense_avg[cnt] = sign_extend32((u32)(tmp_value), 15);
+		else
+			reg_data->vsense_avg[cnt] = tmp_value;
+
+		offset_reg_data_p += PAC1934_VBUS_SENSE_REG_LEN;
+	}
+
+	/* VPOWER */
+	for (cnt = 0; cnt < info->phys_channels; cnt++) {
+		if ((ctrl_regs_tmp << cnt) & 0x80)
+			continue;
+
+		tmp = get_unaligned_be32(offset_reg_data_p) >> 4;
+
+		if (info->bi_dir[cnt])
+			reg_data->vpower[cnt] = sign_extend32(tmp, 27);
+		else
+			reg_data->vpower[cnt] = tmp;
+
+		offset_reg_data_p += PAC1934_VPOWER_REG_LEN;
+	}
+
+	return 0;
+}
+
+static int pac1934_retrieve_data(struct pac1934_chip_info *info,
+				 u32 wait_time)
+{
+	int ret = 0;
+
+	/*
+	 * 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->jiffies_tstamp +
+					 msecs_to_jiffies(PAC1934_MIN_POLLING_TIME_MS))) {
+		ret = pac1934_reg_snapshot(info, true, PAC1934_REFRESH_REG_ADDR,
+					   wait_time);
+
+		/*
+		 * Re-schedule the work for the read registers on timeout
+		 * (to prevent chip regs saturation)
+		 */
+		cancel_delayed_work_sync(&info->work_chip_rfsh);
+		schedule_delayed_work(&info->work_chip_rfsh,
+				      msecs_to_jiffies(PAC1934_MAX_RFSH_LIMIT_MS));
+	}
+
+	return ret;
+}
+
+static int pac1934_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct pac1934_chip_info *info = iio_priv(indio_dev);
+	s64 curr_energy;
+	int ret, channel = chan->channel - 1;
+
+	ret = pac1934_retrieve_data(info, PAC1934_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[channel];
+			return IIO_VAL_INT;
+		case IIO_CURRENT:
+			*val = info->chip_reg_data.vsense[channel];
+			return IIO_VAL_INT;
+		case IIO_POWER:
+			*val = info->chip_reg_data.vpower[channel];
+			return IIO_VAL_INT;
+		case IIO_ENERGY:
+			curr_energy = info->chip_reg_data.energy_sec_acc[channel];
+			*val = (u32)curr_energy;
+			*val2 = (u32)(curr_energy >> 32);
+			return IIO_VAL_INT_64;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_CHAN_INFO_AVERAGE_RAW:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = info->chip_reg_data.vbus_avg[channel];
+			return IIO_VAL_INT;
+		case IIO_CURRENT:
+			*val = info->chip_reg_data.vsense_avg[channel];
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->address) {
+		/* Voltages - scale for millivolts */
+		case PAC1934_VBUS_1_ADDR:
+		case PAC1934_VBUS_2_ADDR:
+		case PAC1934_VBUS_3_ADDR:
+		case PAC1934_VBUS_4_ADDR:
+		case PAC1934_VBUS_AVG_1_ADDR:
+		case PAC1934_VBUS_AVG_2_ADDR:
+		case PAC1934_VBUS_AVG_3_ADDR:
+		case PAC1934_VBUS_AVG_4_ADDR:
+			*val = PAC1934_VOLTAGE_MILLIVOLTS_MAX;
+			if (chan->scan_type.sign == 'u')
+				*val2 = PAC1934_VOLTAGE_U_RES;
+			else
+				*val2 = PAC1934_VOLTAGE_S_RES;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		/*
+		 * Currents - scale for mA - depends on the
+		 * channel's shunt value
+		 * (100mV * 1000000) / (2^16 * shunt(uohm))
+		 */
+		case PAC1934_VSENSE_1_ADDR:
+		case PAC1934_VSENSE_2_ADDR:
+		case PAC1934_VSENSE_3_ADDR:
+		case PAC1934_VSENSE_4_ADDR:
+		case PAC1934_VSENSE_AVG_1_ADDR:
+		case PAC1934_VSENSE_AVG_2_ADDR:
+		case PAC1934_VSENSE_AVG_3_ADDR:
+		case PAC1934_VSENSE_AVG_4_ADDR:
+			*val = PAC1934_MAX_VSENSE_RSHIFTED_BY_16B;
+			if (chan->scan_type.sign == 'u')
+				*val2 = info->shunts[channel];
+			else
+				*val2 = info->shunts[channel] >> 1;
+			return IIO_VAL_FRACTIONAL;
+		/*
+		 * Power - uW - it will use the combined scale
+		 * for current and voltage
+		 * current(mA) * voltage(mV) = power (uW)
+		 */
+		case PAC1934_VPOWER_1_ADDR:
+		case PAC1934_VPOWER_2_ADDR:
+		case PAC1934_VPOWER_3_ADDR:
+		case PAC1934_VPOWER_4_ADDR:
+			*val = PAC1934_MAX_VPOWER_RSHIFTED_BY_28B;
+			if (chan->scan_type.sign == 'u')
+				*val2 = info->shunts[channel];
+			else
+				*val2 = info->shunts[channel] >> 1;
+			return IIO_VAL_FRACTIONAL;
+		case PAC1934_VPOWER_ACC_1_ADDR:
+		case PAC1934_VPOWER_ACC_2_ADDR:
+		case PAC1934_VPOWER_ACC_3_ADDR:
+		case PAC1934_VPOWER_ACC_4_ADDR:
+			/*
+			 * expresses the 32 bit scale value
+			 * here compute the scale for energy (miliWatt-second or miliJoule)
+			 */
+			*val = PAC1934_SCALE_CONSTANT;
+
+			if (chan->scan_type.sign == 'u')
+				*val2 = info->shunts[channel];
+			else
+				*val2 = info->shunts[channel] >> 1;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = info->sample_rate_value;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static int pac1934_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct pac1934_chip_info *info = iio_priv(indio_dev);
+	struct i2c_client *client = info->client;
+	int ret = -EINVAL;
+	s32 old_samp_rate;
+	u8 ctrl_reg;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = pac1934_match_samp_rate(info, val);
+		if (ret)
+			return ret;
+
+		old_samp_rate = info->sample_rate_value;
+		info->sample_rate_value = val;
+
+		/* write the new sampling value and trigger a snapshot(incl refresh) */
+		scoped_guard(mutex, &info->lock) {
+			ctrl_reg = FIELD_PREP(PAC1934_CRTL_SAMPLE_RATE_MASK,
+					      info->crt_samp_spd_bitfield);
+			ret = i2c_smbus_write_byte_data(client, PAC1934_CTRL_REG_ADDR, ctrl_reg);
+			if (ret) {
+				dev_err(&client->dev,
+					"%s - can't update sample rate\n",
+					__func__);
+				info->sample_rate_value = old_samp_rate;
+				return ret;
+			}
+		}
+
+		/*
+		 * 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->jiffies_tstamp -=
+			msecs_to_jiffies(PAC1934_MIN_POLLING_TIME_MS);
+		ret = pac1934_retrieve_data(info, (1024 / old_samp_rate) * 1000);
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"%s - cannot snapshot ctrl and measurement regs\n",
+				__func__);
+			return ret;
+		}
+
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int pac1934_read_label(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, char *label)
+{
+	struct pac1934_chip_info *info = iio_priv(indio_dev);
+
+	switch (chan->address) {
+	case PAC1934_VBUS_1_ADDR:
+	case PAC1934_VBUS_2_ADDR:
+	case PAC1934_VBUS_3_ADDR:
+	case PAC1934_VBUS_4_ADDR:
+		return sprintf(label, "%s_VBUS_%d\n",
+			       info->labels[chan->scan_index],
+			       chan->scan_index + 1);
+	case PAC1934_VBUS_AVG_1_ADDR:
+	case PAC1934_VBUS_AVG_2_ADDR:
+	case PAC1934_VBUS_AVG_3_ADDR:
+	case PAC1934_VBUS_AVG_4_ADDR:
+		return sprintf(label, "%s_VBUS_AVG_%d\n",
+			       info->labels[chan->scan_index],
+			       chan->scan_index + 1);
+	case PAC1934_VSENSE_1_ADDR:
+	case PAC1934_VSENSE_2_ADDR:
+	case PAC1934_VSENSE_3_ADDR:
+	case PAC1934_VSENSE_4_ADDR:
+		return sprintf(label, "%s_IBUS_%d\n",
+			       info->labels[chan->scan_index],
+			       chan->scan_index + 1);
+	case PAC1934_VSENSE_AVG_1_ADDR:
+	case PAC1934_VSENSE_AVG_2_ADDR:
+	case PAC1934_VSENSE_AVG_3_ADDR:
+	case PAC1934_VSENSE_AVG_4_ADDR:
+		return sprintf(label, "%s_IBUS_AVG_%d\n",
+			       info->labels[chan->scan_index],
+			       chan->scan_index + 1);
+	case PAC1934_VPOWER_1_ADDR:
+	case PAC1934_VPOWER_2_ADDR:
+	case PAC1934_VPOWER_3_ADDR:
+	case PAC1934_VPOWER_4_ADDR:
+		return sprintf(label, "%s_POWER_%d\n",
+			       info->labels[chan->scan_index],
+			       chan->scan_index + 1);
+	case PAC1934_VPOWER_ACC_1_ADDR:
+	case PAC1934_VPOWER_ACC_2_ADDR:
+	case PAC1934_VPOWER_ACC_3_ADDR:
+	case PAC1934_VPOWER_ACC_4_ADDR:
+		return sprintf(label, "%s_ENERGY_%d\n",
+			       info->labels[chan->scan_index],
+			       chan->scan_index + 1);
+	}
+
+	return 0;
+}
+
+static void pac1934_work_periodic_rfsh(struct work_struct *work)
+{
+	struct pac1934_chip_info *info = TO_PAC1934_CHIP_INFO((struct delayed_work *)work);
+	struct i2c_client *client = info->client;
+
+	dev_dbg(&client->dev, "%s - Periodic refresh\n", __func__);
+
+	/* do a REFRESH, then read */
+	pac1934_reg_snapshot(info, true, PAC1934_REFRESH_REG_ADDR,
+			     PAC1934_MIN_UPDATE_WAIT_TIME_US);
+
+	schedule_delayed_work(&info->work_chip_rfsh,
+			      msecs_to_jiffies(PAC1934_MAX_RFSH_LIMIT_MS));
+}
+
+static int pac1934_read_revision(struct pac1934_chip_info *info, u8 *buf)
+{
+	int ret;
+	struct i2c_client *client = info->client;
+
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1934_PID_REG_ADDR,
+					    PAC1934_ID_REG_LEN,
+					    buf);
+	if (ret < 0) {
+		dev_err(&client->dev, "cannot read revision\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pac1934_chip_identify(struct pac1934_chip_info *info)
+{
+	u8 rev_info[PAC1934_ID_REG_LEN];
+	struct i2c_client *client = info->client;
+	int ret = 0;
+
+	ret = pac1934_read_revision(info, (u8 *)rev_info);
+	if (ret)
+		return ret;
+
+	info->chip_variant = rev_info[PAC1934_PID_IDX];
+	info->chip_revision = rev_info[PAC1934_RID_IDX];
+
+	dev_dbg(&client->dev, "Chip variant: 0x%02X\n", info->chip_variant);
+	dev_dbg(&client->dev, "Chip revision: 0x%02X\n", info->chip_revision);
+
+	switch (info->chip_variant) {
+	case PAC1934_PID:
+		return PAC1934;
+	case PAC1933_PID:
+		return PAC1933;
+	case PAC1932_PID:
+		return PAC1932;
+	case PAC1931_PID:
+		return PAC1931;
+	default:
+		return -EINVAL;
+	}
+}
+
+/*
+ * documentation related to the ACPI device definition
+ * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC1934-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
+ */
+static bool pac1934_acpi_parse_channel_config(struct i2c_client *client,
+					      struct pac1934_chip_info *info)
+{
+	acpi_handle handle;
+	union acpi_object *rez;
+	struct device *dev = &client->dev;
+	unsigned short bi_dir_mask;
+	int idx, i;
+	guid_t guid;
+	const struct acpi_device_id *id;
+
+	handle = ACPI_HANDLE(&client->dev);
+
+	id = acpi_match_device(dev->driver->acpi_match_table, dev);
+	if (!id)
+		return false;
+
+	guid_parse(PAC1934_DSM_UUID, &guid);
+
+	rez = acpi_evaluate_dsm(handle, &guid, 0, PAC1934_ACPI_GET_NAMES_AND_MOHMS_VALS, NULL);
+	if (!rez)
+		return false;
+
+	for (i = 0; i < rez->package.count; i += 2) {
+		idx = i / 2;
+		info->labels[idx] =
+			devm_kmemdup(&client->dev, rez->package.elements[i].string.pointer,
+				     (size_t)rez->package.elements[i].string.length + 1,
+				     GFP_KERNEL);
+		info->labels[idx][rez->package.elements[i].string.length] = '\0';
+		info->shunts[idx] =
+			rez->package.elements[i + 1].integer.value * 1000;
+		info->active_channels[idx] = (info->shunts[idx] != 0);
+	}
+
+	kfree(rez);
+
+	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1934_ACPI_GET_UOHMS_VALS, NULL);
+	if (!rez) {
+		/*
+		 * initializing with default values
+		 * we assume all channels are unidirectional(the mask is zero)
+		 * and assign the default sampling rate
+		 */
+		info->sample_rate_value = PAC1934_DEFAULT_CHIP_SAMP_SPEED;
+		return true;
+	}
+
+	for (i = 0; i < rez->package.count; i++) {
+		idx = i;
+		info->shunts[idx] = rez->package.elements[i].integer.value;
+		info->active_channels[idx] = (info->shunts[idx] != 0);
+	}
+
+	kfree(rez);
+
+	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1934_ACPI_GET_BIPOLAR_SETTINGS, NULL);
+	if (!rez)
+		return false;
+
+	bi_dir_mask = rez->package.elements[0].integer.value;
+	info->bi_dir[0] = ((bi_dir_mask & (1 << 3)) | (bi_dir_mask & (1 << 7))) != 0;
+	info->bi_dir[1] = ((bi_dir_mask & (1 << 2)) | (bi_dir_mask & (1 << 6))) != 0;
+	info->bi_dir[2] = ((bi_dir_mask & (1 << 1)) | (bi_dir_mask & (1 << 5))) != 0;
+	info->bi_dir[3] = ((bi_dir_mask & (1 << 0)) | (bi_dir_mask & (1 << 4))) != 0;
+
+	kfree(rez);
+
+	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1934_ACPI_GET_SAMP, NULL);
+	if (!rez)
+		return false;
+
+	info->sample_rate_value = rez->package.elements[0].integer.value;
+
+	kfree(rez);
+
+	return true;
+}
+
+static bool pac1934_of_parse_channel_config(struct i2c_client *client,
+					    struct pac1934_chip_info *info)
+{
+	struct fwnode_handle *node, *fwnode;
+	unsigned int current_channel;
+	int idx, ret;
+
+	info->sample_rate_value = 1024;
+	current_channel = 1;
+
+	fwnode = dev_fwnode(&client->dev);
+	fwnode_for_each_available_child_node(fwnode, node) {
+		ret = fwnode_property_read_u32(node, "reg", &idx);
+		if (ret) {
+			dev_err_probe(&client->dev, ret,
+				      "reading invalid channel index\n");
+			goto err_fwnode;
+		}
+		/* adjust idx to match channel index (1 to 4) from the datasheet */
+		idx--;
+
+		if (current_channel >= (info->phys_channels + 1) ||
+		    idx >= info->phys_channels || idx < 0) {
+			dev_err_probe(&client->dev, -EINVAL,
+				      "%s: invalid channel_index %d value\n",
+				      fwnode_get_name(node), idx);
+			goto err_fwnode;
+		}
+
+		/* enable channel */
+		info->active_channels[idx] = true;
+
+		ret = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms",
+					       &info->shunts[idx]);
+		if (ret) {
+			dev_err_probe(&client->dev, ret,
+				      "%s: invalid shunt-resistor value: %d\n",
+				      fwnode_get_name(node), info->shunts[idx]);
+			goto err_fwnode;
+		}
+
+		ret = fwnode_property_read_string(node, "label",
+						  (const char **)&info->labels[idx]);
+		if (ret) {
+			dev_err_probe(&client->dev, ret,
+				      "%s: invalid rail-name value\n",
+				      fwnode_get_name(node));
+			goto err_fwnode;
+		}
+
+		info->bi_dir[idx] = fwnode_property_read_bool(node, "bipolar");
+
+		current_channel++;
+	}
+
+	return true;
+
+err_fwnode:
+	fwnode_handle_put(node);
+
+	return false;
+}
+
+static void pac1934_cancel_delayed_work(void *dwork)
+{
+	cancel_delayed_work_sync(dwork);
+}
+
+static int pac1934_chip_configure(struct pac1934_chip_info *info)
+{
+	int cnt, ret;
+	struct i2c_client *client = info->client;
+	u8 regs[PAC1934_CTRL_STATUS_INFO_LEN], idx, ctrl_reg;
+	u32 wait_time;
+
+	cnt = 0;
+	info->chip_reg_data.num_enabled_channels = 0;
+	for (cnt = 0;  cnt < info->phys_channels; cnt++) {
+		if (info->active_channels[cnt])
+			info->chip_reg_data.num_enabled_channels++;
+		cnt++;
+	}
+
+	/*
+	 * read whatever information was gathered before the driver was loaded
+	 * establish which channels are enabled/disabled and then establish the
+	 * information retrieval mode (using SKIP or no).
+	 * Read the chip ID values
+	 */
+	ret = i2c_smbus_read_i2c_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
+					    sizeof(regs),
+					    (u8 *)regs);
+	if (ret < 0) {
+		dev_err_probe(&client->dev, ret,
+			      "%s - cannot read regs from 0x%02X\n",
+			      __func__, PAC1934_CTRL_STAT_REGS_ADDR);
+		return ret;
+	}
+
+	/* write the CHANNEL_DIS and the NEG_PWR registers */
+	regs[PAC1934_CHANNEL_DIS_REG_OFF] =
+		FIELD_PREP(PAC1934_CHAN_DIS_CH1_OFF_MASK, !(info->active_channels[PAC1934_CH_1])) |
+		FIELD_PREP(PAC1934_CHAN_DIS_CH2_OFF_MASK, !(info->active_channels[PAC1934_CH_2])) |
+		FIELD_PREP(PAC1934_CHAN_DIS_CH3_OFF_MASK, !(info->active_channels[PAC1934_CH_3])) |
+		FIELD_PREP(PAC1934_CHAN_DIS_CH4_OFF_MASK, !(info->active_channels[PAC1934_CH_4])) |
+		FIELD_PREP(PAC1934_SMBUS_TIMEOUT_MASK, 0) |
+		FIELD_PREP(PAC1934_SMBUS_BYTECOUNT_MASK, 0) |
+		FIELD_PREP(PAC1934_SMBUS_NO_SKIP_MASK, 0);
+
+	regs[PAC1934_NEG_PWR_REG_OFF] =
+		FIELD_PREP(PAC1934_NEG_PWR_CH1_BIDI_MASK, info->bi_dir[PAC1934_CH_1]) |
+		FIELD_PREP(PAC1934_NEG_PWR_CH2_BIDI_MASK, info->bi_dir[PAC1934_CH_2]) |
+		FIELD_PREP(PAC1934_NEG_PWR_CH3_BIDI_MASK, info->bi_dir[PAC1934_CH_3]) |
+		FIELD_PREP(PAC1934_NEG_PWR_CH4_BIDI_MASK, info->bi_dir[PAC1934_CH_4]) |
+		FIELD_PREP(PAC1934_NEG_PWR_CH1_BIDV_MASK, info->bi_dir[PAC1934_CH_1]) |
+		FIELD_PREP(PAC1934_NEG_PWR_CH2_BIDV_MASK, info->bi_dir[PAC1934_CH_2]) |
+		FIELD_PREP(PAC1934_NEG_PWR_CH3_BIDV_MASK, info->bi_dir[PAC1934_CH_3]) |
+		FIELD_PREP(PAC1934_NEG_PWR_CH4_BIDV_MASK, info->bi_dir[PAC1934_CH_4]);
+
+	/* no SLOW triggered REFRESH, clear POR */
+	regs[PAC1934_SLOW_REG_OFF] = 0;
+
+	ret =  i2c_smbus_write_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR, 3, (u8 *)regs);
+	if (ret)
+		return ret;
+
+	ctrl_reg = FIELD_PREP(PAC1934_CRTL_SAMPLE_RATE_MASK, info->crt_samp_spd_bitfield);
+
+	ret = i2c_smbus_write_byte_data(client, PAC1934_CTRL_REG_ADDR, ctrl_reg);
+	if (ret)
+		return ret;
+
+	/*
+	 * send a REFRESH to the chip, so the new settings take place
+	 * as well as resetting the accumulators
+	 */
+	ret = i2c_smbus_write_byte(client, PAC1934_REFRESH_REG_ADDR);
+	if (ret) {
+		dev_err(&client->dev,
+			"%s - cannot send 0x%02X\n",
+			__func__, PAC1934_REFRESH_REG_ADDR);
+		return ret;
+	}
+
+	/*
+	 * get the current(in the chip) sampling speed and compute the
+	 * required timeout based on its value
+	 * the timeout is 1/sampling_speed
+	 */
+	idx = regs[PAC1934_CTRL_ACT_REG_OFF] >> PAC1934_SAMPLE_RATE_SHIFT;
+	wait_time = (1024 / samp_rate_map_tbl[idx]) * 1000;
+
+	/*
+	 * wait the maximum amount of time to be on the safe side
+	 * the maximum wait time is for 8sps
+	 */
+	usleep_range(wait_time, wait_time + 100);
+
+	INIT_DELAYED_WORK(&info->work_chip_rfsh, pac1934_work_periodic_rfsh);
+	/* Setup the latest moment for reading the regs before saturation */
+	schedule_delayed_work(&info->work_chip_rfsh,
+			      msecs_to_jiffies(PAC1934_MAX_RFSH_LIMIT_MS));
+
+	devm_add_action_or_reset(&client->dev, pac1934_cancel_delayed_work,
+				 &info->work_chip_rfsh);
+
+	return 0;
+}
+
+static int pac1934_prep_iio_channels(struct pac1934_chip_info *info, struct iio_dev *indio_dev)
+{
+	struct i2c_client *client;
+	struct iio_chan_spec *ch_sp;
+	int channel_size, attribute_count, cnt;
+	void *dyn_ch_struct, *tmp_data;
+
+	client = info->client;
+
+	/* find out dynamically how many IIO channels we need */
+	attribute_count = 0;
+	channel_size = 0;
+	for (cnt = 0; cnt < info->phys_channels; cnt++) {
+		if (!info->active_channels[cnt])
+			continue;
+
+		/* add the size of the properties of one chip physical channel */
+		channel_size += sizeof(pac1934_single_channel);
+		/* count how many enabled channels we have */
+		attribute_count += ARRAY_SIZE(pac1934_single_channel);
+		dev_info(&client->dev,
+			 ":%s: Channel %d active\n",
+			 __func__, cnt + 1);
+	}
+
+	dyn_ch_struct = devm_kzalloc(&client->dev, channel_size, GFP_KERNEL);
+	if (!dyn_ch_struct)
+		return -EINVAL;
+
+	tmp_data = dyn_ch_struct;
+
+	/* populate the dynamic channels and make all the adjustments */
+	for (cnt = 0; cnt < info->phys_channels; cnt++) {
+		if (!info->active_channels[cnt])
+			continue;
+
+		memcpy(tmp_data, pac1934_single_channel, sizeof(pac1934_single_channel));
+		ch_sp = (struct iio_chan_spec *)tmp_data;
+		ch_sp[IIO_EN].channel = cnt + 1;
+		ch_sp[IIO_EN].scan_index = cnt;
+		ch_sp[IIO_EN].address = cnt + PAC1934_VPOWER_ACC_1_ADDR;
+		ch_sp[IIO_POW].channel = cnt + 1;
+		ch_sp[IIO_POW].scan_index = cnt;
+		ch_sp[IIO_POW].address = cnt + PAC1934_VPOWER_1_ADDR;
+		ch_sp[IIO_VOLT].channel = cnt + 1;
+		ch_sp[IIO_VOLT].scan_index = cnt;
+		ch_sp[IIO_VOLT].address = cnt + PAC1934_VBUS_1_ADDR;
+		ch_sp[IIO_CRT].channel = cnt + 1;
+		ch_sp[IIO_CRT].scan_index = cnt;
+		ch_sp[IIO_CRT].address = cnt + PAC1934_VSENSE_1_ADDR;
+		/*
+		 * In order to be able to use labels for IIO_VOLT and IIO_VOLTAVG,
+		 * respectively IIO_CRT and IIO_CRTAVG we need to use different
+		 * channel numbers. We will add  +5 (+1 to maximum PAC channels).
+		 */
+		ch_sp[IIO_VOLTAVG].channel = cnt + 5;
+		ch_sp[IIO_VOLTAVG].scan_index = cnt;
+		ch_sp[IIO_VOLTAVG].address = cnt + PAC1934_VBUS_AVG_1_ADDR;
+		ch_sp[IIO_CRTAVG].channel = cnt + 5;
+		ch_sp[IIO_CRTAVG].scan_index = cnt;
+		ch_sp[IIO_CRTAVG].address = cnt + PAC1934_VSENSE_AVG_1_ADDR;
+
+		/*
+		 * now modify the parameters in all channels if the
+		 * whole chip rail(channel) is bi-directional
+		 */
+		if (info->bi_dir[cnt]) {
+			ch_sp[IIO_EN].scan_type.sign = 's';
+			ch_sp[IIO_EN].scan_type.realbits = 47;
+			ch_sp[IIO_POW].scan_type.sign = 's';
+			ch_sp[IIO_POW].scan_type.realbits = 27;
+			ch_sp[IIO_VOLT].scan_type.sign = 's';
+			ch_sp[IIO_VOLT].scan_type.realbits = 15;
+			ch_sp[IIO_CRT].scan_type.sign = 's';
+			ch_sp[IIO_CRT].scan_type.realbits = 15;
+			ch_sp[IIO_VOLTAVG].scan_type.sign = 's';
+			ch_sp[IIO_VOLTAVG].scan_type.realbits = 15;
+			ch_sp[IIO_CRTAVG].scan_type.sign = 's';
+			ch_sp[IIO_CRTAVG].scan_type.realbits = 15;
+		}
+		tmp_data += sizeof(pac1934_single_channel);
+	}
+
+	/*
+	 * send the updated dynamic channel structure information towards IIO
+	 * prepare the required field for IIO class registration
+	 */
+	indio_dev->num_channels = attribute_count;
+
+	indio_dev->channels = (const struct iio_chan_spec *)dyn_ch_struct;
+
+	if (!indio_dev->channels)
+		return -EINVAL;
+
+	return 0;
+}
+
+static ssize_t reset_accumulators_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 pac1934_chip_info *info = iio_priv(indio_dev);
+	int ret, i;
+	u8 refresh_cmd = PAC1934_REFRESH_REG_ADDR;
+
+	ret = i2c_smbus_write_byte(info->client, refresh_cmd);
+	if (ret) {
+		dev_err(&indio_dev->dev,
+			"%s - cannot send 0x%02X\n",
+			__func__, refresh_cmd);
+		return ret;
+	}
+
+	for (i = 0 ; i < info->phys_channels; i++)
+		info->chip_reg_data.energy_sec_acc[i] = 0;
+
+	return count;
+}
+
+static IIO_DEVICE_ATTR(in_shunt_resistor_1, 0644, shunt_value_show, shunt_value_store, 0);
+static IIO_DEVICE_ATTR(in_shunt_resistor_2, 0644, shunt_value_show, shunt_value_store, 0);
+static IIO_DEVICE_ATTR(in_shunt_resistor_3, 0644, shunt_value_show, shunt_value_store, 0);
+static IIO_DEVICE_ATTR(in_shunt_resistor_4, 0644, shunt_value_show, shunt_value_store, 0);
+static IIO_DEVICE_ATTR(reset_accumulators, 0200, NULL, reset_accumulators_store, 0);
+
+static struct attribute *pac1934_all_attributes[] = {
+	PAC1934_DEV_ATTR(in_shunt_resistor_1),
+	PAC1934_DEV_ATTR(in_shunt_resistor_2),
+	PAC1934_DEV_ATTR(in_shunt_resistor_3),
+	PAC1934_DEV_ATTR(in_shunt_resistor_4),
+	PAC1934_DEV_ATTR(reset_accumulators),
+	NULL
+};
+
+static int pac1934_prep_custom_attributes(struct pac1934_chip_info *info,
+					  struct iio_dev *indio_dev)
+{
+	int i, j, active_channels_count = 0;
+	struct attribute **pac1934_custom_attributes;
+	struct attribute_group *pac1934_group;
+	struct i2c_client *client = info->client;
+
+	for (i = 0 ; i < info->phys_channels; i++)
+		if (info->active_channels[i])
+			active_channels_count++;
+
+	pac1934_group = devm_kzalloc(&client->dev, sizeof(*pac1934_group), GFP_KERNEL);
+
+	pac1934_custom_attributes = devm_kzalloc(&client->dev,
+						 (PAC1934_CUSTOM_ATTR_FOR_CHANNEL *
+						 active_channels_count +
+						 PAC1934_SHARED_DEVATTRS_COUNT)
+						 * sizeof(*pac1934_group) + 1,
+						 GFP_KERNEL);
+	j = 0;
+
+	for (i = 0 ; i < info->phys_channels; i++) {
+		if (info->active_channels[i]) {
+			pac1934_custom_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL * j] =
+			pac1934_all_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL * i];
+			pac1934_custom_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL * j + 1] =
+			pac1934_all_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL * i + 1];
+			j++;
+		}
+	}
+
+	for (i = 0; i < PAC1934_SHARED_DEVATTRS_COUNT; i++)
+		pac1934_custom_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL *
+			active_channels_count + i] =
+			pac1934_all_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL *
+			info->phys_channels + i];
+
+	pac1934_group->attrs = pac1934_custom_attributes;
+	info->pac1934_info.attrs = pac1934_group;
+
+	return 0;
+}
+
+static void pac1934_mutex_destroy(void *data)
+{
+	struct mutex *lock = data;
+
+	mutex_destroy(lock);
+}
+
+static int pac1934_probe(struct i2c_client *client)
+{
+	struct pac1934_chip_info *info;
+	const struct pac1934_features *chip;
+	struct iio_dev *indio_dev;
+	int cnt, ret;
+	bool match = false;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+
+	i2c_set_clientdata(client, indio_dev);
+	info->client = client;
+
+	/*
+	 * load default settings - all channels disabled,
+	 * uni directional flow
+	 */
+	for (cnt = 0; cnt < PAC1934_MAX_NUM_CHANNELS; cnt++) {
+		info->active_channels[cnt] = false;
+		info->bi_dir[cnt] = false;
+	}
+
+	info->crt_samp_spd_bitfield = PAC1934_SAMP_1024SPS;
+
+	ret = pac1934_chip_identify(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->phys_channels = chip->phys_channels;
+		indio_dev->name = chip->name;
+	} else {
+		info->phys_channels = pac1934_chip_config[ret].phys_channels;
+		indio_dev->name = pac1934_chip_config[ret].name;
+	}
+
+	if (ACPI_HANDLE(&client->dev))
+		match = pac1934_acpi_parse_channel_config(client, info);
+	else
+		match = pac1934_of_parse_channel_config(client, info);
+
+	if (!match) {
+		dev_dbg(&client->dev, "parameter parsing returned an error\n");
+		return -EINVAL;
+	}
+
+	mutex_init(&info->lock);
+	ret = devm_add_action_or_reset(&client->dev, pac1934_mutex_destroy,
+				       &info->lock);
+
+	/*
+	 * 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 = pac1934_chip_configure(info);
+	if (ret < 0)
+		return ret;
+
+	/* prepare the channel information */
+	ret = pac1934_prep_iio_channels(info, indio_dev);
+	if (ret < 0)
+		return ret;
+
+	ret = pac1934_prep_custom_attributes(info, indio_dev);
+	if (ret < 0) {
+		dev_err_probe(&client->dev, ret,
+			      "Can't configure custom attributes for PAC1934 device\n");
+		return ret;
+	}
+
+	info->pac1934_info.read_raw = pac1934_read_raw;
+	info->pac1934_info.read_avail = pac1934_read_avail;
+	info->pac1934_info.write_raw = pac1934_write_raw;
+	info->pac1934_info.read_label = pac1934_read_label;
+
+	indio_dev->info = &info->pac1934_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	/*
+	 * read whatever has been accumulated in the chip so far
+	 * and reset the accumulators
+	 */
+	ret = pac1934_reg_snapshot(info, true, PAC1934_REFRESH_REG_ADDR,
+				   PAC1934_MIN_UPDATE_WAIT_TIME_US);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret < 0)
+		dev_err_probe(&client->dev, ret,
+			      "Can't register IIO device\n");
+
+	return ret;
+}
+
+static const struct i2c_device_id pac1934_id[] = {
+	{ .name = "pac1931", .driver_data = (kernel_ulong_t)&pac1934_chip_config[PAC1931] },
+	{ .name = "pac1932", .driver_data = (kernel_ulong_t)&pac1934_chip_config[PAC1932] },
+	{ .name = "pac1933", .driver_data = (kernel_ulong_t)&pac1934_chip_config[PAC1933] },
+	{ .name = "pac1934", .driver_data = (kernel_ulong_t)&pac1934_chip_config[PAC1934] },
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, pac1934_id);
+
+static const struct of_device_id pac1934_of_match[] = {
+	{
+		.compatible = "microchip,pac1931",
+		.data = &pac1934_chip_config[PAC1931]
+	},
+	{
+		.compatible = "microchip,pac1932",
+		.data = &pac1934_chip_config[PAC1932]
+	},
+	{
+		.compatible = "microchip,pac1933",
+		.data = &pac1934_chip_config[PAC1933]
+	},
+	{
+		.compatible = "microchip,pac1934",
+		.data = &pac1934_chip_config[PAC1934]
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, pac1934_of_match);
+
+/* using MCHP1930 to be compatible with WINDOWS ACPI */
+static const struct acpi_device_id pac1934_acpi_match[] = {
+	{ "MCHP1930", .driver_data = (kernel_ulong_t)&pac1934_chip_config[PAC1934] },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, pac1934_acpi_match);
+
+static struct i2c_driver pac1934_driver = {
+	.driver	 = {
+		.name = "pac1934",
+		.of_match_table = pac1934_of_match,
+		.acpi_match_table = pac1934_acpi_match
+		},
+	.probe_new = pac1934_probe,
+	.id_table = pac1934_id,
+};
+
+module_i2c_driver(pac1934_driver);
+
+MODULE_AUTHOR("Bogdan Bolocan <bogdan.bolocan@microchip.com>");
+MODULE_AUTHOR("Victor Tudose");
+MODULE_AUTHOR("Marius Cristea <marius.cristea@microchip.com>");
+MODULE_DESCRIPTION("IIO driver for PAC1934 Multi-Channel DC Power/Energy Monitor");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v3 0/2] adding support for Microchip PAC193X Power Monitor

[Guenter Roeck]

On Wed, Nov 15, 2023 at 03:44:51PM +0200, marius.cristea@microchip.com wrote:
> From: Marius Cristea <marius.cristea@microchip.com>
> 
> Adding support for Microchip PAC193X series of Power Monitor with
> Accumulator chip family. This driver covers the following part numbers:
>  - PAC1931, PAC1932, PAC1933 and PAC1934
> 
>   This device is at the boundary between IIO and HWMON (if you are
> looking just at the "shunt resistors, vsense, power, energy"). The
> device also has ADC internally that can measure voltages (up to 4
> channels) and also currents (up to 4 channels). The current is measured as
> voltage across the shunt_resistor.
> 
>   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 power consumption of the
> processor itself, or a peripheral device, or a battery, this kind of functionality
> was requested by our customers).
> 

I sdon't immediately see any typical hwmon properties such as limit registers
or alarms. The hwmon subsystem also doesn't support data buffering.
Given that, the iio implementation seems more appropriate to me.
Anyone using the chip for hardware monitoring can use the iio->hwmon bridge.

Thanks,
Guenter

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

[Krzysztof Kozlowski]

On 15/11/2023 14:44, marius.cristea@microchip.com wrote:
> From: Marius Cristea <marius.cristea@microchip.com>
> 
> This is the device tree schema for iio driver for
> Microchip PAC193X series of Power Monitors with Accumulator.
> 
> Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> ---
>  .../bindings/iio/adc/microchip,pac1934.yaml   | 137 ++++++++++++++++++
>  1 file changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> new file mode 100644
> index 000000000000..2609cb19c377
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> @@ -0,0 +1,137 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PAC1934 Power Monitors with Accumulator
> +
> +maintainers:
> +  - Marius Cristea <marius.cristea@microchip.com>
> +
> +description: |
> +  This device is part of the Microchip family of Power Monitors with Accumulator.
> +  The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here:
> +    https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,pac1931
> +      - microchip,pac1932
> +      - microchip,pac1933
> +      - microchip,pac1934
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  microchip,slow-io:
> +    type: boolean
> +    description: |
> +      A GPIO used to trigger a change is sampling rate (lowering the chip power consumption).

Use Linux coding style wrapping (as described in Linux Coding style). I
am not going to tell you numbers because I want you to read the document
first.

This is boolean, not GPIO. I don't understand. "A GPIO", so any GPIO or
some specific? How is this property related to GPIO?


> +      If configured in SLOW mode, if this pin is forced high, sampling rate is forced to eight

This pin? This is boolean, not a GPIO. GPIOs are phandles.

> +      samples/second. When it is forced low, the sampling rate is 1024 samples/second unless
> +      a different sample rate has been programmed.
> +
> +patternProperties:
> +  "^channel@[1-4]+$":
> +    type: object
> +    $ref: adc.yaml
> +    description: Represents the external channels which are connected to the ADC.
> +
> +    properties:
> +      reg:
> +        items:
> +          minimum: 1
> +          maximum: 4
> +
> +      shunt-resistor-micro-ohms:
> +        description: |
> +          Value in micro Ohms of the shunt resistor connected between
> +          the SENSE+ and SENSE- inputs, across which the current is measured. Value
> +          is needed to compute the scaling of the measured current.
> +
> +    required:
> +      - reg
> +      - shunt-resistor-micro-ohms
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: interrupts


I don't understand what do you want to say here. I am also 100% sure you
did not test it on a real case (maybe example passes but nothing more).

> +    then:
> +      properties:
> +        microchip,slow-io: false
> +    else:
> +      if:
> +        properties:
> +          compatible:
> +            contains:
> +              const: microchip,slow-io
> +      then:
> +        properties:
> +          interrupts: false

Best regards,
Krzysztof

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

[Conor Dooley]

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

On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote:
> On 15/11/2023 14:44, marius.cristea@microchip.com wrote:
> > From: Marius Cristea <marius.cristea@microchip.com>
> > 
> > This is the device tree schema for iio driver for
> > Microchip PAC193X series of Power Monitors with Accumulator.
> > 
> > Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> > ---
> >  .../bindings/iio/adc/microchip,pac1934.yaml   | 137 ++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > new file mode 100644
> > index 000000000000..2609cb19c377
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > @@ -0,0 +1,137 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip PAC1934 Power Monitors with Accumulator
> > +
> > +maintainers:
> > +  - Marius Cristea <marius.cristea@microchip.com>
> > +
> > +description: |
> > +  This device is part of the Microchip family of Power Monitors with Accumulator.
> > +  The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here:
> > +    https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - microchip,pac1931
> > +      - microchip,pac1932
> > +      - microchip,pac1933
> > +      - microchip,pac1934
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  microchip,slow-io:
> > +    type: boolean
> > +    description: |
> > +      A GPIO used to trigger a change is sampling rate (lowering the chip power consumption).
> 
> Use Linux coding style wrapping (as described in Linux Coding style). I
> am not going to tell you numbers because I want you to read the document
> first.
> 
> This is boolean, not GPIO. I don't understand. "A GPIO", so any GPIO or
> some specific? How is this property related to GPIO?
> 
> 
> > +      If configured in SLOW mode, if this pin is forced high, sampling rate is forced to eight
> 
> This pin? This is boolean, not a GPIO. GPIOs are phandles.

I said it on the previous version, but this really seems like it should
be something like "slow-io-gpios". I know Jonathan expressed some
concerns about having to deal with it on the operating system side (as
the pin is either an input & used for this slow-io control, or an output
and used as an interrupt) but that is, in my opinion, a problem for the
operating system & the binding should describe how the hardware works,
even if that is not convenient. With this sort of property, a GPIO hog
would be required to be set up (and the driver for that gpio controller
bound etc before the pac driver loads) for correction functionality if
this property was in the non-default state.

> > +      samples/second. When it is forced low, the sampling rate is 1024 samples/second unless
> > +      a different sample rate has been programmed.
> > +
> > +patternProperties:
> > +  "^channel@[1-4]+$":
> > +    type: object
> > +    $ref: adc.yaml
> > +    description: Represents the external channels which are connected to the ADC.
> > +
> > +    properties:
> > +      reg:
> > +        items:
> > +          minimum: 1
> > +          maximum: 4
> > +
> > +      shunt-resistor-micro-ohms:
> > +        description: |
> > +          Value in micro Ohms of the shunt resistor connected between
> > +          the SENSE+ and SENSE- inputs, across which the current is measured. Value
> > +          is needed to compute the scaling of the measured current.
> > +
> > +    required:
> > +      - reg
> > +      - shunt-resistor-micro-ohms
> > +
> > +    unevaluatedProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: interrupts
> 
> 
> I don't understand what do you want to say here. I am also 100% sure you
> did not test it on a real case (maybe example passes but nothing more).

As far as I understand, the same pin on the device is used for both an
output or an input depending on the configuration. As an input, it is
the "slow-io" control, and as an output it is an interrupt.
I think Marius is trying to convey that either this pin can be in
exclusively one state or another.

_However_ I am not sure that that is really the right thing to do - they
might well be mutually exclusive modes, but I think the decision can be
made at runtime, rather than at devicetree creation time. Say for
example the GPIO controller this is connected to is capable of acting as
an interrupt controller. Unless I am misunderstanding the runtime
configurability of this hardware, I think it is possible to actually
provide a "slow-io-gpios" and an interrupt property & let the operating
system decide at runtime which mode it wants to work in.

I'm off travelling at the moment Marius, but I should be back in work on
Monday if you want to have a chat about it & explain a bit more to me?

Cheers,
Conor.

> 
> > +    then:
> > +      properties:
> > +        microchip,slow-io: false
> > +    else:
> > +      if:
> > +        properties:
> > +          compatible:
> > +            contains:
> > +              const: microchip,slow-io
> > +      then:
> > +        properties:
> > +          interrupts: false
> 
> Best regards,
> Krzysztof
> 

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

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

[Krzysztof Kozlowski]

On 16/11/2023 19:21, Conor Dooley wrote:

>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: interrupts
>>
>>
>> I don't understand what do you want to say here. I am also 100% sure you
>> did not test it on a real case (maybe example passes but nothing more).
> 
> As far as I understand, the same pin on the device is used for both an
> output or an input depending on the configuration. As an input, it is
> the "slow-io" control, and as an output it is an interrupt.
> I think Marius is trying to convey that either this pin can be in
> exclusively one state or another.
> 
> _However_ I am not sure that that is really the right thing to do - they
> might well be mutually exclusive modes, but I think the decision can be
> made at runtime, rather than at devicetree creation time. Say for
> example the GPIO controller this is connected to is capable of acting as
> an interrupt controller. Unless I am misunderstanding the runtime
> configurability of this hardware, I think it is possible to actually
> provide a "slow-io-gpios" and an interrupt property & let the operating
> system decide at runtime which mode it wants to work in.
> 
> I'm off travelling at the moment Marius, but I should be back in work on
> Monday if you want to have a chat about it & explain a bit more to me?

Sure, but which compatible contains "interrupts"?

Best regards,
Krzysztof

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

[Conor Dooley]

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

On Thu, Nov 16, 2023 at 09:00:50PM +0100, Krzysztof Kozlowski wrote:
> On 16/11/2023 19:21, Conor Dooley wrote:
> 
> >>> +allOf:
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: interrupts
> >>
> >>
> >> I don't understand what do you want to say here. I am also 100% sure you
> >> did not test it on a real case (maybe example passes but nothing more).
> > 
> > As far as I understand, the same pin on the device is used for both an
> > output or an input depending on the configuration. As an input, it is
> > the "slow-io" control, and as an output it is an interrupt.
> > I think Marius is trying to convey that either this pin can be in
> > exclusively one state or another.
> > 
> > _However_ I am not sure that that is really the right thing to do - they
> > might well be mutually exclusive modes, but I think the decision can be
> > made at runtime, rather than at devicetree creation time. Say for
> > example the GPIO controller this is connected to is capable of acting as
> > an interrupt controller. Unless I am misunderstanding the runtime
> > configurability of this hardware, I think it is possible to actually
> > provide a "slow-io-gpios" and an interrupt property & let the operating
> > system decide at runtime which mode it wants to work in.
> > 
> > I'm off travelling at the moment Marius, but I should be back in work on
> > Monday if you want to have a chat about it & explain a bit more to me?
> 
> Sure, but which compatible contains "interrupts"?

Yeah, I did notice that - I figured you understood that that was meant
to not be a check on compatibles, but rather on regular old properties &
the rationale for the mutual exclusion was what you were missing.

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

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

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 5e99f692d4e32e3250ab18d511894ca797407aec]

url:    https://github.com/intel-lab-lkp/linux/commits/marius-cristea-microchip-com/dt-bindings-iio-adc-adding-support-for-PAC193X/20231115-214733
base:   5e99f692d4e32e3250ab18d511894ca797407aec
patch link:    https://lore.kernel.org/r/20231115134453.6656-3-marius.cristea%40microchip.com
patch subject: [PATCH v3 2/2] iio: adc: adding support for PAC193x
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231121/202311212310.QEKkmOfh-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231121/202311212310.QEKkmOfh-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/202311212310.QEKkmOfh-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/pac1934.c:1239:3: warning: variable 'cnt' is incremented both in the loop header and in the loop body [-Wfor-loop-analysis]
    1239 |                 cnt++;
         |                 ^
   drivers/iio/adc/pac1934.c:1236:44: note: incremented here
    1236 |         for (cnt = 0;  cnt < info->phys_channels; cnt++) {
         |                                                   ^
   1 warning generated.


vim +/cnt +1239 drivers/iio/adc/pac1934.c

  1226	
  1227	static int pac1934_chip_configure(struct pac1934_chip_info *info)
  1228	{
  1229		int cnt, ret;
  1230		struct i2c_client *client = info->client;
  1231		u8 regs[PAC1934_CTRL_STATUS_INFO_LEN], idx, ctrl_reg;
  1232		u32 wait_time;
  1233	
  1234		cnt = 0;
  1235		info->chip_reg_data.num_enabled_channels = 0;
  1236		for (cnt = 0;  cnt < info->phys_channels; cnt++) {
  1237			if (info->active_channels[cnt])
  1238				info->chip_reg_data.num_enabled_channels++;
> 1239			cnt++;
  1240		}
  1241	
  1242		/*
  1243		 * read whatever information was gathered before the driver was loaded
  1244		 * establish which channels are enabled/disabled and then establish the
  1245		 * information retrieval mode (using SKIP or no).
  1246		 * Read the chip ID values
  1247		 */
  1248		ret = i2c_smbus_read_i2c_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
  1249						    sizeof(regs),
  1250						    (u8 *)regs);
  1251		if (ret < 0) {
  1252			dev_err_probe(&client->dev, ret,
  1253				      "%s - cannot read regs from 0x%02X\n",
  1254				      __func__, PAC1934_CTRL_STAT_REGS_ADDR);
  1255			return ret;
  1256		}
  1257	
  1258		/* write the CHANNEL_DIS and the NEG_PWR registers */
  1259		regs[PAC1934_CHANNEL_DIS_REG_OFF] =
  1260			FIELD_PREP(PAC1934_CHAN_DIS_CH1_OFF_MASK, !(info->active_channels[PAC1934_CH_1])) |
  1261			FIELD_PREP(PAC1934_CHAN_DIS_CH2_OFF_MASK, !(info->active_channels[PAC1934_CH_2])) |
  1262			FIELD_PREP(PAC1934_CHAN_DIS_CH3_OFF_MASK, !(info->active_channels[PAC1934_CH_3])) |
  1263			FIELD_PREP(PAC1934_CHAN_DIS_CH4_OFF_MASK, !(info->active_channels[PAC1934_CH_4])) |
  1264			FIELD_PREP(PAC1934_SMBUS_TIMEOUT_MASK, 0) |
  1265			FIELD_PREP(PAC1934_SMBUS_BYTECOUNT_MASK, 0) |
  1266			FIELD_PREP(PAC1934_SMBUS_NO_SKIP_MASK, 0);
  1267	
  1268		regs[PAC1934_NEG_PWR_REG_OFF] =
  1269			FIELD_PREP(PAC1934_NEG_PWR_CH1_BIDI_MASK, info->bi_dir[PAC1934_CH_1]) |
  1270			FIELD_PREP(PAC1934_NEG_PWR_CH2_BIDI_MASK, info->bi_dir[PAC1934_CH_2]) |
  1271			FIELD_PREP(PAC1934_NEG_PWR_CH3_BIDI_MASK, info->bi_dir[PAC1934_CH_3]) |
  1272			FIELD_PREP(PAC1934_NEG_PWR_CH4_BIDI_MASK, info->bi_dir[PAC1934_CH_4]) |
  1273			FIELD_PREP(PAC1934_NEG_PWR_CH1_BIDV_MASK, info->bi_dir[PAC1934_CH_1]) |
  1274			FIELD_PREP(PAC1934_NEG_PWR_CH2_BIDV_MASK, info->bi_dir[PAC1934_CH_2]) |
  1275			FIELD_PREP(PAC1934_NEG_PWR_CH3_BIDV_MASK, info->bi_dir[PAC1934_CH_3]) |
  1276			FIELD_PREP(PAC1934_NEG_PWR_CH4_BIDV_MASK, info->bi_dir[PAC1934_CH_4]);
  1277	
  1278		/* no SLOW triggered REFRESH, clear POR */
  1279		regs[PAC1934_SLOW_REG_OFF] = 0;
  1280	
  1281		ret =  i2c_smbus_write_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR, 3, (u8 *)regs);
  1282		if (ret)
  1283			return ret;
  1284	
  1285		ctrl_reg = FIELD_PREP(PAC1934_CRTL_SAMPLE_RATE_MASK, info->crt_samp_spd_bitfield);
  1286	
  1287		ret = i2c_smbus_write_byte_data(client, PAC1934_CTRL_REG_ADDR, ctrl_reg);
  1288		if (ret)
  1289			return ret;
  1290	
  1291		/*
  1292		 * send a REFRESH to the chip, so the new settings take place
  1293		 * as well as resetting the accumulators
  1294		 */
  1295		ret = i2c_smbus_write_byte(client, PAC1934_REFRESH_REG_ADDR);
  1296		if (ret) {
  1297			dev_err(&client->dev,
  1298				"%s - cannot send 0x%02X\n",
  1299				__func__, PAC1934_REFRESH_REG_ADDR);
  1300			return ret;
  1301		}
  1302	
  1303		/*
  1304		 * get the current(in the chip) sampling speed and compute the
  1305		 * required timeout based on its value
  1306		 * the timeout is 1/sampling_speed
  1307		 */
  1308		idx = regs[PAC1934_CTRL_ACT_REG_OFF] >> PAC1934_SAMPLE_RATE_SHIFT;
  1309		wait_time = (1024 / samp_rate_map_tbl[idx]) * 1000;
  1310	
  1311		/*
  1312		 * wait the maximum amount of time to be on the safe side
  1313		 * the maximum wait time is for 8sps
  1314		 */
  1315		usleep_range(wait_time, wait_time + 100);
  1316	
  1317		INIT_DELAYED_WORK(&info->work_chip_rfsh, pac1934_work_periodic_rfsh);
  1318		/* Setup the latest moment for reading the regs before saturation */
  1319		schedule_delayed_work(&info->work_chip_rfsh,
  1320				      msecs_to_jiffies(PAC1934_MAX_RFSH_LIMIT_MS));
  1321	
  1322		devm_add_action_or_reset(&client->dev, pac1934_cancel_delayed_work,
  1323					 &info->work_chip_rfsh);
  1324	
  1325		return 0;
  1326	}
  1327	

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

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

[Jonathan Cameron]

On Thu, 16 Nov 2023 18:21:33 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote:
> > On 15/11/2023 14:44, marius.cristea@microchip.com wrote:  
> > > From: Marius Cristea <marius.cristea@microchip.com>
> > > 
> > > This is the device tree schema for iio driver for
> > > Microchip PAC193X series of Power Monitors with Accumulator.
> > > 
> > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> > > ---
> > >  .../bindings/iio/adc/microchip,pac1934.yaml   | 137 ++++++++++++++++++
> > >  1 file changed, 137 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > > new file mode 100644
> > > index 000000000000..2609cb19c377
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > > @@ -0,0 +1,137 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Microchip PAC1934 Power Monitors with Accumulator
> > > +
> > > +maintainers:
> > > +  - Marius Cristea <marius.cristea@microchip.com>
> > > +
> > > +description: |
> > > +  This device is part of the Microchip family of Power Monitors with Accumulator.
> > > +  The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here:
> > > +    https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - microchip,pac1931
> > > +      - microchip,pac1932
> > > +      - microchip,pac1933
> > > +      - microchip,pac1934
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  microchip,slow-io:
> > > +    type: boolean
> > > +    description: |
> > > +      A GPIO used to trigger a change is sampling rate (lowering the chip power consumption).  
> > 
> > Use Linux coding style wrapping (as described in Linux Coding style). I
> > am not going to tell you numbers because I want you to read the document
> > first.
> > 
> > This is boolean, not GPIO. I don't understand. "A GPIO", so any GPIO or
> > some specific? How is this property related to GPIO?
> > 
> >   
> > > +      If configured in SLOW mode, if this pin is forced high, sampling rate is forced to eight  
> > 
> > This pin? This is boolean, not a GPIO. GPIOs are phandles.  
> 
> I said it on the previous version, but this really seems like it should
> be something like "slow-io-gpios". I know Jonathan expressed some
> concerns about having to deal with it on the operating system side (as
> the pin is either an input & used for this slow-io control, or an output
> and used as an interrupt) but that is, in my opinion, a problem for the
> operating system & the binding should describe how the hardware works,
> even if that is not convenient. With this sort of property, a GPIO hog
> would be required to be set up (and the driver for that gpio controller
> bound etc before the pac driver loads) for correction functionality if
> this property was in the non-default state.

I'd forgotten the discussion completely ;)
My main question was why bother with slow?  You can do it without the GPIO
anyway as there is a register bit for it.

I can conceive of various possible reasons, the evil one being that it's
actually out of the host processors control. Is that what we care about here?
If so it's nothing to do with a GPIO in the Linux sense at all and we can
assume that it's not connected to an interrupt at the same time.

We 'might' need a control for that case that says configure the device for
an external entity to use the slow pin.

It wouldn't be the first device to have that sort of thing, but normally they
are sequencing pins that are wired up to some mechanical device or similar.


> 
> > > +      samples/second. When it is forced low, the sampling rate is 1024 samples/second unless
> > > +      a different sample rate has been programmed.
> > > +
> > > +patternProperties:
> > > +  "^channel@[1-4]+$":
> > > +    type: object
> > > +    $ref: adc.yaml
> > > +    description: Represents the external channels which are connected to the ADC.
> > > +
> > > +    properties:
> > > +      reg:
> > > +        items:
> > > +          minimum: 1
> > > +          maximum: 4
> > > +
> > > +      shunt-resistor-micro-ohms:
> > > +        description: |
> > > +          Value in micro Ohms of the shunt resistor connected between
> > > +          the SENSE+ and SENSE- inputs, across which the current is measured. Value
> > > +          is needed to compute the scaling of the measured current.
> > > +
> > > +    required:
> > > +      - reg
> > > +      - shunt-resistor-micro-ohms
> > > +
> > > +    unevaluatedProperties: false
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - "#address-cells"
> > > +  - "#size-cells"
> > > +
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: interrupts  
> > 
> > 
> > I don't understand what do you want to say here. I am also 100% sure you
> > did not test it on a real case (maybe example passes but nothing more).  
> 
> As far as I understand, the same pin on the device is used for both an
> output or an input depending on the configuration. As an input, it is
> the "slow-io" control, and as an output it is an interrupt.
> I think Marius is trying to convey that either this pin can be in
> exclusively one state or another.
> 
> _However_ I am not sure that that is really the right thing to do - they
> might well be mutually exclusive modes, but I think the decision can be
> made at runtime, rather than at devicetree creation time. Say for
> example the GPIO controller this is connected to is capable of acting as
> an interrupt controller. Unless I am misunderstanding the runtime
> configurability of this hardware, I think it is possible to actually
> provide a "slow-io-gpios" and an interrupt property & let the operating
> system decide at runtime which mode it wants to work in.

I'll admit I've long forgotten what was going on here, but based just on
this bit of text I agree. There is nothing 'stopping' us having a pin
uses as either / or / both interrupt and gpio.

It'll be a bit messy to support in the driver as IIRC there are some sanity
checks that limit combinations on IRQs and output GPIOS.  Can't remember
how bad the dance to navigate it safely is.

First version I'd just say pick one option if both are provided and
don't support configuring it at runtime.

Jonathan
 


> 
> I'm off travelling at the moment Marius, but I should be back in work on
> Monday if you want to have a chat about it & explain a bit more to me?
> 
> Cheers,
> Conor.
> 
> >   
> > > +    then:
> > > +      properties:
> > > +        microchip,slow-io: false
> > > +    else:
> > > +      if:
> > > +        properties:
> > > +          compatible:
> > > +            contains:
> > > +              const: microchip,slow-io
> > > +      then:
> > > +        properties:
> > > +          interrupts: false  
> > 
> > Best regards,
> > Krzysztof
> >   

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

[Jonathan Cameron]

On Wed, 15 Nov 2023 15:44:53 +0200
<marius.cristea@microchip.com> wrote:

> From: Marius Cristea <marius.cristea@microchip.com>
> 
> This is the iio driver for Microchip
> PAC193X series of Power Monitor with Accumulator chip family.
> 
> Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-adc-pac1934     |   15 +
>  MAINTAINERS                                   |    7 +
>  drivers/iio/adc/Kconfig                       |   12 +
>  drivers/iio/adc/Makefile                      |    1 +
>  drivers/iio/adc/pac1934.c                     | 1673 +++++++++++++++++
>  5 files changed, 1708 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
>  create mode 100644 drivers/iio/adc/pac1934.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
> new file mode 100644
> index 000000000000..533429eaac6a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
> @@ -0,0 +1,15 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor_X
> +KernelVersion:	6.7
> +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.
Why the device being in the directory name and the shunt resistor name?

If you drop that, we need to move this abi to a more generic file because
we can't have duplicate entries in the ABI directory.
sysfs-bus-iio-power-mon or similar would do then remove the entries elsewhere.

Or does it mean channel index? In which case don't use X.

I'd love to make shunt resistor a DT problem... I guess that's a non starter
because of potential calibrations etc?


> +
> +What:		/sys/bus/iio/devices/iio:deviceX/reset_accumulators
> +KernelVersion:	6.7
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Writing any value resets the accumulated power device's statistics
> +		for all enabled channels.

Hmm. We have used channel enables for this before. Might be better to map to that
though it's a bit inelegant to have a write of 1 to in_powerY_enable reset a bunch of
other channels.

I'm not sure if we have an precedent that aligns perfectly with this.
Closest is step counters were we do use enable, but then the reset only covers one
clearly defined channel.

Having in_enable seems a bit odd as I assume some channels don't involve accumulation...

> diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
> new file mode 100644
> index 000000000000..d2be205c3cb4
> --- /dev/null
> +++ b/drivers/iio/adc/pac1934.c
> @@ -0,0 +1,1673 @@

> +
> +#define PAC1934_MAX_NUM_CHANNELS		4
> +#define PAC1934_CH_1				0
> +#define PAC1934_CH_2				1
> +#define PAC1934_CH_3				2
> +#define PAC1934_CH_4				3

The channel 1 extra naming doesn't actually seem relevant anywhere inside
the driver other than maybe making relationship to datasheet clear.
You might as well use the 0,1,23 values directly I think.

> +#define PAC1934_MEAS_REG_LEN			76

Can you compute these from register addresses and sizes?  If so that will
be self documenting and make it clear these are correct.

> +#define PAC1934_CTRL_REG_LEN			12
> +
> +
> +#define PAC1934_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)


> +/*
> + * Universal Unique Identifier (UUID),
> + * 033771E0-1705-47B4-9535-D1BBE14D9A09, is
> + * reserved to Microchip for the PAC1934 and must not be changed
No need to say it must not be changed. We can't change register addresses either
and this is similar to those.

> + */
> +#define PAC1934_DSM_UUID		"033771E0-1705-47B4-9535-D1BBE14D9A09"
> +
...

> +/**
> + * struct reg_data - data from the registers
> + * @meas_regs:			snapshot of raw measurements registers
> + * @ctrl_regs:			snapshot of control registers
> + * @energy_sec_acc:		snapshot of energy values
> + * @vpower_acc:			accumulated vpower values
> + * @vpower:			snapshot of vpower registers
> + * @vbus:			snapshot of vbus registers
> + * @vbus_avg:			averages of vbus registers
> + * @vsense:			snapshot of vsense registers
> + * @vsense_avg:			averages of vsense registers
> + * @num_enabled_channels:	count of how many chip channels are currently enabled
> + */
> +struct reg_data {
> +	u8	meas_regs[PAC1934_MEAS_REG_LEN];
> +	u8	ctrl_regs[PAC1934_CTRL_REG_LEN];

I guess the weird block read stuff where it skips addresses prevents using regmap and
regcache to avoid hand spinning caching?

> +	s64	energy_sec_acc[PAC1934_MAX_NUM_CHANNELS];
> +	s64	vpower_acc[PAC1934_MAX_NUM_CHANNELS];
> +	s32	vpower[PAC1934_MAX_NUM_CHANNELS];
> +	s32	vbus[PAC1934_MAX_NUM_CHANNELS];
> +	s32	vbus_avg[PAC1934_MAX_NUM_CHANNELS];
> +	s32	vsense[PAC1934_MAX_NUM_CHANNELS];
> +	s32	vsense_avg[PAC1934_MAX_NUM_CHANNELS];
> +	u8	num_enabled_channels;
> +};
> +
> +/**
> + * struct pac1934_chip_info - information about the chip
> + * @client:			the i2c-client attached to the device
> + * @lock:			synchronize access to driver's state members
> + * @work_chip_rfsh:		work queue used for refresh commands
> + * @phys_channels:		phys channels count
> + * @active_channels:		array of values, true means that channel is active
> + * @bi_dir:			array of bools, true means that channel is bidirectional
> + * @chip_variant:		chip variant
> + * @chip_revision:		chip revision
> + * @shunts:			shunts
> + * @chip_reg_data:		chip reg data
> + * @sample_rate_value:		sampling frequency
> + * @labels:			table with channels labels
> + * @pac1934_info:		pac1934_info
> + * @crt_samp_spd_bitfield:	the current sampling speed
> + * @jiffies_tstamp:		chip's uptime
> + */
> +struct pac1934_chip_info {
> +	struct i2c_client	*client;
> +	struct mutex		lock; /* synchronize access to driver's state members */
> +	struct delayed_work	work_chip_rfsh;
> +	u8			phys_channels;
> +	bool			active_channels[PAC1934_MAX_NUM_CHANNELS];
> +	bool			bi_dir[PAC1934_MAX_NUM_CHANNELS];
> +	u8			chip_variant;
> +	u8			chip_revision;
> +	u32			shunts[PAC1934_MAX_NUM_CHANNELS];
> +	struct reg_data		chip_reg_data;
> +	s32			sample_rate_value;
> +	char			*labels[PAC1934_MAX_NUM_CHANNELS];
> +	struct iio_info		pac1934_info;

I'd call it iio_info, or just info as the pac1934 bit is obvious from context.

> +	u8			crt_samp_spd_bitfield;
> +	unsigned long		jiffies_tstamp;
> +};
> +
> +#define TO_PAC1934_CHIP_INFO(d) container_of(d, struct pac1934_chip_info, work_chip_rfsh)
> +
> +#define PAC1934_VPOWER_ACC_CHANNEL(_index, _si, _address) {			\
> +	.type = IIO_ENERGY,							\
> +	.address = (_address),							\
> +	.indexed = 1,								\
> +	.channel = (_index),							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)	|			\
> +			      BIT(IIO_CHAN_INFO_SCALE),				\
> +	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +	.scan_index = (_si),							\
> +	.scan_type = {								\
> +		.sign = 'u',							\
> +		.realbits = 48,							\
> +		.storagebits = 48,						\

IIO elements are always in naturally aligned power of 2. So that needs to be 64 bits.
Standard userspace tooling expects this so we can't change it.
So needs to be 
.storagebits = 64

You may have to repack data to meet this requirement.  It's annoying but one
of the costs we pay for a simple ABI.

I'd not add this stuff until you add the buffer support though.
For now it's just noise.



> +		.endianness = IIO_CPU,						\
> +	}									\
> +}


> +
> +/* Low-level I2c functions used to transfer up to 76 bytes at once */
> +static int pac1934_i2c_read(struct i2c_client *client, u8 reg_addr, void *databuf, u8 len)
> +{
> +	int ret;
> +	struct i2c_msg msgs[2] = {
> +		{ .addr = client->addr,
> +		 .len = 1,
> +		 .buf = (u8 *)&reg_addr,
odd indent.
		{
			.addr = client->addr,
			.len = 1,
			.buf
		}, {
			.addr = ..

> +		},
> +		{ .addr = client->addr,
> +		 .len = len,
> +		 .buf = databuf,

can you use i2c_smbus_read_i2c_block_data() here?

I'm rubbish at remembering all the smbus variants, so it might not fit, but
it's close to this.  You used it elsewhere so maybe this one got missed?


> +		 .flags = I2C_M_RD
> +		}
> +	};
> +
> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +

> +static ssize_t shunt_value_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct pac1934_chip_info *info = iio_priv(indio_dev);
> +	int len = 0;
> +	int target = (int)(attr->attr.name[strlen(attr->attr.name) - 1] - '0') - 1;
There is an address field in the IIO_DEVICE_ATTR().
Use that (bit of container of magic to get to it)


> +
> +	len += sprintf(buf, "%u\n", info->shunts[target]);
> +
> +	return len;

	return sysfs_emit()

> +}
> +
> +static ssize_t 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 pac1934_chip_info *info = iio_priv(indio_dev);
> +	int sh_val, target;
> +
> +	target = (int)(attr->attr.name[strlen(attr->attr.name) - 1] - '0') - 1;

Again, address in the attribute for this. Not messy string mangling.

> +	if (kstrtouint(buf, 10, &sh_val)) {
> +		dev_err(dev, "Shunt value is not valid\n");
> +		return -EINVAL;
> +	}
> +
> +	scoped_guard(mutex, &info->lock)
> +		info->shunts[target] = sh_val;
> +
> +	return count;
> +}
> +
...


> +
> +static int pac1934_send_refresh(struct pac1934_chip_info *info,
> +				u8 refresh_cmd, u32 wait_time)
> +{
> +	/* this function only sends REFRESH or REFRESH_V */
> +	struct i2c_client *client = info->client;
> +	int ret;
> +	u8 bidir_reg;
> +	bool revision_bug = false;
> +
> +	if (info->chip_revision == 2 || info->chip_revision == 3) {
> +		/*
> +		 * chip rev 2 and 3 bug workaround
> +		 * see: PAC1934 Family Data Sheet Errata DS80000836A.pdf
> +		 */
> +		revision_bug = true;
> +
> +		bidir_reg =
> +			FIELD_PREP(PAC1934_NEG_PWR_CH1_BIDI_MASK, info->bi_dir[PAC1934_CH_1]) |

as mentioned above, just use [0], [1], [2], [3] as the CH_1 mapping doesn't make anything much clearer.
 
> +			FIELD_PREP(PAC1934_NEG_PWR_CH2_BIDI_MASK, info->bi_dir[PAC1934_CH_2]) |
> +			FIELD_PREP(PAC1934_NEG_PWR_CH3_BIDI_MASK, info->bi_dir[PAC1934_CH_3]) |
> +			FIELD_PREP(PAC1934_NEG_PWR_CH4_BIDI_MASK, info->bi_dir[PAC1934_CH_4]) |
> +			FIELD_PREP(PAC1934_NEG_PWR_CH1_BIDV_MASK, info->bi_dir[PAC1934_CH_1]) |
> +			FIELD_PREP(PAC1934_NEG_PWR_CH2_BIDV_MASK, info->bi_dir[PAC1934_CH_2]) |
> +			FIELD_PREP(PAC1934_NEG_PWR_CH3_BIDV_MASK, info->bi_dir[PAC1934_CH_3]) |
> +			FIELD_PREP(PAC1934_NEG_PWR_CH4_BIDV_MASK, info->bi_dir[PAC1934_CH_4]);
> +
...

> +	return ret;
> +}
> +

> +static int pac1934_retrieve_data(struct pac1934_chip_info *info,
> +				 u32 wait_time)
> +{
> +	int ret = 0;
> +
> +	/*
> +	 * 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->jiffies_tstamp +
> +					 msecs_to_jiffies(PAC1934_MIN_POLLING_TIME_MS))) {
> +		ret = pac1934_reg_snapshot(info, true, PAC1934_REFRESH_REG_ADDR,
> +					   wait_time);
> +
> +		/*
> +		 * Re-schedule the work for the read registers on timeout
> +		 * (to prevent chip regs saturation)
> +		 */
> +		cancel_delayed_work_sync(&info->work_chip_rfsh);
> +		schedule_delayed_work(&info->work_chip_rfsh,
> +				      msecs_to_jiffies(PAC1934_MAX_RFSH_LIMIT_MS));

Doesn't mod_delayed_work() cover this pair more cheaply?
Or is there are race I'm missing?  I think it's safe for this usecase.

> +	}
> +
> +	return ret;
> +}
> +
> +static int pac1934_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct pac1934_chip_info *info = iio_priv(indio_dev);
> +	s64 curr_energy;
> +	int ret, channel = chan->channel - 1;
> +
> +	ret = pac1934_retrieve_data(info, PAC1934_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[channel];
> +			return IIO_VAL_INT;
> +		case IIO_CURRENT:
> +			*val = info->chip_reg_data.vsense[channel];
> +			return IIO_VAL_INT;
> +		case IIO_POWER:
> +			*val = info->chip_reg_data.vpower[channel];
> +			return IIO_VAL_INT;
> +		case IIO_ENERGY:
> +			curr_energy = info->chip_reg_data.energy_sec_acc[channel];
> +			*val = (u32)curr_energy;
> +			*val2 = (u32)(curr_energy >> 32);
> +			return IIO_VAL_INT_64;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
Not sure these break; at end of case can be reached.

> +	case IIO_CHAN_INFO_AVERAGE_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = info->chip_reg_data.vbus_avg[channel];
> +			return IIO_VAL_INT;
> +		case IIO_CURRENT:
> +			*val = info->chip_reg_data.vsense_avg[channel];
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->address) {
> +		/* Voltages - scale for millivolts */
> +		case PAC1934_VBUS_1_ADDR:
> +		case PAC1934_VBUS_2_ADDR:
> +		case PAC1934_VBUS_3_ADDR:
> +		case PAC1934_VBUS_4_ADDR:
> +		case PAC1934_VBUS_AVG_1_ADDR:
> +		case PAC1934_VBUS_AVG_2_ADDR:
> +		case PAC1934_VBUS_AVG_3_ADDR:
> +		case PAC1934_VBUS_AVG_4_ADDR:
> +			*val = PAC1934_VOLTAGE_MILLIVOLTS_MAX;
> +			if (chan->scan_type.sign == 'u')
> +				*val2 = PAC1934_VOLTAGE_U_RES;
> +			else
> +				*val2 = PAC1934_VOLTAGE_S_RES;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		/*
> +		 * Currents - scale for mA - depends on the
> +		 * channel's shunt value
> +		 * (100mV * 1000000) / (2^16 * shunt(uohm))
> +		 */
> +		case PAC1934_VSENSE_1_ADDR:
> +		case PAC1934_VSENSE_2_ADDR:
> +		case PAC1934_VSENSE_3_ADDR:
> +		case PAC1934_VSENSE_4_ADDR:
> +		case PAC1934_VSENSE_AVG_1_ADDR:
> +		case PAC1934_VSENSE_AVG_2_ADDR:
> +		case PAC1934_VSENSE_AVG_3_ADDR:
> +		case PAC1934_VSENSE_AVG_4_ADDR:
> +			*val = PAC1934_MAX_VSENSE_RSHIFTED_BY_16B;
> +			if (chan->scan_type.sign == 'u')
> +				*val2 = info->shunts[channel];
> +			else
> +				*val2 = info->shunts[channel] >> 1;
> +			return IIO_VAL_FRACTIONAL;
> +		/*
> +		 * Power - uW - it will use the combined scale
> +		 * for current and voltage
> +		 * current(mA) * voltage(mV) = power (uW)
> +		 */
> +		case PAC1934_VPOWER_1_ADDR:
> +		case PAC1934_VPOWER_2_ADDR:
> +		case PAC1934_VPOWER_3_ADDR:
> +		case PAC1934_VPOWER_4_ADDR:
> +			*val = PAC1934_MAX_VPOWER_RSHIFTED_BY_28B;
> +			if (chan->scan_type.sign == 'u')
> +				*val2 = info->shunts[channel];
> +			else
> +				*val2 = info->shunts[channel] >> 1;
> +			return IIO_VAL_FRACTIONAL;
> +		case PAC1934_VPOWER_ACC_1_ADDR:
> +		case PAC1934_VPOWER_ACC_2_ADDR:
> +		case PAC1934_VPOWER_ACC_3_ADDR:
> +		case PAC1934_VPOWER_ACC_4_ADDR:
> +			/*
> +			 * expresses the 32 bit scale value
> +			 * here compute the scale for energy (miliWatt-second or miliJoule)
> +			 */
> +			*val = PAC1934_SCALE_CONSTANT;
> +
> +			if (chan->scan_type.sign == 'u')
> +				*val2 = info->shunts[channel];
> +			else
> +				*val2 = info->shunts[channel] >> 1;
> +			return IIO_VAL_FRACTIONAL;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = info->sample_rate_value;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;

Shouldn't be able to get here.

> +}
> +

> +static bool pac1934_acpi_parse_channel_config(struct i2c_client *client,
> +					      struct pac1934_chip_info *info)
> +{
> +	acpi_handle handle;
> +	union acpi_object *rez;
> +	struct device *dev = &client->dev;
> +	unsigned short bi_dir_mask;
> +	int idx, i;
> +	guid_t guid;
> +	const struct acpi_device_id *id;
> +
> +	handle = ACPI_HANDLE(&client->dev);
> +
> +	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> +	if (!id)
> +		return false;
> +
> +	guid_parse(PAC1934_DSM_UUID, &guid);
> +
> +	rez = acpi_evaluate_dsm(handle, &guid, 0, PAC1934_ACPI_GET_NAMES_AND_MOHMS_VALS, NULL);
> +	if (!rez)
> +		return false;
> +
> +	for (i = 0; i < rez->package.count; i += 2) {
> +		idx = i / 2;
> +		info->labels[idx] =
> +			devm_kmemdup(&client->dev, rez->package.elements[i].string.pointer,
> +				     (size_t)rez->package.elements[i].string.length + 1,
> +				     GFP_KERNEL);
> +		info->labels[idx][rez->package.elements[i].string.length] = '\0';
> +		info->shunts[idx] =
> +			rez->package.elements[i + 1].integer.value * 1000;
> +		info->active_channels[idx] = (info->shunts[idx] != 0);
> +	}
> +
> +	kfree(rez);

ACPI_FREE()? I think it's preferred as there is some ACPI tracking code for
allocations that might be enabled.


...

> +	return true;
> +}
> +

> +static int pac1934_chip_configure(struct pac1934_chip_info *info)
> +{
> +	int cnt, ret;
> +	struct i2c_client *client = info->client;
> +	u8 regs[PAC1934_CTRL_STATUS_INFO_LEN], idx, ctrl_reg;
> +	u32 wait_time;
> +
> +	cnt = 0;
No need.
> +	info->chip_reg_data.num_enabled_channels = 0;
> +	for (cnt = 0;  cnt < info->phys_channels; cnt++) {
> +		if (info->active_channels[cnt])
> +			info->chip_reg_data.num_enabled_channels++;
> +		cnt++;

? 0-day spotted this one.


> +	}
...




> +static IIO_DEVICE_ATTR(in_shunt_resistor_1, 0644, shunt_value_show, shunt_value_store, 0);
> +static IIO_DEVICE_ATTR(in_shunt_resistor_2, 0644, shunt_value_show, shunt_value_store, 0);

Put address as index.

> +static IIO_DEVICE_ATTR(in_shunt_resistor_3, 0644, shunt_value_show, shunt_value_store, 0);
> +static IIO_DEVICE_ATTR(in_shunt_resistor_4, 0644, shunt_value_show, shunt_value_store, 0);
> +static IIO_DEVICE_ATTR(reset_accumulators, 0200, NULL, reset_accumulators_store, 0);
> +
> +static struct attribute *pac1934_all_attributes[] = {
> +	PAC1934_DEV_ATTR(in_shunt_resistor_1),
> +	PAC1934_DEV_ATTR(in_shunt_resistor_2),
> +	PAC1934_DEV_ATTR(in_shunt_resistor_3),
> +	PAC1934_DEV_ATTR(in_shunt_resistor_4),
> +	PAC1934_DEV_ATTR(reset_accumulators),
> +	NULL
> +};
> +
> +static int pac1934_prep_custom_attributes(struct pac1934_chip_info *info,
> +					  struct iio_dev *indio_dev)
> +{
> +	int i, j, active_channels_count = 0;
> +	struct attribute **pac1934_custom_attributes;
> +	struct attribute_group *pac1934_group;
> +	struct i2c_client *client = info->client;
> +
> +	for (i = 0 ; i < info->phys_channels; i++)
> +		if (info->active_channels[i])
> +			active_channels_count++;
> +
> +	pac1934_group = devm_kzalloc(&client->dev, sizeof(*pac1934_group), GFP_KERNEL);
Can fail
> +
> +	pac1934_custom_attributes = devm_kzalloc(&client->dev,
> +						 (PAC1934_CUSTOM_ATTR_FOR_CHANNEL *
> +						 active_channels_count +
> +						 PAC1934_SHARED_DEVATTRS_COUNT)
> +						 * sizeof(*pac1934_group) + 1,
> +						 GFP_KERNEL);
as can this (in theory anyway and we should always handle that)

> +	j = 0;
> +
> +	for (i = 0 ; i < info->phys_channels; i++) {
> +		if (info->active_channels[i]) {
> +			pac1934_custom_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL * j] =
> +			pac1934_all_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL * i];

Indent the second line one more table as this is very hard to read.

I'm not sure copying it from that table is worth doing. I'd just build the table here
from the attributes.

> +			pac1934_custom_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL * j + 1] =
> +			pac1934_all_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL * i + 1];
> +			j++;
> +		}
> +	}
> +
> +	for (i = 0; i < PAC1934_SHARED_DEVATTRS_COUNT; i++)
> +		pac1934_custom_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL *
> +			active_channels_count + i] =
> +			pac1934_all_attributes[PAC1934_CUSTOM_ATTR_FOR_CHANNEL *
> +			info->phys_channels + i];
> +
> +	pac1934_group->attrs = pac1934_custom_attributes;
> +	info->pac1934_info.attrs = pac1934_group;
> +
> +	return 0;
> +}

> +static int pac1934_probe(struct i2c_client *client)
> +{
> +	struct pac1934_chip_info *info;
> +	const struct pac1934_features *chip;
> +	struct iio_dev *indio_dev;
> +	int cnt, ret;
> +	bool match = false;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	i2c_set_clientdata(client, indio_dev);
> +	info->client = client;
> +
> +	/*
> +	 * load default settings - all channels disabled,
> +	 * uni directional flow
> +	 */
> +	for (cnt = 0; cnt < PAC1934_MAX_NUM_CHANNELS; cnt++) {
> +		info->active_channels[cnt] = false;
> +		info->bi_dir[cnt] = false;

info is zero initialized. Could just not bother setting this as they
are already 0.  If you think it's useful documentation fine to leave it.
Hopefully the compiler will elide it anyway.

> +	}
> +
...


> +}
> +
> +static const struct i2c_device_id pac1934_id[] = {
> +	{ .name = "pac1931", .driver_data = (kernel_ulong_t)&pac1934_chip_config[PAC1931] },
> +	{ .name = "pac1932", .driver_data = (kernel_ulong_t)&pac1934_chip_config[PAC1932] },
> +	{ .name = "pac1933", .driver_data = (kernel_ulong_t)&pac1934_chip_config[PAC1933] },
> +	{ .name = "pac1934", .driver_data = (kernel_ulong_t)&pac1934_chip_config[PAC1934] },
> +	{}
> +};
> +

No blank line here.

> +MODULE_DEVICE_TABLE(i2c, pac1934_id);
> +
> +static const struct of_device_id pac1934_of_match[] = {
> +	{
> +		.compatible = "microchip,pac1931",
> +		.data = &pac1934_chip_config[PAC1931]
> +	},
> +	{
> +		.compatible = "microchip,pac1932",
> +		.data = &pac1934_chip_config[PAC1932]
> +	},
> +	{
> +		.compatible = "microchip,pac1933",
> +		.data = &pac1934_chip_config[PAC1933]
> +	},
> +	{
> +		.compatible = "microchip,pac1934",
> +		.data = &pac1934_chip_config[PAC1934]
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, pac1934_of_match);
> +
> +/* using MCHP1930 to be compatible with WINDOWS ACPI */

Presumably BIOS rather than windows, but I guess the point is clear :)

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

[Conor Dooley]

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

On Sat, Nov 25, 2023 at 07:47:54PM +0000, Jonathan Cameron wrote:
> On Thu, 16 Nov 2023 18:21:33 +0000
> Conor Dooley <conor@kernel.org> wrote:
> > On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote:
> > > On 15/11/2023 14:44, marius.cristea@microchip.com wrote:  
> > > > From: Marius Cristea <marius.cristea@microchip.com>

> > > > +patternProperties:
> > > > +  "^channel@[1-4]+$":
> > > > +    type: object
> > > > +    $ref: adc.yaml
> > > > +    description: Represents the external channels which are connected to the ADC.
> > > > +
> > > > +    properties:
> > > > +      reg:
> > > > +        items:
> > > > +          minimum: 1
> > > > +          maximum: 4
> > > > +
> > > > +      shunt-resistor-micro-ohms:
> > > > +        description: |
> > > > +          Value in micro Ohms of the shunt resistor connected between
> > > > +          the SENSE+ and SENSE- inputs, across which the current is measured. Value
> > > > +          is needed to compute the scaling of the measured current.
> > > > +
> > > > +    required:
> > > > +      - reg
> > > > +      - shunt-resistor-micro-ohms
> > > > +
> > > > +    unevaluatedProperties: false
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - "#address-cells"
> > > > +  - "#size-cells"
> > > > +
> > > > +allOf:
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            const: interrupts  
> > > 
> > > 
> > > I don't understand what do you want to say here. I am also 100% sure you
> > > did not test it on a real case (maybe example passes but nothing more).  
> > 
> > As far as I understand, the same pin on the device is used for both an
> > output or an input depending on the configuration. As an input, it is
> > the "slow-io" control, and as an output it is an interrupt.
> > I think Marius is trying to convey that either this pin can be in
> > exclusively one state or another.
> > 
> > _However_ I am not sure that that is really the right thing to do - they
> > might well be mutually exclusive modes, but I think the decision can be
> > made at runtime, rather than at devicetree creation time. Say for
> > example the GPIO controller this is connected to is capable of acting as
> > an interrupt controller. Unless I am misunderstanding the runtime
> > configurability of this hardware, I think it is possible to actually
> > provide a "slow-io-gpios" and an interrupt property & let the operating
> > system decide at runtime which mode it wants to work in.
> 
> I'll admit I've long forgotten what was going on here, but based just on
> this bit of text I agree. There is nothing 'stopping' us having a pin
> uses as either / or / both interrupt and gpio.
> 
> It'll be a bit messy to support in the driver as IIRC there are some sanity
> checks that limit combinations on IRQs and output GPIOS.  Can't remember
> how bad the dance to navigate it safely is.
> 
> First version I'd just say pick one option if both are provided and
> don't support configuring it at runtime.

Just to be clear, you are suggesting having the
"microchip,slow-io-gpios" and "interrupts" properties in the binding,
but the driver will just (for example) put that pin into alert mode
always & leave it there?
If that is what you are suggesting, that seems pragmatic to me.

Cheers,
Conor.


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

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

[Jonathan Cameron]

On Sun, 26 Nov 2023 11:24:56 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Sat, Nov 25, 2023 at 07:47:54PM +0000, Jonathan Cameron wrote:
> > On Thu, 16 Nov 2023 18:21:33 +0000
> > Conor Dooley <conor@kernel.org> wrote:  
> > > On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote:  
> > > > On 15/11/2023 14:44, marius.cristea@microchip.com wrote:    
> > > > > From: Marius Cristea <marius.cristea@microchip.com>  
> 
> > > > > +patternProperties:
> > > > > +  "^channel@[1-4]+$":
> > > > > +    type: object
> > > > > +    $ref: adc.yaml
> > > > > +    description: Represents the external channels which are connected to the ADC.
> > > > > +
> > > > > +    properties:
> > > > > +      reg:
> > > > > +        items:
> > > > > +          minimum: 1
> > > > > +          maximum: 4
> > > > > +
> > > > > +      shunt-resistor-micro-ohms:
> > > > > +        description: |
> > > > > +          Value in micro Ohms of the shunt resistor connected between
> > > > > +          the SENSE+ and SENSE- inputs, across which the current is measured. Value
> > > > > +          is needed to compute the scaling of the measured current.
> > > > > +
> > > > > +    required:
> > > > > +      - reg
> > > > > +      - shunt-resistor-micro-ohms
> > > > > +
> > > > > +    unevaluatedProperties: false
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - "#address-cells"
> > > > > +  - "#size-cells"
> > > > > +
> > > > > +allOf:
> > > > > +  - if:
> > > > > +      properties:
> > > > > +        compatible:
> > > > > +          contains:
> > > > > +            const: interrupts    
> > > > 
> > > > 
> > > > I don't understand what do you want to say here. I am also 100% sure you
> > > > did not test it on a real case (maybe example passes but nothing more).    
> > > 
> > > As far as I understand, the same pin on the device is used for both an
> > > output or an input depending on the configuration. As an input, it is
> > > the "slow-io" control, and as an output it is an interrupt.
> > > I think Marius is trying to convey that either this pin can be in
> > > exclusively one state or another.
> > > 
> > > _However_ I am not sure that that is really the right thing to do - they
> > > might well be mutually exclusive modes, but I think the decision can be
> > > made at runtime, rather than at devicetree creation time. Say for
> > > example the GPIO controller this is connected to is capable of acting as
> > > an interrupt controller. Unless I am misunderstanding the runtime
> > > configurability of this hardware, I think it is possible to actually
> > > provide a "slow-io-gpios" and an interrupt property & let the operating
> > > system decide at runtime which mode it wants to work in.  
> > 
> > I'll admit I've long forgotten what was going on here, but based just on
> > this bit of text I agree. There is nothing 'stopping' us having a pin
> > uses as either / or / both interrupt and gpio.
> > 
> > It'll be a bit messy to support in the driver as IIRC there are some sanity
> > checks that limit combinations on IRQs and output GPIOS.  Can't remember
> > how bad the dance to navigate it safely is.
> > 
> > First version I'd just say pick one option if both are provided and
> > don't support configuring it at runtime.  
> 
> Just to be clear, you are suggesting having the
> "microchip,slow-io-gpios" and "interrupts" properties in the binding,
> but the driver will just (for example) put that pin into alert mode
> always & leave it there?

Yes.

> If that is what you are suggesting, that seems pragmatic to me.

If a use case to do something else comes along later, then we can
be smarter, but I'd like to keep it simple initially at least.

Jonathan

> 
> Cheers,
> Conor.
> 

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

[Conor Dooley]

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

On Sun, Nov 26, 2023 at 04:04:38PM +0000, Jonathan Cameron wrote:
> On Sun, 26 Nov 2023 11:24:56 +0000
> Conor Dooley <conor@kernel.org> wrote:
> > On Sat, Nov 25, 2023 at 07:47:54PM +0000, Jonathan Cameron wrote:
> > > On Thu, 16 Nov 2023 18:21:33 +0000
> > > Conor Dooley <conor@kernel.org> wrote:  
> > > > On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote:  
> > > > > On 15/11/2023 14:44, marius.cristea@microchip.com wrote:    
> > > > > > From: Marius Cristea <marius.cristea@microchip.com>  
> > > > > > +patternProperties:
> > > > > > +  "^channel@[1-4]+$":
> > > > > > +    type: object
> > > > > > +    $ref: adc.yaml
> > > > > > +    description: Represents the external channels which are connected to the ADC.
> > > > > > +
> > > > > > +    properties:
> > > > > > +      reg:
> > > > > > +        items:
> > > > > > +          minimum: 1
> > > > > > +          maximum: 4
> > > > > > +
> > > > > > +      shunt-resistor-micro-ohms:
> > > > > > +        description: |
> > > > > > +          Value in micro Ohms of the shunt resistor connected between
> > > > > > +          the SENSE+ and SENSE- inputs, across which the current is measured. Value
> > > > > > +          is needed to compute the scaling of the measured current.
> > > > > > +
> > > > > > +    required:
> > > > > > +      - reg
> > > > > > +      - shunt-resistor-micro-ohms
> > > > > > +
> > > > > > +    unevaluatedProperties: false
> > > > > > +
> > > > > > +required:
> > > > > > +  - compatible
> > > > > > +  - reg
> > > > > > +  - "#address-cells"
> > > > > > +  - "#size-cells"
> > > > > > +
> > > > > > +allOf:
> > > > > > +  - if:
> > > > > > +      properties:
> > > > > > +        compatible:
> > > > > > +          contains:
> > > > > > +            const: interrupts    
> > > > > 
> > > > > 
> > > > > I don't understand what do you want to say here. I am also 100% sure you
> > > > > did not test it on a real case (maybe example passes but nothing more).    
> > > > 
> > > > As far as I understand, the same pin on the device is used for both an
> > > > output or an input depending on the configuration. As an input, it is
> > > > the "slow-io" control, and as an output it is an interrupt.
> > > > I think Marius is trying to convey that either this pin can be in
> > > > exclusively one state or another.
> > > > 
> > > > _However_ I am not sure that that is really the right thing to do - they
> > > > might well be mutually exclusive modes, but I think the decision can be
> > > > made at runtime, rather than at devicetree creation time. Say for
> > > > example the GPIO controller this is connected to is capable of acting as
> > > > an interrupt controller. Unless I am misunderstanding the runtime
> > > > configurability of this hardware, I think it is possible to actually
> > > > provide a "slow-io-gpios" and an interrupt property & let the operating
> > > > system decide at runtime which mode it wants to work in.  
> > > 
> > > I'll admit I've long forgotten what was going on here, but based just on
> > > this bit of text I agree. There is nothing 'stopping' us having a pin
> > > uses as either / or / both interrupt and gpio.
> > > 
> > > It'll be a bit messy to support in the driver as IIRC there are some sanity
> > > checks that limit combinations on IRQs and output GPIOS.  Can't remember
> > > how bad the dance to navigate it safely is.
> > > 
> > > First version I'd just say pick one option if both are provided and
> > > don't support configuring it at runtime.  
> > 
> > Just to be clear, you are suggesting having the
> > "microchip,slow-io-gpios" and "interrupts" properties in the binding,
> > but the driver will just (for example) put that pin into alert mode
> > always & leave it there?
> 
> Yes.
> 
> > If that is what you are suggesting, that seems pragmatic to me.
> 
> If a use case to do something else comes along later, then we can
> be smarter, but I'd like to keep it simple initially at least.

Sounds good to me, thanks Jonathan. Seems like a good compromise of
depicting the hardware accurately and not overcomplicating the driver
implementation.

Marius, I completely forgot to get in touch with you about this - give
me a shout on teams if there's anything outstanding that I can help you
with.

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