Message-ID: <4a1cb223-448c-3e93-eb38-d86f543659e3@roeck-us.net>
Date:   Wed, 20 Sep 2023 09:29:26 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>, patrick@...cx.xyz,
        Jean Delvare <jdelvare@...e.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] hwmon: Add support for ina233

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@...ynn.com>

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", &current_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@...ynn.com>");
> +MODULE_DESCRIPTION("PMBus driver for Texas Instruments INA233 and compatible chips");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);
> +

Powered by blists - more mailing lists