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@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?
>
> 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.
--
i.
