| Message ID | 20230314225535.1321736-3-sre@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add DT support for generic ADC battery | expand |
On 3/15/23 00:55, Sebastian Reichel wrote: > Add support for automatically exposing data from the > simple-battery firmware node with a single configuration > option in the power-supply device. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> > --- > drivers/power/supply/power_supply_core.c | 173 +++++++++++++++++++--- > drivers/power/supply/power_supply_sysfs.c | 15 ++ > include/linux/power_supply.h | 8 + > 3 files changed, 178 insertions(+), 18 deletions(-) > > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c > index f3d7c1da299f..842c27de4fac 100644 > --- a/drivers/power/supply/power_supply_core.c > +++ b/drivers/power/supply/power_supply_core.c > @@ -388,7 +388,7 @@ static int __power_supply_get_supplier_property(struct device *dev, void *_data) > struct psy_get_supplier_prop_data *data = _data; > > if (__power_supply_is_supplied_by(epsy, data->psy)) > - if (!epsy->desc->get_property(epsy, data->psp, data->val)) > + if (!power_supply_get_property(epsy, data->psp, data->val)) > return 1; /* Success */ > > return 0; /* Continue iterating */ > @@ -832,6 +832,133 @@ void power_supply_put_battery_info(struct power_supply *psy, > } > EXPORT_SYMBOL_GPL(power_supply_put_battery_info); > > +const enum power_supply_property power_supply_battery_info_properties[] = { > + POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN, > + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, > + POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN, > + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, > + POWER_SUPPLY_PROP_PRECHARGE_CURRENT, > + POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT, > + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX, > + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX, > + POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN, > + POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX, > + POWER_SUPPLY_PROP_TEMP_ALERT_MIN, > + POWER_SUPPLY_PROP_TEMP_ALERT_MAX, > + POWER_SUPPLY_PROP_TEMP_MIN, > + POWER_SUPPLY_PROP_TEMP_MAX, > +}; > +EXPORT_SYMBOL_GPL(power_supply_battery_info_properties); > + > +const size_t power_supply_battery_info_properties_size = ARRAY_SIZE(power_supply_battery_info_properties); > +EXPORT_SYMBOL_GPL(power_supply_battery_info_properties_size); > + > +bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info, > + enum power_supply_property psp) > +{ > + if (!info) > + return false; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_TECHNOLOGY: > + return info->technology != POWER_SUPPLY_TECHNOLOGY_UNKNOWN; > + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN: > + return info->energy_full_design_uwh >= 0; > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > + return info->charge_full_design_uah >= 0; > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: > + return info->voltage_min_design_uv >= 0; > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: > + return info->voltage_max_design_uv >= 0; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + return info->precharge_current_ua >= 0; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + return info->charge_term_current_ua >= 0; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: > + return info->constant_charge_current_max_ua >= 0; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: > + return info->constant_charge_voltage_max_uv >= 0; > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN: > + return info->temp_ambient_alert_min > INT_MIN; > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX: > + return info->temp_ambient_alert_max < INT_MAX; > + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN: > + return info->temp_alert_min > INT_MIN; > + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: > + return info->temp_alert_max < INT_MAX; > + case POWER_SUPPLY_PROP_TEMP_MIN: > + return info->temp_min > INT_MIN; > + case POWER_SUPPLY_PROP_TEMP_MAX: > + return info->temp_max < INT_MAX; > + default: > + return false; > + } > +} > +EXPORT_SYMBOL_GPL(power_supply_battery_info_has_prop); > + > +int power_supply_battery_info_get_prop(struct power_supply_battery_info *info, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + if (!info) > + return -EINVAL; > + > + if (!power_supply_battery_info_has_prop(info, psp)) > + return -EINVAL; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_TECHNOLOGY: > + val->intval = info->technology; > + return 0; > + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN: > + val->intval = info->energy_full_design_uwh; > + return 0; > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > + val->intval = info->charge_full_design_uah; > + return 0; > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: > + val->intval = info->voltage_min_design_uv; > + return 0; > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: > + val->intval = info->voltage_max_design_uv; > + return 0; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + val->intval = info->precharge_current_ua; > + return 0; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + val->intval = info->charge_term_current_ua; > + return 0; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: > + val->intval = info->constant_charge_current_max_ua; > + return 0; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: > + val->intval = info->constant_charge_voltage_max_uv; > + return 0; > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN: > + val->intval = info->temp_ambient_alert_min; > + return 0; > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX: > + val->intval = info->temp_ambient_alert_max; > + return 0; > + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN: > + val->intval = info->temp_alert_min; > + return 0; > + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: > + val->intval = info->temp_alert_max; > + return 0; > + case POWER_SUPPLY_PROP_TEMP_MIN: > + val->intval = info->temp_min; > + return 0; > + case POWER_SUPPLY_PROP_TEMP_MAX: > + val->intval = info->temp_max; > + return 0; > + default: > + return -EINVAL; > + } > +} > +EXPORT_SYMBOL_GPL(power_supply_battery_info_get_prop); > + > /** > * power_supply_temp2resist_simple() - find the battery internal resistance > * percent from temperature > @@ -1046,6 +1173,22 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info, > } > EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range); > > +static bool psy_has_property(const struct power_supply_desc *psy_desc, > + enum power_supply_property psp) > +{ > + bool found = false; > + int i; > + > + for (i = 0; i < psy_desc->num_properties; i++) { > + if (psy_desc->properties[i] == psp) { > + found = true; > + break; > + } > + } > + > + return found; > +} > + > int power_supply_get_property(struct power_supply *psy, > enum power_supply_property psp, > union power_supply_propval *val) > @@ -1056,7 +1199,12 @@ int power_supply_get_property(struct power_supply *psy, > return -ENODEV; > } > > - return psy->desc->get_property(psy, psp, val); > + if (psy_has_property(psy->desc, psp)) > + return psy->desc->get_property(psy, psp, val); > + else if (power_supply_battery_info_has_prop(psy->battery_info, psp)) > + return power_supply_battery_info_get_prop(psy->battery_info, psp, val); > + else > + return -EINVAL; > } > EXPORT_SYMBOL_GPL(power_supply_get_property); > > @@ -1117,22 +1265,6 @@ void power_supply_unreg_notifier(struct notifier_block *nb) > } > EXPORT_SYMBOL_GPL(power_supply_unreg_notifier); > > -static bool psy_has_property(const struct power_supply_desc *psy_desc, > - enum power_supply_property psp) > -{ > - bool found = false; > - int i; > - > - for (i = 0; i < psy_desc->num_properties; i++) { > - if (psy_desc->properties[i] == psp) { > - found = true; > - break; > - } > - } > - > - return found; > -} > - I do really like everything in this patch up to this point :) Core providing properties to the user based on the battery-info seems great to me. > #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 */ > + 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. 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. 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. 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 :) > 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. I didn't cascade deep into the code so I can't say if it is a problem to add duplicates? So, if this is safe, and if what I wrote above is not something you want to consider: Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > + ret = add_prop_uevent(dev, env, battery_props[j], > + prop_buf); > + if (ret) > + goto out; > + } > + > out: > free_page((unsigned long)prop_buf); > > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index aa2c4a7c4826..a427f13c757f 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -301,6 +301,7 @@ struct power_supply { > bool initialized; > bool removing; > atomic_t use_cnt; > + struct power_supply_battery_info *battery_info; > #ifdef CONFIG_THERMAL > struct thermal_zone_device *tzd; > struct thermal_cooling_device *tcd; > @@ -791,10 +792,17 @@ devm_power_supply_get_by_phandle(struct device *dev, const char *property) > { return NULL; } > #endif /* CONFIG_OF */ > > +extern const enum power_supply_property power_supply_battery_info_properties[]; > +extern const size_t power_supply_battery_info_properties_size; > extern int power_supply_get_battery_info(struct power_supply *psy, > struct power_supply_battery_info **info_out); > extern void power_supply_put_battery_info(struct power_supply *psy, > struct power_supply_battery_info *info); > +extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info, > + enum power_supply_property psp); > +extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info, > + enum power_supply_property psp, > + union power_supply_propval *val); > extern int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table, > int table_len, int ocv); > extern struct power_supply_battery_ocv_table *
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@gmail.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 [08/12] power: supply: generic-adc-battery: use simple-battery API > [...] Thanks for your reviews, -- Sebastian
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@gmail.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
On 3/16/23 09:10, Matti Vaittinen wrote: > On 3/16/23 02:41, Sebastian Reichel wrote: >> Hi, >> [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. Sorry! I mixed this patch with another one. This indeed did have some changes - I must've accidentally skipped this one. Will check this after eating my breakfast :) > >> >>> [...] >> >> 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 >
Hi, On Thu, Mar 16, 2023 at 09:10:03AM +0200, Matti Vaittinen wrote: > > 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(?) Yes. > 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 ^_^; Nothing will explode. But charger devices are supposed to provide charger information and the data from simple-battery is about the battery, so it should be exposed through a battery typed device. Exposing e.g. POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN for a charger makes no sense. Why would the charger have a design capacity? > [...] > > [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 :) From what I can tell the original author had to debounce the GPIO. -- Sebastian
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c index f3d7c1da299f..842c27de4fac 100644 --- a/drivers/power/supply/power_supply_core.c +++ b/drivers/power/supply/power_supply_core.c @@ -388,7 +388,7 @@ static int __power_supply_get_supplier_property(struct device *dev, void *_data) struct psy_get_supplier_prop_data *data = _data; if (__power_supply_is_supplied_by(epsy, data->psy)) - if (!epsy->desc->get_property(epsy, data->psp, data->val)) + if (!power_supply_get_property(epsy, data->psp, data->val)) return 1; /* Success */ return 0; /* Continue iterating */ @@ -832,6 +832,133 @@ void power_supply_put_battery_info(struct power_supply *psy, } EXPORT_SYMBOL_GPL(power_supply_put_battery_info); +const enum power_supply_property power_supply_battery_info_properties[] = { + POWER_SUPPLY_PROP_TECHNOLOGY, + POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN, + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, + POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN, + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, + POWER_SUPPLY_PROP_PRECHARGE_CURRENT, + POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT, + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX, + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX, + POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN, + POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX, + POWER_SUPPLY_PROP_TEMP_ALERT_MIN, + POWER_SUPPLY_PROP_TEMP_ALERT_MAX, + POWER_SUPPLY_PROP_TEMP_MIN, + POWER_SUPPLY_PROP_TEMP_MAX, +}; +EXPORT_SYMBOL_GPL(power_supply_battery_info_properties); + +const size_t power_supply_battery_info_properties_size = ARRAY_SIZE(power_supply_battery_info_properties); +EXPORT_SYMBOL_GPL(power_supply_battery_info_properties_size); + +bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info, + enum power_supply_property psp) +{ + if (!info) + return false; + + switch (psp) { + case POWER_SUPPLY_PROP_TECHNOLOGY: + return info->technology != POWER_SUPPLY_TECHNOLOGY_UNKNOWN; + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN: + return info->energy_full_design_uwh >= 0; + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: + return info->charge_full_design_uah >= 0; + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: + return info->voltage_min_design_uv >= 0; + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: + return info->voltage_max_design_uv >= 0; + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: + return info->precharge_current_ua >= 0; + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: + return info->charge_term_current_ua >= 0; + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: + return info->constant_charge_current_max_ua >= 0; + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: + return info->constant_charge_voltage_max_uv >= 0; + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN: + return info->temp_ambient_alert_min > INT_MIN; + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX: + return info->temp_ambient_alert_max < INT_MAX; + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN: + return info->temp_alert_min > INT_MIN; + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: + return info->temp_alert_max < INT_MAX; + case POWER_SUPPLY_PROP_TEMP_MIN: + return info->temp_min > INT_MIN; + case POWER_SUPPLY_PROP_TEMP_MAX: + return info->temp_max < INT_MAX; + default: + return false; + } +} +EXPORT_SYMBOL_GPL(power_supply_battery_info_has_prop); + +int power_supply_battery_info_get_prop(struct power_supply_battery_info *info, + enum power_supply_property psp, + union power_supply_propval *val) +{ + if (!info) + return -EINVAL; + + if (!power_supply_battery_info_has_prop(info, psp)) + return -EINVAL; + + switch (psp) { + case POWER_SUPPLY_PROP_TECHNOLOGY: + val->intval = info->technology; + return 0; + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN: + val->intval = info->energy_full_design_uwh; + return 0; + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: + val->intval = info->charge_full_design_uah; + return 0; + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: + val->intval = info->voltage_min_design_uv; + return 0; + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: + val->intval = info->voltage_max_design_uv; + return 0; + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: + val->intval = info->precharge_current_ua; + return 0; + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: + val->intval = info->charge_term_current_ua; + return 0; + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: + val->intval = info->constant_charge_current_max_ua; + return 0; + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: + val->intval = info->constant_charge_voltage_max_uv; + return 0; + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN: + val->intval = info->temp_ambient_alert_min; + return 0; + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX: + val->intval = info->temp_ambient_alert_max; + return 0; + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN: + val->intval = info->temp_alert_min; + return 0; + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: + val->intval = info->temp_alert_max; + return 0; + case POWER_SUPPLY_PROP_TEMP_MIN: + val->intval = info->temp_min; + return 0; + case POWER_SUPPLY_PROP_TEMP_MAX: + val->intval = info->temp_max; + return 0; + default: + return -EINVAL; + } +} +EXPORT_SYMBOL_GPL(power_supply_battery_info_get_prop); + /** * power_supply_temp2resist_simple() - find the battery internal resistance * percent from temperature @@ -1046,6 +1173,22 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info, } EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range); +static bool psy_has_property(const struct power_supply_desc *psy_desc, + enum power_supply_property psp) +{ + bool found = false; + int i; + + for (i = 0; i < psy_desc->num_properties; i++) { + if (psy_desc->properties[i] == psp) { + found = true; + break; + } + } + + return found; +} + int power_supply_get_property(struct power_supply *psy, enum power_supply_property psp, union power_supply_propval *val) @@ -1056,7 +1199,12 @@ int power_supply_get_property(struct power_supply *psy, return -ENODEV; } - return psy->desc->get_property(psy, psp, val); + if (psy_has_property(psy->desc, psp)) + return psy->desc->get_property(psy, psp, val); + else if (power_supply_battery_info_has_prop(psy->battery_info, psp)) + return power_supply_battery_info_get_prop(psy->battery_info, psp, val); + else + return -EINVAL; } EXPORT_SYMBOL_GPL(power_supply_get_property); @@ -1117,22 +1265,6 @@ void power_supply_unreg_notifier(struct notifier_block *nb) } EXPORT_SYMBOL_GPL(power_supply_unreg_notifier); -static bool psy_has_property(const struct power_supply_desc *psy_desc, - enum power_supply_property psp) -{ - bool found = false; - int i; - - for (i = 0; i < psy_desc->num_properties; i++) { - if (psy_desc->properties[i] == psp) { - found = true; - break; - } - } - - return found; -} - #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 */ + rc = power_supply_get_battery_info(psy, &psy->battery_info); + if (rc && rc != -ENODEV) + goto check_supplies_failed; + 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; + ret = add_prop_uevent(dev, env, battery_props[j], + prop_buf); + if (ret) + goto out; + } + out: free_page((unsigned long)prop_buf); diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h index aa2c4a7c4826..a427f13c757f 100644 --- a/include/linux/power_supply.h +++ b/include/linux/power_supply.h @@ -301,6 +301,7 @@ struct power_supply { bool initialized; bool removing; atomic_t use_cnt; + struct power_supply_battery_info *battery_info; #ifdef CONFIG_THERMAL struct thermal_zone_device *tzd; struct thermal_cooling_device *tcd; @@ -791,10 +792,17 @@ devm_power_supply_get_by_phandle(struct device *dev, const char *property) { return NULL; } #endif /* CONFIG_OF */ +extern const enum power_supply_property power_supply_battery_info_properties[]; +extern const size_t power_supply_battery_info_properties_size; extern int power_supply_get_battery_info(struct power_supply *psy, struct power_supply_battery_info **info_out); extern void power_supply_put_battery_info(struct power_supply *psy, struct power_supply_battery_info *info); +extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info, + enum power_supply_property psp); +extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info, + enum power_supply_property psp, + union power_supply_propval *val); extern int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table, int table_len, int ocv); extern struct power_supply_battery_ocv_table *
Add support for automatically exposing data from the simple-battery firmware node with a single configuration option in the power-supply device. Signed-off-by: Sebastian Reichel <sre@kernel.org> --- drivers/power/supply/power_supply_core.c | 173 +++++++++++++++++++--- drivers/power/supply/power_supply_sysfs.c | 15 ++ include/linux/power_supply.h | 8 + 3 files changed, 178 insertions(+), 18 deletions(-)