Re: [PATCH v1 2/2] hwmon: Add support for ina233
From: Guenter Roeck
Date: Wed Sep 20 2023 - 12:29:39 EST
On 9/19/23 22:47, Delphine CC Chiu wrote:
The INA233 device is a current, voltage and power monitor
with an I2C-, SMBus-,and PMBus-compatible interface
that is compliant with digital bus voltages from 1.8 V to 5.0 V.
The device monitors and reports values for current, voltage and power.
The integrated power accumulator can be used for energy
or average power calculations. Programmable calibration value,
conversion times and averaging when combined with an internal multiplier
enable direct readouts of current in amperes and power in watts.
Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@xxxxxxxxxx>
checkpatch says:
WARNING: Missing a blank line after declarations
#181: FILE: drivers/hwmon/pmbus/ina233.c:39:
+ u16 current_lsb;
+ of_property_read_u16(client->dev.of_node, "resistor-calibration", &shunt);
WARNING: else is not generally useful after a break or return
#192: FILE: drivers/hwmon/pmbus/ina233.c:50:
+ return ret;
+ } else {
I should not have to say this, but please run your patches through checkpatch.
---
drivers/hwmon/pmbus/Kconfig | 9 ++++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/ina233.c | 89 ++++++++++++++++++++++++++++++++++++
Documentation missing.
3 files changed, 99 insertions(+)
create mode 100644 drivers/hwmon/pmbus/ina233.c
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index b4e93bd5835e..0abc1fd20bbb 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -509,4 +509,13 @@ config SENSORS_ZL6100
This driver can also be built as a module. If so, the module will
be called zl6100.
+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.
+
endif # PMBUS
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 84ee960a6c2d..c8888e7ac94f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -51,3 +51,4 @@ obj-$(CONFIG_SENSORS_XDPE122) += xdpe12284.o
obj-$(CONFIG_SENSORS_XDPE152) += xdpe152c4.o
obj-$(CONFIG_SENSORS_ZL6100) += zl6100.o
obj-$(CONFIG_SENSORS_PIM4328) += pim4328.o
+obj-$(CONFIG_SENSORS_INA233) += ina233.o
diff --git a/drivers/hwmon/pmbus/ina233.c b/drivers/hwmon/pmbus/ina233.c
new file mode 100644
index 000000000000..393b595344c5
--- /dev/null
+++ b/drivers/hwmon/pmbus/ina233.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Hardware monitoring driver for Texas Instruments INA233
+ *
+ * Copyright (c) 2017 Google Inc
That doesn't make sense.
+ *
+ */
+
+#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_CALIBRATION 0xd4
+
+struct pmbus_driver_info ina233_info = {
+ .pages = 1,
+ .format[PSC_VOLTAGE_IN] = direct,
+ .format[PSC_VOLTAGE_OUT] = direct,
+ .format[PSC_CURRENT_IN] = direct,
+ .format[PSC_CURRENT_OUT] = direct,
+ .format[PSC_POWER] = direct,
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
+ | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT
+ | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT,
+ .m[PSC_VOLTAGE_IN] = 8,
+ .m[PSC_VOLTAGE_OUT] = 8,
+ .R[PSC_VOLTAGE_IN] = 2,
+ .R[PSC_VOLTAGE_OUT] = 2,
+};
+
+static int ina233_probe(struct i2c_client *client)
+{
+ int ret;
+ u16 shunt;
+ u16 current_lsb;
+ of_property_read_u16(client->dev.of_node, "resistor-calibration", &shunt);
+
If of_property_read_u16() fails, this will result in writing a random value
into MFR_CALIBRATION. Also, the lack of a range check seems questionable.
Also, as mentioned in my comments to the bindings, the value to write
into MFR_CALIBRATION should be calculated and not be provided as raw value
in the devicetree properties.
+ ret = i2c_smbus_write_word_data(client, MFR_CALIBRATION, shunt);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to set calibration\n");
+ return ret;
+ }
+ ret = of_property_read_u16(client->dev.of_node, "current-lsb", ¤t_lsb);
This accepts a current-lsb of 0, which would result in a divide by 0
error below.
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to set current_lsb\n");
+ return ret;
+ } else {
+ // Referenced by table of Telemetryand WarningConversionCoefficients in datasheet
Please no C++ comments in hwmon drivers.
+ ina233_info.m[PSC_CURRENT_IN] = 1000 / current_lsb;
+ ina233_info.m[PSC_CURRENT_OUT] = 1000 / current_lsb;
+ ina233_info.m[PSC_POWER] = 40 / current_lsb;
The lack of units makes it all but impossible to validate those equations.
The datasheet says that R must also be calculated. The datasheet specifically says
"Moving the decimal point to maximize the value of m is critical to minimize rounding
errors" and "Care must be taken to adjust the exponent coefficient, R, such that the
value of m remains within the range of –32768 to 32767". If you think that is not
necessary, please explain.
Actually, the values for m will never exceed the range but be _much_ less based on
above equations, which must result in significant loss of precision. "40 / current_lsb"
means the m value for power will never be above 40 and quite likely 0. I really
don't see why this would make sense.
+ }
+
+ 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);
+
+static struct i2c_driver ina233_driver = {
+ .driver = {
+ .name = "ina233",
+ .of_match_table = of_match_ptr(ina233_of_match),
+ },
+ .probe_new = ina233_probe,
+ .id_table = ina233_id,
+};
+
+module_i2c_driver(ina233_driver);
+
+MODULE_AUTHOR("Eli Huang <eli_huang@xxxxxxxxxx>");
+MODULE_DESCRIPTION("PMBus driver for Texas Instruments INA233 and compatible chips");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PMBUS);
+