| Message ID | 20250407201845.332348-2-jvanderw@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | support charge_types in the extensions API | expand |
Hi Jelle, Thank you for your patch. On 7-Apr-25 10:18 PM, Jelle van der Waa wrote: > From: Jelle van der Waa <jvanderwaa@redhat.com> > > Similar to charge_behaviour, charge_types is an enum option with > multiple possible values. To be able to use it with a power_supply > extension its known values have to be exposed. "multiple possible values" sounds like the actual setting is a bitmask where multiple options can be enabled at once. And "its known values" sounds like this is about the currently active value(s). Instead I suggest a text like this: """ Similar to charge_behaviour, charge_types is an enum option where reading the property shows the supported values, with the active value surrounded by brackets. To be able to use it with a power_supply extension a bitmask with the supported charge_Types values has to be added to power_supply_ext. """ Feel free to re-use this verbatim, or just the bits you like. > Signed-off-by: Jelle van der Waa <jvanderwaa@redhat.com> > --- > drivers/power/supply/power_supply_sysfs.c | 23 ++++++++++++++++++++++- > drivers/power/supply/test_power.c | 20 ++++++++++++++++++-- > include/linux/power_supply.h | 1 + > 3 files changed, 41 insertions(+), 3 deletions(-) > > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c > index edb058c19c9c..6d80640511b5 100644 > --- a/drivers/power/supply/power_supply_sysfs.c > +++ b/drivers/power/supply/power_supply_sysfs.c > @@ -321,6 +321,27 @@ static ssize_t power_supply_show_charge_behaviour(struct device *dev, > value->intval, buf); > } > > +static ssize_t power_supply_show_charge_types(struct device *dev, > + struct power_supply *psy, > + enum power_supply_charge_type current_type, > + char *buf) > +{ > + struct power_supply_ext_registration *reg; > + > + scoped_guard(rwsem_read, &psy->extensions_sem) { > + power_supply_for_each_extension(reg, psy) { > + if (power_supply_ext_has_property(reg->ext, > + POWER_SUPPLY_PROP_CHARGE_TYPES)) > + return power_supply_charge_types_show(dev, > + reg->ext->charge_types, > + current_type, buf); > + } > + } > + > + return power_supply_charge_types_show(dev, psy->desc->charge_types, > + current_type, buf); > +} > + Once the last external user of power_supply_charge_types_show() (dell-laptop) is ported to become a power-supply extension, we should stop exporting power_supply_charge_types_show(). The question then becomes do we just stop exporting it; or do we remove it entirely ? My vote would go to remove it entirely. This could be done if the above function were to directly call power_supply_show_enum_with_available() instead of using the helper, e.g.: static ssize_t power_supply_show_charge_types(struct device *dev, struct power_supply *psy, enum power_supply_charge_type current_type, char *buf) { struct power_supply_ext_registration *reg; scoped_guard(rwsem_read, &psy->extensions_sem) { power_supply_for_each_extension(reg, psy) { if (power_supply_ext_has_property(reg->ext, POWER_SUPPLY_PROP_CHARGE_TYPES)) return power_supply_show_enum_with_available(dev, POWER_SUPPLY_CHARGE_TYPE_TEXT, ARRAY_SIZE(POWER_SUPPLY_CHARGE_TYPE_TEXT), reg->ext->charge_types, current_type, buf); } } return power_supply_show_enum_with_available(dev, POWER_SUPPLY_CHARGE_TYPE_TEXT, ARRAY_SIZE(POWER_SUPPLY_CHARGE_TYPE_TEXT), psy->desc->charge_types, current_type, buf); } Since we already know we want to eventually remove power_supply_charge_types_show() I think it would be better to write this as above right away. Sebastian, Thomas any opinions on this ? > static ssize_t power_supply_format_property(struct device *dev, > bool uevent, > struct device_attribute *attr, > @@ -365,7 +386,7 @@ static ssize_t power_supply_format_property(struct device *dev, > case POWER_SUPPLY_PROP_CHARGE_TYPES: > if (uevent) /* no possible values in uevents */ > goto default_format; > - ret = power_supply_charge_types_show(dev, psy->desc->charge_types, > + ret = power_supply_show_charge_types(dev, psy, > value.intval, buf); > break; > case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER: > diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c > index 2a975a110f48..0d9cc0c5613e 100644 > --- a/drivers/power/supply/test_power.c > +++ b/drivers/power/supply/test_power.c > @@ -37,6 +37,8 @@ static int battery_charge_counter = -1000; > static int battery_current = -1600; > static enum power_supply_charge_behaviour battery_charge_behaviour = > POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO; > +static enum power_supply_charge_type battery_charge_types = > + POWER_SUPPLY_CHARGE_TYPE_NONE; > static bool battery_extension; > > static bool module_initialized; > @@ -87,7 +89,7 @@ static int test_power_get_battery_property(struct power_supply *psy, > val->intval = battery_status; > break; > case POWER_SUPPLY_PROP_CHARGE_TYPE: > - val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST; > + val->intval = battery_charge_types; > break; > case POWER_SUPPLY_PROP_HEALTH: > val->intval = battery_health; > @@ -129,6 +131,9 @@ static int test_power_get_battery_property(struct power_supply *psy, > case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: > val->intval = battery_charge_behaviour; > break; > + case POWER_SUPPLY_PROP_CHARGE_TYPES: > + val->intval = battery_charge_types; > + break; > default: > pr_info("%s: some properties deliberately report errors.\n", > __func__); > @@ -140,7 +145,7 @@ static int test_power_get_battery_property(struct power_supply *psy, > static int test_power_battery_property_is_writeable(struct power_supply *psy, > enum power_supply_property psp) > { > - return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR; > + return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR | POWER_SUPPLY_PROP_CHARGE_TYPES; > } > > static int test_power_set_battery_property(struct power_supply *psy, > @@ -156,6 +161,14 @@ static int test_power_set_battery_property(struct power_supply *psy, > } > battery_charge_behaviour = val->intval; > break; > + case POWER_SUPPLY_PROP_CHARGE_TYPES: > + if (val->intval < 0 || > + val->intval >= BITS_PER_TYPE(typeof(psy->desc->charge_types)) || > + !(BIT(val->intval) & psy->desc->charge_types)) { > + return -EINVAL; > + } > + battery_charge_types = val->intval; > + break; > default: > return -EINVAL; > } > @@ -188,6 +201,7 @@ static enum power_supply_property test_power_battery_props[] = { > POWER_SUPPLY_PROP_CURRENT_AVG, > POWER_SUPPLY_PROP_CURRENT_NOW, > POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, > + POWER_SUPPLY_PROP_CHARGE_TYPES, > }; > > static char *test_power_ac_supplied_to[] = { > @@ -215,6 +229,8 @@ static const struct power_supply_desc test_power_desc[] = { > .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) > | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE) > | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE), > + .charge_types = BIT(POWER_SUPPLY_CHARGE_TYPE_NONE) > + | BIT(POWER_SUPPLY_CHARGE_TYPE_LONGLIFE) > }, > [TEST_USB] = { > .name = "test_usb", > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index 888824592953..c4cb854971f5 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -288,6 +288,7 @@ struct power_supply_desc { > struct power_supply_ext { > const char *const name; > u8 charge_behaviours; > + u32 charge_types; > const enum power_supply_property *properties; > size_t num_properties; > Otherwise this patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans
Hi Jelle, kernel test robot noticed the following build warnings: [auto build test WARNING on sre-power-supply/for-next] [also build test WARNING on linus/master v6.15-rc1 next-20250408] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Jelle-van-der-Waa/power-supply-support-charge_types-in-extensions/20250408-045501 base: https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next patch link: https://lore.kernel.org/r/20250407201845.332348-2-jvanderw%40redhat.com patch subject: [PATCH 1/1] power: supply: support charge_types in extensions config: i386-buildonly-randconfig-004-20250409 (https://download.01.org/0day-ci/archive/20250409/202504090646.w9GLxe17-lkp@intel.com/config) compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250409/202504090646.w9GLxe17-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202504090646.w9GLxe17-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/power/supply/test_power.c:148:51: warning: | has lower precedence than ==; == will be evaluated first [-Wparentheses] 148 | return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR | POWER_SUPPLY_PROP_CHARGE_TYPES; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ drivers/power/supply/test_power.c:148:51: note: place parentheses around the '==' expression to silence this warning 148 | return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR | POWER_SUPPLY_PROP_CHARGE_TYPES; | ^ | ( ) drivers/power/supply/test_power.c:148:51: note: place parentheses around the | expression to evaluate it first 148 | return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR | POWER_SUPPLY_PROP_CHARGE_TYPES; | ^ | ( ) 1 warning generated. vim +148 drivers/power/supply/test_power.c 144 145 static int test_power_battery_property_is_writeable(struct power_supply *psy, 146 enum power_supply_property psp) 147 { > 148 return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR | POWER_SUPPLY_PROP_CHARGE_TYPES; 149 } 150
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c index edb058c19c9c..6d80640511b5 100644 --- a/drivers/power/supply/power_supply_sysfs.c +++ b/drivers/power/supply/power_supply_sysfs.c @@ -321,6 +321,27 @@ static ssize_t power_supply_show_charge_behaviour(struct device *dev, value->intval, buf); } +static ssize_t power_supply_show_charge_types(struct device *dev, + struct power_supply *psy, + enum power_supply_charge_type current_type, + char *buf) +{ + struct power_supply_ext_registration *reg; + + scoped_guard(rwsem_read, &psy->extensions_sem) { + power_supply_for_each_extension(reg, psy) { + if (power_supply_ext_has_property(reg->ext, + POWER_SUPPLY_PROP_CHARGE_TYPES)) + return power_supply_charge_types_show(dev, + reg->ext->charge_types, + current_type, buf); + } + } + + return power_supply_charge_types_show(dev, psy->desc->charge_types, + current_type, buf); +} + static ssize_t power_supply_format_property(struct device *dev, bool uevent, struct device_attribute *attr, @@ -365,7 +386,7 @@ static ssize_t power_supply_format_property(struct device *dev, case POWER_SUPPLY_PROP_CHARGE_TYPES: if (uevent) /* no possible values in uevents */ goto default_format; - ret = power_supply_charge_types_show(dev, psy->desc->charge_types, + ret = power_supply_show_charge_types(dev, psy, value.intval, buf); break; case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER: diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c index 2a975a110f48..0d9cc0c5613e 100644 --- a/drivers/power/supply/test_power.c +++ b/drivers/power/supply/test_power.c @@ -37,6 +37,8 @@ static int battery_charge_counter = -1000; static int battery_current = -1600; static enum power_supply_charge_behaviour battery_charge_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO; +static enum power_supply_charge_type battery_charge_types = + POWER_SUPPLY_CHARGE_TYPE_NONE; static bool battery_extension; static bool module_initialized; @@ -87,7 +89,7 @@ static int test_power_get_battery_property(struct power_supply *psy, val->intval = battery_status; break; case POWER_SUPPLY_PROP_CHARGE_TYPE: - val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST; + val->intval = battery_charge_types; break; case POWER_SUPPLY_PROP_HEALTH: val->intval = battery_health; @@ -129,6 +131,9 @@ static int test_power_get_battery_property(struct power_supply *psy, case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: val->intval = battery_charge_behaviour; break; + case POWER_SUPPLY_PROP_CHARGE_TYPES: + val->intval = battery_charge_types; + break; default: pr_info("%s: some properties deliberately report errors.\n", __func__); @@ -140,7 +145,7 @@ static int test_power_get_battery_property(struct power_supply *psy, static int test_power_battery_property_is_writeable(struct power_supply *psy, enum power_supply_property psp) { - return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR; + return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR | POWER_SUPPLY_PROP_CHARGE_TYPES; } static int test_power_set_battery_property(struct power_supply *psy, @@ -156,6 +161,14 @@ static int test_power_set_battery_property(struct power_supply *psy, } battery_charge_behaviour = val->intval; break; + case POWER_SUPPLY_PROP_CHARGE_TYPES: + if (val->intval < 0 || + val->intval >= BITS_PER_TYPE(typeof(psy->desc->charge_types)) || + !(BIT(val->intval) & psy->desc->charge_types)) { + return -EINVAL; + } + battery_charge_types = val->intval; + break; default: return -EINVAL; } @@ -188,6 +201,7 @@ static enum power_supply_property test_power_battery_props[] = { POWER_SUPPLY_PROP_CURRENT_AVG, POWER_SUPPLY_PROP_CURRENT_NOW, POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, + POWER_SUPPLY_PROP_CHARGE_TYPES, }; static char *test_power_ac_supplied_to[] = { @@ -215,6 +229,8 @@ static const struct power_supply_desc test_power_desc[] = { .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE) | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE), + .charge_types = BIT(POWER_SUPPLY_CHARGE_TYPE_NONE) + | BIT(POWER_SUPPLY_CHARGE_TYPE_LONGLIFE) }, [TEST_USB] = { .name = "test_usb", diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h index 888824592953..c4cb854971f5 100644 --- a/include/linux/power_supply.h +++ b/include/linux/power_supply.h @@ -288,6 +288,7 @@ struct power_supply_desc { struct power_supply_ext { const char *const name; u8 charge_behaviours; + u32 charge_types; const enum power_supply_property *properties; size_t num_properties;