linux-kernel - Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <428c034c-79a2-4753-9af8-4dc5af7d310f@kernel.org>
Date: Mon, 6 Jan 2025 11:52:46 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Leo Yang <leo.yang.sy0@...il.com>, jdelvare@...e.com, linux@...ck-us.net,
 robh@...nel.org, 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
Subject: Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power
 Monitor

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@...ynn.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@...ntatw.com>
> +M:	Leo Yang <leo.yang.sy0@...il.com>
> +L:	linux-hwmon@...r.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", &current_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