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@xxxxxxxxxx
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@xxxxxxxxxxxx>
---
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@xxxxxxxxxxxx>
+
+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@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/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@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..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", ¤t_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@xxxxxxxxxxxx>");
+MODULE_DESCRIPTION("PMBus driver for INA233 and compatible chips");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("PMBUS");
