Re: [PATCHv2 02/12] power: supply: core: auto-exposure of simple-battery data

[Sebastian Reichel]

Hi,

On Wed, Mar 15, 2023 at 09:01:50AM +0200, Matti Vaittinen wrote:
> On 3/15/23 00:55, Sebastian Reichel wrote:
> [...]
> >   #ifdef CONFIG_THERMAL
> >   static int power_supply_read_temp(struct thermal_zone_device *tzd,
> >   		int *temp)
> > @@ -1255,6 +1387,11 @@ __power_supply_register(struct device *parent,
> >   		goto check_supplies_failed;
> >   	}
> > +	/* psy->battery_info is optional */

I forgot to add a POWER_SUPPLY_TYPE_BATTERY limitation when removing
the opt-in method. Will be added in the next revision.

> > +	rc = power_supply_get_battery_info(psy, &psy->battery_info);
> > +	if (rc && rc != -ENODEV)
> > +		goto check_supplies_failed;
> > +
> 
> This is what rubs me in a slightly wrong way - but again, you probably know
> better than I what's the direction things are heading so please ignore me if
> I am talking nonsense :)
> 
> Anyways, I think the battery information may be relevant to the driver which
> is registering the power-supply. It may be there is a fuel-gauge which needs
> to know the capacity and OCV tables etc. Or some other thingy. And - I may
> be wrong - but I have a feeling it might be something that should be known
> prior registering the power-supply.

You can still do that, just like before. It's a bit inefficient,
since the battery data is allocated twice, but the driver probe
routine is not a hot path.

> So, in my head it should be the driver which is getting the information
> about the battery (whether it is in the DT node or coded in some tables and
> fetched by battery type) - using helpers provided by core.
> 
> I further think it should be the driver who can pass the battery information
> to core at registration - core may 'fall-back' finding information itself if
> driver did not provide it.

This implements the fallback route.

> So, I think the core should not unconditionally populate the battery-info
> here but it should first check if the driver had it already filled.

Not until there is a user (i.e. a driver using that feature). FWIW
it's quite easy to implement once it is needed. Just adding a field
in power_supply_config and taking it over here is enough, no other
code changes are required.

The alternative is adding some kind of probe/remove callback for the
power_supply device itself to properly initialize the device. That
would also be useful to have a sensible place for e.g. shutting of
chargers when the device is removed. Anyways it's a bit out of scope
for this patchset :)

> Well, as I said, I recognize I may not (do not) know all the dirty details
> and I do trust you to evaluate if what I wrote here makes any sense :) All
> in all, I think this auto-exposure is great.
> 
> Please, bear with me if what I wrote above does not make sense to you and
> just assume I don't see the big picture :)

Right now the following battery drivers use power_supply_get_battery_info():

 * cw2015_battery
 * bq27xxx_battery
 * axp20x_battery
 * ug3105_battery
 * ingenic-battery
 * sc27xx_fuel_gauge
 * (generic-adc-battery)

All of them call it after the power-supply device has been
registered. Thus the way to go for them is removing the second call
to power_supply_get_battery_info() and instead use the battery-info
acquired by the core. I think that work deserves its own series.

For chargers the situation is different (they usually want the data
before registration), but they should not expose the battery data
anyways. Also ideally chargers get the information from the battery
power-supply device, which might supply the data from fuel-gauge
registers (or fallback to battery-info after this series).

> >   	spin_lock_init(&psy->changed_lock);
> >   	rc = device_add(dev);
> >   	if (rc)
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index c228205e0953..5842dfe5dfb7 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -380,6 +380,9 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
> >   		}
> >   	}
> > +	if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
> > +		return mode;
> > +
> >   	return 0;
> >   }
> > @@ -461,6 +464,8 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
> >   int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
> >   {
> >   	const struct power_supply *psy = dev_get_drvdata(dev);
> > +	const enum power_supply_property *battery_props =
> > +		power_supply_battery_info_properties;
> >   	int ret = 0, j;
> >   	char *prop_buf;
> > @@ -488,6 +493,16 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
> >   			goto out;
> >   	}
> > +	for (j = 0; j < power_supply_battery_info_properties_size; j++) {
> > +		if (!power_supply_battery_info_has_prop(psy->battery_info,
> > +				battery_props[j]))
> > +			continue;
> 
> Hmm. I just noticed that there can probably be same properties in the
> psy->desc->properties and in the battery-info.

That's intended, so that battery drivers can implement their own
behaviour for the properties.

> I didn't cascade deep into the code so I can't say if it is a
> problem to add duplicates?

It does not break anything (we used to have this for the TYPE
property in a driver), but confuses userspace. I will fix the
duplication in uevents and send a new version.

> So, if this is safe, and if what I wrote above is not something
> you want to consider:
>
> Reviewed-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>

Due to the changes I will not take this over in v3. Just to make
sure - is it correct, that you do not want your R-b tag for the
following two patches?

[05/12] power: supply: generic-adc-battery: drop jitter delay support
[08/12] power: supply: generic-adc-battery: use simple-battery API

> [...]

Thanks for your reviews,

-- Sebastian

Attachment: signature.asc
Description: PGP signature