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

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

[Ninad Malwade]

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.

Jon Hunter (2):
  dt-bindings: hwmon: ina3221: Add summation-bypass
  arm64: tegra: Populate ina3221 for Tegra234 boards

Ninad Malwade (1):
  hwmon: ina3221: Add selective support for summation channel control

Thierry Reding (1):
  dt-bindings: hwmon: ina3221: Convert to json-schema

 .../devicetree/bindings/hwmon/ina3221.txt     |  54 --------
 .../devicetree/bindings/hwmon/ti,ina3221.yaml | 127 ++++++++++++++++++
 .../arm64/boot/dts/nvidia/tegra234-p3701.dtsi |  53 ++++++++
 .../arm64/boot/dts/nvidia/tegra234-p3767.dtsi |  29 ++++
 drivers/hwmon/ina3221.c                       |  39 +++++-
 5 files changed, 243 insertions(+), 59 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
 create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml

-- 
2.17.1

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

[Ninad Malwade]

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

Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
---
 .../devicetree/bindings/hwmon/ina3221.txt     |  54 ---------
 .../devicetree/bindings/hwmon/ti,ina3221.yaml | 109 ++++++++++++++++++
 2 files changed, 109 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..0c6d41423d8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%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.
+    type: object
+    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
+
+    additionalProperties: false
+
+    required:
+      - reg
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/clock/tegra186-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/reset/tegra186-reset.h>
+
+    i2c@3160000 {
+        compatible = "nvidia,tegra186-i2c";
+        reg = <0x03160000 0x10000>;
+        interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&bpmp TEGRA186_CLK_I2C1>;
+        clock-names = "div-clk";
+        resets = <&bpmp TEGRA186_RESET_I2C1>;
+        reset-names = "i2c";
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        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>;
+            };
+        };
+    };
-- 
2.17.1

[PATCH V2 2/4] dt-bindings: hwmon: ina3221: Add summation-bypass

[Ninad Malwade]

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 property, 'summation-bypass', to allow specific channels to be
excluded from the summation control function if the shunt resistor
is different to other channels.

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

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
index 0c6d41423d8c..20c23febf575 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
@@ -55,6 +55,24 @@ patternProperties:
       shunt-resistor-micro-ohms:
         description: shunt resistor value in micro-Ohm
 
+      summation-bypass:
+        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
+          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. If
+          this is not the case for specific channels, then the
+          'summation-bypass' can be populated for a specific channel to
+          exclude from the summation control function.
+
     additionalProperties: false
 
     required:
-- 
2.17.1

[PATCH V2 3/4] hwmon: ina3221: add support for summation channel control

[Ninad Malwade]

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, we can't use summation control function to add
the individual conversion values directly together in the
Shunt-Voltage Sum register to report the total current.

To address this we add support to BYPASS channels
via kernel device tree property "summation-bypass".

The channel which has this property would be excluded from
the calculation of summation control function, so we can easily
exclude the one which uses different shunt-resistor value or
bus voltage.

For example, summation control function calculates
Shunt-Voltage Sum like
- input_shunt_voltage_summaion = input_shunt_voltage_channel1
                               + input_shunt_voltage_channel2
                               + input_shunt_voltage_channel3

But if we want the summation to consider only channel1
and channel3, we can add 'summation-bypass' property
in device tree node of channel2.

Then the calculation will skip channel2.
- input_shunt_voltage_summaion = input_shunt_voltage_channel1
                               + input_shunt_voltage_channel3

Please note that we only want the channel to be skipped
for summation control function rather than completely disabled.
Therefore, even if we add the device tree node, the functionality
of the single channel would still be retained.

The below sysfs nodes are added to check if the channel is included
or excluded from the summation control function.

in*_sum_bypass = 0 --> channel voltage is included for sum of
                       shunt voltages.

in*_sum_bypass = 1 --> channel voltage is excluded for sum
                       of shunt voltages.

Signed-off-by: Rajkumar Kasirajan <rkasirajan@nvidia.com>
Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
---
 drivers/hwmon/ina3221.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 5ab944056ec0..093ebf9f1f8d 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -104,6 +104,7 @@ struct ina3221_input {
 	const char *label;
 	int shunt_resistor;
 	bool disconnected;
+	bool summation_bypass;
 };
 
 /**
@@ -125,6 +126,7 @@ struct ina3221_data {
 	struct mutex lock;
 	u32 reg_config;
 	int summation_shunt_resistor;
+	u32 summation_channel_control;
 
 	bool single_shot;
 };
@@ -154,7 +156,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_bypass)
 			continue;
 		if (!shunt_resistor) {
 			/* Found the reference shunt resistor value */
@@ -731,10 +734,29 @@ static SENSOR_DEVICE_ATTR_RW(shunt1_resistor, ina3221_shunt, INA3221_CHANNEL1);
 static SENSOR_DEVICE_ATTR_RW(shunt2_resistor, ina3221_shunt, INA3221_CHANNEL2);
 static SENSOR_DEVICE_ATTR_RW(shunt3_resistor, ina3221_shunt, INA3221_CHANNEL3);
 
+static ssize_t ina3221_summation_bypass_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	unsigned int channel = sd_attr->index;
+	struct ina3221_input *input = &ina->inputs[channel];
+
+	return sysfs_emit(buf, "%d\n", input->summation_bypass);
+}
+
+/* summation bypass */
+static SENSOR_DEVICE_ATTR_RO(in1_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL1);
+static SENSOR_DEVICE_ATTR_RO(in2_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL2);
+static SENSOR_DEVICE_ATTR_RO(in3_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL3);
+
 static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_shunt1_resistor.dev_attr.attr,
 	&sensor_dev_attr_shunt2_resistor.dev_attr.attr,
 	&sensor_dev_attr_shunt3_resistor.dev_attr.attr,
+	&sensor_dev_attr_in1_sum_bypass.dev_attr.attr,
+	&sensor_dev_attr_in2_sum_bypass.dev_attr.attr,
+	&sensor_dev_attr_in3_sum_bypass.dev_attr.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(ina3221);
@@ -786,6 +808,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_bypass = of_property_read_bool(child, "summation-bypass");
+
 	/* Overwrite default shunt resistor value optionally */
 	if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", &val)) {
 		if (val < 1 || val > INT_MAX) {
@@ -873,6 +898,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_bypass)
+			ina->summation_channel_control |= (BIT(14 - i));
+	}
 
 	ina->pm_dev = dev;
 	mutex_init(&ina->lock);
@@ -978,13 +1007,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
-		 * Shunt measurements of disconnected channels should
-		 * be 0, so it does not matter for summation.
+		 * Sum only channels that are not 'bypassed' for summation
+		 * by default. 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.17.1

[PATCH V2 4/4] arm64: tegra: Populate ina3221 for Tegra234 boards

[Ninad Malwade]

Populate the ina3221 current/voltage monitor sensors for the various
Tegra234 boards. The ina3221 sensors are located on the Tegra234 module
boards. The sensor configuration for the Jetson AGX Orin and NVIDIA
IGX Orin boards are the same and the sensor configuration for Jetson Orin
NX and Jetson Orin Nano are the same.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
---
 .../arm64/boot/dts/nvidia/tegra234-p3701.dtsi | 53 +++++++++++++++++++
 .../arm64/boot/dts/nvidia/tegra234-p3767.dtsi | 29 ++++++++++
 2 files changed, 82 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3701.dtsi b/arch/arm64/boot/dts/nvidia/tegra234-p3701.dtsi
index 5e7797df50c2..aaab248c8844 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234-p3701.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234-p3701.dtsi
@@ -1987,5 +1987,58 @@
 				status = "okay";
 			};
 		};
+
+		i2c@c240000 {
+			status = "okay";
+
+			ina3221@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>;
+					summation-bypass;
+				};
+			};
+
+			ina3221@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..e4ce7da7850c 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234-p3767.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234-p3767.dtsi
@@ -55,6 +55,35 @@
 			avdd-usb-supply = <&vdd_3v3_ao>;
 		};
 
+		i2c@c240000 {
+			status = "okay";
+
+			ina3221@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.17.1

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

[Guenter Roeck]

On Sat, Aug 26, 2023 at 12:42:45AM +0800, Ninad Malwade 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.
> 

V2, but no change log. I am not going to review this series.

Guenter

> Jon Hunter (2):
>   dt-bindings: hwmon: ina3221: Add summation-bypass
>   arm64: tegra: Populate ina3221 for Tegra234 boards
> 
> Ninad Malwade (1):
>   hwmon: ina3221: Add selective support for summation channel control
> 
> Thierry Reding (1):
>   dt-bindings: hwmon: ina3221: Convert to json-schema
> 
>  .../devicetree/bindings/hwmon/ina3221.txt     |  54 --------
>  .../devicetree/bindings/hwmon/ti,ina3221.yaml | 127 ++++++++++++++++++
>  .../arm64/boot/dts/nvidia/tegra234-p3701.dtsi |  53 ++++++++
>  .../arm64/boot/dts/nvidia/tegra234-p3767.dtsi |  29 ++++
>  drivers/hwmon/ina3221.c                       |  39 +++++-
>  5 files changed, 243 insertions(+), 59 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> 
> -- 
> 2.17.1
> 

Re: [PATCH V2 3/4] hwmon: ina3221: add support for summation channel control

[kernel test robot]

Hi Ninad,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20230825]
[also build test WARNING on linus/master v6.5-rc7]
[cannot apply to groeck-staging/hwmon-next robh/for-next v6.5-rc7 v6.5-rc6 v6.5-rc5]
[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/Ninad-Malwade/dt-bindings-hwmon-ina3221-Convert-to-json-schema/20230826-004606
base:   next-20230825
patch link:    https://lore.kernel.org/r/20230825164249.22860-4-nmalwade%40nvidia.com
patch subject: [PATCH V2 3/4] hwmon: ina3221: add support for summation channel control
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230826/202308260438.7OwsGAd8-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230826/202308260438.7OwsGAd8-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/202308260438.7OwsGAd8-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hwmon/ina3221.c:108: warning: Function parameter or member 'summation_bypass' not described in 'ina3221_input'
>> drivers/hwmon/ina3221.c:132: warning: Function parameter or member 'summation_channel_control' not described in 'ina3221_data'


vim +108 drivers/hwmon/ina3221.c

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

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

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

[Krzysztof Kozlowski]

On 25/08/2023 18:42, Ninad Malwade wrote:
> Convert the TI INA3221 bindings from the free-form text format to
> json-schema.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
> ---

This is v2, so where is the changelog?

>  .../devicetree/bindings/hwmon/ina3221.txt     |  54 ---------
>  .../devicetree/bindings/hwmon/ti,ina3221.yaml | 109 ++++++++++++++++++
>  2 files changed, 109 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/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> new file mode 100644
> index 000000000000..0c6d41423d8c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> @@ -0,0 +1,109 @@
> +# SPDX-License-Identifier: GPL-2.0-only

I assume you do not use standard license because of copying the description?

> +%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.
> +    type: object
> +    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
> +
> +    additionalProperties: false

This should be rather after type:object for readability.

> +
> +    required:
> +      - reg
> +
> +additionalProperties: false

And this please keep like in example schema, so after required:.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/tegra186-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/reset/tegra186-reset.h>
> +
> +    i2c@3160000 {
> +        compatible = "nvidia,tegra186-i2c";
> +        reg = <0x03160000 0x10000>;
> +        interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&bpmp TEGRA186_CLK_I2C1>;
> +        clock-names = "div-clk";
> +        resets = <&bpmp TEGRA186_RESET_I2C1>;
> +        reset-names = "i2c";

Drop all this. Not related, You only need i2c node with address/size-cells.

> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ina3221@40 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +            compatible = "ti,ina3221";
> +            reg = <0x40>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            input@0 {
> +                reg = <0x0>;
> +                status = "disabled";

Why is this node present? Binding said nodes are optional, so I assume
it can be just skipped. If all children must be there, then you should
actually require them in the binding (and mention it briefly in commit msg).

> +            };
Best regards,
Krzysztof

Re: [PATCH V2 2/4] dt-bindings: hwmon: ina3221: Add summation-bypass

[Krzysztof Kozlowski]

On 25/08/2023 18:42, Ninad Malwade 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 property, 'summation-bypass', to allow specific channels to be
> excluded from the summation control function if the shunt resistor
> is different to other channels.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
> ---
>  .../devicetree/bindings/hwmon/ti,ina3221.yaml  | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> index 0c6d41423d8c..20c23febf575 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> @@ -55,6 +55,24 @@ patternProperties:
>        shunt-resistor-micro-ohms:
>          description: shunt resistor value in micro-Ohm
>  
> +      summation-bypass:

What is the type? There is no vendor prefix here, so you added it as a
generic property. Which other devices use or can use it?

> +        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
> +          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. If
> +          this is not the case for specific channels, then the
> +          'summation-bypass' can be populated for a specific channel to
> +          exclude from the summation control function.

I don't understand what this property does. You described feature in the
device, that's good, but how does it map to the property? Bypass means
disable?

> +
>      additionalProperties: false
>  
>      required:

Best regards,
Krzysztof

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

[Jon Hunter]

On 26/08/2023 09:53, Krzysztof Kozlowski wrote:
> On 25/08/2023 18:42, Ninad Malwade wrote:
>> Convert the TI INA3221 bindings from the free-form text format to
>> json-schema.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
>> ---
> 
> This is v2, so where is the changelog?

Indeed. This was the first time this patch was included with the patch 
that extends the ina3221 driver. The original patch was here:

https://lore.kernel.org/lkml/20221108045243.24143-1-nmalwade@nvidia.com/


> 
>>   .../devicetree/bindings/hwmon/ina3221.txt     |  54 ---------
>>   .../devicetree/bindings/hwmon/ti,ina3221.yaml | 109 ++++++++++++++++++
>>   2 files changed, 109 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/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> new file mode 100644
>> index 000000000000..0c6d41423d8c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> @@ -0,0 +1,109 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
> 
> I assume you do not use standard license because of copying the description?


Probably just an oversight. I assume we can just update to be dual licensed?

>> +%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.
>> +    type: object
>> +    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
>> +
>> +    additionalProperties: false
> 
> This should be rather after type:object for readability.

OK

>> +
>> +    required:
>> +      - reg
>> +
>> +additionalProperties: false
> 
> And this please keep like in example schema, so after required:.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/tegra186-clock.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/reset/tegra186-reset.h>
>> +
>> +    i2c@3160000 {
>> +        compatible = "nvidia,tegra186-i2c";
>> +        reg = <0x03160000 0x10000>;
>> +        interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
>> +        clocks = <&bpmp TEGRA186_CLK_I2C1>;
>> +        clock-names = "div-clk";
>> +        resets = <&bpmp TEGRA186_RESET_I2C1>;
>> +        reset-names = "i2c";
> 
> Drop all this. Not related, You only need i2c node with address/size-cells.

OK

>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        ina3221@40 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> 
>> +            compatible = "ti,ina3221";
>> +            reg = <0x40>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            input@0 {
>> +                reg = <0x0>;
>> +                status = "disabled";
> 
> Why is this node present? Binding said nodes are optional, so I assume
> it can be just skipped. If all children must be there, then you should
> actually require them in the binding (and mention it briefly in commit msg).

We can check this.

Thanks
Jon

-- 
nvpublic

Re: [PATCH V2 2/4] dt-bindings: hwmon: ina3221: Add summation-bypass

[Jon Hunter]

On 26/08/2023 09:56, Krzysztof Kozlowski wrote:
> On 25/08/2023 18:42, Ninad Malwade 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 property, 'summation-bypass', to allow specific channels to be
>> excluded from the summation control function if the shunt resistor
>> is different to other channels.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
>> ---
>>   .../devicetree/bindings/hwmon/ti,ina3221.yaml  | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> index 0c6d41423d8c..20c23febf575 100644
>> --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> @@ -55,6 +55,24 @@ patternProperties:
>>         shunt-resistor-micro-ohms:
>>           description: shunt resistor value in micro-Ohm
>>   
>> +      summation-bypass:
> 
> What is the type? There is no vendor prefix here, so you added it as a
> generic property. Which other devices use or can use it?
> 
>> +        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
>> +          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. If
>> +          this is not the case for specific channels, then the
>> +          'summation-bypass' can be populated for a specific channel to
>> +          exclude from the summation control function.
> 
> I don't understand what this property does. You described feature in the
> device, that's good, but how does it map to the property? Bypass means
> disable?


Yes it means 'disable'. I kept as 'bypass' to align with the original 
patch [0], but if it is clearer, we could change this to be 
'summation-disable'.

Jon

[0] https://lore.kernel.org/lkml/20221108045243.24143-1-nmalwade@nvidia.com/

-- 
nvpublic

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

[Jon Hunter]

On 25/08/2023 18:11, Guenter Roeck wrote:
> On Sat, Aug 26, 2023 at 12:42:45AM +0800, Ninad Malwade 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.
>>
> 
> V2, but no change log. I am not going to review this series.

Sorry about that the changelog is ...

Changes since V1:
- Added patch to convert dt-binding to json
- Added patch to add new dt property for bypassing/disabling summation
   support for a channel
- Added patch to populate ina3221 devices for Tegra234 boards
- Updated summation bypass patch to display summation status via sysfs

Jon

-- 
nvpublic

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

[Guenter Roeck]

On 8/29/23 05:52, Jon Hunter wrote:
> 
> On 25/08/2023 18:11, Guenter Roeck wrote:
>> On Sat, Aug 26, 2023 at 12:42:45AM +0800, Ninad Malwade 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.
>>>
>>
>> V2, but no change log. I am not going to review this series.
> 
> Sorry about that the changelog is ...
> 
> Changes since V1:
> - Added patch to convert dt-binding to json
> - Added patch to add new dt property for bypassing/disabling summation
>    support for a channel
> - Added patch to populate ina3221 devices for Tegra234 boards
> - Updated summation bypass patch to display summation status via sysfs
> 

I have not looked at the patches, but this non-standard sysfs attribute
is a no-go. Use debugfs if you want such information to be available.

Guenter

Re: [PATCH V2 3/4] hwmon: ina3221: add support for summation channel control

[Guenter Roeck]

On Sat, Aug 26, 2023 at 12:42:48AM +0800, Ninad Malwade wrote:
> 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, we can't use summation control function to add
> the individual conversion values directly together in the
> Shunt-Voltage Sum register to report the total current.
> 
> To address this we add support to BYPASS channels
> via kernel device tree property "summation-bypass".
> 
> The channel which has this property would be excluded from
> the calculation of summation control function, so we can easily
> exclude the one which uses different shunt-resistor value or
> bus voltage.
> 
> For example, summation control function calculates
> Shunt-Voltage Sum like
> - input_shunt_voltage_summaion = input_shunt_voltage_channel1

summation

>                                + input_shunt_voltage_channel2
>                                + input_shunt_voltage_channel3
> 
> But if we want the summation to consider only channel1
> and channel3, we can add 'summation-bypass' property
> in device tree node of channel2.
> 
> Then the calculation will skip channel2.
> - input_shunt_voltage_summaion = input_shunt_voltage_channel1
>                                + input_shunt_voltage_channel3
> 
summation

> Please note that we only want the channel to be skipped
> for summation control function rather than completely disabled.
> Therefore, even if we add the device tree node, the functionality
> of the single channel would still be retained.
> 
> The below sysfs nodes are added to check if the channel is included
> or excluded from the summation control function.
> 
> in*_sum_bypass = 0 --> channel voltage is included for sum of
>                        shunt voltages.
> 
> in*_sum_bypass = 1 --> channel voltage is excluded for sum
>                        of shunt voltages.
> 
This is not an acceptable sysfs attribute. Use debugfs.

> Signed-off-by: Rajkumar Kasirajan <rkasirajan@nvidia.com>
> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
> ---
>  drivers/hwmon/ina3221.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 5ab944056ec0..093ebf9f1f8d 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -104,6 +104,7 @@ struct ina3221_input {
>  	const char *label;
>  	int shunt_resistor;
>  	bool disconnected;
> +	bool summation_bypass;
>  };
>  
>  /**
> @@ -125,6 +126,7 @@ struct ina3221_data {
>  	struct mutex lock;
>  	u32 reg_config;
>  	int summation_shunt_resistor;
> +	u32 summation_channel_control;
>  
>  	bool single_shot;
>  };
> @@ -154,7 +156,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_bypass)
>  			continue;
>  		if (!shunt_resistor) {
>  			/* Found the reference shunt resistor value */
> @@ -731,10 +734,29 @@ static SENSOR_DEVICE_ATTR_RW(shunt1_resistor, ina3221_shunt, INA3221_CHANNEL1);
>  static SENSOR_DEVICE_ATTR_RW(shunt2_resistor, ina3221_shunt, INA3221_CHANNEL2);
>  static SENSOR_DEVICE_ATTR_RW(shunt3_resistor, ina3221_shunt, INA3221_CHANNEL3);
>  
> +static ssize_t ina3221_summation_bypass_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	unsigned int channel = sd_attr->index;
> +	struct ina3221_input *input = &ina->inputs[channel];
> +
> +	return sysfs_emit(buf, "%d\n", input->summation_bypass);
> +}
> +
> +/* summation bypass */
> +static SENSOR_DEVICE_ATTR_RO(in1_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL1);
> +static SENSOR_DEVICE_ATTR_RO(in2_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL2);
> +static SENSOR_DEVICE_ATTR_RO(in3_sum_bypass, ina3221_summation_bypass, INA3221_CHANNEL3);
> +

As mentioned, use debugfs to display this information.

>  static struct attribute *ina3221_attrs[] = {
>  	&sensor_dev_attr_shunt1_resistor.dev_attr.attr,
>  	&sensor_dev_attr_shunt2_resistor.dev_attr.attr,
>  	&sensor_dev_attr_shunt3_resistor.dev_attr.attr,
> +	&sensor_dev_attr_in1_sum_bypass.dev_attr.attr,
> +	&sensor_dev_attr_in2_sum_bypass.dev_attr.attr,
> +	&sensor_dev_attr_in3_sum_bypass.dev_attr.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(ina3221);
> @@ -786,6 +808,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_bypass = of_property_read_bool(child, "summation-bypass");
> +
>  	/* Overwrite default shunt resistor value optionally */
>  	if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", &val)) {
>  		if (val < 1 || val > INT_MAX) {
> @@ -873,6 +898,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_bypass)
> +			ina->summation_channel_control |= (BIT(14 - i));

Unnecessary ( ) around BIT().

> +	}
>  
>  	ina->pm_dev = dev;
>  	mutex_init(&ina->lock);
> @@ -978,13 +1007,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
> -		 * Shunt measurements of disconnected channels should
> -		 * be 0, so it does not matter for summation.
> +		 * Sum only channels that are not 'bypassed' for summation
> +		 * by default. Shunt measurements of disconnected channels

Drop "by default".

> +		 * 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.17.1
> 

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

[Krzysztof Kozlowski]

On 29/08/2023 14:46, Jon Hunter wrote:
> it a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>>> new file mode 100644
>>> index 000000000000..0c6d41423d8c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>>> @@ -0,0 +1,109 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>
>> I assume you do not use standard license because of copying the description?
> 
> 
> Probably just an oversight. I assume we can just update to be dual licensed?

checkpatch would complain, which means it was not run?

Dual license, please, as asked by checkpatch.



Best regards,
Krzysztof

Re: [PATCH V2 2/4] dt-bindings: hwmon: ina3221: Add summation-bypass

[Krzysztof Kozlowski]

On 29/08/2023 14:48, Jon Hunter wrote:
> 
> 
> On 26/08/2023 09:56, Krzysztof Kozlowski wrote:
>> On 25/08/2023 18:42, Ninad Malwade 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 property, 'summation-bypass', to allow specific channels to be
>>> excluded from the summation control function if the shunt resistor
>>> is different to other channels.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
>>> ---
>>>   .../devicetree/bindings/hwmon/ti,ina3221.yaml  | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>>> index 0c6d41423d8c..20c23febf575 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>>> @@ -55,6 +55,24 @@ patternProperties:
>>>         shunt-resistor-micro-ohms:
>>>           description: shunt resistor value in micro-Ohm
>>>   
>>> +      summation-bypass:
>>
>> What is the type? There is no vendor prefix here, so you added it as a
>> generic property. Which other devices use or can use it?
>>
>>> +        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
>>> +          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. If
>>> +          this is not the case for specific channels, then the
>>> +          'summation-bypass' can be populated for a specific channel to

"populated" is confusing here. I guess you wanted "can be used"?

>>> +          exclude from the summation control function.
>>
>> I don't understand what this property does. You described feature in the
>> device, that's good, but how does it map to the property? Bypass means
>> disable?
> 
> 
> Yes it means 'disable'. I kept as 'bypass' to align with the original 
> patch [0], but if it is clearer, we could change this to be 
> 'summation-disable'.

Sounds better, but the description could also start with it, e.g.
"Disable the summation on specific channel. .... "

Best regards,
Krzysztof

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

[Jon Hunter]

On 26/08/2023 09:53, Krzysztof Kozlowski wrote:
> On 25/08/2023 18:42, Ninad Malwade wrote:
>> Convert the TI INA3221 bindings from the free-form text format to
>> json-schema.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>

...

>> +            compatible = "ti,ina3221";
>> +            reg = <0x40>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            input@0 {
>> +                reg = <0x0>;
>> +                status = "disabled";
> 
> Why is this node present? Binding said nodes are optional, so I assume
> it can be just skipped. If all children must be there, then you should
> actually require them in the binding (and mention it briefly in commit msg).


I have taken a look at this and if the 'input@0' is omitted above the 
driver still enables it. It only disables it or marks as disconnected if 
the node is present but no enabled. So we can mark these as required.

Is there a better way to mark them as required apart from listing all 
input channels under required?

Jon

-- 
nvpublic

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

[Guenter Roeck]

On Fri, Sep 01, 2023 at 05:34:00PM +0100, Jon Hunter wrote:
> 
> On 26/08/2023 09:53, Krzysztof Kozlowski wrote:
> > On 25/08/2023 18:42, Ninad Malwade wrote:
> > > Convert the TI INA3221 bindings from the free-form text format to
> > > json-schema.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
> 
> ...
> 
> > > +            compatible = "ti,ina3221";
> > > +            reg = <0x40>;
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +
> > > +            input@0 {
> > > +                reg = <0x0>;
> > > +                status = "disabled";
> > 
> > Why is this node present? Binding said nodes are optional, so I assume
> > it can be just skipped. If all children must be there, then you should
> > actually require them in the binding (and mention it briefly in commit msg).
> 
> 
> I have taken a look at this and if the 'input@0' is omitted above the driver
> still enables it. It only disables it or marks as disconnected if the node
> is present but no enabled. So we can mark these as required.
> 
> Is there a better way to mark them as required apart from listing all input
> channels under required?
> 

Channels should by default be enabled because they are enabled by default
in the chip. Requiring that all nodes be listed in devicetree just to
select the default behavior would be overkill.

Jusat because the chip has the ability to disable channels, that should
really not be made the default.

Guenter

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

[Jon Hunter]

On 01/09/2023 17:44, Guenter Roeck wrote:
> On Fri, Sep 01, 2023 at 05:34:00PM +0100, Jon Hunter wrote:
>>
>> On 26/08/2023 09:53, Krzysztof Kozlowski wrote:
>>> On 25/08/2023 18:42, Ninad Malwade wrote:
>>>> Convert the TI INA3221 bindings from the free-form text format to
>>>> json-schema.
>>>>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com>
>>
>> ...
>>
>>>> +            compatible = "ti,ina3221";
>>>> +            reg = <0x40>;
>>>> +            #address-cells = <1>;
>>>> +            #size-cells = <0>;
>>>> +
>>>> +            input@0 {
>>>> +                reg = <0x0>;
>>>> +                status = "disabled";
>>>
>>> Why is this node present? Binding said nodes are optional, so I assume
>>> it can be just skipped. If all children must be there, then you should
>>> actually require them in the binding (and mention it briefly in commit msg).
>>
>>
>> I have taken a look at this and if the 'input@0' is omitted above the driver
>> still enables it. It only disables it or marks as disconnected if the node
>> is present but no enabled. So we can mark these as required.
>>
>> Is there a better way to mark them as required apart from listing all input
>> channels under required?
>>
> 
> Channels should by default be enabled because they are enabled by default
> in the chip. Requiring that all nodes be listed in devicetree just to
> select the default behavior would be overkill.
> 
> Jusat because the chip has the ability to disable channels, that should
> really not be made the default.


Yes and that is fine. What we are trying to sort out here is what should 
the example contain in the dt-binding doc? The original example from the 
text binding doc was replicated to the yaml version and based upon the 
above that seems like a valid example.

What I could do is add an extra comment under the 'input' property that 
"Input channels default to enabled in the chip. Unless channels are 
explicitly disabled in device-tree, input channels will be enabled".

Jon

-- 
nvpublic