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@xxxxxxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> 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).
--
i.
>
> 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;
>
