Date:   Thu, 16 Mar 2023 09:10:03 +0200
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Sebastian Reichel <sre@...nel.org>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCHv2 02/12] power: supply: core: auto-exposure of
 simple-battery data

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@...il.com>
> 
> 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! ~~

Powered by blists - more mailing lists