| Message ID | 20241211174451.355421-5-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | power: supply: Add new "charge_types" property | expand |
On Wed, 11 Dec 2024, Hans de Goede wrote: > Make battery_modes a map between tokens and enum power_supply_charge_type > values instead of between tokens and strings and use the new > power_supply_charge_types_show/_parse() helpers for show()/store() > to ensure that things are handled in the same way as in other drivers. > > This also changes battery_supported_modes to be a bitmap of charge-types > (enum power_supply_charge_type values) rather then a bitmap of indices > into battery_modes[]. > > Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/dell/dell-laptop.c | 54 ++++++++++++------------- > 1 file changed, 25 insertions(+), 29 deletions(-) > > diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c > index 5671bd0deee7..9a4cfcb8bbe0 100644 > --- a/drivers/platform/x86/dell/dell-laptop.c > +++ b/drivers/platform/x86/dell/dell-laptop.c > @@ -103,15 +103,15 @@ static bool mute_led_registered; > > struct battery_mode_info { > int token; > - const char *label; > + enum power_supply_charge_type charge_type; > }; > > static const struct battery_mode_info battery_modes[] = { > - { BAT_PRI_AC_MODE_TOKEN, "Trickle" }, > - { BAT_EXPRESS_MODE_TOKEN, "Fast" }, > - { BAT_STANDARD_MODE_TOKEN, "Standard" }, > - { BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" }, > - { BAT_CUSTOM_MODE_TOKEN, "Custom" }, > + { BAT_PRI_AC_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_TRICKLE }, > + { BAT_EXPRESS_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_FAST }, > + { BAT_STANDARD_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_STANDARD }, > + { BAT_ADAPTIVE_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE }, > + { BAT_CUSTOM_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_CUSTOM }, > }; > static u32 battery_supported_modes; > > @@ -2261,46 +2261,42 @@ static ssize_t charge_types_show(struct device *dev, > struct device_attribute *attr, > char *buf) > { > - ssize_t count = 0; > + enum power_supply_charge_type charge_type; > int i; > > for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { > - bool active; > + charge_type = battery_modes[i].charge_type; > > - if (!(battery_supported_modes & BIT(i))) > + if (!(battery_supported_modes & BIT(charge_type))) > continue; > > - active = dell_battery_mode_is_active(battery_modes[i].token); > - count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ", > - battery_modes[i].label); > + if (!dell_battery_mode_is_active(battery_modes[i].token)) > + continue; > + > + return power_supply_charge_types_show(dev, battery_supported_modes, > + charge_type, buf); > } > > - /* convert the last space to a newline */ > - if (count > 0) > - count--; > - count += sysfs_emit_at(buf, count, "\n"); > - > - return count; > + /* No active mode found */ > + return -EIO; > } > > static ssize_t charge_types_store(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t size) > { > - bool matched = false; > - int err, i; > + int charge_type, err, i; > + > + charge_type = power_supply_charge_types_parse(battery_supported_modes, buf); > + if (charge_type < 0) > + return charge_type; > > for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { > - if (!(battery_supported_modes & BIT(i))) > - continue; > - > - if (sysfs_streq(battery_modes[i].label, buf)) { > - matched = true; > + if (battery_modes[i].charge_type == charge_type) > break; > - } > } > - if (!matched) > - return -EINVAL; > + if (i == ARRAY_SIZE(battery_modes)) > + return -EIO; Hi Hans, Is this errno change helpful/correct? There is zero I/O done before reaching this point, just input validation, so why does it return errno that is "I/O error"? If you want to differentiate from -EINVAL, I suggest using -ENOENT (but I personally think -EINVAL would be fine as well because it's still an invalid argument even if it passed one stage of the input checks).
Hi, On 17-Dec-24 1:01 PM, Ilpo Järvinen wrote: > On Wed, 11 Dec 2024, Hans de Goede wrote: > >> Make battery_modes a map between tokens and enum power_supply_charge_type >> values instead of between tokens and strings and use the new >> power_supply_charge_types_show/_parse() helpers for show()/store() >> to ensure that things are handled in the same way as in other drivers. >> >> This also changes battery_supported_modes to be a bitmap of charge-types >> (enum power_supply_charge_type values) rather then a bitmap of indices >> into battery_modes[]. >> >> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/platform/x86/dell/dell-laptop.c | 54 ++++++++++++------------- >> 1 file changed, 25 insertions(+), 29 deletions(-) >> >> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c >> index 5671bd0deee7..9a4cfcb8bbe0 100644 >> --- a/drivers/platform/x86/dell/dell-laptop.c >> +++ b/drivers/platform/x86/dell/dell-laptop.c >> @@ -103,15 +103,15 @@ static bool mute_led_registered; >> >> struct battery_mode_info { >> int token; >> - const char *label; >> + enum power_supply_charge_type charge_type; >> }; >> >> static const struct battery_mode_info battery_modes[] = { >> - { BAT_PRI_AC_MODE_TOKEN, "Trickle" }, >> - { BAT_EXPRESS_MODE_TOKEN, "Fast" }, >> - { BAT_STANDARD_MODE_TOKEN, "Standard" }, >> - { BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" }, >> - { BAT_CUSTOM_MODE_TOKEN, "Custom" }, >> + { BAT_PRI_AC_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_TRICKLE }, >> + { BAT_EXPRESS_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_FAST }, >> + { BAT_STANDARD_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_STANDARD }, >> + { BAT_ADAPTIVE_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE }, >> + { BAT_CUSTOM_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_CUSTOM }, >> }; >> static u32 battery_supported_modes; >> >> @@ -2261,46 +2261,42 @@ static ssize_t charge_types_show(struct device *dev, >> struct device_attribute *attr, >> char *buf) >> { >> - ssize_t count = 0; >> + enum power_supply_charge_type charge_type; >> int i; >> >> for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { >> - bool active; >> + charge_type = battery_modes[i].charge_type; >> >> - if (!(battery_supported_modes & BIT(i))) >> + if (!(battery_supported_modes & BIT(charge_type))) >> continue; >> >> - active = dell_battery_mode_is_active(battery_modes[i].token); >> - count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ", >> - battery_modes[i].label); >> + if (!dell_battery_mode_is_active(battery_modes[i].token)) >> + continue; >> + >> + return power_supply_charge_types_show(dev, battery_supported_modes, >> + charge_type, buf); >> } >> >> - /* convert the last space to a newline */ >> - if (count > 0) >> - count--; >> - count += sysfs_emit_at(buf, count, "\n"); >> - >> - return count; >> + /* No active mode found */ >> + return -EIO; >> } >> >> static ssize_t charge_types_store(struct device *dev, >> struct device_attribute *attr, >> const char *buf, size_t size) >> { >> - bool matched = false; >> - int err, i; >> + int charge_type, err, i; >> + >> + charge_type = power_supply_charge_types_parse(battery_supported_modes, buf); >> + if (charge_type < 0) >> + return charge_type; >> >> for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { >> - if (!(battery_supported_modes & BIT(i))) >> - continue; >> - >> - if (sysfs_streq(battery_modes[i].label, buf)) { >> - matched = true; >> + if (battery_modes[i].charge_type == charge_type) >> break; >> - } >> } >> - if (!matched) >> - return -EINVAL; >> + if (i == ARRAY_SIZE(battery_modes)) >> + return -EIO; > > Hi Hans, > > Is this errno change helpful/correct? power_supply_charge_types_parse() already checks that the user-input is one of the values advertised in the passed in battery_supported_modes, so when we loop to translate the enum power_supply_charge_type value returned by power_supply_charge_types_parse() then we should find a matching entry in battery_modes[] if not something is wrong at the driver level. So not -EINVAL, since this is a driver issue not a user input issue. > There is zero I/O done before > reaching this point, just input validation, so why does it return errno > that is "I/O error"? If you want to differentiate from -EINVAL, I suggest > using -ENOENT (but I personally think -EINVAL would be fine as well > because it's still an invalid argument even if it passed one stage of > the input checks). -ENOENT instead of -EIO works for me. Shall I send out a new version with that changed? Note that merging this requires the earlier patches from this series which have been merged into: https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next so this either requires an immutable tag from Sebastian for you to merge, or this should be merged through Sebastian's tree. Regards, Hans
Hi Hans, On 2024-12-17 16:18:47+0100, Hans de Goede wrote: > On 17-Dec-24 1:01 PM, Ilpo Järvinen wrote: > > On Wed, 11 Dec 2024, Hans de Goede wrote: > > > >> Make battery_modes a map between tokens and enum power_supply_charge_type > >> values instead of between tokens and strings and use the new > >> power_supply_charge_types_show/_parse() helpers for show()/store() > >> to ensure that things are handled in the same way as in other drivers. > >> > >> This also changes battery_supported_modes to be a bitmap of charge-types > >> (enum power_supply_charge_type values) rather then a bitmap of indices > >> into battery_modes[]. > >> > >> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> [..] > Note that merging this requires the earlier patches from this > series which have been merged into: > > https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next > > so this either requires an immutable tag from Sebastian for you to merge, > or this should be merged through Sebastian's tree. If this goes in via the psy tree, you could already make it a power supply extension. The necessary code is in psy/for-next. Not necessary obviously. Thomas
Hi, On Tue, Dec 17, 2024 at 04:18:47PM +0100, Hans de Goede wrote: > Note that merging this requires the earlier patches from this > series which have been merged into: > > https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next > > so this either requires an immutable tag from Sebastian for you to merge, > or this should be merged through Sebastian's tree. Unfortunately I cannot easily create an immutable tag, which does not pull in quite a bit of other clutter from my for-next branch (or require a big rebase of everything in my for-next branch, which I usually try to avoid). I noticed too late that it would have been a good idea to merge all of this through a topic branch. Greetings, -- Sebastian
Hi, On 17-Dec-24 7:49 PM, Thomas Weißschuh wrote: > Hi Hans, > > On 2024-12-17 16:18:47+0100, Hans de Goede wrote: >> On 17-Dec-24 1:01 PM, Ilpo Järvinen wrote: >>> On Wed, 11 Dec 2024, Hans de Goede wrote: >>> >>>> Make battery_modes a map between tokens and enum power_supply_charge_type >>>> values instead of between tokens and strings and use the new >>>> power_supply_charge_types_show/_parse() helpers for show()/store() >>>> to ensure that things are handled in the same way as in other drivers. >>>> >>>> This also changes battery_supported_modes to be a bitmap of charge-types >>>> (enum power_supply_charge_type values) rather then a bitmap of indices >>>> into battery_modes[]. >>>> >>>> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > [..] > >> Note that merging this requires the earlier patches from this >> series which have been merged into: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next >> >> so this either requires an immutable tag from Sebastian for you to merge, >> or this should be merged through Sebastian's tree. > > If this goes in via the psy tree, you could already make it a power > supply extension. The necessary code is in psy/for-next. Yes I noticed that the power-supply extension support was just merged, that is great. Thank you for your work on that! > Not necessary obviously. Right I'm afraid I don't have time to work on converting this to a power-supply extension atm, so lets go with this incremental improvement for now. Regards, Hans p.s. IK do have hw to test this on, so if someone else were to do a conversion to the new power-supply extension model I would be happy to test.
diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c index 5671bd0deee7..9a4cfcb8bbe0 100644 --- a/drivers/platform/x86/dell/dell-laptop.c +++ b/drivers/platform/x86/dell/dell-laptop.c @@ -103,15 +103,15 @@ static bool mute_led_registered; struct battery_mode_info { int token; - const char *label; + enum power_supply_charge_type charge_type; }; static const struct battery_mode_info battery_modes[] = { - { BAT_PRI_AC_MODE_TOKEN, "Trickle" }, - { BAT_EXPRESS_MODE_TOKEN, "Fast" }, - { BAT_STANDARD_MODE_TOKEN, "Standard" }, - { BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" }, - { BAT_CUSTOM_MODE_TOKEN, "Custom" }, + { BAT_PRI_AC_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_TRICKLE }, + { BAT_EXPRESS_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_FAST }, + { BAT_STANDARD_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_STANDARD }, + { BAT_ADAPTIVE_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE }, + { BAT_CUSTOM_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_CUSTOM }, }; static u32 battery_supported_modes; @@ -2261,46 +2261,42 @@ static ssize_t charge_types_show(struct device *dev, struct device_attribute *attr, char *buf) { - ssize_t count = 0; + enum power_supply_charge_type charge_type; int i; for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { - bool active; + charge_type = battery_modes[i].charge_type; - if (!(battery_supported_modes & BIT(i))) + if (!(battery_supported_modes & BIT(charge_type))) continue; - active = dell_battery_mode_is_active(battery_modes[i].token); - count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ", - battery_modes[i].label); + if (!dell_battery_mode_is_active(battery_modes[i].token)) + continue; + + return power_supply_charge_types_show(dev, battery_supported_modes, + charge_type, buf); } - /* convert the last space to a newline */ - if (count > 0) - count--; - count += sysfs_emit_at(buf, count, "\n"); - - return count; + /* No active mode found */ + return -EIO; } static ssize_t charge_types_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t size) { - bool matched = false; - int err, i; + int charge_type, err, i; + + charge_type = power_supply_charge_types_parse(battery_supported_modes, buf); + if (charge_type < 0) + return charge_type; for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { - if (!(battery_supported_modes & BIT(i))) - continue; - - if (sysfs_streq(battery_modes[i].label, buf)) { - matched = true; + if (battery_modes[i].charge_type == charge_type) break; - } } - if (!matched) - return -EINVAL; + if (i == ARRAY_SIZE(battery_modes)) + return -EIO; err = dell_battery_set_mode(battery_modes[i].token); if (err) @@ -2430,7 +2426,7 @@ static u32 __init battery_get_supported_modes(void) for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { if (dell_smbios_find_token(battery_modes[i].token)) - modes |= BIT(i); + modes |= BIT(battery_modes[i].charge_type); } return modes;