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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
Closes: https://lore.kernel.org/oe-kbuild-all/202501092213.X9mbPW5Q-lkp@xxxxxxxxx/
Closes: https://lore.kernel.org/oe-kbuild-all/202501061734.nPNdRKqO-lkp@xxxxxxxxx/
Signed-off-by: Leo Yang <Leo-Yang@xxxxxxxxxxxx>

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@xxxxxxxxxxxx>
+
+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@xxxxxxxxxxxxxxx
  S:	Orphan
  F:	drivers/video/fbdev/imsttfb.c
+INA233 HARDWARE MONITOR DRIVER
+M:	Leo Yang <Leo-Yang@xxxxxxxxxxxx>
+M:	Leo Yang <leo.yang.sy0@xxxxxxxxx>
+L:	linux-hwmon@xxxxxxxxxxxxxxx
+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@xxxxxxxxx>
  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@xxxxxxxxxxxx>");
+MODULE_DESCRIPTION("PMBus driver for INA233 and compatible chips");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("PMBUS");





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux