Re: [PATCH 3/3] arm64: dts: rockchip: Add Battery Gauge to Anbernic Vita-Pro

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] power: supply: cw2217: 1000x unit error in shunt-resistor-micro-ohms
- [High] power: supply: cw2217: wrong temperature units in cw2217_get_temp()
- [High] power: supply: cw2217: NULL deref when battery-profile is omitted
- [Medium] power: supply: cw2217: missing psy_cfg.fwnode in cw2217_probe()
- [Medium] power: supply: cw2217: unsynchronized work cancel in cw2217_bat_suspend()
- [Low] arm64: dts: rockchip: cw2217 fuel gauge node named charger@64
--

commit b2629263398e52e8a7a0c66b2ac2935fa26ad154
Author: Chris Morgan <macromorgan@hotmail.com>

arm64: dts: rockchip: Add Battery Gauge to Anbernic Vita-Pro

This commit adds the Devicetree node for the Cellwise CW2217 battery fuel
gauge on the Anbernic RG Vita Pro.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts b/arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts
> index 358dea43f9b66..6a730499148db 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts
> @@ -954,7 +954,26 @@ hym8563: rtc@51 {
>  		wakeup-source;
>  	};
>  
> -	/* cellwise,cw221X battery manager at 0x64 */
> +	cw2217: charger@64 {

[Severity: Low]
Since the CW2217 is a battery fuel gauge (registered as
POWER_SUPPLY_TYPE_BATTERY) and this I2C bus already has a separate charger
node (sgm41542: charger@3b), should this node be named fuel-gauge@64 or
battery@64 instead of charger@64?

> +		compatible = "cellwise,cw2217";
> +		reg = <0x64>;

[Severity: High]
In the underlying driver for cellwise,cw2217
(drivers/power/supply/cw2217_battery.c), cw2217_update_data() calls
cw2217_get_temp(), which computes:

	cw_bat->temp = ((int)reg / 2) - 40;

and cw2217_battery_get_property() returns that value directly for
POWER_SUPPLY_PROP_TEMP:

	case POWER_SUPPLY_PROP_TEMP:
		val->intval = cw_bat->temp;
		break;

The power_supply class ABI expects POWER_SUPPLY_PROP_TEMP in tenths of a
degree Celsius (for example, power_supply_read_temp() in
drivers/power/supply/power_supply_core.c multiplies val.intval by 100 to
convert to millidegrees Celsius):

	/* Convert tenths of degree Celsius to milli degree Celsius. */
	*temp = val.intval * 100;

Since the CW2217 temperature register has a 0.5 degree Celsius step with a
-40 degree Celsius offset, doesn't reporting ((int)reg / 2) - 40 instead of
(int)reg * 5 - 400 cause all temperature readings to be 10x too low and
discard the 0.5 degree resolution?

[Severity: Medium]
Also in drivers/power/supply/cw2217_battery.c, cw2217_probe() allocates
cw_bat->battery_workqueue without WQ_FREEZABLE, and cw2217_bat_suspend()
cancels the polling work asynchronously:

	cancel_delayed_work(&cw_bat->battery_delay_work);
	return 0;

while cw2217_bat_work() performs I2C reads and unconditionally re-queues
itself:

	ret = cw2217_update_data(cw_bat);
	if (ret)
		dev_err(cw_bat->dev, "i2c read error when update data");

	queue_delayed_work(cw_bat->battery_workqueue,
			   &cw_bat->battery_delay_work,
			   msecs_to_jiffies(cw_bat->poll_interval_ms));

If cw2217_bat_work() is already running when cw2217_bat_suspend() executes,
won't it race with I2C controller suspend and re-arm battery_delay_work
while the system is suspended unless cancel_delayed_work_sync() is used?

> +		cellwise,battery-profile = /bits/ 8 <
> +			0x5a 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> +			0xab 0xb5 0xae 0xc0 0xa9 0xa4 0xcf 0xaf
> +			0x96 0xff 0xff 0xd7 0x9a 0x83 0x70 0x69
> +			0x65 0x65 0x65 0xb8 0x81 0xd9 0xaf 0xd9
> +			0xd2 0xce 0xcd 0xcb 0xca 0xc7 0xc8 0xc5
> +			0xc2 0xba 0xa8 0x96 0x8a 0x85 0x7d 0x7a
> +			0x7f 0x88 0x92 0x93 0x94 0xd3 0xff 0x5f
> +			0x20 0x00 0xab 0x10 0x00 0x91 0x68 0x00
> +			0x00 0x00 0x64 0x3c 0xa0 0x89 0x00 0x00
> +			0x00 0x00 0x00 0x00 0x00 0x00 0x00 0xb2
> +		>;

[Severity: High]
While this board provides cellwise,battery-profile, the property is optional
in Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml, and
cw2217_parse_properties() in drivers/power/supply/cw2217_battery.c leaves
cw_bat->bat_profile as NULL when it is absent:

	length = device_property_count_u8(dev, "cellwise,battery-profile");
	if (length < 0) {
		dev_warn(cw_bat->dev,
			 "No battery-profile found, using current flash contents\n");
	} else if (length != CW2217_PROFILE_SZ) {

However, cw2217_init() unconditionally calls cw2217_check_profile(), which
either passes cw_bat->bat_profile to memcmp():

	if (memcmp(bat_info, cw_bat->bat_profile, CW2217_PROFILE_SZ)) {
		dev_warn(cw_bat->dev, "Replacing stored battery profile\n");
		cw_bat->update_profile = true;
	}

or sets cw_bat->update_profile = true, causing cw2217_init() ->
cw2217_update_config() to pass cw_bat->bat_profile to regmap_raw_write():

	/* update new battery info */
	ret = regmap_raw_write(cw_bat->regmap, CW2217_BAT_PROFILE,
			       cw_bat->bat_profile, CW2217_PROFILE_SZ);

Won't probing a node without cellwise,battery-profile trigger a NULL pointer
dereference on cw_bat->bat_profile?

> +		monitored-battery = <&battery>;
> +		power-supplies = <&sgm41542>;

[Severity: Medium]
Does power-supplies = <&sgm41542> take effect with the current cw2217 driver?

In drivers/power/supply/cw2217_battery.c, cw2217_probe() registers the power
supply without setting psy_cfg.fwnode:

	psy_cfg.drv_data = cw_bat;
	cw_bat->cw_bat = devm_power_supply_register(&client->dev,
						    &cw2217_bat_desc, &psy_cfg);

When __power_supply_register() calls power_supply_check_supplies() in
drivers/power/supply/power_supply_core.c, it checks:

	/* No device node found, nothing to do */
	if (!psy->dev.fwnode)
		return 0;

Without psy_cfg.fwnode = dev_fwnode(cw_bat->dev), won't
power_supply_check_supplies() return early, ignoring the power-supplies
phandle and skipping probe deferral if sgm41542 has not probed yet?

> +		shunt-resistor-micro-ohms = <2000000>;

[Severity: High]
Is shunt-resistor-micro-ohms scaled by an extra factor of 1000 (nano-ohms
instead of micro-ohms) here, in the binding schema, and in the driver?

A 2 mOhm sense resistor is 2,000 micro-ohms, whereas <2000000> micro-ohms is
2 Ohms.

In Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml, the
range for a 1 mOhm to 50 mOhm shunt resistor is also specified 1000x too
high (1 Ohm to 50 Ohms):

  shunt-resistor-micro-ohms:
    description: |
      The value, in micro-ohms, of the shunt resistor used for sampling
      the battery current.
    minimum: 1000000
    maximum: 50000000

And in drivers/power/supply/cw2217_battery.c, CW2217_USER_RSENSE is defined
as 1500 (1.5 mOhm in micro-ohms), but cw2217_parse_properties() clamps
cw_bat->user_rsense to 1000000..50000000 (which also overwrites the 1500
fallback to 1000000 whenever the property is omitted):

	ret = device_property_read_u32(dev,
				       "shunt-resistor-micro-ohms",
				       &cw_bat->user_rsense);
	if (ret) {
		cw_bat->user_rsense = CW2217_USER_RSENSE;
		dev_warn(cw_bat->dev,
			 "Using default resistor value, reported mA may be wrong\n");
	}

	/* Clamp, but also warn we're clamping due to out of range. */
	if ((cw_bat->user_rsense < 1000000) || (cw_bat->user_rsense > 50000000)) {
		cw_bat->user_rsense = clamp(cw_bat->user_rsense, 1000000, 50000000);
		dev_warn(cw_bat->dev, "Shunt resistor value out of range, setting to %d",
			 cw_bat->user_rsense);
	}

followed by cw2217_get_current() dividing cw_bat->user_rsense by 1000:

	case CW2215_ID:
	case CW2217_ID:
		cw_current = (cw_current * 1600) / (cw_bat->user_rsense / 1000);
		cw_bat->cw_current = cw_current * 1000;
		break;
	case CW2218_ID:
		cw_current = (cw_current * 3815) / (cw_bat->user_rsense / 1000);
		cw_bat->cw_current = cw_current * 1000;
		break;

Should shunt-resistor-micro-ohms use actual micro-ohms (2000 in this DT node,
and 1000..50000 in the schema and driver)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027131751.1724448-1-macroalpha82@gmail.com?part=3