On 3/16/23 02:41, Sebastian Reichel wrote:
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.
This is out of scope of your series, but as far as I remember the
"battery info getter" power_supply_get_battery_info() provided by the
core required the struct power_supply - which, I believe, should not be
used prior supply registration.
(I think I did drop this requirement and added a getter which allowed
just passing the device (or fwnode) in a simple-gauge RFC series I sent
couple of years ago - but as I said, this is out of the scope)
It's a bit inefficient,
since the battery data is allocated twice, but the driver probe
routine is not a hot path.
This is true. OTOH, people are constantly struggling to push down the
start-up times so maybe we should avoid feeling too comfortable with
probes being slow. Still, I am not opposed to this series!
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.
Fair enough. You are probably right in that things should not be
complicated if there is no need.
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.
I agree.
For chargers the situation is different (they usually want the data
before registration), but they should not expose the battery data
anyways.
I probably should go back studying how the power-supply class works
before continuing this discussion :)
So, is it so that when a single IC contains both the charger logic and
battery monitoring - then the driver is expected to create two
power_supply devices. One for the battery and the other for the charger?
I assume both of these pover_supply devices will then find the same
battery_info - which means that one of the devices (probably the
charger) should not auto-expose properties(?)
Well, as I said I should go study things better before continuing - but
as I have limited time for studying this now I'll just ask if there is a
danger we auto-expose battery details from existing drivers via two
devices? And as I did no study I will just accept what ever answer you
give and trust you to know this better ^_^;
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.
Yes, I thought so. But this is why I asked if doubling the uevents below
was a problem.
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
I didn't feel technically capable of reviewing the above delay patch as
I don't know what kind of delays are typically needed - or if they need
to be configurable - or what is the purpose of this delay (some
stabilization period after charging?)
So, it's not that your patch had something I didn't agree with - I was
just not feeling I understand the consequences of the changes. Purely
from coding perspective it looked good to me :)
[08/12] power: supply: generic-adc-battery: use simple-battery API
This one did look good to me but as it was pretty trivial one I didn't
think my review made much of a difference :) I can reply with my tag on
that one though as I did review what there was to review.
[...]
Thanks for your reviews,
Thanks to you! You are the one making things better here, I am just
treating this as an opportunity to learn ;)
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
