[PATCH 0/2] hwmon: Add support for INA233

[PATCH 0/2] hwmon: Add support for INA233

[Leo Yang]

Support ina233 driver with binding documents.

Leo Yang (2):
  dt-bindings: Add INA233 device
  hwmon: Add driver for TI INA233 Current and Power Monitor

 .../bindings/hwmon/pmbus/ti,ina233.yaml       |  57 +++++
 Documentation/hwmon/ina233.rst                |  77 +++++++
 Documentation/hwmon/index.rst                 |   1 +
 MAINTAINERS                                   |   9 +
 drivers/hwmon/pmbus/Kconfig                   |   9 +
 drivers/hwmon/pmbus/Makefile                  |   1 +
 drivers/hwmon/pmbus/ina233.c                  | 200 ++++++++++++++++++
 7 files changed, 354 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
 create mode 100644 Documentation/hwmon/ina233.rst
 create mode 100644 drivers/hwmon/pmbus/ina233.c

-- 
2.39.2

[PATCH 1/2] dt-bindings: Add INA233 device

[Leo Yang]

Add TI INA233 Current and Power Monitor bindings.

Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
---
 .../bindings/hwmon/pmbus/ti,ina233.yaml       | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
new file mode 100644
index 000000000000..2677c98dadd1
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/pmbus/ti,ina233.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments INA233 of power/voltage/current monitors
+
+maintainers:
+  - Leo Yang <Leo-Yang@quantatw.com>
+
+description: |
+  INA233 High-Side or Low-Side Measurement, Bidirectional Current
+  and Power Monitor With I2C-, SMBus-, and PMBus-Compatible Interface.
+
+  Datasheet: https://www.ti.com/lit/ds/symlink/ina233.pdf
+
+properties:
+  compatible:
+    enum:
+      - ti,ina233
+
+  reg:
+    maxItems: 1
+
+  shunt-resistor:
+    description:
+      Shunt resistor value in micro-Ohms, Please refer to the actual circuit
+      resistor used.
+    default: 2000
+
+  current-lsb:
+    description:
+	    Calculate the Maximum Expected Current(A) / 2^15 in micro ampere (uA/bit).
+	    e.g. 30A / 2^15 = 915 uA/bit
+    default: 1000
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-sensor@40 {
+            compatible = "ti,ina233";
+            reg = <0x40>;
+            shunt-resistor = /bits/ 32 <5000>;
+            current-lsb = /bits/ 16 <1000>;
+        };
+    };
\ No newline at end of file
-- 
2.39.2

Re: [PATCH 1/2] dt-bindings: Add INA233 device

[Krzysztof Kozlowski]

On 06/01/2025 08:13, Leo Yang wrote:
> Add TI INA233 Current and Power Monitor bindings.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

> 
> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
> ---
>  .../bindings/hwmon/pmbus/ti,ina233.yaml       | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
> new file mode 100644
> index 000000000000..2677c98dadd1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +

Drop blank line

> +$id: http://devicetree.org/schemas/hwmon/pmbus/ti,ina233.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments INA233 of power/voltage/current monitors
> +
> +maintainers:
> +  - Leo Yang <Leo-Yang@quantatw.com>
> +
> +description: |
> +  INA233 High-Side or Low-Side Measurement, Bidirectional Current
> +  and Power Monitor With I2C-, SMBus-, and PMBus-Compatible Interface.
> +
> +  Datasheet: https://www.ti.com/lit/ds/symlink/ina233.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ina233

This does not fit existing ti,ina bindings?

> +
> +  reg:
> +    maxItems: 1
> +
> +  shunt-resistor:
> +    description:
> +      Shunt resistor value in micro-Ohms, Please refer to the actual circuit
> +      resistor used.
> +    default: 2000

Which schema brings the $ref for this property?

> +
> +  current-lsb:
> +    description:
> +	    Calculate the Maximum Expected Current(A) / 2^15 in micro ampere (uA/bit).
> +	    e.g. 30A / 2^15 = 915 uA/bit
> +    default: 1000

Missing ref, missing vendor prefix and missing actual explanation what
it is for. I don't understand the description at all.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        power-sensor@40 {
> +            compatible = "ti,ina233";
> +            reg = <0x40>;
> +            shunt-resistor = /bits/ 32 <5000>;

Drop unusual syntax. Please take a look how all other bindings and DTS
is doing this.

> +            current-lsb = /bits/ 16 <1000>;
> +        };
> +    };
> \ No newline at end of file

You have patch errors.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: Add INA233 device

[Guenter Roeck]

On 1/6/25 02:50, Krzysztof Kozlowski wrote:
> On 06/01/2025 08:13, Leo Yang wrote:
>> Add TI INA233 Current and Power Monitor bindings.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 
>>
>> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
>> ---
>>   .../bindings/hwmon/pmbus/ti,ina233.yaml       | 57 +++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
>> new file mode 100644
>> index 000000000000..2677c98dadd1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
>> @@ -0,0 +1,57 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +
> 
> Drop blank line
> 
>> +$id: http://devicetree.org/schemas/hwmon/pmbus/ti,ina233.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments INA233 of power/voltage/current monitors
>> +
>> +maintainers:
>> +  - Leo Yang <Leo-Yang@quantatw.com>
>> +
>> +description: |
>> +  INA233 High-Side or Low-Side Measurement, Bidirectional Current
>> +  and Power Monitor With I2C-, SMBus-, and PMBus-Compatible Interface.
>> +
>> +  Datasheet: https://www.ti.com/lit/ds/symlink/ina233.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,ina233
> 
> This does not fit existing ti,ina bindings?
> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  shunt-resistor:

Should probably be shunt-resistor-micro-ohms.

>> +    description:
>> +      Shunt resistor value in micro-Ohms, Please refer to the actual circuit
>> +      resistor used.
>> +    default: 2000
> 
> Which schema brings the $ref for this property?
> 
>> +
>> +  current-lsb:
>> +    description:
>> +	    Calculate the Maximum Expected Current(A) / 2^15 in micro ampere (uA/bit).
>> +	    e.g. 30A / 2^15 = 915 uA/bit
>> +    default: 1000
> 
> Missing ref, missing vendor prefix and missing actual explanation what
> it is for. I don't understand the description at all.
> 

Also, the unit is missing.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        power-sensor@40 {
>> +            compatible = "ti,ina233";
>> +            reg = <0x40>;
>> +            shunt-resistor = /bits/ 32 <5000>;
> 
> Drop unusual syntax. Please take a look how all other bindings and DTS
> is doing this.
> 
>> +            current-lsb = /bits/ 16 <1000>;
>> +        };
>> +    };
>> \ No newline at end of file
> 
> You have patch errors.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dt-bindings: Add INA233 device

[Rob Herring (Arm)]

On Mon, 06 Jan 2025 15:13:36 +0800, Leo Yang wrote:
> Add TI INA233 Current and Power Monitor bindings.
> 
> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
> ---
>  .../bindings/hwmon/pmbus/ti,ina233.yaml       | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml:35:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml: ignoring, error parsing file
./Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml:35:1: found character that cannot start any token
make[2]: *** Deleting file 'Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.example.dts'
Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml:35:1: found character that cannot start any token
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250106071337.3017926-2-Leo-Yang@quantatw.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

[Leo Yang]

Support ina233 driver for Meta Yosemite V4.

Driver for Texas Instruments INA233 Current and Power Monitor
With I2C-, SMBus-, and PMBus-Compatible Interface

According to the mail
https://lore.kernel.org/all/
20230920054739.1561080-1-Delphine_CC_Chiu@wiwynn.com
maintainer's suggested rewrite driver

Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
---
 Documentation/hwmon/ina233.rst |  77 +++++++++++++
 Documentation/hwmon/index.rst  |   1 +
 MAINTAINERS                    |   9 ++
 drivers/hwmon/pmbus/Kconfig    |   9 ++
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/ina233.c   | 200 +++++++++++++++++++++++++++++++++
 6 files changed, 297 insertions(+)
 create mode 100644 Documentation/hwmon/ina233.rst
 create mode 100644 drivers/hwmon/pmbus/ina233.c

diff --git a/Documentation/hwmon/ina233.rst b/Documentation/hwmon/ina233.rst
new file mode 100644
index 000000000000..5c9e5ed2d6d5
--- /dev/null
+++ b/Documentation/hwmon/ina233.rst
@@ -0,0 +1,77 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver ina233
+====================
+
+Supported chips:
+
+  * TI INA233
+
+    Prefix: 'ina233'
+
+  * Datasheet
+
+    Publicly available at the TI website : https://www.ti.com/lit/ds/symlink/ina233.pdf
+
+Author:
+
+	Leo Yang <Leo-Yang@quantatw.com>
+
+Usage Notes
+-----------
+
+The shunt resistor value can be configured by a device tree property;
+see Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml for details.
+
+
+Description
+-----------
+
+This driver supports hardware monitoring for TI INA233.
+
+The driver is a client driver to the core PMBus driver. Please see
+Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
+
+The driver provides the following attributes for input voltage:
+
+**in1_input**
+
+**in1_label**
+
+**in1_max**
+
+**in1_max_alarm**
+
+**in1_min**
+
+**in1_min_alarm**
+
+The driver provides the following attributes for shunt voltage:
+
+**in2_input**
+
+**in2_label**
+
+The driver provides the following attributes for output voltage:
+
+**in3_input**
+
+**in3_label**
+
+**in3_alarm**
+
+The driver provides the following attributes for output current:
+
+**curr1_input**
+
+**curr1_label**
+
+**curr1_max**
+
+**curr1_max_alarm**
+
+The driver provides the following attributes for input power:
+
+**power1_input**
+
+**power1_label**
\ No newline at end of file
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 55f1111594b2..f280fa6e7d95 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -89,6 +89,7 @@ Hardware Monitoring Kernel Drivers
    ibmpowernv
    ina209
    ina2xx
+   ina233
    ina238
    ina3221
    inspur-ipsps1
diff --git a/MAINTAINERS b/MAINTAINERS
index c575de4903db..061da798536d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11226,6 +11226,15 @@ L:	linux-fbdev@vger.kernel.org
 S:	Orphan
 F:	drivers/video/fbdev/imsttfb.c
 
+INA233 HARDWARE MONITOR DRIVER
+M:	Leo Yang <Leo-Yang@quantatw.com>
+M:	Leo Yang <leo.yang.sy0@gmail.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Odd Fixes
+F:	Documentation/devicetree/bindings/hwmon/ina233.txt
+F:	Documentation/hwmon/ina233.rst
+F:	drivers/hwmon/pmbus/ina233.c
+
 INDEX OF FURTHER KERNEL DOCUMENTATION
 M:	Carlos Bilbao <carlos.bilbao.osdev@gmail.com>
 S:	Maintained
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index f6d352841953..55db0ddc94ed 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -124,6 +124,15 @@ config SENSORS_DPS920AB
 	  This driver can also be built as a module. If so, the module will
 	  be called dps920ab.
 
+config SENSORS_INA233
+	tristate "Texas Instruments INA233 and compatibles"
+	help
+	  If you say yes here you get hardware monitoring support for Texas
+	  Instruments INA233.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ina233.
+
 config SENSORS_INSPUR_IPSPS
 	tristate "INSPUR Power System Power Supply"
 	help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index d00bcc758b97..3c4b06fb939e 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o
 obj-$(CONFIG_SENSORS_FSP_3Y)	+= fsp-3y.o
 obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
 obj-$(CONFIG_SENSORS_DPS920AB)	+= dps920ab.o
+obj-$(CONFIG_SENSORS_INA233)	+= ina233.o
 obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
 obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
 obj-$(CONFIG_SENSORS_IR36021)	+= ir36021.o
diff --git a/drivers/hwmon/pmbus/ina233.c b/drivers/hwmon/pmbus/ina233.c
new file mode 100644
index 000000000000..1f7be600dea4
--- /dev/null
+++ b/drivers/hwmon/pmbus/ina233.c
@@ -0,0 +1,200 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for ina233
+ *
+ * Copyright (c) 2024 Leo Yang
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include "pmbus.h"
+
+#define MFR_READ_VSHUNT 0xd1
+#define MFR_CALIBRATION 0xd4
+
+#define INA233_RSHUNT_DEFAULT		2000 /* uOhm */
+#define INA233_CURRENT_LSB_DEFAULT	1000 /* uA/bit */
+
+#define MAX_M_VAL 32767
+#define MIN_M_VAL -32768
+
+static int calculate_coef(int *m, int *R, bool power)
+{
+	s64 scaled_m;
+	int scale_factor = 0;
+	int scale_coef = 1;
+	int power_coef = 1;
+	bool is_integer = false;
+
+	if (*m == 0) {
+		*R = 0;
+		return -1;
+	}
+
+	if (power)
+		power_coef = 25;
+
+	if (1000000 % *m) {
+		/* Default value, Scaling to keep integer precision,
+		 * Change it if you need
+		 */
+		scale_factor = -3;
+		scale_coef = 1000;
+	} else {
+		is_integer = true;
+	}
+
+	/*
+	 * Unit Conversion (Current_LSB A->uA) and use scaling(scale_factor)
+	 * to keep integer precision.
+	 * Formulae referenced from spec.
+	 */
+	scaled_m = div_s64(1000000 * scale_coef, *m * power_coef);
+
+	/* Maximize while keeping it bounded.*/
+	while (scaled_m > MAX_M_VAL || scaled_m < MIN_M_VAL) {
+		scaled_m /= 10;
+		scale_factor++;
+	}
+	/* Scale up only if fractional part exists. */
+	while (scaled_m * 10 < MAX_M_VAL && scaled_m * 10 > MIN_M_VAL && !is_integer) {
+		scaled_m *= 10;
+		scale_factor--;
+	}
+
+	*m = scaled_m;
+	*R = scale_factor;
+	return 0;
+}
+
+static int ina233_read_word_data(struct i2c_client *client, int page,
+				 int phase, int reg)
+{
+	int ret;
+
+	switch (reg) {
+	case PMBUS_VIRT_READ_VMON:
+		ret = pmbus_read_word_data(client, 0, 0xff, MFR_READ_VSHUNT);
+
+		/* Adjust returned value to match VIN coefficients */
+		/* VIN: 1.25 mV VSHUNT: 2.5 uV LSB */
+		ret = DIV_ROUND_CLOSEST(ret * 25, 12500);
+		break;
+	default:
+		ret = -ENODATA;
+		break;
+	}
+	return ret;
+}
+
+struct pmbus_driver_info ina233_info = {
+	.pages = 1,
+	.format[PSC_VOLTAGE_IN] = direct,
+	.format[PSC_VOLTAGE_OUT] = direct,
+	.format[PSC_CURRENT_OUT] = direct,
+	.format[PSC_POWER] = direct,
+	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_INPUT
+		| PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
+		| PMBUS_HAVE_POUT
+		| PMBUS_HAVE_VMON | PMBUS_HAVE_STATUS_VMON,
+	.m[PSC_VOLTAGE_IN] = 8,
+	.R[PSC_VOLTAGE_IN] = 2,
+	.m[PSC_VOLTAGE_OUT] = 8,
+	.R[PSC_VOLTAGE_OUT] = 2,
+	.read_word_data = ina233_read_word_data,
+};
+
+static int ina233_probe(struct i2c_client *client)
+{
+	int ret, m, R;
+	u32 rshunt;
+	u16 current_lsb;
+	u16 calibration;
+
+	/* If INA233 skips current/power, shunt-resistor and current-lsb aren't needed.	*/
+
+	/* read rshunt value (uOhm) */
+	ret = of_property_read_u32(client->dev.of_node, "shunt-resistor", &rshunt);
+	if (ret < 0 || !rshunt) {
+		dev_err(&client->dev, "Unable to read shunt-resistor or value is 0, default value %d uOhm is used.\n",
+			INA233_RSHUNT_DEFAULT);
+		rshunt = INA233_RSHUNT_DEFAULT;
+	}
+
+	/* read current_lsb value (uA/bit) */
+	ret = of_property_read_u16(client->dev.of_node, "current-lsb", &current_lsb);
+	if (ret < 0 || !current_lsb) {
+		dev_err(&client->dev, "Unable to read current_lsb or value is 0, default value %d uA/bit is used.\n",
+			INA233_CURRENT_LSB_DEFAULT);
+		current_lsb = INA233_CURRENT_LSB_DEFAULT;
+	}
+
+	/* calculate current coefficient */
+	m = current_lsb;
+	ret = calculate_coef(&m, &R, false);
+	if (ret < 0) {
+		dev_err(&client->dev, "Calculate_coef error\n");
+	} else {
+		ina233_info.m[PSC_CURRENT_OUT] = m;
+		ina233_info.R[PSC_CURRENT_OUT] = R;
+	}
+
+	/* calculate power coefficient */
+	m = current_lsb;
+	ret = calculate_coef(&m, &R, true);
+	if (ret < 0) {
+		dev_err(&client->dev, "Calculate_coef error\n");
+	} else {
+		ina233_info.m[PSC_POWER] = m;
+		ina233_info.R[PSC_POWER] = R;
+	}
+
+	/* write MFR_CALIBRATION register, Apply formula from spec with unit scaling. */
+	calibration = div_u64((u64)5120000000, (u64)rshunt * current_lsb);
+	if (calibration <= 0) {
+		dev_err(&client->dev, "Calibration error\n");
+		return -1;
+	}
+	ret = i2c_smbus_write_word_data(client, MFR_CALIBRATION, calibration);
+	if (ret < 0) {
+		dev_err(&client->dev, "Unable to write calibration\n");
+		return ret;
+	}
+
+	dev_info(&client->dev, "power monitor %s (Rshunt = %u uOhm, Current_LSB = %u uA/bit)\n",
+		 client->name, rshunt, current_lsb);
+
+	return pmbus_do_probe(client, &ina233_info);
+}
+
+static const struct i2c_device_id ina233_id[] = {
+	{"ina233", 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ina233_id);
+
+static const struct of_device_id __maybe_unused ina233_of_match[] = {
+	{ .compatible = "ti,ina233" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ina233_of_match);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver ina233_driver = {
+	.driver = {
+		   .name = "ina233",
+		   .of_match_table = of_match_ptr(ina233_of_match),
+	},
+	.probe = ina233_probe,
+	.id_table = ina233_id,
+};
+
+module_i2c_driver(ina233_driver);
+
+MODULE_AUTHOR("Leo Yang <Leo-Yang@quantatw.com>");
+MODULE_DESCRIPTION("PMBus driver for INA233 and compatible chips");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("PMBUS");
-- 
2.39.2

Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

[kernel test robot]

Hi Leo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.13-rc6 next-20241220]
[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/Leo-Yang/dt-bindings-Add-INA233-device/20250106-151934
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20250106071337.3017926-3-Leo-Yang%40quantatw.com
patch subject: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor
reproduce: (https://download.01.org/0day-ci/archive/20250106/202501061734.nPNdRKqO-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/202501061734.nPNdRKqO-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt
   Warning: Documentation/translations/ja_JP/SubmittingPatches references a file that doesn't exist: linux-2.6.12-vanilla/Documentation/dontdiff
   Warning: Documentation/userspace-api/netlink/index.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
   Warning: Documentation/userspace-api/netlink/specs.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/ina233.txt
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
   Warning: lib/Kconfig.debug references a file that doesn't exist: Documentation/dev-tools/fault-injection/fault-injection.rst
   Using alabaster theme

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

Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

[Krzysztof Kozlowski]

On 06/01/2025 08:13, Leo Yang wrote:
> Support ina233 driver for Meta Yosemite V4.
> 
> Driver for Texas Instruments INA233 Current and Power Monitor
> With I2C-, SMBus-, and PMBus-Compatible Interface
> 
> According to the mail
> https://lore.kernel.org/all/
> 20230920054739.1561080-1-Delphine_CC_Chiu@wiwynn.com

Don't break the URLs. It makes them difficult to use.


> maintainer's suggested rewrite driver
> 



> +INA233 HARDWARE MONITOR DRIVER
> +M:	Leo Yang <Leo-Yang@quantatw.com>
> +M:	Leo Yang <leo.yang.sy0@gmail.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Odd Fixes
> +F:	Documentation/devicetree/bindings/hwmon/ina233.txt

There is no such file.


...

> +
> +struct pmbus_driver_info ina233_info = {

Why this cannot be const and static?

> +	.pages = 1,
> +	.format[PSC_VOLTAGE_IN] = direct,
> +	.format[PSC_VOLTAGE_OUT] = direct,
> +	.format[PSC_CURRENT_OUT] = direct,
> +	.format[PSC_POWER] = direct,
> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_INPUT
> +		| PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
> +		| PMBUS_HAVE_POUT
> +		| PMBUS_HAVE_VMON | PMBUS_HAVE_STATUS_VMON,
> +	.m[PSC_VOLTAGE_IN] = 8,
> +	.R[PSC_VOLTAGE_IN] = 2,
> +	.m[PSC_VOLTAGE_OUT] = 8,
> +	.R[PSC_VOLTAGE_OUT] = 2,
> +	.read_word_data = ina233_read_word_data,
> +};
> +
> +static int ina233_probe(struct i2c_client *client)
> +{
> +	int ret, m, R;
> +	u32 rshunt;
> +	u16 current_lsb;
> +	u16 calibration;
> +
> +	/* If INA233 skips current/power, shunt-resistor and current-lsb aren't needed.	*/
> +
> +	/* read rshunt value (uOhm) */
> +	ret = of_property_read_u32(client->dev.of_node, "shunt-resistor", &rshunt);
> +	if (ret < 0 || !rshunt) {
> +		dev_err(&client->dev, "Unable to read shunt-resistor or value is 0, default value %d uOhm is used.\n",
> +			INA233_RSHUNT_DEFAULT);

Your binding said this is optional, so how this can be an error?

> +		rshunt = INA233_RSHUNT_DEFAULT;
> +	}
> +
> +	/* read current_lsb value (uA/bit) */
> +	ret = of_property_read_u16(client->dev.of_node, "current-lsb", &current_lsb);
> +	if (ret < 0 || !current_lsb) {
> +		dev_err(&client->dev, "Unable to read current_lsb or value is 0, default value %d uA/bit is used.\n",
> +			INA233_CURRENT_LSB_DEFAULT);

Same problem

> +		current_lsb = INA233_CURRENT_LSB_DEFAULT;
> +	}
> +
Best regards,
Krzysztof

Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

[Guenter Roeck]

On 1/5/25 23:13, Leo Yang wrote:
> Support ina233 driver for Meta Yosemite V4.
> 
> Driver for Texas Instruments INA233 Current and Power Monitor
> With I2C-, SMBus-, and PMBus-Compatible Interface
> 
> According to the mail
> https://lore.kernel.org/all/
> 20230920054739.1561080-1-Delphine_CC_Chiu@wiwynn.com
> maintainer's suggested rewrite driver
> 
Drop the last sentence, or move after "---".

Besides, while I did point out a number of problems, but I did not suggest
to "rewrite the driver".

Since this is v2 of this driver, the submission should have been versioned,
and a change log should have been provided.

> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
> ---
>   Documentation/hwmon/ina233.rst |  77 +++++++++++++
>   Documentation/hwmon/index.rst  |   1 +
>   MAINTAINERS                    |   9 ++
>   drivers/hwmon/pmbus/Kconfig    |   9 ++
>   drivers/hwmon/pmbus/Makefile   |   1 +
>   drivers/hwmon/pmbus/ina233.c   | 200 +++++++++++++++++++++++++++++++++
>   6 files changed, 297 insertions(+)
>   create mode 100644 Documentation/hwmon/ina233.rst
>   create mode 100644 drivers/hwmon/pmbus/ina233.c
> 
> diff --git a/Documentation/hwmon/ina233.rst b/Documentation/hwmon/ina233.rst
> new file mode 100644
> index 000000000000..5c9e5ed2d6d5
> --- /dev/null
> +++ b/Documentation/hwmon/ina233.rst
> @@ -0,0 +1,77 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver ina233
> +====================
> +
> +Supported chips:
> +
> +  * TI INA233
> +
> +    Prefix: 'ina233'
> +
> +  * Datasheet
> +
> +    Publicly available at the TI website : https://www.ti.com/lit/ds/symlink/ina233.pdf
> +
> +Author:
> +
> +	Leo Yang <Leo-Yang@quantatw.com>
> +
> +Usage Notes
> +-----------
> +
> +The shunt resistor value can be configured by a device tree property;
> +see Documentation/devicetree/bindings/hwmon/pmbus/ti,ina233.yaml for details.
> +
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for TI INA233.
> +
> +The driver is a client driver to the core PMBus driver. Please see
> +Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> +
> +The driver provides the following attributes for input voltage:
> +
> +**in1_input**
> +
> +**in1_label**
> +
> +**in1_max**
> +
> +**in1_max_alarm**
> +
> +**in1_min**
> +
> +**in1_min_alarm**
> +
> +The driver provides the following attributes for shunt voltage:
> +
> +**in2_input**
> +
> +**in2_label**
> +
> +The driver provides the following attributes for output voltage:
> +
> +**in3_input**
> +
> +**in3_label**
> +
> +**in3_alarm**
> +
> +The driver provides the following attributes for output current:
> +
> +**curr1_input**
> +
> +**curr1_label**
> +
> +**curr1_max**
> +
> +**curr1_max_alarm**
> +
> +The driver provides the following attributes for input power:
> +
> +**power1_input**
> +
> +**power1_label**
> \ No newline at end of file
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 55f1111594b2..f280fa6e7d95 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -89,6 +89,7 @@ Hardware Monitoring Kernel Drivers
>      ibmpowernv
>      ina209
>      ina2xx
> +   ina233
>      ina238
>      ina3221
>      inspur-ipsps1
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c575de4903db..061da798536d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11226,6 +11226,15 @@ L:	linux-fbdev@vger.kernel.org
>   S:	Orphan
>   F:	drivers/video/fbdev/imsttfb.c
>   
> +INA233 HARDWARE MONITOR DRIVER
> +M:	Leo Yang <Leo-Yang@quantatw.com>
> +M:	Leo Yang <leo.yang.sy0@gmail.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Odd Fixes
> +F:	Documentation/devicetree/bindings/hwmon/ina233.txt
> +F:	Documentation/hwmon/ina233.rst
> +F:	drivers/hwmon/pmbus/ina233.c
> +
>   INDEX OF FURTHER KERNEL DOCUMENTATION
>   M:	Carlos Bilbao <carlos.bilbao.osdev@gmail.com>
>   S:	Maintained
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index f6d352841953..55db0ddc94ed 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -124,6 +124,15 @@ config SENSORS_DPS920AB
>   	  This driver can also be built as a module. If so, the module will
>   	  be called dps920ab.
>   
> +config SENSORS_INA233
> +	tristate "Texas Instruments INA233 and compatibles"
> +	help
> +	  If you say yes here you get hardware monitoring support for Texas
> +	  Instruments INA233.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ina233.
> +
>   config SENSORS_INSPUR_IPSPS
>   	tristate "INSPUR Power System Power Supply"
>   	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index d00bcc758b97..3c4b06fb939e 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o
>   obj-$(CONFIG_SENSORS_FSP_3Y)	+= fsp-3y.o
>   obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
>   obj-$(CONFIG_SENSORS_DPS920AB)	+= dps920ab.o
> +obj-$(CONFIG_SENSORS_INA233)	+= ina233.o
>   obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
>   obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
>   obj-$(CONFIG_SENSORS_IR36021)	+= ir36021.o
> diff --git a/drivers/hwmon/pmbus/ina233.c b/drivers/hwmon/pmbus/ina233.c
> new file mode 100644
> index 000000000000..1f7be600dea4
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ina233.c
> @@ -0,0 +1,200 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for ina233
> + *
> + * Copyright (c) 2024 Leo Yang
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>

Alphabetic include file order, please.

> +#include "pmbus.h"
> +
> +#define MFR_READ_VSHUNT 0xd1
> +#define MFR_CALIBRATION 0xd4
> +
> +#define INA233_RSHUNT_DEFAULT		2000 /* uOhm */
> +#define INA233_CURRENT_LSB_DEFAULT	1000 /* uA/bit */

"bit" is implied in "LSB".

> +
> +#define MAX_M_VAL 32767
> +#define MIN_M_VAL -32768
> +
> +static int calculate_coef(int *m, int *R, bool power)
> +{
> +	s64 scaled_m;
> +	int scale_factor = 0;
> +	int scale_coef = 1;
> +	int power_coef = 1;
> +	bool is_integer = false;
> +
> +	if (*m == 0) {
> +		*R = 0;
> +		return -1;
> +	}

*m is never 0.

> +
> +	if (power)
> +		power_coef = 25;
> +

Why not just pass the power coefficient directly as parameter ?

> +	if (1000000 % *m) {

I fail to understand the logic here. Why scale if and only if m is a clean
multiple of 1000000 ? Scale if m == 1000001 but not if m == 1000000 ?
Please explain.

> +		/* Default value, Scaling to keep integer precision,
> +		 * Change it if you need

This comment does not provide any actual information and thus does not
add any value. Change to what ? Why ? And, again, why not scale if
m is a multiple of 1000000, no matter how large or small it is ?

> +		 */
> +		scale_factor = -3;
> +		scale_coef = 1000;
> +	} else {
> +		is_integer = true;
> +	}
> +
> +	/*
> +	 * Unit Conversion (Current_LSB A->uA) and use scaling(scale_factor)
> +	 * to keep integer precision.
> +	 * Formulae referenced from spec.
> +	 */
> +	scaled_m = div_s64(1000000 * scale_coef, *m * power_coef);
> +
> +	/* Maximize while keeping it bounded.*/
> +	while (scaled_m > MAX_M_VAL || scaled_m < MIN_M_VAL) {
> +		scaled_m /= 10;

This looks wrong. If scaled_m < MIN_M_VAL it doesn't make sense
to decrease it even more.

> +		scale_factor++;
> +	}
> +	/* Scale up only if fractional part exists. */
> +	while (scaled_m * 10 < MAX_M_VAL && scaled_m * 10 > MIN_M_VAL && !is_integer) {

This looks just as wrong. If scaled_m > 10 * MIN_M_VAL, why increase it even more ?

> +		scaled_m *= 10;
> +		scale_factor--;
> +	}
> +
> +	*m = scaled_m;
> +	*R = scale_factor;
> +	return 0;
> +}
> +
> +static int ina233_read_word_data(struct i2c_client *client, int page,
> +				 int phase, int reg)
> +{
> +	int ret;
> +
> +	switch (reg) {
> +	case PMBUS_VIRT_READ_VMON:
> +		ret = pmbus_read_word_data(client, 0, 0xff, MFR_READ_VSHUNT);
> +
> +		/* Adjust returned value to match VIN coefficients */
> +		/* VIN: 1.25 mV VSHUNT: 2.5 uV LSB */
> +		ret = DIV_ROUND_CLOSEST(ret * 25, 12500);
> +		break;
> +	default:
> +		ret = -ENODATA;
> +		break;
> +	}
> +	return ret;
> +}
> +
> +struct pmbus_driver_info ina233_info = {
> +	.pages = 1,
> +	.format[PSC_VOLTAGE_IN] = direct,
> +	.format[PSC_VOLTAGE_OUT] = direct,
> +	.format[PSC_CURRENT_OUT] = direct,
> +	.format[PSC_POWER] = direct,
> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_INPUT
> +		| PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
> +		| PMBUS_HAVE_POUT
> +		| PMBUS_HAVE_VMON | PMBUS_HAVE_STATUS_VMON,
> +	.m[PSC_VOLTAGE_IN] = 8,
> +	.R[PSC_VOLTAGE_IN] = 2,
> +	.m[PSC_VOLTAGE_OUT] = 8,
> +	.R[PSC_VOLTAGE_OUT] = 2,
> +	.read_word_data = ina233_read_word_data,
> +};
> +
> +static int ina233_probe(struct i2c_client *client)
> +{
> +	int ret, m, R;
> +	u32 rshunt;
> +	u16 current_lsb;
> +	u16 calibration;
> +
> +	/* If INA233 skips current/power, shunt-resistor and current-lsb aren't needed.	*/
> +
> +	/* read rshunt value (uOhm) */
> +	ret = of_property_read_u32(client->dev.of_node, "shunt-resistor", &rshunt);
> +	if (ret < 0 || !rshunt) {
An rshunt value of 0 would be wrong and must not be accepted.

> +		dev_err(&client->dev, "Unable to read shunt-resistor or value is 0, default value %d uOhm is used.\n",
> +			INA233_RSHUNT_DEFAULT);

This is not an error and thus must not result in an error message.

> +		rshunt = INA233_RSHUNT_DEFAULT;
> +	}
> +
> +	/* read current_lsb value (uA/bit) */
> +	ret = of_property_read_u16(client->dev.of_node, "current-lsb", &current_lsb);
> +	if (ret < 0 || !current_lsb) {
> +		dev_err(&client->dev, "Unable to read current_lsb or value is 0, default value %d uA/bit is used.\n",
> +			INA233_CURRENT_LSB_DEFAULT);

Thjs is not an error and thus must not result in an error message.
Also, a current-lsb value of 0 in devicetree would be wrong and
must not be accepted.

> +		current_lsb = INA233_CURRENT_LSB_DEFAULT;
> +	}
> +
> +	/* calculate current coefficient */
> +	m = current_lsb;
> +	ret = calculate_coef(&m, &R, false);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Calculate_coef error\n");

And ignore the error ? This is a no-go. Either it is an error, and the driver
should abort, or it isn't, and there should be no error message.

Besides, current_lsb is never 0, and the function will never return an error,
so this "error handling" is not only unnecessary but misleading.

> +	} else {
> +		ina233_info.m[PSC_CURRENT_OUT] = m;
> +		ina233_info.R[PSC_CURRENT_OUT] = R;

This is a no-go. There could be multiple INA233 with different parameters
in the system. The second one would overwrite the first one's parameters.

> +	}
> +
> +	/* calculate power coefficient */
> +	m = current_lsb;
> +	ret = calculate_coef(&m, &R, true);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Calculate_coef error\n");
> +	} else {
> +		ina233_info.m[PSC_POWER] = m;
> +		ina233_info.R[PSC_POWER] = R;
> +	}

Same comments as above.

> +
> +	/* write MFR_CALIBRATION register, Apply formula from spec with unit scaling. */
> +	calibration = div_u64((u64)5120000000, (u64)rshunt * current_lsb);

5120000000ULL, and drop the type cast. Also, the divisor of div_u64() is u32,
so type casting its parameter to u64 won't help. If rshunt * current_lsb can
be larger than 32 bit, this would have to use div64_u64().

> +	if (calibration <= 0) {

The result of this calculation is never < 0.

> +		dev_err(&client->dev, "Calibration error\n");
> +		return -1;

This is not a valid error code, and the message is useless. It needs to explain why
the calibration failed, and the returned error code should be -EINVAL.

> +	}
> +	ret = i2c_smbus_write_word_data(client, MFR_CALIBRATION, calibration);

This only writes 16 bit. What if the calibration value is larger than 0xffff ?

> +	if (ret < 0) {
> +		dev_err(&client->dev, "Unable to write calibration\n");
> +		return ret;
> +	}
> +
> +	dev_info(&client->dev, "power monitor %s (Rshunt = %u uOhm, Current_LSB = %u uA/bit)\n",
> +		 client->name, rshunt, current_lsb);

Please refrain from logging noise and make this dev_dbg().

> +
> +	return pmbus_do_probe(client, &ina233_info);
> +}
> +
> +static const struct i2c_device_id ina233_id[] = {
> +	{"ina233", 0},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ina233_id);
> +
> +static const struct of_device_id __maybe_unused ina233_of_match[] = {
> +	{ .compatible = "ti,ina233" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ina233_of_match);
> +
> +/* This is the driver that will be inserted */

Useless comment. Please drop. Yes, it is kind of common in hwmon drivers,
and its existence is partly my fault, but that doesn't make it better.

Thanks,
Guenter

> +static struct i2c_driver ina233_driver = {
> +	.driver = {
> +		   .name = "ina233",
> +		   .of_match_table = of_match_ptr(ina233_of_match),
> +	},
> +	.probe = ina233_probe,
> +	.id_table = ina233_id,
> +};
> +
> +module_i2c_driver(ina233_driver);
> +
> +MODULE_AUTHOR("Leo Yang <Leo-Yang@quantatw.com>");
> +MODULE_DESCRIPTION("PMBus driver for INA233 and compatible chips");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PMBUS");

Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

[Leo Yang]

Hi Guenter,

On Mon, Jan 6, 2025 at 11:31 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Besides, while I did point out a number of problems, but I did not suggest
> to "rewrite the driver".
>
> Since this is v2 of this driver, the submission should have been versioned,
> and a change log should have been provided.
>

Sorry this is my first v2 patch,
I should have been more aware of this problem, thank you.

>
> Why not just pass the power coefficient directly as parameter ?
>
> > +     if (1000000 % *m) {
>
> I fail to understand the logic here. Why scale if and only if m is a clean
> multiple of 1000000 ? Scale if m == 1000001 but not if m == 1000000 ?
> Please explain.
>
> > +             /* Default value, Scaling to keep integer precision,
> > +              * Change it if you need
>
> This comment does not provide any actual information and thus does not
> add any value. Change to what ? Why ? And, again, why not scale if
> m is a multiple of 1000000, no matter how large or small it is ?
>

When we calculate the Telemetry and Warning Conversion Coefficients,
the m-value of the current needs to be calculated via Equation:
1(A)/Current_LSB(A).

The number 1000000 comes from A->uA to match the unit uA of Current_LSB.
Try to prevent the loss of fractional precision in integer.

But this is not enough,
according to spec 7.5.4 Reading Telemetry Data and Warning Thresholds
If there is decimal information in m, we should try to move the decimal point
so that the value of m is between -32768 and 32767 and maximize it as much
as possible to minimize rounding errors.

Therefore, if m does not have decimal information, even if the value of m is
scaled up, it is not possible to minimize rounding errors.

But my comments are not clear enough, I'll fix it.

> > +
> > +     /* Maximize while keeping it bounded.*/
> > +     while (scaled_m > MAX_M_VAL || scaled_m < MIN_M_VAL) {
> > +             scaled_m /= 10;
>
> This looks wrong. If scaled_m < MIN_M_VAL it doesn't make sense
> to decrease it even more.
>

In this part, I try to move the decimal point so that the value of m is between
-32768 and 32767.
Assuming scaled_m = -40001, I can scale it to m = -4000 and adjust it by R++

> > +             scale_factor++;
> > +     }
> > +     /* Scale up only if fractional part exists. */
> > +     while (scaled_m * 10 < MAX_M_VAL && scaled_m * 10 > MIN_M_VAL && !is_integer) {
>
> This looks just as wrong. If scaled_m > 10 * MIN_M_VAL, why increase it even more ?
>

I think the purpose of spec is to keep as many integers as possible in m, and
then save the information in decimals via R to minimize rounding errors.
So here I keep the positive numbers as close to 32767 as possible, and the
negative numbers as close to -32768 as possible.

And thank you for the suggestions, they are very helpful and I will
try to fix them.


Best Regards,

Leo Yang

Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

[Guenter Roeck]

On 1/8/25 16:50, Leo Yang wrote:
> Hi Guenter,
> 
> On Mon, Jan 6, 2025 at 11:31 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Besides, while I did point out a number of problems, but I did not suggest
>> to "rewrite the driver".
>>
>> Since this is v2 of this driver, the submission should have been versioned,
>> and a change log should have been provided.
>>
> 
> Sorry this is my first v2 patch,
> I should have been more aware of this problem, thank you.
> 
>>
>> Why not just pass the power coefficient directly as parameter ?
>>
>>> +     if (1000000 % *m) {
>>
>> I fail to understand the logic here. Why scale if and only if m is a clean
>> multiple of 1000000 ? Scale if m == 1000001 but not if m == 1000000 ?
>> Please explain.
>>
>>> +             /* Default value, Scaling to keep integer precision,
>>> +              * Change it if you need
>>
>> This comment does not provide any actual information and thus does not
>> add any value. Change to what ? Why ? And, again, why not scale if
>> m is a multiple of 1000000, no matter how large or small it is ?
>>
> 
> When we calculate the Telemetry and Warning Conversion Coefficients,
> the m-value of the current needs to be calculated via Equation:
> 1(A)/Current_LSB(A).
> 
> The number 1000000 comes from A->uA to match the unit uA of Current_LSB.
> Try to prevent the loss of fractional precision in integer.
> 
> But this is not enough,
> according to spec 7.5.4 Reading Telemetry Data and Warning Thresholds
> If there is decimal information in m, we should try to move the decimal point
> so that the value of m is between -32768 and 32767 and maximize it as much
> as possible to minimize rounding errors.
> 
> Therefore, if m does not have decimal information, even if the value of m is
> scaled up, it is not possible to minimize rounding errors.
> 
> But my comments are not clear enough, I'll fix it.
> 
>>> +
>>> +     /* Maximize while keeping it bounded.*/
>>> +     while (scaled_m > MAX_M_VAL || scaled_m < MIN_M_VAL) {
>>> +             scaled_m /= 10;
>>
>> This looks wrong. If scaled_m < MIN_M_VAL it doesn't make sense
>> to decrease it even more.
>>
> 
> In this part, I try to move the decimal point so that the value of m is between
> -32768 and 32767.
> Assuming scaled_m = -40001, I can scale it to m = -4000 and adjust it by R++
> 
Sorry, I missed that MIN_M_VAL is negative.

Guenter

>>> +             scale_factor++;
>>> +     }
>>> +     /* Scale up only if fractional part exists. */
>>> +     while (scaled_m * 10 < MAX_M_VAL && scaled_m * 10 > MIN_M_VAL && !is_integer) {
>>
>> This looks just as wrong. If scaled_m > 10 * MIN_M_VAL, why increase it even more ?
>>
> 
> I think the purpose of spec is to keep as many integers as possible in m, and
> then save the information in decimals via R to minimize rounding errors.
> So here I keep the positive numbers as close to 32767 as possible, and the
> negative numbers as close to -32768 as possible.
> 
> And thank you for the suggestions, they are very helpful and I will
> try to fix them.
> 
> 
> Best Regards,
> 
> Leo Yang

Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

[kernel test robot]

Hi Leo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.13-rc6 next-20250107]
[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/Leo-Yang/dt-bindings-Add-INA233-device/20250106-151934
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20250106071337.3017926-3-Leo-Yang%40quantatw.com
patch subject: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor
config: csky-randconfig-r131-20250107 (https://download.01.org/0day-ci/archive/20250108/202501080011.H8YAGThn-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.2.0
reproduce: (https://download.01.org/0day-ci/archive/20250108/202501080011.H8YAGThn-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/202501080011.H8YAGThn-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/hwmon/pmbus/ina233.c:93:26: sparse: sparse: symbol 'ina233_info' was not declared. Should it be static?

vim +/ina233_info +93 drivers/hwmon/pmbus/ina233.c

    92	
  > 93	struct pmbus_driver_info ina233_info = {
    94		.pages = 1,
    95		.format[PSC_VOLTAGE_IN] = direct,
    96		.format[PSC_VOLTAGE_OUT] = direct,
    97		.format[PSC_CURRENT_OUT] = direct,
    98		.format[PSC_POWER] = direct,
    99		.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_INPUT
   100			| PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
   101			| PMBUS_HAVE_POUT
   102			| PMBUS_HAVE_VMON | PMBUS_HAVE_STATUS_VMON,
   103		.m[PSC_VOLTAGE_IN] = 8,
   104		.R[PSC_VOLTAGE_IN] = 2,
   105		.m[PSC_VOLTAGE_OUT] = 8,
   106		.R[PSC_VOLTAGE_OUT] = 2,
   107		.read_word_data = ina233_read_word_data,
   108	};
   109	

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

Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

[kernel test robot]

Hi Leo,

kernel test robot noticed the following build errors:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.13-rc6 next-20250108]
[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/Leo-Yang/dt-bindings-Add-INA233-device/20250106-151934
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20250106071337.3017926-3-Leo-Yang%40quantatw.com
patch subject: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor
config: i386-randconfig-001-20250108 (https://download.01.org/0day-ci/archive/20250109/202501091702.8ZdJcvFC-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250109/202501091702.8ZdJcvFC-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/202501091702.8ZdJcvFC-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__divdi3" [drivers/hwmon/pmbus/ina233.ko] undefined!

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

Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

[kernel test robot]

Hi Leo,

kernel test robot noticed the following build errors:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.13-rc6 next-20250109]
[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/Leo-Yang/dt-bindings-Add-INA233-device/20250106-151934
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20250106071337.3017926-3-Leo-Yang%40quantatw.com
patch subject: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor
config: i386-randconfig-r072-20250109 (https://download.01.org/0day-ci/archive/20250109/202501092213.X9mbPW5Q-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250109/202501092213.X9mbPW5Q-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/202501092213.X9mbPW5Q-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/hwmon/pmbus/ina233.o: in function `calculate_coef':
>> drivers/hwmon/pmbus/ina233.c:59: undefined reference to `__divdi3'


vim +59 drivers/hwmon/pmbus/ina233.c

    23	
    24	static int calculate_coef(int *m, int *R, bool power)
    25	{
    26		s64 scaled_m;
    27		int scale_factor = 0;
    28		int scale_coef = 1;
    29		int power_coef = 1;
    30		bool is_integer = false;
    31	
    32		if (*m == 0) {
    33			*R = 0;
    34			return -1;
    35		}
    36	
    37		if (power)
    38			power_coef = 25;
    39	
    40		if (1000000 % *m) {
    41			/* Default value, Scaling to keep integer precision,
    42			 * Change it if you need
    43			 */
    44			scale_factor = -3;
    45			scale_coef = 1000;
    46		} else {
    47			is_integer = true;
    48		}
    49	
    50		/*
    51		 * Unit Conversion (Current_LSB A->uA) and use scaling(scale_factor)
    52		 * to keep integer precision.
    53		 * Formulae referenced from spec.
    54		 */
    55		scaled_m = div_s64(1000000 * scale_coef, *m * power_coef);
    56	
    57		/* Maximize while keeping it bounded.*/
    58		while (scaled_m > MAX_M_VAL || scaled_m < MIN_M_VAL) {
  > 59			scaled_m /= 10;
    60			scale_factor++;
    61		}
    62		/* Scale up only if fractional part exists. */
    63		while (scaled_m * 10 < MAX_M_VAL && scaled_m * 10 > MIN_M_VAL && !is_integer) {
    64			scaled_m *= 10;
    65			scale_factor--;
    66		}
    67	
    68		*m = scaled_m;
    69		*R = scale_factor;
    70		return 0;
    71	}
    72	

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