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

[Guenter Roeck <linux@roeck-us.net>]

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@intel.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@quantatw.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@quantatw.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@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/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@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..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@quantatw.com>");
> +MODULE_DESCRIPTION("PMBus driver for INA233 and compatible chips");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PMBUS");