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

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

[Leo Yang]

Support ina233 driver with binding documents.

---
Change in v2:    
  1. Merge the binding files into ti,ina2xx.yaml.  
  2. Fix typo and non-sense dev_err.
  3. Improve program logic.
  4. pmbus_driver_info changed to be allocated in probe.
  - Link to v1: https://lore.kernel.org/linux-hwmon/20250106071337.3017926-1-Leo-Yang@quantatw.com/
---
Change in v1:     
  1. Change to pmbus standard practice of getting MFR_READ_VSHUNT
     through vmon.
  2. Change the way MFR_CALIBRATION is hard-coded to be obtained by
     calculation.
  - Link to origin: https://lore.kernel.org/linux-hwmon/20230920054739.1561080-1-Delphine_CC_Chiu@wiwynn.com/
---

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

 .../devicetree/bindings/hwmon/ti,ina2xx.yaml  |  17 ++
 Documentation/hwmon/ina233.rst                |  77 ++++++++
 MAINTAINERS                                   |   8 +
 drivers/hwmon/pmbus/Kconfig                   |   9 +
 drivers/hwmon/pmbus/Makefile                  |   1 +
 drivers/hwmon/pmbus/ina233.c                  | 184 ++++++++++++++++++
 6 files changed, 296 insertions(+)
 create mode 100644 Documentation/hwmon/ina233.rst
 create mode 100644 drivers/hwmon/pmbus/ina233.c

-- 
2.39.2

[PATCH v2 1/2] dt-bindings: hwmon: ti,ina2xx: Add INA233 device

[Leo Yang]

Add TI INA233 Current and Power Monitor bindings.

Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
---
 .../devicetree/bindings/hwmon/ti,ina2xx.yaml    | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
index 05a9cb36cd82..9759c69b904b 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
@@ -27,6 +27,7 @@ properties:
       - ti,ina226
       - ti,ina230
       - ti,ina231
+      - ti,ina233
       - ti,ina237
       - ti,ina238
       - ti,ina260
@@ -75,6 +76,22 @@ properties:
       the alert polarity to active-high.
     $ref: /schemas/types.yaml#/definitions/flag
 
+  ti,current-lsb:
+    description:
+      This value depends on the maximum current that can be expected to be
+      measured by ina233 in your circuit, divide Maximum Expected Current
+      by 2^15 and express it in microamps.
+
+      This value will be used to calculate the current/power coefficient for
+      the pmbus and to calibrate the IC.
+
+      For device INA233.
+
+      e.g. Calculate the Maximum Expected Current(A) / 2^15 in micro ampere (uA).
+      30A / 2^15 = 915 uA
+      If value is close to 1000 uA, can use 1000 uA to minimize rounding errors.
+    $ref: /schemas/types.yaml#/definitions/uint16
+
 required:
   - compatible
   - reg
-- 
2.39.2

Re: [PATCH v2 1/2] dt-bindings: hwmon: ti,ina2xx: Add INA233 device

[Krzysztof Kozlowski]

On 10/01/2025 09:15, Leo Yang wrote:
> Add TI INA233 Current and Power Monitor bindings.
> 
> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
> ---
>  .../devicetree/bindings/hwmon/ti,ina2xx.yaml    | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
> index 05a9cb36cd82..9759c69b904b 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
> @@ -27,6 +27,7 @@ properties:
>        - ti,ina226
>        - ti,ina230
>        - ti,ina231
> +      - ti,ina233
>        - ti,ina237
>        - ti,ina238
>        - ti,ina260
> @@ -75,6 +76,22 @@ properties:
>        the alert polarity to active-high.
>      $ref: /schemas/types.yaml#/definitions/flag
>  
> +  ti,current-lsb:
> +    description:
> +      This value depends on the maximum current that can be expected to be

Then use microamp unit suffix and drop the ref and all these calculations.

> +      measured by ina233 in your circuit, divide Maximum Expected Current
> +      by 2^15 and express it in microamps.
> +
> +      This value will be used to calculate the current/power coefficient for
> +      the pmbus and to calibrate the IC.
> +
> +      For device INA233.

And not valid for other devices? Then in allOf:if:then: you should
disallow it for others and drop redundant descriptions (don't repeat the
schema).

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: hwmon: ti,ina2xx: Add INA233 device

[Krzysztof Kozlowski]

On Fri, Jan 10, 2025 at 04:15:45PM +0800, Leo Yang wrote:
> Add TI INA233 Current and Power Monitor bindings.
> 
> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>

Also, SoB does not match From.

Please run scripts/checkpatch.pl and fix reported warnings. After that,
run also 'scripts/checkpatch.pl --strict' and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.

Best regards,
Krzysztof

[PATCH v2 2/2] hwmon: Add driver for TI INA232 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

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202501092213.X9mbPW5Q-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202501061734.nPNdRKqO-lkp@intel.com/
Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
---
 Documentation/hwmon/ina233.rst |  77 ++++++++++++++
 MAINTAINERS                    |   8 ++
 drivers/hwmon/pmbus/Kconfig    |   9 ++
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/ina233.c   | 184 +++++++++++++++++++++++++++++++++
 5 files changed, 279 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..41537f89bed5
--- /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/ti,ina2xx.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/MAINTAINERS b/MAINTAINERS
index c575de4903db..fde1713dff9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11226,6 +11226,14 @@ 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/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..7899c3e99eeb
--- /dev/null
+++ b/drivers/hwmon/pmbus/ina233.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for ina233
+ *
+ * Copyright (c) 2025 Leo Yang
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+#define MFR_READ_VSHUNT 0xd1
+#define MFR_CALIBRATION 0xd4
+
+#define INA233_CURRENT_LSB_DEFAULT	1000 /* uA */
+#define INA233_RSHUNT_DEFAULT		2000 /* uOhm */
+
+#define MAX_M_VAL 32767
+#define MIN_M_VAL -32768
+
+static void calculate_coef(int *m, int *R, int power_coef)
+{
+	s64 scaled_m;
+	int scale_factor = 0;
+	int scale_coef = 1;
+	bool is_integer = false;
+
+	/*
+	 * 1000000 from Current_LSB A->uA .
+	 * scale_coef is for scaling up to minimize rounding errors,
+	 * If there is no decimal information, No need to scale.
+	 */
+	if (1000000 % *m) {
+		/* Scaling to keep integer precision */
+		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 = div_s64(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;
+}
+
+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;
+}
+
+static int ina233_probe(struct i2c_client *client)
+{
+	int ret, m, R;
+	u32 rshunt;
+	u16 current_lsb;
+	u16 calibration;
+	struct pmbus_driver_info *info;
+
+	info = devm_kzalloc(&client->dev, sizeof(struct pmbus_driver_info),
+			    GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->pages = 1;
+	info->format[PSC_VOLTAGE_IN] = direct;
+	info->format[PSC_VOLTAGE_OUT] = direct;
+	info->format[PSC_CURRENT_OUT] = direct;
+	info->format[PSC_POWER] = direct;
+	info->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;
+	info->m[PSC_VOLTAGE_IN] = 8;
+	info->R[PSC_VOLTAGE_IN] = 2;
+	info->m[PSC_VOLTAGE_OUT] = 8;
+	info->R[PSC_VOLTAGE_OUT] = 2;
+	info->read_word_data = ina233_read_word_data;
+
+	/* If INA233 skips current/power, shunt-resistor and current-lsb aren't needed.	*/
+	/* read rshunt value (uOhm) */
+	if (of_property_read_u32(client->dev.of_node, "shunt-resistor", &rshunt) < 0)
+		rshunt = INA233_RSHUNT_DEFAULT;
+
+	/* read current_lsb value (uA) */
+	if (of_property_read_u16(client->dev.of_node, "ti,current-lsb", &current_lsb) < 0)
+		current_lsb = INA233_CURRENT_LSB_DEFAULT;
+
+	if (!rshunt || !current_lsb) {
+		dev_err(&client->dev, "shunt-resistor and current-lsb cannot be zero.\n");
+		return -EINVAL;
+	}
+
+	/* calculate current coefficient */
+	m = current_lsb;
+	calculate_coef(&m, &R, 1);
+	info->m[PSC_CURRENT_OUT] = m;
+	info->R[PSC_CURRENT_OUT] = R;
+
+	/* calculate power coefficient */
+	m = current_lsb;
+	calculate_coef(&m, &R, 25);
+	info->m[PSC_POWER] = m;
+	info->R[PSC_POWER] = R;
+
+	/* write MFR_CALIBRATION register, Apply formula from spec with unit scaling. */
+	calibration = div64_u64(5120000000ULL, (u64)rshunt * current_lsb);
+	if (calibration > 0x7FFF) {
+		dev_err(&client->dev, "Exceeds MFR_CALIBRATION limit, Use a reasonable Current_LSB with Shunt resistor value.\n");
+		return -EINVAL;
+	}
+	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_dbg(&client->dev, "power monitor %s (Rshunt = %u uOhm, Current_LSB = %u uA/bit)\n",
+		client->name, rshunt, current_lsb);
+
+	return pmbus_do_probe(client, 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);
+
+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 v2 2/2] hwmon: Add driver for TI INA232 Current and Power Monitor

[Krzysztof Kozlowski]

On 10/01/2025 09:15, 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
> 
> Reported-by: kernel test robot <lkp@intel.com>

No, what did the robot report? Drop.

> Closes: https://lore.kernel.org/oe-kbuild-all/202501092213.X9mbPW5Q-lkp@intel.com/

Drop

> Closes: https://lore.kernel.org/oe-kbuild-all/202501061734.nPNdRKqO-lkp@intel.com/

Drop

> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
> ---
>  Documentation/hwmon/ina233.rst |  77 ++++++++++++++
>  MAINTAINERS                    |   8 ++
>  drivers/hwmon/pmbus/Kconfig    |   9 ++
>  drivers/hwmon/pmbus/Makefile   |   1 +
>  drivers/hwmon/pmbus/ina233.c   | 184 +++++++++++++++++++++++++++++++++
>  5 files changed, 279 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..41537f89bed5
> --- /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/ti,ina2xx.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

You still have patch warnings. I already commented on this, so you have
to fix it everywhere.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index c575de4903db..fde1713dff9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11226,6 +11226,14 @@ 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>

One email.

> +L:	linux-hwmon@vger.kernel.org
> +S:	Odd Fixes

Why would we like to have unmaintained driver? Odd fixes is candidate to
removal, so shall we accept it and remove immediately?


> +F:	Documentation/hwmon/ina233.rst
> +F:	drivers/hwmon/pmbus/ina233.c
> +

...

> +
> +	/* If INA233 skips current/power, shunt-resistor and current-lsb aren't needed.	*/
> +	/* read rshunt value (uOhm) */
> +	if (of_property_read_u32(client->dev.of_node, "shunt-resistor", &rshunt) < 0)
> +		rshunt = INA233_RSHUNT_DEFAULT;
> +
> +	/* read current_lsb value (uA) */
> +	if (of_property_read_u16(client->dev.of_node, "ti,current-lsb", &current_lsb) < 0)
> +		current_lsb = INA233_CURRENT_LSB_DEFAULT;
> +
> +	if (!rshunt || !current_lsb) {
> +		dev_err(&client->dev, "shunt-resistor and current-lsb cannot be zero.\n");

Then properties must have constraints in your schema.

> +		return -EINVAL;


Best regards,
Krzysztof

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

[Guenter Roeck]

On 1/10/25 00:15, Leo Yang wrote:
> Support ina233 driver for Meta Yosemite V4.
> 

Irrelevant. Please drop.

The subject wrongly refers to INA232.

> Driver for Texas Instruments INA233 Current and Power Monitor
> With I2C-, SMBus-, and PMBus-Compatible Interface
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202501092213.X9mbPW5Q-lkp@intel.com/
> Closes: https://lore.kernel.org/oe-kbuild-all/202501061734.nPNdRKqO-lkp@intel.com/
> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>

Other feedback is on top of feedback from Krzysztof.

> ---
>   Documentation/hwmon/ina233.rst |  77 ++++++++++++++
>   MAINTAINERS                    |   8 ++
>   drivers/hwmon/pmbus/Kconfig    |   9 ++
>   drivers/hwmon/pmbus/Makefile   |   1 +
>   drivers/hwmon/pmbus/ina233.c   | 184 +++++++++++++++++++++++++++++++++
>   5 files changed, 279 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..41537f89bed5
> --- /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/ti,ina2xx.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/MAINTAINERS b/MAINTAINERS
> index c575de4903db..fde1713dff9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11226,6 +11226,14 @@ 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/hwmon/ina233.rst
> +F:	drivers/hwmon/pmbus/ina233.c
> +

In addition to other comments: No "Odd Fixes" for hwmon drivers, please.
Just drop this entry.

>   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..7899c3e99eeb
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ina233.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for ina233
> + *
> + * Copyright (c) 2025 Leo Yang
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include "pmbus.h"
> +
> +#define MFR_READ_VSHUNT 0xd1
> +#define MFR_CALIBRATION 0xd4
> +
> +#define INA233_CURRENT_LSB_DEFAULT	1000 /* uA */
> +#define INA233_RSHUNT_DEFAULT		2000 /* uOhm */
> +
> +#define MAX_M_VAL 32767
> +#define MIN_M_VAL -32768
> +
> +static void calculate_coef(int *m, int *R, int power_coef)
> +{
> +	s64 scaled_m;
> +	int scale_factor = 0;
> +	int scale_coef = 1;
> +	bool is_integer = false;
> +
> +	/*
> +	 * 1000000 from Current_LSB A->uA .
> +	 * scale_coef is for scaling up to minimize rounding errors,
> +	 * If there is no decimal information, No need to scale.

s/No/no/

> +	 */
> +	if (1000000 % *m) {
> +		/* Scaling to keep integer precision */
> +		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 = div_s64(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;
> +}
> +
> +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;
> +}
> +
> +static int ina233_probe(struct i2c_client *client)
> +{
> +	int ret, m, R;
> +	u32 rshunt;
> +	u16 current_lsb;
> +	u16 calibration;
> +	struct pmbus_driver_info *info;
> +
> +	info = devm_kzalloc(&client->dev, sizeof(struct pmbus_driver_info),
> +			    GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->pages = 1;
> +	info->format[PSC_VOLTAGE_IN] = direct;
> +	info->format[PSC_VOLTAGE_OUT] = direct;
> +	info->format[PSC_CURRENT_OUT] = direct;
> +	info->format[PSC_POWER] = direct;
> +	info->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;
> +	info->m[PSC_VOLTAGE_IN] = 8;
> +	info->R[PSC_VOLTAGE_IN] = 2;
> +	info->m[PSC_VOLTAGE_OUT] = 8;
> +	info->R[PSC_VOLTAGE_OUT] = 2;
> +	info->read_word_data = ina233_read_word_data;
> +
> +	/* If INA233 skips current/power, shunt-resistor and current-lsb aren't needed.	*/
> +	/* read rshunt value (uOhm) */
> +	if (of_property_read_u32(client->dev.of_node, "shunt-resistor", &rshunt) < 0)
> +		rshunt = INA233_RSHUNT_DEFAULT;
> +
> +	/* read current_lsb value (uA) */
> +	if (of_property_read_u16(client->dev.of_node, "ti,current-lsb", &current_lsb) < 0)
> +		current_lsb = INA233_CURRENT_LSB_DEFAULT;
> +
of_property_read_u16() returns -EOVERFLOW if the value provided was too large.
This should be checked to avoid situations where the value provided in devicetree
is too large.

A more higher level question: why not use device_property functions ?

> +	if (!rshunt || !current_lsb) {
> +		dev_err(&client->dev, "shunt-resistor and current-lsb cannot be zero.\n");
> +		return -EINVAL;

		return dev_err_probe(...);

Please use a consistent term for current LSB in error messages.

> +	}
> +
> +	/* calculate current coefficient */
> +	m = current_lsb;
> +	calculate_coef(&m, &R, 1);
> +	info->m[PSC_CURRENT_OUT] = m;
> +	info->R[PSC_CURRENT_OUT] = R;
> +
> +	/* calculate power coefficient */
> +	m = current_lsb;
> +	calculate_coef(&m, &R, 25);
> +	info->m[PSC_POWER] = m;
> +	info->R[PSC_POWER] = R;
> +
> +	/* write MFR_CALIBRATION register, Apply formula from spec with unit scaling. */
> +	calibration = div64_u64(5120000000ULL, (u64)rshunt * current_lsb);
> +	if (calibration > 0x7FFF) {
> +		dev_err(&client->dev, "Exceeds MFR_CALIBRATION limit, Use a reasonable Current_LSB with Shunt resistor value.\n");

Use dev_err_probe(). The error message is not actionable. Provide the values.
Something like "Current_LSB %u too small for shunt resistor value %u" or similar
would be more helpful.

> +		return -EINVAL;
> +	}
> +	ret = i2c_smbus_write_word_data(client, MFR_CALIBRATION, calibration);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Unable to write calibration\n");
> +		return ret;

		return dev_err_probe(...);

> +	}
> +
> +	dev_dbg(&client->dev, "power monitor %s (Rshunt = %u uOhm, Current_LSB = %u uA/bit)\n",
> +		client->name, rshunt, current_lsb);
> +
> +	return pmbus_do_probe(client, 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);
> +
> +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 v2 2/2] hwmon: Add driver for TI INA232 Current and Power Monitor

[Leo Yang]

Hi Guenter,

On Sat, Jan 11, 2025 at 12:22 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> > +
> > +     /* If INA233 skips current/power, shunt-resistor and current-lsb aren't needed. */
> > +     /* read rshunt value (uOhm) */
> > +     if (of_property_read_u32(client->dev.of_node, "shunt-resistor", &rshunt) < 0)
> > +             rshunt = INA233_RSHUNT_DEFAULT;
> > +
> > +     /* read current_lsb value (uA) */
> > +     if (of_property_read_u16(client->dev.of_node, "ti,current-lsb", &current_lsb) < 0)
> > +             current_lsb = INA233_CURRENT_LSB_DEFAULT;
> > +
> of_property_read_u16() returns -EOVERFLOW if the value provided was too large.
> This should be checked to avoid situations where the value provided in devicetree
> is too large.
>
Sorry I have a question, I can't get it to return -EOVERFLOW when I test it
I am using the following properties:
test16 = /bits/ 16 <0xfffd>;
of_property_read_u16 reads 0xfffd

test16o = /bits/ 16 <0xfffdd>;
of_property_read_u16 reads 0xffdd

test16o = <0xfffdd>;
of_property_read_u16 reads 0xf

test16array = /bits/ 16 <0xfffd 0xfffe>;
of_property_read_u16 reads 0xfffd

The same result for device_property_read_u16, it seems that a data
truncation occurs and none of them return -EOVERFLOW.

So maybe there is no need to check EOVERFLOW?
Or maybe we could use the minimum and maximum of the binding to
indicate the range.


Best Regards,

Leo Yang

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

[Guenter Roeck]

On 1/14/25 00:02, Leo Yang wrote:
> Hi Guenter,
> 
> On Sat, Jan 11, 2025 at 12:22 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>>> +
>>> +     /* If INA233 skips current/power, shunt-resistor and current-lsb aren't needed. */
>>> +     /* read rshunt value (uOhm) */
>>> +     if (of_property_read_u32(client->dev.of_node, "shunt-resistor", &rshunt) < 0)
>>> +             rshunt = INA233_RSHUNT_DEFAULT;
>>> +
>>> +     /* read current_lsb value (uA) */
>>> +     if (of_property_read_u16(client->dev.of_node, "ti,current-lsb", &current_lsb) < 0)
>>> +             current_lsb = INA233_CURRENT_LSB_DEFAULT;
>>> +
>> of_property_read_u16() returns -EOVERFLOW if the value provided was too large.
>> This should be checked to avoid situations where the value provided in devicetree
>> is too large.
>>
> Sorry I have a question, I can't get it to return -EOVERFLOW when I test it
> I am using the following properties:
> test16 = /bits/ 16 <0xfffd>;
> of_property_read_u16 reads 0xfffd
> 
> test16o = /bits/ 16 <0xfffdd>;
> of_property_read_u16 reads 0xffdd
> 
> test16o = <0xfffdd>;
> of_property_read_u16 reads 0xf
> 
> test16array = /bits/ 16 <0xfffd 0xfffe>;
> of_property_read_u16 reads 0xfffd
> 
> The same result for device_property_read_u16, it seems that a data
> truncation occurs and none of them return -EOVERFLOW.
> 
> So maybe there is no need to check EOVERFLOW?
> Or maybe we could use the minimum and maximum of the binding to
> indicate the range.
> 

of_property_read_u16() calls of_property_read_u16_array(). The API documentation
of of_property_read_u16_array() lists -ENODATA and -EOVERFLOW as possible error
return values, in addition to -EINVAL. Ignoring those errors is not a good idea,
even if it is not immediately obvious how to reproduce them.

Guenter

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

[Christophe JAILLET]

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

Should the subject be INA233?


Le 10/01/2025 à 09:15, Leo Yang a écrit :
> Support ina233 driver for Meta Yosemite V4.
> 
> Driver for Texas Instruments INA233 Current and Power Monitor
> With I2C-, SMBus-, and PMBus-Compatible Interface
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202501092213.X9mbPW5Q-lkp@intel.com/
> Closes: https://lore.kernel.org/oe-kbuild-all/202501061734.nPNdRKqO-lkp@intel.com/
> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>

...

> +static void calculate_coef(int *m, int *R, int power_coef)
> +{
> +	s64 scaled_m;
> +	int scale_factor = 0;
> +	int scale_coef = 1;
> +	bool is_integer = false;

Is is_integer really needed?

See below.

> +
> +	/*
> +	 * 1000000 from Current_LSB A->uA .
> +	 * scale_coef is for scaling up to minimize rounding errors,
> +	 * If there is no decimal information, No need to scale.
> +	 */
> +	if (1000000 % *m) {
> +		/* Scaling to keep integer precision */
> +		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 = div_s64(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) {

s/!is_integer/scale_coef != 1/
?

> +		scaled_m *= 10;
> +		scale_factor--;
> +	}
> +
> +	*m = scaled_m;
> +	*R = scale_factor;
> +}

...

> +static const struct i2c_device_id ina233_id[] = {
> +	{"ina233", 0},

Extra spaces to be consistant with ina233_of_match below?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ina233_id);
> +
> +static const struct of_device_id __maybe_unused ina233_of_match[] = {
> +	{ .compatible = "ti,ina233" },
> +	{ },

No need for an extra comma after a terminator.

> +};
> +MODULE_DEVICE_TABLE(of, ina233_of_match);

...

CJ