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

[Krzysztof Kozlowski <krzk@kernel.org>]

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", &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