Re: [PATCH 5/5] power: supply: rk817-charger: Clear SYS_CAN_SD register by default

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] power: supply: rk817-charger: property read on node-less charger->dev in rk817_battery_init()
- [High] power: supply: rk817-charger: gate-function-disable read from NULL dev fwnode
--

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

power: supply: rk817-charger: Clear SYS_CAN_SD register by default

This commit clears the RK817_SYS_CAN_SD bit in the RK817_PMIC_CHRG_TERM
register during battery initialization unless the
rockchip,gate-function-disable property is present, in order to prevent
persistent power drain while the device is powered off.

>     power: supply: rk817-charger: Clear SYS_CAN_SD register by default
>     
>     The BSP driver for the rk817 charger by default sets the SYS_CAN_SD
>     register unless a specific attribute is set in the device tree. This
>     register tells the PMIC that the system voltage can be shut down in
>     battery off mode.
>     
>     Though the BSP sets this value by default, it also has a device tree
>     value to override it.
>     
>     When this value is not set, it can result in a persistent power
>     drain while the device is off, as observed by users.
>     
>     Fixes: 11cb8da0189b ("power: supply: Add charger driver for Rockchip RK817")
>     Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
>     Suggested-by: Zetarancio <zetarancio@users.noreply.github.com>

[Severity: Low]
This isn't a bug in the code, but the commit message body says the driver by
default sets the SYS_CAN_SD bit and that power drain occurs when this value
is not set, whereas the commit subject and the code call regmap_clear_bits()
to clear RK817_SYS_CAN_SD. Should the commit message body say "clears"
instead of "sets"?

> diff --git a/drivers/power/supply/rk817_charger.c b/drivers/power/supply/rk817_charger.c
> index b6714d8e4fed9..6971f572c68d0 100644
> --- a/drivers/power/supply/rk817_charger.c
> +++ b/drivers/power/supply/rk817_charger.c
> @@ -1148,6 +1148,15 @@ static int rk817_battery_init(struct rk817_charger *charger,
>  	regmap_write_bits(rk808->regmap, RK817_PMIC_CHRG_IN, RK817_USB_ILIM_EN,
>  			  (0x01 << 3));
>  
> +	/*
> +	 * Shutdown the system voltage in bat_off mode, unless
> +	 * gate-function-disable is set.
> +	 */
> +	if (!device_property_read_bool(charger->dev,
> +				       "rockchip,gate-function-disable"))
> +		regmap_clear_bits(rk808->regmap,
> +				  RK817_PMIC_CHRG_TERM, RK817_SYS_CAN_SD);

[Severity: High]
Will device_property_read_bool(charger->dev, "rockchip,gate-function-disable")
ever find the property here?

In drivers/mfd/rk8xx-core.c, the mfd_cell entry in rk817s[] for
rk817-charger does not set .of_compatible (and the charger child node in
Documentation/devicetree/bindings/mfd/rockchip,rk817.yaml has no compatible
property):

drivers/mfd/rk8xx-core.c:
	{
		.name = "rk817-charger",
		.num_resources = ARRAY_SIZE(rk817_charger_resources),
		.resources = &rk817_charger_resources[0],
	},

Because of this, mfd_add_device() leaves pdev->dev.of_node and
pdev->dev.fwnode as NULL.

In rk817_charger_probe(), the driver manually looks up the child node from
the parent PMIC and only attaches its fwnode to pscfg.fwnode for the
registered power_supply devices, while reading all other charger properties
directly from node:

drivers/power/supply/rk817_charger.c:rk817_charger_probe() {
	node = of_get_child_by_name(dev->parent->of_node, "charger");
	if (!node)
		return -ENODEV;
	...
	charger->dev = &pdev->dev;
	platform_set_drvdata(pdev, charger);

	rk817_bat_calib_vol(charger);

	pscfg.drv_data = charger;
	pscfg.fwnode = &node->fwnode;
	...
	ret = of_property_read_u32(node, "rockchip,resistor-sense-micro-ohms",
				   &of_value);
}

Since dev_fwnode(charger->dev) is NULL when rk817_charger_probe() calls
rk817_battery_init(), device_property_read_bool(charger->dev, ...) always
returns false and RK817_SYS_CAN_SD is cleared unconditionally even when
rockchip,gate-function-disable is present in the charger Device Tree node.

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