| Message ID | 20250106071337.3017926-3-Leo-Yang@quantatw.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: Add support for INA233 | expand |
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
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", ¤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); Same problem > + current_lsb = INA233_CURRENT_LSB_DEFAULT; > + } > + Best regards, Krzysztof
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", ¤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@quantatw.com>"); > +MODULE_DESCRIPTION("PMBus driver for INA233 and compatible chips"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS("PMBUS");
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
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
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
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!
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
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", ¤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); + 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");
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