Messages in this thread | | | Date | Wed, 27 Mar 2024 11:43:12 +0100 | Subject | Re: [PATCH v2 3/4] power: supply: core: fix charge_behaviour formatting | From | Hans de Goede <> |
| |
Hi Thomas,
On 3/3/24 4:31 PM, Thomas Weißschuh wrote: > This property is documented to have a special format which exposes all > available behaviours and the currently active one at the same time. > For this special format some helpers are provided. > > However the default property logic in power_supply_sysfs.c is not using > the helper and the default logic only prints the currently active > behaviour. > > Adjust power_supply_sysfs.c to follow the documented format. > > There are currently two in-tree drivers exposing charge behaviours: > thinkpad_acpi and mm8013. > thinkpad_acpi is not affected by the change, as it directly uses the > helpers and does not use the power_supply_sysfs.c logic. > > As mm8013 does not set implement desc->charge_behaviours. > the new logic will preserve the simple output format in this case.
Since patch 1/3 now drops the charge behaviours from mm8013 I believe these 2 paragraphs should be replaced with:
""" thinkpad_acpi. is the only in-tree drivers currently exposing charge behaviours. thinkpad_acpi is not affected by the change, as it directly uses the helpers and does not use the power_supply_sysfs.c logic. """
> Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes") > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++++++ > include/linux/power_supply.h | 1 + > 2 files changed, 21 insertions(+) > > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c > index 10fec411794b..a20aa0156b0a 100644 > --- a/drivers/power/supply/power_supply_sysfs.c > +++ b/drivers/power/supply/power_supply_sysfs.c > @@ -271,6 +271,23 @@ static ssize_t power_supply_show_usb_type(struct device *dev, > return count; > } > > +static ssize_t power_supply_show_charge_behaviour(struct device *dev, > + struct power_supply *psy, > + union power_supply_propval *value, > + char *buf) > +{ > + int ret; > + > + ret = power_supply_get_property(psy, > + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, > + value); > + if (ret < 0) > + return ret;
power_supply_show_property() has already called power_supply_get_property() before calling this, so this call can be dropped. At which point power_supply_show_charge_behaviour() becomes just a wrapper around power_supply_charge_behaviour_show() and thus can be dropped itself.
And then ... (continued below).
> + > + return power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours, > + value->intval, buf); > +} > + > static ssize_t power_supply_show_property(struct device *dev, > struct device_attribute *attr, > char *buf) { > @@ -303,6 +320,9 @@ static ssize_t power_supply_show_property(struct device *dev, > ret = power_supply_show_usb_type(dev, psy->desc, > &value, buf); > break; > + case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: > + ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
replace this line with:
ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours, value.intval, buf);
Making this patch a nice simple patch :)
Regards,
Hans
> + break; > case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER: > ret = sysfs_emit(buf, "%s\n", value.strval); > break; > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index c0992a77feea..a50ee69503bf 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -242,6 +242,7 @@ struct power_supply_config { > struct power_supply_desc { > const char *name; > enum power_supply_type type; > + u8 charge_behaviours; > const enum power_supply_usb_type *usb_types; > size_t num_usb_types; > const enum power_supply_property *properties; >
| |