| Message ID | 20241211174451.355421-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| 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
On Tue, 17 Dec 2024, 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> > >> --- > >> 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. Yes, please send a new version with -ENOENT as I was going to ask Sebastian to take this patch but noticed this small errno thing while reading the patch one more time before acking it.
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;