| Message ID | 20240303-power_supply-charge_behaviour_prop-v2-3-8ebb0a7c2409@weissschuh.net |
|---|---|
| State | Accepted |
| Commit | 4e61f1e9d58fb0765f59f47d4d1f318b36c14d95 |
| Headers | show |
| Series | power: supply: core: align charge_behaviour format with docs | expand |
Hi, On Sun, Mar 03, 2024 at 04:31:15PM +0100, 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. > > Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes") > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- Thanks, I also queued this one, but reworked the commit message and removed the Fixes tag considering we have only thinkpad_acpi using this feature in-tree. -- Sebastian > 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; > + > + 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); > + 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; > > -- > 2.44.0 > >
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; >
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; + + 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); + 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;
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. 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(+)