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

[PATCH V5 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 V4:
- Moved dt-binding comment added in V4 from patch #2 to patch #1.

Changes since V3:
- Added missing descriptions for new structure members that was reported
  by the kernel-test-bot.
- Added comment in the ina3221 dt-binding doc example to explain why we
  need to explicitly disable channels.
- Added more commentary in the commit message for the new DT property
  to explain that this property does not change the behaviour of the
  driver unless it is populated.

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 | 121 ++++++++++++++++++
 .../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                       |  33 ++++-
 6 files changed, 266 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 V5 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 | 102 ++++++++++++++++++
 2 files changed, 102 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..0fd8ae5f6a22
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
@@ -0,0 +1,102 @@
+# 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>;
+                /*
+                 * Input channels are enabled by default in the device and so
+                 * to disable, must be explicitly disabled in device-tree.
+                 */
+                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

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

[Rob Herring]

On Fri, 29 Sep 2023 11:36:47 +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 | 102 ++++++++++++++++++
>  2 files changed, 102 insertions(+), 54 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Guenter Roeck]

On Fri, Sep 29, 2023 at 11:36:47AM +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>
> Reviewed-by: Rob Herring <robh@kernel.org>

Applied.

Note that checkpatch complains

WARNING: DT binding docs and includes should be a separate patch.
See: Documentation/devicetree/bindings/submitting-patches.rst

but I take that as a false positive.

Guenter

[PATCH V5 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.

Note that the ina3221 has always supported summing the various input
channels and summation is enabled by default if the shunt-resistor
values are the same. This change simply provides a way to exclude
inputs from the summation. If this property is not populated, then the
functionality of the driver does not change.

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 0fd8ae5f6a22..5f10f1207d69 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

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

[Rob Herring]

On Fri, 29 Sep 2023 11:36:48 +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.
> 
> Note that the ina3221 has always supported summing the various input
> channels and summation is enabled by default if the shunt-resistor
> values are the same. This change simply provides a way to exclude
> inputs from the summation. If this property is not populated, then the
> functionality of the driver does not change.
> 
> 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(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Guenter Roeck]

On Fri, Sep 29, 2023 at 11:36:48AM +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.
> 
> Note that the ina3221 has always supported summing the various input
> channels and summation is enabled by default if the shunt-resistor
> values are the same. This change simply provides a way to exclude
> inputs from the summation. If this property is not populated, then the
> functionality of the driver does not change.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Applied.

Thanks,
Guenter

[PATCH V5 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 | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 5ab944056ec0..5ffdc94db436 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>
@@ -99,11 +100,13 @@ enum ina3221_channels {
  * @label: label of channel input source
  * @shunt_resistor: shunt resistor value of channel input source
  * @disconnected: connection status of channel input source
+ * @summation_disable: channel summation status of input source
  */
 struct ina3221_input {
 	const char *label;
 	int shunt_resistor;
 	bool disconnected;
+	bool summation_disable;
 };
 
 /**
@@ -113,8 +116,10 @@ struct ina3221_input {
  * @fields: Register fields of the device
  * @inputs: Array of channel input source specific structures
  * @lock: mutex lock to serialize sysfs attribute accesses
+ * @debugfs: Pointer to debugfs entry for device
  * @reg_config: Register value of INA3221_CONFIG
  * @summation_shunt_resistor: equivalent shunt resistor value for summation
+ * @summation_channel_control: Value written to SCC field in INA3221_MASK_ENABLE
  * @single_shot: running in single-shot operating mode
  */
 struct ina3221_data {
@@ -123,8 +128,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 +161,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 +794,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 +838,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 +885,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 +916,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 +943,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 +1005,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

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

[Jon Hunter]

Hi Jean, Guenter,

On 29/09/2023 11:36, Jon Hunter wrote:
> 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>


Any feedback on this?

Thanks
Jon

-- 
nvpublic

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

[Guenter Roeck]

On Fri, Sep 29, 2023 at 11:36:49AM +0100, Jon Hunter wrote:
> 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>

Applied.

Thanks,
Guenter

[PATCH V5 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 V5 0/4] hwmon: ina3221: Add selective summation support

[Thierry Reding]

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

On Fri, Sep 29, 2023 at 11:36:46AM +0100, Jon Hunter wrote:
> 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 V4:
> - Moved dt-binding comment added in V4 from patch #2 to patch #1.
> 
> Changes since V3:
> - Added missing descriptions for new structure members that was reported
>   by the kernel-test-bot.
> - Added comment in the ina3221 dt-binding doc example to explain why we
>   need to explicitly disable channels.
> - Added more commentary in the commit message for the new DT property
>   to explain that this property does not change the behaviour of the
>   driver unless it is populated.
> 
> 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

Jean, Guenter,

do you mind if I pick up patches 1, 2 and 4 into the Tegra tree? It's
usually convenient to keep the DT bindings and DT additions in the same
tree for validation.

Thanks,
Thierry

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

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

[Jon Hunter]

Hi Jean, Guenter,

On 11/10/2023 21:28, Thierry Reding wrote:
> On Fri, Sep 29, 2023 at 11:36:46AM +0100, Jon Hunter wrote:
>> 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 V4:
>> - Moved dt-binding comment added in V4 from patch #2 to patch #1.
>>
>> Changes since V3:
>> - Added missing descriptions for new structure members that was reported
>>    by the kernel-test-bot.
>> - Added comment in the ina3221 dt-binding doc example to explain why we
>>    need to explicitly disable channels.
>> - Added more commentary in the commit message for the new DT property
>>    to explain that this property does not change the behaviour of the
>>    driver unless it is populated.
>>
>> 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
> 
> Jean, Guenter,
> 
> do you mind if I pick up patches 1, 2 and 4 into the Tegra tree? It's
> usually convenient to keep the DT bindings and DT additions in the same
> tree for validation.


I have not seen any feedback on this from you? Please let me know if 
this version is now OK with you?

Thanks
Jon

-- 
nvpublic

Re: (subset) [PATCH V5 0/4] hwmon: ina3221: Add selective summation support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>


On Fri, 29 Sep 2023 11:36:46 +0100, Jon Hunter wrote:
> 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 V4:
> - Moved dt-binding comment added in V4 from patch #2 to patch #1.
> 
> [...]

Applied, thanks!

[4/4] arm64: tegra: Add power-sensors for Tegra234 boards
      commit: 9152ed09309de1a876680e6309c8eccb509b44b0

Best regards,
-- 
Thierry Reding <treding@nvidia.com>