| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <190bb6f2-7bbe-4145-b45f-4ad6672884b8@roeck-us.net>
Date: Fri, 10 Jan 2025 08:22:00 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Leo Yang <leo.yang.sy0@...il.com>, jdelvare@...e.com, robh@...nel.org,
davem@...emloft.net, krzk+dt@...nel.org, conor+dt@...nel.org,
Leo-Yang@...ntatw.com, corbet@....net, Delphine_CC_Chiu@...ynn.com,
linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Cc: kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 2/2] hwmon: Add driver for TI INA232 Current and Power
Monitor
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@...el.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@...ntatw.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@...ntatw.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@...r.kernel.org
> S: Orphan
> F: drivers/video/fbdev/imsttfb.c
>
> +INA233 HARDWARE MONITOR DRIVER
> +M: Leo Yang <Leo-Yang@...ntatw.com>
> +M: Leo Yang <leo.yang.sy0@...il.com>
> +L: linux-hwmon@...r.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@...il.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", ¤t_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@...ntatw.com>");
> +MODULE_DESCRIPTION("PMBus driver for INA233 and compatible chips");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PMBUS");
Powered by blists - more mailing lists