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,