Re: [PATCH v2 8/9] power: supply: rt5033_battery: Adopt status property from charger

[Jakob Hauser]

Hi Sebastian,

I noticed a small mistake in patch 8.

On 16.04.23 14:44, Jakob Hauser wrote:
> The rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger
> can, let's get this value.
>
> Tested-by: Raymond Hackley <raymondhackley@xxxxxxxxxxxxxx>
> Signed-off-by: Jakob Hauser <jahau@xxxxxxxxxxxxxx>
> ---
>   drivers/power/supply/rt5033_battery.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>
> diff --git a/drivers/power/supply/rt5033_battery.c b/drivers/power/supply/rt5033_battery.c
> index 5c04cf305219..48d4cccce4f6 100644
> --- a/drivers/power/supply/rt5033_battery.c
> +++ b/drivers/power/supply/rt5033_battery.c
> @@ -12,6 +12,26 @@
>   #include <linux/mfd/rt5033-private.h>
>   #include <linux/mfd/rt5033.h>
>   
> +static int rt5033_battery_get_status(struct i2c_client *client)
> +{
> +	struct power_supply *charger;
> +	union power_supply_propval val;
> +	int ret;
> +
> +	charger = power_supply_get_by_name("rt5033-charger");
> +	if (!charger)
> +		return -ENODEV;
> +
> +	ret = power_supply_get_property(charger, POWER_SUPPLY_PROP_STATUS, &val);
> +	if (ret) {
> +		power_supply_put(charger);
> +		return POWER_SUPPLY_STATUS_UNKNOWN;
> +	}
> +
> +	power_supply_put(charger);
> +	return val.intval;
> +}
> +

If the rt5033-charger driver is not available, this function returns 
"-ENODEV". Instead of an error, in fact the status node in sysfs just 
reports "-19" then. Userspace layer UPower makes status "unknown" out of 
this.

An error message would spam dmesg anyway, as it would be issued every 
time the battery gets polled by UPower, which is quite regularly. The 
scenario of a missing rt5033-charger driver is not unlikely for devices 
where it's not yet implemented in the devicetree or in the configs of 
the compiled kernel. For the displayed battery icon, UPower assumes 
"discharging" for a single battery in "unknown" state.

It makes more sense to return "POWER_SUPPLY_STATUS_UNKNOWN" right away. 
I'll change that line in v3.

>   static int rt5033_battery_get_capacity(struct i2c_client *client)
>   {
>   	struct rt5033_battery *battery = i2c_get_clientdata(client);
> @@ -84,6 +104,9 @@ static int rt5033_battery_get_property(struct power_supply *psy,
>   	case POWER_SUPPLY_PROP_CAPACITY:
>   		val->intval = rt5033_battery_get_capacity(battery->client);
>   		break;
> +	case POWER_SUPPLY_PROP_STATUS:
> +		val->intval = rt5033_battery_get_status(battery->client);
> +		break;
>   	default:
>   		return -EINVAL;
>   	}
> @@ -96,6 +119,7 @@ static enum power_supply_property rt5033_battery_props[] = {
>   	POWER_SUPPLY_PROP_VOLTAGE_OCV,
>   	POWER_SUPPLY_PROP_PRESENT,
>   	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_STATUS,
>   };
>   
>   static const struct regmap_config rt5033_battery_regmap_config = {

Kind regards,
Jakob