[PATCH V3 0/4] hwmon: ina3221: Add selective summation support

[PATCH V3 0/4] hwmon: ina3221: Add selective summation support

[Jon Hunter]

The current INA3221 driver always sums the shunt voltage for all enabled
channels regardless of the shunt-resistor used for each channel. Summing
the shunt-voltage for channels is only meaningful if the shunt resistor
is the same for each channel. This series adds device-tree support to
allow which channels are summed in device-tree.

Changes since V2:
- Added note to binding-doc to indicate that input channels must be
  explicitly disabled.
- Corrected ordering of properties in the binding-doc
- Updated license for the binding-doc to be dual licensed.
- Changed newly added property from 'summation-bypass' to
  summation-disable'.
- Documented type for the new 'summation-disable' property.
- Corrected spelling and comments as per the feedback received.
- Used debugfs instead of sysfs for exposing the 'summation-disable'
  status for each input channel.
- Populated missing instances for the ina3221 device for Tegra234
  boards.
- Populated ina219 device for the NVIDIA IGX board (not strictly
  related to this series but related to populating all
  power-sensors for Tegra234 boards)

Changes since V1:
- Added yaml conversion patch for binding-doc
- Added binding-doc documentation patch for new property
- Added patch to populate ina3221 devices for Tegra234.

Jon Hunter (2):
  dt-bindings: hwmon: ina3221: Add ti,summation-disable
  arm64: tegra: Add power-sensors for Tegra234 boards

Ninad Malwade (2):
  dt-bindings: hwmon: ina3221: Convert to json-schema
  hwmon: ina3221: Add support for channel summation disable

 .../devicetree/bindings/hwmon/ina3221.txt     |  54 --------
 .../devicetree/bindings/hwmon/ti,ina3221.yaml | 117 ++++++++++++++++++
 .../boot/dts/nvidia/tegra234-p3701-0008.dtsi  |  33 +++++
 .../arm64/boot/dts/nvidia/tegra234-p3701.dtsi |  53 ++++++++
 .../arm64/boot/dts/nvidia/tegra234-p3767.dtsi |  29 +++++
 drivers/hwmon/ina3221.c                       |  30 ++++-
 6 files changed, 259 insertions(+), 57 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
 create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml

-- 
2.34.1

[PATCH V3 1/4] dt-bindings: hwmon: ina3221: Convert to json-schema

[Jon Hunter]

From: Ninad Malwade <nmalwade@nvidia.com>

Convert the TI INA3221 bindings from the free-form text format to
json-schema.

Note that the INA3221 input channels default to enabled in the chip.
Unless channels are explicitly disabled in device-tree, input
channels will be enabled.

Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 .../devicetree/bindings/hwmon/ina3221.txt     | 54 ----------
 .../devicetree/bindings/hwmon/ti,ina3221.yaml | 98 +++++++++++++++++++
 2 files changed, 98 insertions(+), 54 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
 create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt
deleted file mode 100644
index fa63b6171407..000000000000
--- a/Documentation/devicetree/bindings/hwmon/ina3221.txt
+++ /dev/null
@@ -1,54 +0,0 @@
-Texas Instruments INA3221 Device Tree Bindings
-
-1) ina3221 node
-  Required properties:
-  - compatible: Must be "ti,ina3221"
-  - reg: I2C address
-
-  Optional properties:
-  - ti,single-shot: This chip has two power modes: single-shot (chip takes one
-                    measurement and then shuts itself down) and continuous (
-                    chip takes continuous measurements). The continuous mode is
-                    more reliable and suitable for hardware monitor type device,
-                    but the single-shot mode is more power-friendly and useful
-                    for battery-powered device which cares power consumptions
-                    while still needs some measurements occasionally.
-                    If this property is present, the single-shot mode will be
-                    used, instead of the default continuous one for monitoring.
-
-  = The node contains optional child nodes for three channels =
-  = Each child node describes the information of input source =
-
-  - #address-cells: Required only if a child node is present. Must be 1.
-  - #size-cells: Required only if a child node is present. Must be 0.
-
-2) child nodes
-  Required properties:
-  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
-
-  Optional properties:
-  - label: Name of the input source
-  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
-
-Example:
-
-ina3221@40 {
-	compatible = "ti,ina3221";
-	reg = <0x40>;
-	#address-cells = <1>;
-	#size-cells = <0>;
-
-	input@0 {
-		reg = <0x0>;
-		status = "disabled";
-	};
-	input@1 {
-		reg = <0x1>;
-		shunt-resistor-micro-ohms = <5000>;
-	};
-	input@2 {
-		reg = <0x2>;
-		label = "VDD_5V";
-		shunt-resistor-micro-ohms = <5000>;
-	};
-};
diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
new file mode 100644
index 000000000000..d0e64a72af5b
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ti,ina3221.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments INA3221 Current and Voltage Monitor
+
+maintainers:
+  - Jean Delvare <jdelvare@suse.com>
+  - Guenter Roeck <linux@roeck-us.net>
+
+properties:
+  compatible:
+    const: ti,ina3221
+
+  reg:
+    maxItems: 1
+
+  ti,single-shot:
+    description: |
+      This chip has two power modes: single-shot (chip takes one measurement
+      and then shuts itself down) and continuous (chip takes continuous
+      measurements). The continuous mode is more reliable and suitable for
+      hardware monitor type device, but the single-shot mode is more power-
+      friendly and useful for battery-powered device which cares power
+      consumptions while still needs some measurements occasionally.
+
+      If this property is present, the single-shot mode will be used, instead
+      of the default continuous one for monitoring.
+    $ref: /schemas/types.yaml#/definitions/flag
+
+  "#address-cells":
+    description: Required only if a child node is present.
+    const: 1
+
+  "#size-cells":
+    description: Required only if a child node is present.
+    const: 0
+
+patternProperties:
+  "^input@[0-2]$":
+    description: The node contains optional child nodes for three channels.
+      Each child node describes the information of input source. Input channels
+      default to enabled in the chip. Unless channels are explicitly disabled
+      in device-tree, input channels will be enabled.
+    type: object
+    additionalProperties: false
+    properties:
+      reg:
+        description: Must be 0, 1 and 2, corresponding to the IN1, IN2 or IN3
+          ports of the INA3221, respectively.
+        enum: [ 0, 1, 2 ]
+
+      label:
+        description: name of the input source
+
+      shunt-resistor-micro-ohms:
+        description: shunt resistor value in micro-Ohm
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-sensor@40 {
+            compatible = "ti,ina3221";
+            reg = <0x40>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            input@0 {
+                reg = <0x0>;
+                status = "disabled";
+            };
+
+            input@1 {
+                reg = <0x1>;
+                shunt-resistor-micro-ohms = <5000>;
+            };
+
+            input@2 {
+                reg = <0x2>;
+                label = "VDD_5V";
+                shunt-resistor-micro-ohms = <5000>;
+            };
+        };
+    };
-- 
2.34.1

[PATCH V3 2/4] dt-bindings: hwmon: ina3221: Add ti,summation-disable

[Jon Hunter]

The INA3221 has a critical alert pin that can be controlled by the
summation control function. This function adds the single
shunt-voltage conversions for the desired channels in order to
compare the combined sum to the programmed limit. The Shunt-Voltage
Sum Limit register contains the programmed value that is compared
to the value in the Shunt-Voltage Sum register in order to
determine if the total summed limit is exceeded. If the
shunt-voltage sum limit value is exceeded, the critical alert pin
pulls low.

For the summation limit to have a meaningful value, it is necessary
to use the same shunt-resistor value on all included channels. Add a new
vendor specific property, 'ti,summation-disable', to allow specific
channels to be excluded from the summation control function if the shunt
resistor is different to other channels or the channel should not be
considered for triggering the critical alert pin.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
---
 .../devicetree/bindings/hwmon/ti,ina3221.yaml | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
index d0e64a72af5b..8afbe729076f 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
@@ -58,6 +58,25 @@ patternProperties:
       shunt-resistor-micro-ohms:
         description: shunt resistor value in micro-Ohm
 
+      ti,summation-disable:
+        description: |
+          The INA3221 has a critical alert pin that can be controlled by the
+          summation control function. This function adds the single
+          shunt-voltage conversions for the desired channels in order to
+          compare the combined sum to the programmed limit. The Shunt-Voltage
+          Sum Limit register contains the programmed value that is compared
+          to the value in the Shunt-Voltage Sum register in order to
+          determine if the total summed limit is exceeded. If the
+          shunt-voltage sum limit value is exceeded, the critical alert pin
+          is asserted.
+
+          For the summation limit to have a meaningful value, it is necessary
+          to use the same shunt-resistor value on all enabled channels. If
+          this is not the case or if a channel should not be used for
+          triggering the critical alert pin, then this property can be used
+          exclude specific channels from the summation control function.
+        type: boolean
+
     required:
       - reg
 
-- 
2.34.1

[PATCH V3 3/4] hwmon: ina3221: Add support for channel summation disable

[Jon Hunter]

From: Ninad Malwade <nmalwade@nvidia.com>

The INA3221 allows the Critical alert pin to be controlled by the
summation control function. This function adds the single
shunt-voltage conversions for the desired channels in order to compare
the combined sum to the programmed limit. The Shunt-Voltage Sum Limit
register contains the programmed value that is compared to the value in
the Shunt-Voltage Sum register in order to determine if the total summed
limit is exceeded. If the shunt-voltage sum limit value is exceeded, the
Critical alert pin pulls low.

For the summation limit to have a meaningful value, we have to use the
same shunt-resistor value on all included channels. Unless equal
shunt-resistor values are used for each channel, the summation control
function cannot be used and it is not enabled by the driver.

To address this, add support to disable the summation of specific
channels via device tree property "ti,summation-disable". The channel
which has this property would be excluded from the calculation of
summation control function.

For example, summation control function calculates Shunt-Voltage Sum as:

- input_shunt_voltage_summation = input_shunt_voltage_channel1
                                + input_shunt_voltage_channel2
                                + input_shunt_voltage_channel3

If we want the summation to only use channel1 and channel3, we can add
'ti,summation-disable' property in device tree node for channel2. Then
the calculation will skip channel2.

- input_shunt_voltage_summation = input_shunt_voltage_channel1
                                + input_shunt_voltage_channel3

Note that we only want the channel to be skipped for summation control
function rather than completely disabled. Therefore, even if we add the
property 'ti,summation-disable', the channel is still enabled and
functional.

Finally, create debugfs entries that display if summation is disabled
for each of the channels.

Signed-off-by: Rajkumar Kasirajan <rkasirajan@nvidia.com>
Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
Co-developed-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/hwmon/ina3221.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 5ab944056ec0..359b758e9f03 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -6,6 +6,7 @@
  *	Andrew F. Davis <afd@ti.com>
  */
 
+#include <linux/debugfs.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
@@ -104,6 +105,7 @@ struct ina3221_input {
 	const char *label;
 	int shunt_resistor;
 	bool disconnected;
+	bool summation_disable;
 };
 
 /**
@@ -123,8 +125,10 @@ struct ina3221_data {
 	struct regmap_field *fields[F_MAX_FIELDS];
 	struct ina3221_input inputs[INA3221_NUM_CHANNELS];
 	struct mutex lock;
+	struct dentry *debugfs;
 	u32 reg_config;
 	int summation_shunt_resistor;
+	u32 summation_channel_control;
 
 	bool single_shot;
 };
@@ -154,7 +158,8 @@ static inline int ina3221_summation_shunt_resistor(struct ina3221_data *ina)
 	int i, shunt_resistor = 0;
 
 	for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
-		if (input[i].disconnected || !input[i].shunt_resistor)
+		if (input[i].disconnected || !input[i].shunt_resistor ||
+		    input[i].summation_disable)
 			continue;
 		if (!shunt_resistor) {
 			/* Found the reference shunt resistor value */
@@ -786,6 +791,9 @@ static int ina3221_probe_child_from_dt(struct device *dev,
 	/* Save the connected input label if available */
 	of_property_read_string(child, "label", &input->label);
 
+	/* summation channel control */
+	input->summation_disable = of_property_read_bool(child, "ti,summation-disable");
+
 	/* Overwrite default shunt resistor value optionally */
 	if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", &val)) {
 		if (val < 1 || val > INT_MAX) {
@@ -827,6 +835,7 @@ static int ina3221_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct ina3221_data *ina;
 	struct device *hwmon_dev;
+	char name[32];
 	int i, ret;
 
 	ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL);
@@ -873,6 +882,10 @@ static int ina3221_probe(struct i2c_client *client)
 
 	/* Initialize summation_shunt_resistor for summation channel control */
 	ina->summation_shunt_resistor = ina3221_summation_shunt_resistor(ina);
+	for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
+		if (!ina->inputs[i].summation_disable)
+			ina->summation_channel_control |= BIT(14 - i);
+	}
 
 	ina->pm_dev = dev;
 	mutex_init(&ina->lock);
@@ -900,6 +913,15 @@ static int ina3221_probe(struct i2c_client *client)
 		goto fail;
 	}
 
+	scnprintf(name, sizeof(name), "%s-%s", INA3221_DRIVER_NAME, dev_name(dev));
+	ina->debugfs = debugfs_create_dir(name, NULL);
+
+	for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
+		scnprintf(name, sizeof(name), "in%d_summation_disable", i);
+		debugfs_create_bool(name, 0400, ina->debugfs,
+				    &ina->inputs[i].summation_disable);
+	}
+
 	return 0;
 
 fail:
@@ -918,6 +940,8 @@ static void ina3221_remove(struct i2c_client *client)
 	struct ina3221_data *ina = dev_get_drvdata(&client->dev);
 	int i;
 
+	debugfs_remove_recursive(ina->debugfs);
+
 	pm_runtime_disable(ina->pm_dev);
 	pm_runtime_set_suspended(ina->pm_dev);
 
@@ -978,13 +1002,13 @@ static int ina3221_resume(struct device *dev)
 	/* Initialize summation channel control */
 	if (ina->summation_shunt_resistor) {
 		/*
-		 * Take all three channels into summation by default
+		 * Sum only channels that are not disabled for summation.
 		 * Shunt measurements of disconnected channels should
 		 * be 0, so it does not matter for summation.
 		 */
 		ret = regmap_update_bits(ina->regmap, INA3221_MASK_ENABLE,
 					 INA3221_MASK_ENABLE_SCC_MASK,
-					 INA3221_MASK_ENABLE_SCC_MASK);
+					 ina->summation_channel_control);
 		if (ret) {
 			dev_err(dev, "Unable to control summation channel\n");
 			return ret;
-- 
2.34.1

[PATCH V3 4/4] arm64: tegra: Add power-sensors for Tegra234 boards

[Jon Hunter]

Populate the ina219 and ina3221 power-sensors for the various Tegra234
boards. These sensors are located on the Tegra234 module boards and the
configuration of some sensors is common across the different Tegra234
modules. Therefore, add any common sensor configurations to appropriate
device tree source file so it can be re-used across modules.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 .../boot/dts/nvidia/tegra234-p3701-0008.dtsi  | 33 ++++++++++++
 .../arm64/boot/dts/nvidia/tegra234-p3701.dtsi | 53 +++++++++++++++++++
 .../arm64/boot/dts/nvidia/tegra234-p3767.dtsi | 29 ++++++++++
 3 files changed, 115 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3701-0008.dtsi b/arch/arm64/boot/dts/nvidia/tegra234-p3701-0008.dtsi
index 62c4fdad0b60..553fa4ba1cd4 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234-p3701-0008.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234-p3701-0008.dtsi
@@ -44,6 +44,39 @@ i2c@c240000 {
 			status = "okay";
 		};
 
+		i2c@c250000 {
+			power-sensor@41 {
+				compatible = "ti,ina3221";
+				reg = <0x41>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				input@0 {
+					reg = <0x0>;
+					label = "CVB_ATX_12V";
+					shunt-resistor-micro-ohms = <2000>;
+				};
+
+				input@1 {
+					reg = <0x1>;
+					label = "CVB_ATX_3V3";
+					shunt-resistor-micro-ohms = <2000>;
+				};
+
+				input@2 {
+					reg = <0x2>;
+					label = "CVB_ATX_5V";
+					shunt-resistor-micro-ohms = <2000>;
+				};
+			};
+
+			power-sensor@44 {
+				compatible = "ti,ina219";
+				reg = <0x44>;
+				shunt-resistor = <2000>;
+			};
+		};
+
 		rtc@c2a0000 {
 			status = "okay";
 		};
diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3701.dtsi b/arch/arm64/boot/dts/nvidia/tegra234-p3701.dtsi
index 5e7797df50c2..db6ef711674a 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234-p3701.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234-p3701.dtsi
@@ -1987,5 +1987,58 @@ interrupt-controller@2a40000 {
 				status = "okay";
 			};
 		};
+
+		i2c@c240000 {
+			status = "okay";
+
+			power-sensor@40 {
+				compatible = "ti,ina3221";
+				reg = <0x40>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				input@0 {
+					reg = <0x0>;
+					label = "VDD_GPU_SOC";
+					shunt-resistor-micro-ohms = <2000>;
+				};
+
+				input@1 {
+					reg = <0x1>;
+					label = "VDD_CPU_CV";
+					shunt-resistor-micro-ohms = <2000>;
+				};
+
+				input@2 {
+					reg = <0x2>;
+					label = "VIN_SYS_5V0";
+					shunt-resistor-micro-ohms = <2000>;
+					ti,summation-disable;
+				};
+			};
+
+			power-sensor@41 {
+				compatible = "ti,ina3221";
+				reg = <0x41>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				input@0 {
+					reg = <0x0>;
+					status = "disabled";
+				};
+
+				input@1 {
+					reg = <0x1>;
+					label = "VDDQ_VDD2_1V8AO";
+					shunt-resistor-micro-ohms = <2000>;
+				};
+
+				input@2 {
+					reg = <0x2>;
+					status = "disabled";
+				};
+			};
+		};
 	};
 };
diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3767.dtsi b/arch/arm64/boot/dts/nvidia/tegra234-p3767.dtsi
index fe08e131b7b9..59c14ded5e9f 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234-p3767.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234-p3767.dtsi
@@ -55,6 +55,35 @@ padctl@3520000 {
 			avdd-usb-supply = <&vdd_3v3_ao>;
 		};
 
+		i2c@c240000 {
+			status = "okay";
+
+			power-sensor@40 {
+				compatible = "ti,ina3221";
+				reg = <0x40>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				input@0 {
+					reg = <0x0>;
+					label = "VDD_IN";
+					shunt-resistor-micro-ohms = <5000>;
+				};
+
+				input@1 {
+					reg = <0x1>;
+					label = "VDD_CPU_GPU_CV";
+					shunt-resistor-micro-ohms = <5000>;
+				};
+
+				input@2 {
+					reg = <0x2>;
+					label = "VDD_SOC";
+					shunt-resistor-micro-ohms = <5000>;
+				};
+			};
+		};
+
 		rtc@c2a0000 {
 			status = "okay";
 		};
-- 
2.34.1

Re: [PATCH V3 3/4] hwmon: ina3221: Add support for channel summation disable

[kernel test robot]

Hi Jon,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on robh/for-next linus/master v6.6-rc2 next-20230921]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jon-Hunter/dt-bindings-hwmon-ina3221-Add-ti-summation-disable/20230922-011119
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230921130818.21247-4-jonathanh%40nvidia.com
patch subject: [PATCH V3 3/4] hwmon: ina3221: Add support for channel summation disable
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230922/202309220454.kCi2xD48-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230922/202309220454.kCi2xD48-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/202309220454.kCi2xD48-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hwmon/ina3221.c:109: warning: Function parameter or member 'summation_disable' not described in 'ina3221_input'
>> drivers/hwmon/ina3221.c:134: warning: Function parameter or member 'debugfs' not described in 'ina3221_data'
>> drivers/hwmon/ina3221.c:134: warning: Function parameter or member 'summation_channel_control' not described in 'ina3221_data'


vim +109 drivers/hwmon/ina3221.c

7cb6dcff1956ec Andrew F. Davis 2016-06-10   97  
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01   98  /**
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01   99   * struct ina3221_input - channel input source specific information
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  100   * @label: label of channel input source
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  101   * @shunt_resistor: shunt resistor value of channel input source
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  102   * @disconnected: connection status of channel input source
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  103   */
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  104  struct ina3221_input {
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  105  	const char *label;
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  106  	int shunt_resistor;
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  107  	bool disconnected;
ac0a832fd3617c Ninad Malwade   2023-09-21  108  	bool summation_disable;
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01 @109  };
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  110  
7cb6dcff1956ec Andrew F. Davis 2016-06-10  111  /**
7cb6dcff1956ec Andrew F. Davis 2016-06-10  112   * struct ina3221_data - device specific information
323aeb0eb5d9a6 Nicolin Chen    2018-11-05  113   * @pm_dev: Device pointer for pm runtime
7cb6dcff1956ec Andrew F. Davis 2016-06-10  114   * @regmap: Register map of the device
7cb6dcff1956ec Andrew F. Davis 2016-06-10  115   * @fields: Register fields of the device
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  116   * @inputs: Array of channel input source specific structures
87625b24986bc2 Nicolin Chen    2018-11-05  117   * @lock: mutex lock to serialize sysfs attribute accesses
59d608e152e582 Nicolin Chen    2018-09-29  118   * @reg_config: Register value of INA3221_CONFIG
2057bdfb7184e9 Nicolin Chen    2019-10-16  119   * @summation_shunt_resistor: equivalent shunt resistor value for summation
43dece162de047 Nicolin Chen    2019-01-17  120   * @single_shot: running in single-shot operating mode
7cb6dcff1956ec Andrew F. Davis 2016-06-10  121   */
7cb6dcff1956ec Andrew F. Davis 2016-06-10  122  struct ina3221_data {
323aeb0eb5d9a6 Nicolin Chen    2018-11-05  123  	struct device *pm_dev;
7cb6dcff1956ec Andrew F. Davis 2016-06-10  124  	struct regmap *regmap;
7cb6dcff1956ec Andrew F. Davis 2016-06-10  125  	struct regmap_field *fields[F_MAX_FIELDS];
a9e9dd9c6de5d8 Nicolin Chen    2018-10-01  126  	struct ina3221_input inputs[INA3221_NUM_CHANNELS];
87625b24986bc2 Nicolin Chen    2018-11-05  127  	struct mutex lock;
ac0a832fd3617c Ninad Malwade   2023-09-21  128  	struct dentry *debugfs;
59d608e152e582 Nicolin Chen    2018-09-29  129  	u32 reg_config;
2057bdfb7184e9 Nicolin Chen    2019-10-16  130  	int summation_shunt_resistor;
ac0a832fd3617c Ninad Malwade   2023-09-21  131  	u32 summation_channel_control;
43dece162de047 Nicolin Chen    2019-01-17  132  
43dece162de047 Nicolin Chen    2019-01-17  133  	bool single_shot;
7cb6dcff1956ec Andrew F. Davis 2016-06-10 @134  };
7cb6dcff1956ec Andrew F. Davis 2016-06-10  135  

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

Re: [PATCH V3 1/4] dt-bindings: hwmon: ina3221: Convert to json-schema

[Rob Herring]

On Thu, Sep 21, 2023 at 02:08:15PM +0100, Jon Hunter wrote:
> From: Ninad Malwade <nmalwade@nvidia.com>
> 
> Convert the TI INA3221 bindings from the free-form text format to
> json-schema.
> 
> Note that the INA3221 input channels default to enabled in the chip.
> Unless channels are explicitly disabled in device-tree, input
> channels will be enabled.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  .../devicetree/bindings/hwmon/ina3221.txt     | 54 ----------
>  .../devicetree/bindings/hwmon/ti,ina3221.yaml | 98 +++++++++++++++++++
>  2 files changed, 98 insertions(+), 54 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt
> deleted file mode 100644
> index fa63b6171407..000000000000
> --- a/Documentation/devicetree/bindings/hwmon/ina3221.txt
> +++ /dev/null
> @@ -1,54 +0,0 @@
> -Texas Instruments INA3221 Device Tree Bindings
> -
> -1) ina3221 node
> -  Required properties:
> -  - compatible: Must be "ti,ina3221"
> -  - reg: I2C address
> -
> -  Optional properties:
> -  - ti,single-shot: This chip has two power modes: single-shot (chip takes one
> -                    measurement and then shuts itself down) and continuous (
> -                    chip takes continuous measurements). The continuous mode is
> -                    more reliable and suitable for hardware monitor type device,
> -                    but the single-shot mode is more power-friendly and useful
> -                    for battery-powered device which cares power consumptions
> -                    while still needs some measurements occasionally.
> -                    If this property is present, the single-shot mode will be
> -                    used, instead of the default continuous one for monitoring.
> -
> -  = The node contains optional child nodes for three channels =
> -  = Each child node describes the information of input source =
> -
> -  - #address-cells: Required only if a child node is present. Must be 1.
> -  - #size-cells: Required only if a child node is present. Must be 0.
> -
> -2) child nodes
> -  Required properties:
> -  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
> -
> -  Optional properties:
> -  - label: Name of the input source
> -  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
> -
> -Example:
> -
> -ina3221@40 {
> -	compatible = "ti,ina3221";
> -	reg = <0x40>;
> -	#address-cells = <1>;
> -	#size-cells = <0>;
> -
> -	input@0 {
> -		reg = <0x0>;
> -		status = "disabled";
> -	};
> -	input@1 {
> -		reg = <0x1>;
> -		shunt-resistor-micro-ohms = <5000>;
> -	};
> -	input@2 {
> -		reg = <0x2>;
> -		label = "VDD_5V";
> -		shunt-resistor-micro-ohms = <5000>;
> -	};
> -};
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> new file mode 100644
> index 000000000000..d0e64a72af5b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ti,ina3221.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments INA3221 Current and Voltage Monitor
> +
> +maintainers:
> +  - Jean Delvare <jdelvare@suse.com>
> +  - Guenter Roeck <linux@roeck-us.net>

This should be someone with the h/w. You or the original author Nicolin 
(now Cc'ed)?

> +
> +properties:
> +  compatible:
> +    const: ti,ina3221
> +
> +  reg:
> +    maxItems: 1
> +
> +  ti,single-shot:
> +    description: |
> +      This chip has two power modes: single-shot (chip takes one measurement
> +      and then shuts itself down) and continuous (chip takes continuous
> +      measurements). The continuous mode is more reliable and suitable for
> +      hardware monitor type device, but the single-shot mode is more power-
> +      friendly and useful for battery-powered device which cares power
> +      consumptions while still needs some measurements occasionally.
> +
> +      If this property is present, the single-shot mode will be used, instead
> +      of the default continuous one for monitoring.
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
> +  "#address-cells":
> +    description: Required only if a child node is present.

That's always the case. Drop

> +    const: 1
> +
> +  "#size-cells":
> +    description: Required only if a child node is present.
> +    const: 0
> +
> +patternProperties:
> +  "^input@[0-2]$":
> +    description: The node contains optional child nodes for three channels.
> +      Each child node describes the information of input source. Input channels
> +      default to enabled in the chip. Unless channels are explicitly disabled
> +      in device-tree, input channels will be enabled.
> +    type: object
> +    additionalProperties: false
> +    properties:
> +      reg:
> +        description: Must be 0, 1 and 2, corresponding to the IN1, IN2 or IN3
> +          ports of the INA3221, respectively.
> +        enum: [ 0, 1, 2 ]
> +
> +      label:
> +        description: name of the input source
> +
> +      shunt-resistor-micro-ohms:
> +        description: shunt resistor value in micro-Ohm
> +
> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        power-sensor@40 {
> +            compatible = "ti,ina3221";
> +            reg = <0x40>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            input@0 {
> +                reg = <0x0>;
> +                status = "disabled";

Examples should be enabled.

> +            };
> +
> +            input@1 {
> +                reg = <0x1>;
> +                shunt-resistor-micro-ohms = <5000>;
> +            };
> +
> +            input@2 {
> +                reg = <0x2>;
> +                label = "VDD_5V";
> +                shunt-resistor-micro-ohms = <5000>;
> +            };
> +        };
> +    };
> -- 
> 2.34.1
> 

Re: [PATCH V3 2/4] dt-bindings: hwmon: ina3221: Add ti,summation-disable

[Rob Herring]

On Thu, Sep 21, 2023 at 02:08:16PM +0100, Jon Hunter wrote:
> The INA3221 has a critical alert pin that can be controlled by the
> summation control function. This function adds the single
> shunt-voltage conversions for the desired channels in order to
> compare the combined sum to the programmed limit. The Shunt-Voltage
> Sum Limit register contains the programmed value that is compared
> to the value in the Shunt-Voltage Sum register in order to
> determine if the total summed limit is exceeded. If the
> shunt-voltage sum limit value is exceeded, the critical alert pin
> pulls low.
> 
> For the summation limit to have a meaningful value, it is necessary
> to use the same shunt-resistor value on all included channels. Add a new
> vendor specific property, 'ti,summation-disable', to allow specific
> channels to be excluded from the summation control function if the shunt
> resistor is different to other channels or the channel should not be
> considered for triggering the critical alert pin.

You are adding this feature to the driver, but requiring a new property 
to disable it. So what happens with an existing user (old DT) and a 
kernel with the new feature?

Rob

Re: [PATCH V3 1/4] dt-bindings: hwmon: ina3221: Convert to json-schema

[Jon Hunter]

On 22/09/2023 22:01, Rob Herring wrote:
> On Thu, Sep 21, 2023 at 02:08:15PM +0100, Jon Hunter wrote:
>> From: Ninad Malwade <nmalwade@nvidia.com>
>>
>> Convert the TI INA3221 bindings from the free-form text format to
>> json-schema.
>>
>> Note that the INA3221 input channels default to enabled in the chip.
>> Unless channels are explicitly disabled in device-tree, input
>> channels will be enabled.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>   .../devicetree/bindings/hwmon/ina3221.txt     | 54 ----------
>>   .../devicetree/bindings/hwmon/ti,ina3221.yaml | 98 +++++++++++++++++++
>>   2 files changed, 98 insertions(+), 54 deletions(-)
>>   delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml


...

>> +examples:
>> +  - |
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        power-sensor@40 {
>> +            compatible = "ti,ina3221";
>> +            reg = <0x40>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            input@0 {
>> +                reg = <0x0>;
>> +                status = "disabled";
> 
> Examples should be enabled.


Yes normally that would be the case. However, per the discussion with 
Guenter and the comment in the changelog, for this device channels are 
enabled in the chip by default. And so to disable them, we need to 
explicitly disable in device-tree.

Jon

-- 
nvpublic

Re: [PATCH V3 2/4] dt-bindings: hwmon: ina3221: Add ti,summation-disable

[Jon Hunter]

On 22/09/2023 22:06, Rob Herring wrote:
> On Thu, Sep 21, 2023 at 02:08:16PM +0100, Jon Hunter wrote:
>> The INA3221 has a critical alert pin that can be controlled by the
>> summation control function. This function adds the single
>> shunt-voltage conversions for the desired channels in order to
>> compare the combined sum to the programmed limit. The Shunt-Voltage
>> Sum Limit register contains the programmed value that is compared
>> to the value in the Shunt-Voltage Sum register in order to
>> determine if the total summed limit is exceeded. If the
>> shunt-voltage sum limit value is exceeded, the critical alert pin
>> pulls low.
>>
>> For the summation limit to have a meaningful value, it is necessary
>> to use the same shunt-resistor value on all included channels. Add a new
>> vendor specific property, 'ti,summation-disable', to allow specific
>> channels to be excluded from the summation control function if the shunt
>> resistor is different to other channels or the channel should not be
>> considered for triggering the critical alert pin.
> 
> You are adding this feature to the driver, but requiring a new property
> to disable it. So what happens with an existing user (old DT) and a
> kernel with the new feature?


Not exactly. The summation support has always been supported in the 
driver and is enabled (if the shunt resistors for all channels are the 
same). What we want to do is support summation but only for a subset of 
channels which is not supported today. So this new property allows us to 
explicitly tell the driver not to include a specific channel in the 
summation.

Jon

-- 
nvpublic

Re: [PATCH V3 1/4] dt-bindings: hwmon: ina3221: Convert to json-schema

[Conor Dooley]

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

On Mon, Sep 25, 2023 at 11:46:58AM +0100, Jon Hunter wrote:
> 
> 
> On 22/09/2023 22:01, Rob Herring wrote:
> > On Thu, Sep 21, 2023 at 02:08:15PM +0100, Jon Hunter wrote:
> > > From: Ninad Malwade <nmalwade@nvidia.com>
> > > 
> > > Convert the TI INA3221 bindings from the free-form text format to
> > > json-schema.
> > > 
> > > Note that the INA3221 input channels default to enabled in the chip.
> > > Unless channels are explicitly disabled in device-tree, input
> > > channels will be enabled.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
> > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> > > ---
> > >   .../devicetree/bindings/hwmon/ina3221.txt     | 54 ----------
> > >   .../devicetree/bindings/hwmon/ti,ina3221.yaml | 98 +++++++++++++++++++
> > >   2 files changed, 98 insertions(+), 54 deletions(-)
> > >   delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
> > >   create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> 
> 
> ...
> 
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        power-sensor@40 {
> > > +            compatible = "ti,ina3221";
> > > +            reg = <0x40>;
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +
> > > +            input@0 {
> > > +                reg = <0x0>;
> > > +                status = "disabled";
> > 
> > Examples should be enabled.
> 
> 
> Yes normally that would be the case. However, per the discussion with
> Guenter and the comment in the changelog, for this device channels are
> enabled in the chip by default. And so to disable them, we need to
> explicitly disable in device-tree.

Maybe a comment at this location would be good then, to point out that
this is what you are trying to demonstrate with this example?


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

Re: [PATCH V3 1/4] dt-bindings: hwmon: ina3221: Convert to json-schema

[Jon Hunter]

On 26/09/2023 10:18, Conor Dooley wrote:
> On Mon, Sep 25, 2023 at 11:46:58AM +0100, Jon Hunter wrote:
>>
>>
>> On 22/09/2023 22:01, Rob Herring wrote:
>>> On Thu, Sep 21, 2023 at 02:08:15PM +0100, Jon Hunter wrote:
>>>> From: Ninad Malwade <nmalwade@nvidia.com>
>>>>
>>>> Convert the TI INA3221 bindings from the free-form text format to
>>>> json-schema.
>>>>
>>>> Note that the INA3221 input channels default to enabled in the chip.
>>>> Unless channels are explicitly disabled in device-tree, input
>>>> channels will be enabled.
>>>>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>> ---
>>>>    .../devicetree/bindings/hwmon/ina3221.txt     | 54 ----------
>>>>    .../devicetree/bindings/hwmon/ti,ina3221.yaml | 98 +++++++++++++++++++
>>>>    2 files changed, 98 insertions(+), 54 deletions(-)
>>>>    delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
>>>>    create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>>
>>
>> ...
>>
>>>> +examples:
>>>> +  - |
>>>> +    i2c {
>>>> +        #address-cells = <1>;
>>>> +        #size-cells = <0>;
>>>> +
>>>> +        power-sensor@40 {
>>>> +            compatible = "ti,ina3221";
>>>> +            reg = <0x40>;
>>>> +            #address-cells = <1>;
>>>> +            #size-cells = <0>;
>>>> +
>>>> +            input@0 {
>>>> +                reg = <0x0>;
>>>> +                status = "disabled";
>>>
>>> Examples should be enabled.
>>
>>
>> Yes normally that would be the case. However, per the discussion with
>> Guenter and the comment in the changelog, for this device channels are
>> enabled in the chip by default. And so to disable them, we need to
>> explicitly disable in device-tree.
> 
> Maybe a comment at this location would be good then, to point out that
> this is what you are trying to demonstrate with this example?
> 

OK yes I think that that will help convey that this is intentional in 
this case.

Thanks
Jon

-- 
nvpublic