| Message ID | 20240720012220.26d62a54@5400 |
|---|---|
| State | New |
| Headers | show |
| Series | platform/x86:dell-laptop: Add knobs to change battery charge settings | expand |
Hello, I looked at your patch. I wrote some comments below. The main issue is how to correctly interpret read token values. Anyway, Dell in past published documentation and Python GPL code for doing something with batteries. I'm not sure if it is different API or something unrelated, but maybe it could be useful for your research: https://github.com/dell/libsmbios/blob/master/src/bin/smbios-battery-ctl On Saturday 20 July 2024 01:22:20 Andres Salomon wrote: > The Dell BIOS allows you to set custom charging modes, which is useful > in particular for extending battery life. This adds support for tweaking > those various settings from Linux via sysfs knobs. One might, for > example, have their laptop plugged into power at their desk the vast > majority of the time and choose fairly aggressive battery-saving > settings (only charging once the battery drops to 50% and only charging > up to 80%). When leaving for a trip, they might want to switch those > settings to charge to 100% and charge any time power is available; > rebooting into the BIOS to change those settings is a hassle. > > For the Custom charging type mode, we reuse the common > charge_control_{start,end}_threshold sysfs power_supply entries. The > BIOS also has a bunch of other charging modes with varying levels of > usefulness, so this adds a new Dell-specific sysfs entry called > 'charge_type' that's documented in sysfs-class-power-dell (and looks a > lot like sysfs-class-power-wilco). > > This work is based on a patch by Perry Yuan <perry_yuan@dell.com> and > Limonciello Mario <Mario_Limonciello@Dell.com> submitted back in 2020: > https://lore.kernel.org/all/20200729065424.12851-1-Perry_Yuan@Dell.com/ > Both of their email addresses bounce, so I'm assuming they're no longer > with the company. I've reworked most of the patch to make it smaller and > cleaner. Mario is in AMD, I'm adding his address to the loop, so in case is interesting can look at this. > Signed-off-by: Andres Salomon <dilinger@queued.net> > --- > .../ABI/testing/sysfs-class-power-dell | 31 +++ > drivers/platform/x86/dell/Kconfig | 1 + > drivers/platform/x86/dell/dell-laptop.c | 263 ++++++++++++++++++ > drivers/platform/x86/dell/dell-smbios.h | 17 ++ > 4 files changed, 312 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-power-dell > > diff --git a/Documentation/ABI/testing/sysfs-class-power-dell b/Documentation/ABI/testing/sysfs-class-power-dell > new file mode 100644 > index 000000000000..ef72c5f797ce > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-power-dell > @@ -0,0 +1,31 @@ > +What: /sys/class/power_supply/<supply_name>/charge_type > +Date: July 2024 > +KernelVersion: 6.11 > +Contact: linux-pm@vger.kernel.org > +Description: > + Select the charging algorithm to use for the (primary) > + battery. > + > + Standard: > + Fully charge the battery at a moderate rate. > + ExpressCharge™: > + Quickly charge the battery using fast-charge > + technology. This is harder on the battery than > + standard charging and may lower its lifespan. > + Primarily AC Use: > + Users who primarily operate the system while > + plugged into an external power source can extend > + battery life with this mode. > + Adaptive: > + Automatically optimize battery charge rate based > + on typical usage. > + Custom: > + Use the charge_control_* properties to determine > + when to start and stop charging. Advanced users > + can use this to drastically extend battery life. > + > + Access: Read, Write > + Valid values: > + "standard", "express", "primarily_ac", > + "adaptive", "custom" > + > diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig > index 85a78ef91182..02405793163c 100644 > --- a/drivers/platform/x86/dell/Kconfig > +++ b/drivers/platform/x86/dell/Kconfig > @@ -49,6 +49,7 @@ config DELL_LAPTOP > default m > depends on DMI > depends on BACKLIGHT_CLASS_DEVICE > + depends on ACPI_BATTERY > depends on ACPI_VIDEO || ACPI_VIDEO = n > depends on RFKILL || RFKILL = n > depends on DELL_WMI || DELL_WMI = n > diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c > index 6552dfe491c6..54f47b685a46 100644 > --- a/drivers/platform/x86/dell/dell-laptop.c > +++ b/drivers/platform/x86/dell/dell-laptop.c > @@ -22,11 +22,13 @@ > #include <linux/io.h> > #include <linux/rfkill.h> > #include <linux/power_supply.h> > +#include <linux/sysfs.h> > #include <linux/acpi.h> > #include <linux/mm.h> > #include <linux/i8042.h> > #include <linux/debugfs.h> > #include <linux/seq_file.h> > +#include <acpi/battery.h> > #include <acpi/video.h> > #include "dell-rbtn.h" > #include "dell-smbios.h" > @@ -98,6 +100,14 @@ static struct rfkill *wwan_rfkill; > static bool force_rfkill; > static bool micmute_led_registered; > static bool mute_led_registered; > +static enum battery_charging_mode bat_chg_current = DELL_BAT_MODE_NONE; > +static const char * const battery_state[DELL_BAT_MODE_MAX] = { > + [DELL_BAT_MODE_STANDARD] = "standard", > + [DELL_BAT_MODE_EXPRESS] = "express", > + [DELL_BAT_MODE_PRIMARILY_AC] = "primarily_ac", > + [DELL_BAT_MODE_ADAPTIVE] = "adaptive", > + [DELL_BAT_MODE_CUSTOM] = "custom", > +}; > > module_param(force_rfkill, bool, 0444); > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); > @@ -2183,6 +2193,256 @@ static struct led_classdev mute_led_cdev = { > .default_trigger = "audio-mute", > }; > > +static int dell_battery_read_req(const int type, int *val) > +{ > + struct calling_interface_buffer buffer; > + struct calling_interface_token *token; > + int err; > + > + token = dell_smbios_find_token(type); > + if (!token) > + return -ENODEV; > + > + dell_fill_request(&buffer, token->location, 0, 0, 0); > + err = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); > + if (!err) > + *val = buffer.output[1]; > + > + return err; > +} dell_send_request() returns negative value on error. As the read value seems to be always non-negative number, you can change API of the dell_battery_read_req() function to have read value in the return value (instead of in *val pointer). E.g. static int dell_battery_read_req(const int type) { ... err = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); if (err) return err; return buffer.output[1]; } > + > +static int dell_battery_write_req(const int type, int val) > +{ > + struct calling_interface_buffer buffer; > + struct calling_interface_token *token; > + > + token = dell_smbios_find_token(type); > + if (!token) > + return -ENODEV; > + > + dell_fill_request(&buffer, token->location, val, 0, 0); > + return dell_send_request(&buffer, > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); > +} > + > +/* The rules: the minimum start charging value is 50%. The maximum > + * start charging value is 95%. The minimum end charging value is > + * 55%. The maximum end charging value is 100%. And finally, there > + * has to be at least a 5% difference between start & end values. > + */ > +#define CHARGE_START_MIN 50 > +#define CHARGE_START_MAX 95 > +#define CHARGE_END_MIN 55 > +#define CHARGE_END_MAX 100 > +#define CHARGE_MIN_DIFF 5 > + > +static int dell_battery_custom_set(const int type, int val) > +{ > + if (type == BAT_CUSTOM_CHARGE_START) { > + int end = CHARGE_END_MAX; > + > + if (val < CHARGE_START_MIN) > + val = CHARGE_START_MIN; > + else if (val > CHARGE_START_MAX) > + val = CHARGE_START_MAX; > + > + dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); Missing check for failure of dell_battery_read_req. > + if ((end - val) < CHARGE_MIN_DIFF) > + val = end - CHARGE_MIN_DIFF; > + } else if (type == BAT_CUSTOM_CHARGE_END) { > + int start = CHARGE_START_MIN; > + > + if (val < CHARGE_END_MIN) > + val = CHARGE_END_MIN; > + else if (val > CHARGE_END_MAX) > + val = CHARGE_END_MAX; > + > + dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); Missing check for failure of dell_battery_read_req. > + if ((val - start) < CHARGE_MIN_DIFF) > + val = start + CHARGE_MIN_DIFF; > + } > + > + return dell_battery_write_req(type, val); > +} > + > +static int battery_charging_mode_set(enum battery_charging_mode mode) > +{ > + int err; > + > + switch (mode) { > + case DELL_BAT_MODE_STANDARD: > + err = dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode); > + break; > + case DELL_BAT_MODE_EXPRESS: > + err = dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode); > + break; > + case DELL_BAT_MODE_PRIMARILY_AC: > + err = dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode); > + break; > + case DELL_BAT_MODE_ADAPTIVE: > + err = dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode); > + break; > + case DELL_BAT_MODE_CUSTOM: > + err = dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode); > + break; > + default: > + err = -EINVAL; > + } > + > + return err; > +} You can make whole function smaller by avoiding err variable: static int battery_charging_mode_set(enum battery_charging_mode mode) { switch (mode) { case DELL_BAT_MODE_STANDARD: return dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode); case DELL_BAT_MODE_EXPRESS: return dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode); case DELL_BAT_MODE_PRIMARILY_AC: return dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode); case DELL_BAT_MODE_ADAPTIVE: return dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode); case DELL_BAT_MODE_CUSTOM: return dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode); default: return -EINVAL; } } > + > +static ssize_t charge_type_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + enum battery_charging_mode mode; > + ssize_t count = 0; > + > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > + if (battery_state[mode]) { > + count += sysfs_emit_at(buf, count, > + mode == bat_chg_current ? "[%s] " : "%s ", > + battery_state[mode]); > + } > + } > + > + /* convert the last space to a newline */ > + count--; > + count += sysfs_emit_at(buf, count, "\n"); Here is missing protection in the case when number of valid modes is zero, so count is 0 and buf was untouched. > + > + return count; > +} > + > +static ssize_t charge_type_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + enum battery_charging_mode mode; > + const char *label; > + int ret = -EINVAL; > + > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > + label = battery_state[mode]; > + if (label && sysfs_streq(label, buf)) > + break; > + } > + > + if (mode > DELL_BAT_MODE_NONE && mode < DELL_BAT_MODE_MAX) { > + ret = battery_charging_mode_set(mode); > + if (!ret) { > + bat_chg_current = mode; > + ret = size; > + } > + } > + > + return ret; > +} > + > +static ssize_t charge_control_start_threshold_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret, start; > + > + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); > + if (!ret) > + ret = sysfs_emit(buf, "%d\n", start); > + > + return ret; > +} This function and also following 3 functions have unusual error handling. Normally error handling is done by early return, as: ret = func1(); if (ret) return ret; ret = func2(); if (ret) return ret; return 0; You can change it something like: { int ret, start; ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); if (ret) return ret; return sysfs_emit(buf, "%d\n", start); } > +static ssize_t charge_control_start_threshold_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + int ret, start; > + > + ret = kstrtoint(buf, 10, &start); > + if (!ret) > + ret = dell_battery_custom_set(BAT_CUSTOM_CHARGE_START, start); > + if (!ret) > + ret = size; > + > + return ret; > +} > + > +static ssize_t charge_control_end_threshold_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret, end; > + > + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); > + if (!ret) > + ret = sysfs_emit(buf, "%d\n", end); > + > + return ret; > +} > + > +static ssize_t charge_control_end_threshold_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + int ret, end; > + > + ret = kstrtouint(buf, 10, &end); > + if (!ret) > + ret = dell_battery_custom_set(BAT_CUSTOM_CHARGE_END, end); > + if (!ret) > + ret = size; > + > + return ret; > +} > + > +static DEVICE_ATTR_RW(charge_control_start_threshold); > +static DEVICE_ATTR_RW(charge_control_end_threshold); > +static DEVICE_ATTR_RW(charge_type); > + > +static struct attribute *dell_battery_attrs[] = { > + &dev_attr_charge_control_start_threshold.attr, > + &dev_attr_charge_control_end_threshold.attr, > + &dev_attr_charge_type.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(dell_battery); > + > +static int dell_battery_add(struct power_supply *battery, > + struct acpi_battery_hook *hook) > +{ > + return device_add_groups(&battery->dev, dell_battery_groups); > +} > + > +static int dell_battery_remove(struct power_supply *battery, > + struct acpi_battery_hook *hook) > +{ > + device_remove_groups(&battery->dev, dell_battery_groups); > + return 0; > +} > + > +static struct acpi_battery_hook dell_battery_hook = { > + .add_battery = dell_battery_add, > + .remove_battery = dell_battery_remove, > + .name = "Dell Primary Battery Extension", > +}; > + > +static void __init dell_battery_init(struct device *dev) > +{ > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > + > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > + if (current_mode != DELL_BAT_MODE_NONE) { I quite do not understand how is this code suppose to work. Why is there mix of custom kernel enum battery_charging_mode and return value from Dell's API? My feeling is that dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN) checks if the token BAT_CUSTOM_MODE_TOKEN is set or not. Could you please check what is stored in every BAT_*_MODE_TOKEN token at this init stage? I think it should work similarly, like keyboard backlight tokens as implemented in functions: kbd_set_token_bit, kbd_get_token_bit, kbd_get_first_active_token_bit. > + bat_chg_current = current_mode; > + battery_hook_register(&dell_battery_hook); > + } > +} > + > +static void __exit dell_battery_exit(void) > +{ > + if (bat_chg_current != DELL_BAT_MODE_NONE) > + battery_hook_unregister(&dell_battery_hook); > +} > + > static int __init dell_init(void) > { > struct calling_interface_token *token; > @@ -2219,6 +2479,7 @@ static int __init dell_init(void) > touchpad_led_init(&platform_device->dev); > > kbd_led_init(&platform_device->dev); > + dell_battery_init(&platform_device->dev); > > dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL); > debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL, > @@ -2293,6 +2554,7 @@ static int __init dell_init(void) > if (mute_led_registered) > led_classdev_unregister(&mute_led_cdev); > fail_led: > + dell_battery_exit(); > dell_cleanup_rfkill(); > fail_rfkill: > platform_device_del(platform_device); > @@ -2311,6 +2573,7 @@ static void __exit dell_exit(void) > if (quirks && quirks->touchpad_led) > touchpad_led_exit(); > kbd_led_exit(); > + dell_battery_exit(); > backlight_device_unregister(dell_backlight_device); > if (micmute_led_registered) > led_classdev_unregister(&micmute_led_cdev); > diff --git a/drivers/platform/x86/dell/dell-smbios.h b/drivers/platform/x86/dell/dell-smbios.h > index ea0cc38642a2..f7c07b4d72e3 100644 > --- a/drivers/platform/x86/dell/dell-smbios.h > +++ b/drivers/platform/x86/dell/dell-smbios.h > @@ -33,11 +33,28 @@ > #define KBD_LED_AUTO_50_TOKEN 0x02EB > #define KBD_LED_AUTO_75_TOKEN 0x02EC > #define KBD_LED_AUTO_100_TOKEN 0x02F6 > +#define BAT_PRI_AC_MODE_TOKEN 0x0341 > +#define BAT_ADAPTIVE_MODE_TOKEN 0x0342 > +#define BAT_CUSTOM_MODE_TOKEN 0x0343 > +#define BAT_STANDARD_MODE_TOKEN 0x0346 > +#define BAT_EXPRESS_MODE_TOKEN 0x0347 > +#define BAT_CUSTOM_CHARGE_START 0x0349 > +#define BAT_CUSTOM_CHARGE_END 0x034A > #define GLOBAL_MIC_MUTE_ENABLE 0x0364 > #define GLOBAL_MIC_MUTE_DISABLE 0x0365 > #define GLOBAL_MUTE_ENABLE 0x058C > #define GLOBAL_MUTE_DISABLE 0x058D > > +enum battery_charging_mode { > + DELL_BAT_MODE_NONE = 0, > + DELL_BAT_MODE_STANDARD, > + DELL_BAT_MODE_EXPRESS, > + DELL_BAT_MODE_PRIMARILY_AC, > + DELL_BAT_MODE_ADAPTIVE, > + DELL_BAT_MODE_CUSTOM, > + DELL_BAT_MODE_MAX, > +}; > + I think that this is just an internal driver enum, not Dell API. So this enum should be in the dell-laptop.c file. > struct notifier_block; > > struct calling_interface_token { > -- > 2.39.2 > > > > -- > I'm available for contract & employment work, see: > https://spindle.queued.net/~dilinger/resume-tech.pdf
On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > Thanks for the quick feedback! Responses below. > > On Sat, 20 Jul 2024 10:40:19 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > Hello, > > > > I looked at your patch. I wrote some comments below. The main issue is > > how to correctly interpret read token values. > > > [...] > > > > > dell_send_request() returns negative value on error. As the read value > > seems to be always non-negative number, you can change API of the > > dell_battery_read_req() function to have read value in the return value > > (instead of in *val pointer). E.g. > > > > static int dell_battery_read_req(const int type) > > { > > ... > > err = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); > > if (err) > > return err; > > > > return buffer.output[1]; > > } > > > > Good call, I'll change that. > > > > > + > > > +static int dell_battery_write_req(const int type, int val) > > > +{ > > > + struct calling_interface_buffer buffer; > > > + struct calling_interface_token *token; > > > + > > > + token = dell_smbios_find_token(type); > > > + if (!token) > > > + return -ENODEV; > > > + > > > + dell_fill_request(&buffer, token->location, val, 0, 0); > > > + return dell_send_request(&buffer, > > > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); > > > +} > > > + > > > +/* The rules: the minimum start charging value is 50%. The maximum > > > + * start charging value is 95%. The minimum end charging value is > > > + * 55%. The maximum end charging value is 100%. And finally, there > > > + * has to be at least a 5% difference between start & end values. > > > + */ > > > +#define CHARGE_START_MIN 50 > > > +#define CHARGE_START_MAX 95 > > > +#define CHARGE_END_MIN 55 > > > +#define CHARGE_END_MAX 100 > > > +#define CHARGE_MIN_DIFF 5 > > > + > > > +static int dell_battery_custom_set(const int type, int val) > > > +{ > > > + if (type == BAT_CUSTOM_CHARGE_START) { > > > + int end = CHARGE_END_MAX; > > > + > > > + if (val < CHARGE_START_MIN) > > > + val = CHARGE_START_MIN; > > > + else if (val > CHARGE_START_MAX) > > > + val = CHARGE_START_MAX; > > > + > > > + dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); > > > > Missing check for failure of dell_battery_read_req. > > This is intentional; it's just a sanity check, we don't need to bail > if we hit a failure. I'll change the code to make that explicit > though, as it's not currently clear. > > > > > > > > + if ((end - val) < CHARGE_MIN_DIFF) > > > + val = end - CHARGE_MIN_DIFF; > > > + } else if (type == BAT_CUSTOM_CHARGE_END) { > > > + int start = CHARGE_START_MIN; > > > + > > > + if (val < CHARGE_END_MIN) > > > + val = CHARGE_END_MIN; > > > + else if (val > CHARGE_END_MAX) > > > + val = CHARGE_END_MAX; > > > + > > > + dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); > > > > Missing check for failure of dell_battery_read_req. > > > > Ditto. > > > > > + if ((val - start) < CHARGE_MIN_DIFF) > > > + val = start + CHARGE_MIN_DIFF; > > > + } > > > + > > > + return dell_battery_write_req(type, val); > > > +} > > > + > > > +static int battery_charging_mode_set(enum battery_charging_mode mode) > > > +{ > > > + int err; > > > + > > > + switch (mode) { > > > + case DELL_BAT_MODE_STANDARD: > > > + err = dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode); > > > + break; > > > + case DELL_BAT_MODE_EXPRESS: > > > + err = dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode); > > > + break; > > > + case DELL_BAT_MODE_PRIMARILY_AC: > > > + err = dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode); > > > + break; > > > + case DELL_BAT_MODE_ADAPTIVE: > > > + err = dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode); > > > + break; > > > + case DELL_BAT_MODE_CUSTOM: > > > + err = dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode); > > > + break; > > > + default: > > > + err = -EINVAL; > > > + } > > > + > > > + return err; > > > +} > > > > You can make whole function smaller by avoiding err variable: > > > > static int battery_charging_mode_set(enum battery_charging_mode mode) > > { > > switch (mode) { > > case DELL_BAT_MODE_STANDARD: > > return dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode); > > case DELL_BAT_MODE_EXPRESS: > > return dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode); > > case DELL_BAT_MODE_PRIMARILY_AC: > > return dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode); > > case DELL_BAT_MODE_ADAPTIVE: > > return dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode); > > case DELL_BAT_MODE_CUSTOM: > > return dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode); > > default: > > return -EINVAL; > > } > > } > > > > Okay, I'll change it. > > > > > + > > > +static ssize_t charge_type_show(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + enum battery_charging_mode mode; > > > + ssize_t count = 0; > > > + > > > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > > > + if (battery_state[mode]) { > > > + count += sysfs_emit_at(buf, count, > > > + mode == bat_chg_current ? "[%s] " : "%s ", > > > + battery_state[mode]); > > > + } > > > + } > > > + > > > + /* convert the last space to a newline */ > > > + count--; > > > + count += sysfs_emit_at(buf, count, "\n"); > > > > Here is missing protection in the case when number of valid modes is > > zero, so count is 0 and buf was untouched. > > > > This will never be zero (based on the hardcoded value of DELL_BAT_MODE_MAX), Now I see. You are iterating over members of constant array battery_state[]. I overlooked it and I thought that this iteration over mode values. What about writing the for- conditions to be visible that you are iterating over the array? E.g. size_t i; ... for (i = 0; i < ARRAY_SIZE(battery_state); i++) { if (!battery_state[i]) continue; count += sysfs_emit_at(buf, count, i == bat_chg_current ? "[%s] " : "%s ", battery_state[i]); } ... This has an advantage that you do not depend on DELL_BAT_MODE_MAX value, compiler will calculate upper bound automatically. Or another way. You can define array of tokens, like it is done for keyboard backlight. See how the array kbd_tokens[] is used. With this approach you can completely get rid of the DELL_BAT_MODE_MAX. > but perhaps a static_assert or BUILD_BUG_ON to verify that the number of > modes > 0? I think it is not needed. > > > > + > > > + return count; > > > +} > > > + > > > +static ssize_t charge_type_store(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, size_t size) > > > +{ > > > + enum battery_charging_mode mode; > > > + const char *label; > > > + int ret = -EINVAL; > > > + > > > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > > > + label = battery_state[mode]; > > > + if (label && sysfs_streq(label, buf)) > > > + break; > > > + } > > > + > > > + if (mode > DELL_BAT_MODE_NONE && mode < DELL_BAT_MODE_MAX) { > > > + ret = battery_charging_mode_set(mode); > > > + if (!ret) { > > > + bat_chg_current = mode; > > > + ret = size; > > > + } > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +static ssize_t charge_control_start_threshold_show(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + int ret, start; > > > + > > > + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); > > > + if (!ret) > > > + ret = sysfs_emit(buf, "%d\n", start); > > > + > > > + return ret; > > > +} > > > > This function and also following 3 functions have unusual error > > handling. Normally error handling is done by early return, as: > > > > ret = func1(); > > if (ret) > > return ret; > > > > ret = func2(); > > if (ret) > > return ret; > > > > return 0; > > > > You can change it something like: > > > > { > > int ret, start; > > > > ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); > > if (ret) > > return ret; > > > > return sysfs_emit(buf, "%d\n", start); > > } > > > > Okay. > > > > > +static ssize_t charge_control_start_threshold_store(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, size_t size) > > > +{ > [...] > > > > + > > > +static void __init dell_battery_init(struct device *dev) > > > +{ > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > + > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > I quite do not understand how is this code suppose to work. > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > value from Dell's API? > > This is from the original patch from Dell; tbh, I'm not sure. It does > work, though. That is, current_mode ends up holding the correct value > based on what was previously set, even if the charging mode is set from > the BIOS. > > I just scanned through the libsmbios code to see what it's doing, and > it appears to loop through every charging mode to check if its active. > I'm not really sure that makes much more sense, so I'll try some more > tests. Keyboard backlight code (kbd_get_first_active_token_bit) is doing also this type scan. If I remember correctly, for every keyboard backlight token we just know the boolean value - if the token is set or not. It would really nice to see what (raw) value is returned by the dell_battery_read_req(token) function for every battery token and for every initial state. Because it is really suspicious if dell_battery_read_req() would return value of the enum battery_charging_mode (as this is kernel enum). Also another important thing. In past it was possible to buy from Dell special batteries with long warranty (3+ years). I'm not sure if these batteries are still available for end-user customers. With this type of battery, it was not possible to change charging mode to ExpressCharge (bios option was fade-out). I do not have such battery anymore, but I would expect that the firmware disabled BAT_EXPRESS_MODE_TOKEN as mark it as unavailable. I think that we should scan list of available tokens, like it is done for keyboard backlight in kbd_init_tokens(). And export via sysfs only those battery modes for which there is available token. > > > > > My feeling is that dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN) checks > > if the token BAT_CUSTOM_MODE_TOKEN is set or not. > > > > Could you please check what is stored in every BAT_*_MODE_TOKEN token at > > this init stage? > > > > I think it should work similarly, like keyboard backlight tokens as > > implemented in functions: kbd_set_token_bit, kbd_get_token_bit, > > kbd_get_first_active_token_bit. > > > > > + bat_chg_current = current_mode; > > > + battery_hook_register(&dell_battery_hook); > > > + } > > > +} > > > + > [...] > > > > #define GLOBAL_MUTE_ENABLE 0x058C > > > #define GLOBAL_MUTE_DISABLE 0x058D > > > > > > +enum battery_charging_mode { > > > + DELL_BAT_MODE_NONE = 0, > > > + DELL_BAT_MODE_STANDARD, > > > + DELL_BAT_MODE_EXPRESS, > > > + DELL_BAT_MODE_PRIMARILY_AC, > > > + DELL_BAT_MODE_ADAPTIVE, > > > + DELL_BAT_MODE_CUSTOM, > > > + DELL_BAT_MODE_MAX, > > > +}; > > > + > > > > I think that this is just an internal driver enum, not Dell API. So this > > enum should be in the dell-laptop.c file. > > > > Agreed, I'll change it. > > > > > -- > I'm available for contract & employment work, see: > https://spindle.queued.net/~dilinger/resume-tech.pdf
On Sat, 20 Jul 2024 11:55:07 +0200 Pali Rohár <pali@kernel.org> wrote: > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > Thanks for the quick feedback! Responses below. > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > Pali Rohár <pali@kernel.org> wrote: > > > > > Hello, > > > > > > I looked at your patch. I wrote some comments below. The main issue is > > > how to correctly interpret read token values. > > > [...] > > > > + > > > > +static ssize_t charge_type_show(struct device *dev, > > > > + struct device_attribute *attr, > > > > + char *buf) > > > > +{ > > > > + enum battery_charging_mode mode; > > > > + ssize_t count = 0; > > > > + > > > > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > > > > + if (battery_state[mode]) { > > > > + count += sysfs_emit_at(buf, count, > > > > + mode == bat_chg_current ? "[%s] " : "%s ", > > > > + battery_state[mode]); > > > > + } > > > > + } > > > > + > > > > + /* convert the last space to a newline */ > > > > + count--; > > > > + count += sysfs_emit_at(buf, count, "\n"); > > > > > > Here is missing protection in the case when number of valid modes is > > > zero, so count is 0 and buf was untouched. > > > > > > > This will never be zero (based on the hardcoded value of DELL_BAT_MODE_MAX), > > Now I see. You are iterating over members of constant array battery_state[]. > I overlooked it and I thought that this iteration over mode values. > > What about writing the for- conditions to be visible that you are > iterating over the array? E.g. > > size_t i; > ... > for (i = 0; i < ARRAY_SIZE(battery_state); i++) { > if (!battery_state[i]) > continue; > count += sysfs_emit_at(buf, count, i == bat_chg_current ? "[%s] " : "%s ", battery_state[i]); > } > ... > > This has an advantage that you do not depend on DELL_BAT_MODE_MAX value, > compiler will calculate upper bound automatically. > > Or another way. You can define array of tokens, like it is done for > keyboard backlight. See how the array kbd_tokens[] is used. > > With this approach you can completely get rid of the DELL_BAT_MODE_MAX. > See below for a question about charge_type_store() if we get rid of DELL_BAT_MODE_MAX. > > but perhaps a static_assert or BUILD_BUG_ON to verify that the number of > > modes > 0? > > I think it is not needed. > > > > > > > + > > > > + return count; > > > > +} > > > > + > > > > +static ssize_t charge_type_store(struct device *dev, > > > > + struct device_attribute *attr, > > > > + const char *buf, size_t size) > > > > +{ > > > > + enum battery_charging_mode mode; > > > > + const char *label; > > > > + int ret = -EINVAL; > > > > + > > > > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > > > > + label = battery_state[mode]; > > > > + if (label && sysfs_streq(label, buf)) > > > > + break; > > > > + } > > > > + > > > > + if (mode > DELL_BAT_MODE_NONE && mode < DELL_BAT_MODE_MAX) { > > > > + ret = battery_charging_mode_set(mode); > > > > + if (!ret) { > > > > + bat_chg_current = mode; > > > > + ret = size; > > > > + } > > > > + } > > > > + > > > > + return ret; > > > > +} Here we're using DELL_BAT_MODE_MAX to determine if we failed to match any mode strings sent from the user. If we get rid of that, we're either going to have to use a separate variable (eg, 'bool matched = false;' and set it to true in case of a match), or iterate backwards (eg, for (mode = ARRAY_SIZE(battery_state); mode >= DELL_BAT_MODE_NONE; mode--) { ... } if (mode != DELL_BAT_MODE_NONE) { ret = battery_charging_mode_set(mode); Do you have a preference? [...] > > > > +static ssize_t charge_control_start_threshold_store(struct device *dev, > > > > + struct device_attribute *attr, > > > > + const char *buf, size_t size) > > > > +{ > > [...] > > > > > > + > > > > +static void __init dell_battery_init(struct device *dev) > > > > +{ > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > + > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > I quite do not understand how is this code suppose to work. > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > value from Dell's API? > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > work, though. That is, current_mode ends up holding the correct value > > based on what was previously set, even if the charging mode is set from > > the BIOS. > > > > I just scanned through the libsmbios code to see what it's doing, and > > it appears to loop through every charging mode to check if its active. > > I'm not really sure that makes much more sense, so I'll try some more > > tests. > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > this type scan. If I remember correctly, for every keyboard backlight > token we just know the boolean value - if the token is set or not. > > It would really nice to see what (raw) value is returned by the > dell_battery_read_req(token) function for every battery token and for > every initial state. I checked this. The BIOS sets the mode value in every related token location. I'm still not really sure what libsmbios is doing, but the kernel code seems to arbitrarily choose one of the token locations to read from. This makes sense to me now. In the BIOS when I set the mode to "ExpressCharge", this what I pulled for each token location: [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 Similar story when I set it to Custom (all were '5'), or Standard ('1'). When I set it from linux as well, it changed all location values. > > Because it is really suspicious if dell_battery_read_req() would return > value of the enum battery_charging_mode (as this is kernel enum). > > > Also another important thing. In past it was possible to buy from Dell > special batteries with long warranty (3+ years). I'm not sure if these > batteries are still available for end-user customers. With this type of > battery, it was not possible to change charging mode to ExpressCharge > (bios option was fade-out). I do not have such battery anymore, but I > would expect that the firmware disabled BAT_EXPRESS_MODE_TOKEN as mark > it as unavailable. > > I think that we should scan list of available tokens, like it is done > for keyboard backlight in kbd_init_tokens(). And export via sysfs only > those battery modes for which there is available token. I agree, but I'm not seeing a way to do that right now given how the BIOS sets the mode. Maybe libsmbios has some clues...
On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > On Sat, 20 Jul 2024 11:55:07 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > Thanks for the quick feedback! Responses below. > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > Hello, > > > > > > > > I looked at your patch. I wrote some comments below. The main issue is > > > > how to correctly interpret read token values. > > > > > [...] > > > > > + > > > > > +static ssize_t charge_type_show(struct device *dev, > > > > > + struct device_attribute *attr, > > > > > + char *buf) > > > > > +{ > > > > > + enum battery_charging_mode mode; > > > > > + ssize_t count = 0; > > > > > + > > > > > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > > > > > + if (battery_state[mode]) { > > > > > + count += sysfs_emit_at(buf, count, > > > > > + mode == bat_chg_current ? "[%s] " : "%s ", > > > > > + battery_state[mode]); > > > > > + } > > > > > + } > > > > > + > > > > > + /* convert the last space to a newline */ > > > > > + count--; > > > > > + count += sysfs_emit_at(buf, count, "\n"); > > > > > > > > Here is missing protection in the case when number of valid modes is > > > > zero, so count is 0 and buf was untouched. > > > > > > > > > > This will never be zero (based on the hardcoded value of DELL_BAT_MODE_MAX), > > > > Now I see. You are iterating over members of constant array battery_state[]. > > I overlooked it and I thought that this iteration over mode values. > > > > What about writing the for- conditions to be visible that you are > > iterating over the array? E.g. > > > > size_t i; > > ... > > for (i = 0; i < ARRAY_SIZE(battery_state); i++) { > > if (!battery_state[i]) > > continue; > > count += sysfs_emit_at(buf, count, i == bat_chg_current ? "[%s] " : "%s ", battery_state[i]); > > } > > ... > > > > This has an advantage that you do not depend on DELL_BAT_MODE_MAX value, > > compiler will calculate upper bound automatically. > > > > Or another way. You can define array of tokens, like it is done for > > keyboard backlight. See how the array kbd_tokens[] is used. > > > > With this approach you can completely get rid of the DELL_BAT_MODE_MAX. > > > > See below for a question about charge_type_store() if we get rid of > DELL_BAT_MODE_MAX. > > > > but perhaps a static_assert or BUILD_BUG_ON to verify that the number of > > > modes > 0? > > > > I think it is not needed. > > > > > > > > > > + > > > > > + return count; > > > > > +} > > > > > + > > > > > +static ssize_t charge_type_store(struct device *dev, > > > > > + struct device_attribute *attr, > > > > > + const char *buf, size_t size) > > > > > +{ > > > > > + enum battery_charging_mode mode; > > > > > + const char *label; > > > > > + int ret = -EINVAL; > > > > > + > > > > > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > > > > > + label = battery_state[mode]; > > > > > + if (label && sysfs_streq(label, buf)) > > > > > + break; > > > > > + } > > > > > + > > > > > + if (mode > DELL_BAT_MODE_NONE && mode < DELL_BAT_MODE_MAX) { > > > > > + ret = battery_charging_mode_set(mode); > > > > > + if (!ret) { > > > > > + bat_chg_current = mode; > > > > > + ret = size; > > > > > + } > > > > > + } > > > > > + > > > > > + return ret; > > > > > +} > > Here we're using DELL_BAT_MODE_MAX to determine if we failed to match > any mode strings sent from the user. If we get rid of that, we're either > going to have to use a separate variable (eg, 'bool matched = false;' and > set it to true in case of a match), or iterate backwards (eg, > for (mode = ARRAY_SIZE(battery_state); mode >= DELL_BAT_MODE_NONE; mode--) { > ... > } > if (mode != DELL_BAT_MODE_NONE) { > ret = battery_charging_mode_set(mode); > > > Do you have a preference? You can use extra bool variable. Anyway, if we figure out how the token value is going to be used, maybe this code could be changed / simplified... > > [...] > > > > > +static ssize_t charge_control_start_threshold_store(struct device *dev, > > > > > + struct device_attribute *attr, > > > > > + const char *buf, size_t size) > > > > > +{ > > > [...] > > > > > > > > + > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > +{ > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > + > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > value from Dell's API? > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > work, though. That is, current_mode ends up holding the correct value > > > based on what was previously set, even if the charging mode is set from > > > the BIOS. > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > it appears to loop through every charging mode to check if its active. > > > I'm not really sure that makes much more sense, so I'll try some more > > > tests. > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > this type scan. If I remember correctly, for every keyboard backlight > > token we just know the boolean value - if the token is set or not. > > > > It would really nice to see what (raw) value is returned by the > > dell_battery_read_req(token) function for every battery token and for > > every initial state. > > I checked this. The BIOS sets the mode value in every related token > location. I'm still not really sure what libsmbios is doing, but the > kernel code seems to arbitrarily choose one of the token locations > to read from. This makes sense to me now. > > In the BIOS when I set the mode to "ExpressCharge", > this what I pulled for each token location: > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > When I set it from linux as well, it changed all location values. Interesting... Anyway, I still think that the API could be similar to what is used in keyboard backlight. Could you please dump all information about each token? They are in struct calling_interface_token returned by dell_smbios_find_token. I'm interesting in tokenID, location and value. Ideally to compare what is in token->value and then in buffer.output[1] (in case dell_send_request does not fail). > > > > Because it is really suspicious if dell_battery_read_req() would return > > value of the enum battery_charging_mode (as this is kernel enum). > > > > > > Also another important thing. In past it was possible to buy from Dell > > special batteries with long warranty (3+ years). I'm not sure if these > > batteries are still available for end-user customers. With this type of > > battery, it was not possible to change charging mode to ExpressCharge > > (bios option was fade-out). I do not have such battery anymore, but I > > would expect that the firmware disabled BAT_EXPRESS_MODE_TOKEN as mark > > it as unavailable. > > > > I think that we should scan list of available tokens, like it is done > > for keyboard backlight in kbd_init_tokens(). And export via sysfs only > > those battery modes for which there is available token. > > I agree, but I'm not seeing a way to do that right now given how the > BIOS sets the mode. Maybe libsmbios has some clues... Try to get those dumps and I will try to do something with it. > > > > > -- > I'm available for contract & employment work, see: > https://spindle.queued.net/~dilinger/resume-tech.pdf
On Sun, 21 Jul 2024 11:02:38 +0200 Pali Rohár <pali@kernel.org> wrote: > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > On Sat, 20 Jul 2024 11:55:07 +0200 > > Pali Rohár <pali@kernel.org> wrote: > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > Thanks for the quick feedback! Responses below. > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > Pali Rohár <pali@kernel.org> wrote: > > > > [...] > > > > > > + > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > +{ > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > + > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > value from Dell's API? > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > work, though. That is, current_mode ends up holding the correct value > > > > based on what was previously set, even if the charging mode is set from > > > > the BIOS. > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > it appears to loop through every charging mode to check if its active. > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > tests. > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > this type scan. If I remember correctly, for every keyboard backlight > > > token we just know the boolean value - if the token is set or not. > > > > > > It would really nice to see what (raw) value is returned by the > > > dell_battery_read_req(token) function for every battery token and for > > > every initial state. > > > > I checked this. The BIOS sets the mode value in every related token > > location. I'm still not really sure what libsmbios is doing, but the > > kernel code seems to arbitrarily choose one of the token locations > > to read from. This makes sense to me now. > > > > In the BIOS when I set the mode to "ExpressCharge", > > this what I pulled for each token location: > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > When I set it from linux as well, it changed all location values. > > Interesting... Anyway, I still think that the API could be similar to > what is used in keyboard backlight. > > Could you please dump all information about each token? They are in > struct calling_interface_token returned by dell_smbios_find_token. > > I'm interesting in tokenID, location and value. > > Ideally to compare what is in token->value and then in buffer.output[1] > (in case dell_send_request does not fail). Alright, here's what I see: [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > Because it is really suspicious if dell_battery_read_req() would return > > > value of the enum battery_charging_mode (as this is kernel enum). > > > > > > > > > Also another important thing. In past it was possible to buy from Dell > > > special batteries with long warranty (3+ years). I'm not sure if these > > > batteries are still available for end-user customers. With this type of > > > battery, it was not possible to change charging mode to ExpressCharge > > > (bios option was fade-out). I do not have such battery anymore, but I > > > would expect that the firmware disabled BAT_EXPRESS_MODE_TOKEN as mark > > > it as unavailable. > > > > > > I think that we should scan list of available tokens, like it is done > > > for keyboard backlight in kbd_init_tokens(). And export via sysfs only > > > those battery modes for which there is available token. > > > > I agree, but I'm not seeing a way to do that right now given how the > > BIOS sets the mode. Maybe libsmbios has some clues... > > Try to get those dumps and I will try to do something with it. > > > > > > > > > > > -- > > I'm available for contract & employment work, see: > > https://spindle.queued.net/~dilinger/resume-tech.pdf
On Sunday 21 July 2024 19:37:16 Andres Salomon wrote: > On Sun, 21 Jul 2024 11:02:38 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > > On Sat, 20 Jul 2024 11:55:07 +0200 > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > > Thanks for the quick feedback! Responses below. > > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > [...] > > > > > > > > + > > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > > +{ > > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > > + > > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > > value from Dell's API? > > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > > work, though. That is, current_mode ends up holding the correct value > > > > > based on what was previously set, even if the charging mode is set from > > > > > the BIOS. > > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > > it appears to loop through every charging mode to check if its active. > > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > > tests. > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > > this type scan. If I remember correctly, for every keyboard backlight > > > > token we just know the boolean value - if the token is set or not. > > > > > > > > It would really nice to see what (raw) value is returned by the > > > > dell_battery_read_req(token) function for every battery token and for > > > > every initial state. > > > > > > I checked this. The BIOS sets the mode value in every related token > > > location. I'm still not really sure what libsmbios is doing, but the > > > kernel code seems to arbitrarily choose one of the token locations > > > to read from. This makes sense to me now. > > > > > > In the BIOS when I set the mode to "ExpressCharge", > > > this what I pulled for each token location: > > > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > > When I set it from linux as well, it changed all location values. > > > > Interesting... Anyway, I still think that the API could be similar to > > what is used in keyboard backlight. > > > > Could you please dump all information about each token? They are in > > struct calling_interface_token returned by dell_smbios_find_token. > > > > I'm interesting in tokenID, location and value. > > > > Ideally to compare what is in token->value and then in buffer.output[1] > > (in case dell_send_request does not fail). > > > Alright, here's what I see: > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN?
On Mon, 22 Jul 2024 01:40:37 +0200 Pali Rohár <pali@kernel.org> wrote: > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote: > > On Sun, 21 Jul 2024 11:02:38 +0200 > > Pali Rohár <pali@kernel.org> wrote: > > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > > > On Sat, 20 Jul 2024 11:55:07 +0200 > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > > > Thanks for the quick feedback! Responses below. > > > > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > [...] > > > > > > > > > > + > > > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > > > +{ > > > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > > > + > > > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > > > value from Dell's API? > > > > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > > > work, though. That is, current_mode ends up holding the correct value > > > > > > based on what was previously set, even if the charging mode is set from > > > > > > the BIOS. > > > > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > > > it appears to loop through every charging mode to check if its active. > > > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > > > tests. > > > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > > > this type scan. If I remember correctly, for every keyboard backlight > > > > > token we just know the boolean value - if the token is set or not. > > > > > > > > > > It would really nice to see what (raw) value is returned by the > > > > > dell_battery_read_req(token) function for every battery token and for > > > > > every initial state. > > > > > > > > I checked this. The BIOS sets the mode value in every related token > > > > location. I'm still not really sure what libsmbios is doing, but the > > > > kernel code seems to arbitrarily choose one of the token locations > > > > to read from. This makes sense to me now. > > > > > > > > In the BIOS when I set the mode to "ExpressCharge", > > > > this what I pulled for each token location: > > > > > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > > > When I set it from linux as well, it changed all location values. > > > > > > Interesting... Anyway, I still think that the API could be similar to > > > what is used in keyboard backlight. > > > > > > Could you please dump all information about each token? They are in > > > struct calling_interface_token returned by dell_smbios_find_token. > > > > > > I'm interesting in tokenID, location and value. > > > > > > Ideally to compare what is in token->value and then in buffer.output[1] > > > (in case dell_send_request does not fail). > > > > > > Alright, here's what I see: > > > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 > > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 > > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 > > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 > > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 > > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN? Here's Express: [ 5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 [ 5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2 [ 5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2 [ 5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 [ 5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2 [ 5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2 [ 5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 [ 5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2 [ 5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2 [ 5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 [ 5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2 [ 5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2 [ 5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 [ 5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2 [ 5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2 [ 5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 [ 5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65 [ 5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 [ 5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 [ 5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85 [ 5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 And here's Adaptive: [ 5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 [ 5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4 [ 5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4 [ 5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 [ 5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4 [ 5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4 [ 5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 [ 5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4 [ 5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4 [ 5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 [ 5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4 [ 5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4 [ 5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 [ 5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4 [ 5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4 [ 5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 [ 5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65 [ 5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 [ 5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 [ 5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85 [ 5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85
On Sunday 21 July 2024 19:58:51 Andres Salomon wrote: > On Mon, 22 Jul 2024 01:40:37 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote: > > > On Sun, 21 Jul 2024 11:02:38 +0200 > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > > > > On Sat, 20 Jul 2024 11:55:07 +0200 > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > > > > Thanks for the quick feedback! Responses below. > > > > > > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > [...] > > > > > > > > > > > > + > > > > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > > > > +{ > > > > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > > > > + > > > > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > > > > value from Dell's API? > > > > > > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > > > > work, though. That is, current_mode ends up holding the correct value > > > > > > > based on what was previously set, even if the charging mode is set from > > > > > > > the BIOS. > > > > > > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > > > > it appears to loop through every charging mode to check if its active. > > > > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > > > > tests. > > > > > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > > > > this type scan. If I remember correctly, for every keyboard backlight > > > > > > token we just know the boolean value - if the token is set or not. > > > > > > > > > > > > It would really nice to see what (raw) value is returned by the > > > > > > dell_battery_read_req(token) function for every battery token and for > > > > > > every initial state. > > > > > > > > > > I checked this. The BIOS sets the mode value in every related token > > > > > location. I'm still not really sure what libsmbios is doing, but the > > > > > kernel code seems to arbitrarily choose one of the token locations > > > > > to read from. This makes sense to me now. > > > > > > > > > > In the BIOS when I set the mode to "ExpressCharge", > > > > > this what I pulled for each token location: > > > > > > > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > > > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > > > > When I set it from linux as well, it changed all location values. > > > > > > > > Interesting... Anyway, I still think that the API could be similar to > > > > what is used in keyboard backlight. > > > > > > > > Could you please dump all information about each token? They are in > > > > struct calling_interface_token returned by dell_smbios_find_token. > > > > > > > > I'm interesting in tokenID, location and value. > > > > > > > > Ideally to compare what is in token->value and then in buffer.output[1] > > > > (in case dell_send_request does not fail). > > > > > > > > > Alright, here's what I see: > > > > > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 > > > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 > > > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 > > > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 > > > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 > > > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN? > > Here's Express: > > [ 5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > [ 5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > [ 5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2 > [ 5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > [ 5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > [ 5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2 > [ 5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > [ 5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > [ 5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2 > [ 5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > [ 5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > [ 5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2 > [ 5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > [ 5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > [ 5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2 > [ 5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > [ 5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > [ 5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > [ 5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > [ 5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > [ 5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > And here's Adaptive: > > [ 5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > [ 5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > [ 5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4 > [ 5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > [ 5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > [ 5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4 > [ 5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > [ 5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > [ 5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4 > [ 5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > [ 5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > [ 5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4 > [ 5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > [ 5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > [ 5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4 > [ 5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > [ 5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > [ 5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > [ 5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > [ 5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > [ 5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > -- > I'm available for contract & employment work, see: > https://spindle.queued.net/~dilinger/resume-tech.pdf Nice! So it is exactly same as API of keyboard backlight tokens. Thanks. In dell_battery_write_req function you can drop second "val" argument and replace it by token->value. So the dell_fill_request call in that function would look like: dell_fill_request(&buffer, token->location, token->value, 0, 0); And then you can mimic the usage as it is done in keyboard backlight functions (kbd_get_first_active_token_bit). If you do not know what I mean then later (today or tomorrow) I can write code example of the functionality.
On Mon, 22 Jul 2024 09:18:45 +0200 Pali Rohár <pali@kernel.org> wrote: > On Sunday 21 July 2024 19:58:51 Andres Salomon wrote: > > On Mon, 22 Jul 2024 01:40:37 +0200 > > Pali Rohár <pali@kernel.org> wrote: > > > > > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote: > > > > On Sun, 21 Jul 2024 11:02:38 +0200 > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > > > > > On Sat, 20 Jul 2024 11:55:07 +0200 > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > > > > > Thanks for the quick feedback! Responses below. > > > > > > > > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > + > > > > > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > > > > > +{ > > > > > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > > > > > + > > > > > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > > > > > value from Dell's API? > > > > > > > > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > > > > > work, though. That is, current_mode ends up holding the correct value > > > > > > > > based on what was previously set, even if the charging mode is set from > > > > > > > > the BIOS. > > > > > > > > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > > > > > it appears to loop through every charging mode to check if its active. > > > > > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > > > > > tests. > > > > > > > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > > > > > this type scan. If I remember correctly, for every keyboard backlight > > > > > > > token we just know the boolean value - if the token is set or not. > > > > > > > > > > > > > > It would really nice to see what (raw) value is returned by the > > > > > > > dell_battery_read_req(token) function for every battery token and for > > > > > > > every initial state. > > > > > > > > > > > > I checked this. The BIOS sets the mode value in every related token > > > > > > location. I'm still not really sure what libsmbios is doing, but the > > > > > > kernel code seems to arbitrarily choose one of the token locations > > > > > > to read from. This makes sense to me now. > > > > > > > > > > > > In the BIOS when I set the mode to "ExpressCharge", > > > > > > this what I pulled for each token location: > > > > > > > > > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > > > > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > > > > > When I set it from linux as well, it changed all location values. > > > > > > > > > > Interesting... Anyway, I still think that the API could be similar to > > > > > what is used in keyboard backlight. > > > > > > > > > > Could you please dump all information about each token? They are in > > > > > struct calling_interface_token returned by dell_smbios_find_token. > > > > > > > > > > I'm interesting in tokenID, location and value. > > > > > > > > > > Ideally to compare what is in token->value and then in buffer.output[1] > > > > > (in case dell_send_request does not fail). > > > > > > > > > > > > Alright, here's what I see: > > > > > > > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 > > > > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 > > > > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 > > > > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 > > > > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 > > > > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN? > > > > Here's Express: > > > > [ 5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > [ 5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > [ 5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2 > > [ 5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > [ 5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > [ 5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2 > > [ 5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > [ 5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > [ 5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > [ 5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > [ 5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > [ 5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2 > > [ 5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > [ 5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > [ 5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2 > > [ 5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > [ 5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > [ 5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > [ 5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > [ 5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > [ 5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > And here's Adaptive: > > > > [ 5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > [ 5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > [ 5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4 > > [ 5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > [ 5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > [ 5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4 > > [ 5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > [ 5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > [ 5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4 > > [ 5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > [ 5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > [ 5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4 > > [ 5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > [ 5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > [ 5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4 > > [ 5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > [ 5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > [ 5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > [ 5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > [ 5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > [ 5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > -- > > I'm available for contract & employment work, see: > > https://spindle.queued.net/~dilinger/resume-tech.pdf > > Nice! So it is exactly same as API of keyboard backlight tokens. Thanks. > > In dell_battery_write_req function you can drop second "val" argument > and replace it by token->value. So the dell_fill_request call in that > function would look like: > > dell_fill_request(&buffer, token->location, token->value, 0, 0); Well, except that we use dell_battery_write_req for writing the charge start/end values as well (in dell_battery_custom_set). Those can't be obtained from token->value. We could have two separate functions for that, or set 'val' to a sentinel value (0) that, if detected, we set val=token->value. I'm still not really understanding the point, though. > > And then you can mimic the usage as it is done in keyboard backlight > functions (kbd_get_first_active_token_bit). > > If you do not know what I mean then later (today or tomorrow) I can > write code example of the functionality. Sorry, I still don't understand what the goal is here. Is the goal to not pull from a random location to determine the current charging mode? Is the goal to determine what charging modes are currently supported (and if so, I don't see how)? Is the goal to avoid having the kernel hardcode a list of enums that the BIOS might have different values for? Is the goal to merge the keyboard backlight and battery setting functions?
On Monday 22 July 2024 14:34:32 Andres Salomon wrote: > On Mon, 22 Jul 2024 09:18:45 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > On Sunday 21 July 2024 19:58:51 Andres Salomon wrote: > > > On Mon, 22 Jul 2024 01:40:37 +0200 > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote: > > > > > On Sun, 21 Jul 2024 11:02:38 +0200 > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > > > > > > On Sat, 20 Jul 2024 11:55:07 +0200 > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > > > > > > Thanks for the quick feedback! Responses below. > > > > > > > > > > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > + > > > > > > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > > > > > > +{ > > > > > > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > > > > > > + > > > > > > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > > > > > > value from Dell's API? > > > > > > > > > > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > > > > > > work, though. That is, current_mode ends up holding the correct value > > > > > > > > > based on what was previously set, even if the charging mode is set from > > > > > > > > > the BIOS. > > > > > > > > > > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > > > > > > it appears to loop through every charging mode to check if its active. > > > > > > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > > > > > > tests. > > > > > > > > > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > > > > > > this type scan. If I remember correctly, for every keyboard backlight > > > > > > > > token we just know the boolean value - if the token is set or not. > > > > > > > > > > > > > > > > It would really nice to see what (raw) value is returned by the > > > > > > > > dell_battery_read_req(token) function for every battery token and for > > > > > > > > every initial state. > > > > > > > > > > > > > > I checked this. The BIOS sets the mode value in every related token > > > > > > > location. I'm still not really sure what libsmbios is doing, but the > > > > > > > kernel code seems to arbitrarily choose one of the token locations > > > > > > > to read from. This makes sense to me now. > > > > > > > > > > > > > > In the BIOS when I set the mode to "ExpressCharge", > > > > > > > this what I pulled for each token location: > > > > > > > > > > > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > > > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > > > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > > > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > > > > > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > > > > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > > > > > > When I set it from linux as well, it changed all location values. > > > > > > > > > > > > Interesting... Anyway, I still think that the API could be similar to > > > > > > what is used in keyboard backlight. > > > > > > > > > > > > Could you please dump all information about each token? They are in > > > > > > struct calling_interface_token returned by dell_smbios_find_token. > > > > > > > > > > > > I'm interesting in tokenID, location and value. > > > > > > > > > > > > Ideally to compare what is in token->value and then in buffer.output[1] > > > > > > (in case dell_send_request does not fail). > > > > > > > > > > > > > > > Alright, here's what I see: > > > > > > > > > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 > > > > > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 > > > > > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 > > > > > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 > > > > > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 > > > > > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN? > > > > > > Here's Express: > > > > > > [ 5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > [ 5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > [ 5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2 > > > [ 5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > [ 5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > [ 5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2 > > > [ 5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > [ 5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > [ 5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > [ 5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > [ 5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > [ 5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2 > > > [ 5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > [ 5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > [ 5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2 > > > [ 5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > [ 5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > [ 5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > [ 5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > [ 5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > [ 5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > And here's Adaptive: > > > > > > [ 5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > [ 5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > [ 5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4 > > > [ 5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > [ 5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > [ 5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4 > > > [ 5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > [ 5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > [ 5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4 > > > [ 5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > [ 5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > [ 5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4 > > > [ 5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > [ 5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > [ 5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4 > > > [ 5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > [ 5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > [ 5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > [ 5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > [ 5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > [ 5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > > > -- > > > I'm available for contract & employment work, see: > > > https://spindle.queued.net/~dilinger/resume-tech.pdf > > > > Nice! So it is exactly same as API of keyboard backlight tokens. Thanks. > > > > In dell_battery_write_req function you can drop second "val" argument > > and replace it by token->value. So the dell_fill_request call in that > > function would look like: > > > > dell_fill_request(&buffer, token->location, token->value, 0, 0); > > > Well, except that we use dell_battery_write_req for writing the charge > start/end values as well (in dell_battery_custom_set). Those can't be > obtained from token->value. > > We could have two separate functions for that, or set 'val' to a > sentinel value (0) that, if detected, we set val=token->value. I'm > still not really understanding the point, though. I think that two separate functions would be needed. One which set battery mode (enum) and which set custom thresholds. > > > > And then you can mimic the usage as it is done in keyboard backlight > > functions (kbd_get_first_active_token_bit). > > > > If you do not know what I mean then later (today or tomorrow) I can > > write code example of the functionality. > > Sorry, I still don't understand what the goal is here. Is the goal to > not pull from a random location to determine the current charging mode? > Is the goal to determine what charging modes are currently supported > (and if so, I don't see how)? Is the goal to avoid having the kernel > hardcode a list of enums that the BIOS might have different values > for? Is the goal to merge the keyboard backlight and battery setting > functions? Avoid having the kernel hardcoded values for enums which SMBIOS provides. Future (or maybe also older) modes may have different enum values. So we should use what SMBIOS provides to us. Also to determinate which charging modes are supported by the current HW configuration. If BIOS does not support some mode or does not allow to set some mode, kernel should not export this as supported option. If you do not see how to do it, please give me some time, I will send you an example. Going to look at it right now. Merging keyboard backlight and battery code is bonus, not required. But I thought that it would be easier to build a new code from common blocks.
On Mon, 22 Jul 2024 20:41:32 +0200 Pali Rohár <pali@kernel.org> wrote: > On Monday 22 July 2024 14:34:32 Andres Salomon wrote: > > On Mon, 22 Jul 2024 09:18:45 +0200 > > Pali Rohár <pali@kernel.org> wrote: > > > > > On Sunday 21 July 2024 19:58:51 Andres Salomon wrote: > > > > On Mon, 22 Jul 2024 01:40:37 +0200 > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote: > > > > > > On Sun, 21 Jul 2024 11:02:38 +0200 > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > > > > > > > On Sat, 20 Jul 2024 11:55:07 +0200 > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > > > > > > > Thanks for the quick feedback! Responses below. > > > > > > > > > > > > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > > > > > > > +{ > > > > > > > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > > > > > > > + > > > > > > > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > > > > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > > > > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > > > > > > > value from Dell's API? > > > > > > > > > > > > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > > > > > > > work, though. That is, current_mode ends up holding the correct value > > > > > > > > > > based on what was previously set, even if the charging mode is set from > > > > > > > > > > the BIOS. > > > > > > > > > > > > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > > > > > > > it appears to loop through every charging mode to check if its active. > > > > > > > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > > > > > > > tests. > > > > > > > > > > > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > > > > > > > this type scan. If I remember correctly, for every keyboard backlight > > > > > > > > > token we just know the boolean value - if the token is set or not. > > > > > > > > > > > > > > > > > > It would really nice to see what (raw) value is returned by the > > > > > > > > > dell_battery_read_req(token) function for every battery token and for > > > > > > > > > every initial state. > > > > > > > > > > > > > > > > I checked this. The BIOS sets the mode value in every related token > > > > > > > > location. I'm still not really sure what libsmbios is doing, but the > > > > > > > > kernel code seems to arbitrarily choose one of the token locations > > > > > > > > to read from. This makes sense to me now. > > > > > > > > > > > > > > > > In the BIOS when I set the mode to "ExpressCharge", > > > > > > > > this what I pulled for each token location: > > > > > > > > > > > > > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > > > > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > > > > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > > > > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > > > > > > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > > > > > > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > > > > > > > When I set it from linux as well, it changed all location values. > > > > > > > > > > > > > > Interesting... Anyway, I still think that the API could be similar to > > > > > > > what is used in keyboard backlight. > > > > > > > > > > > > > > Could you please dump all information about each token? They are in > > > > > > > struct calling_interface_token returned by dell_smbios_find_token. > > > > > > > > > > > > > > I'm interesting in tokenID, location and value. > > > > > > > > > > > > > > Ideally to compare what is in token->value and then in buffer.output[1] > > > > > > > (in case dell_send_request does not fail). > > > > > > > > > > > > > > > > > > Alright, here's what I see: > > > > > > > > > > > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > > > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 > > > > > > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > > > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 > > > > > > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > > > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 > > > > > > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > > > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 > > > > > > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > > > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 > > > > > > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > > > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > > > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > > > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > > > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > > > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN? > > > > > > > > Here's Express: > > > > > > > > [ 5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > [ 5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > [ 5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > [ 5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > [ 5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > [ 5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > [ 5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > [ 5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2 > > > > [ 5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > [ 5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > [ 5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > [ 5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > [ 5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > [ 5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > [ 5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > [ 5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > And here's Adaptive: > > > > > > > > [ 5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > [ 5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4 > > > > [ 5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > [ 5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4 > > > > [ 5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > [ 5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4 > > > > [ 5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > [ 5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4 > > > > [ 5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > [ 5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4 > > > > [ 5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > [ 5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > [ 5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > [ 5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > [ 5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > [ 5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > > > > > > > -- > > > > I'm available for contract & employment work, see: > > > > https://spindle.queued.net/~dilinger/resume-tech.pdf > > > > > > Nice! So it is exactly same as API of keyboard backlight tokens. Thanks. > > > > > > In dell_battery_write_req function you can drop second "val" argument > > > and replace it by token->value. So the dell_fill_request call in that > > > function would look like: > > > > > > dell_fill_request(&buffer, token->location, token->value, 0, 0); > > > > > > Well, except that we use dell_battery_write_req for writing the charge > > start/end values as well (in dell_battery_custom_set). Those can't be > > obtained from token->value. > > > > We could have two separate functions for that, or set 'val' to a > > sentinel value (0) that, if detected, we set val=token->value. I'm > > still not really understanding the point, though. > > I think that two separate functions would be needed. One which set > battery mode (enum) and which set custom thresholds. > > > > > > > And then you can mimic the usage as it is done in keyboard backlight > > > functions (kbd_get_first_active_token_bit). > > > > > > If you do not know what I mean then later (today or tomorrow) I can > > > write code example of the functionality. > > > > Sorry, I still don't understand what the goal is here. Is the goal to > > not pull from a random location to determine the current charging mode? > > Is the goal to determine what charging modes are currently supported > > (and if so, I don't see how)? Is the goal to avoid having the kernel > > hardcode a list of enums that the BIOS might have different values > > for? Is the goal to merge the keyboard backlight and battery setting > > functions? > > Avoid having the kernel hardcoded values for enums which SMBIOS > provides. Future (or maybe also older) modes may have different enum > values. So we should use what SMBIOS provides to us. > *nod*, that makes sense. > Also to determinate which charging modes are supported by the current HW > configuration. If BIOS does not support some mode or does not allow to > set some mode, kernel should not export this as supported option. Alright, I'll see what I can do about this. Unfortunately, with no real way to test it, it's very theoretical for me. For what it's worth, I've checked on 6 separate laptops (all Latitudes, ranging from an e7240 to a 5310), and they all have the exact same (primary) battery charging options in the BIOS; nothing greyed out. That includes a newer one that's missing a (non-removable) battery, and the e7240 with removable battery. Even without batteries, the BIOS settings didn't change. The only real change was the BIOS in the e7240, which had a separate section for slice batteries. I don't doubt that the long life batteries have a way to grey out the ExpressCharge option, but it's unclear if querying ExpressCharge when a long life battery is installed actually returns an error, or has an empty value in its SMBIOS token value, or returns empty output (like kbd_get_token_bit checks for), or what. And for the older removable batteries, the BIOS probably had to be smart enough to handle the case where you have a normal battery installed, set the BIOS to ExpressCharge, and then swap in a long life battery. I would suspect the BIOS can handle it, but probably still keeps the current value set to ExpressCharge and just quietly does a Standard charge? The only way to grey out any of the primary battery options is to select "Enable Advanced Battery Charge Mode", which is a separate thing that allows setting ExpressCharge during workdays and then automatically disabling it at night. When that option is set and the primary battery charge options are greyed out, the dump from the kernel driver looks the same. > > If you do not see how to do it, please give me some time, I will send > you an example. Going to look at it right now. > > Merging keyboard backlight and battery code is bonus, not required. > But I thought that it would be easier to build a new code from common > blocks.
On Monday 22 July 2024 20:41:32 Pali Rohár wrote: > On Monday 22 July 2024 14:34:32 Andres Salomon wrote: > > On Mon, 22 Jul 2024 09:18:45 +0200 > > Pali Rohár <pali@kernel.org> wrote: > > > > > On Sunday 21 July 2024 19:58:51 Andres Salomon wrote: > > > > On Mon, 22 Jul 2024 01:40:37 +0200 > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote: > > > > > > On Sun, 21 Jul 2024 11:02:38 +0200 > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > > > > > > > On Sat, 20 Jul 2024 11:55:07 +0200 > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > > > > > > > Thanks for the quick feedback! Responses below. > > > > > > > > > > > > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > > > > > > > +{ > > > > > > > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > > > > > > > + > > > > > > > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > > > > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > > > > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > > > > > > > value from Dell's API? > > > > > > > > > > > > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > > > > > > > work, though. That is, current_mode ends up holding the correct value > > > > > > > > > > based on what was previously set, even if the charging mode is set from > > > > > > > > > > the BIOS. > > > > > > > > > > > > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > > > > > > > it appears to loop through every charging mode to check if its active. > > > > > > > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > > > > > > > tests. > > > > > > > > > > > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > > > > > > > this type scan. If I remember correctly, for every keyboard backlight > > > > > > > > > token we just know the boolean value - if the token is set or not. > > > > > > > > > > > > > > > > > > It would really nice to see what (raw) value is returned by the > > > > > > > > > dell_battery_read_req(token) function for every battery token and for > > > > > > > > > every initial state. > > > > > > > > > > > > > > > > I checked this. The BIOS sets the mode value in every related token > > > > > > > > location. I'm still not really sure what libsmbios is doing, but the > > > > > > > > kernel code seems to arbitrarily choose one of the token locations > > > > > > > > to read from. This makes sense to me now. > > > > > > > > > > > > > > > > In the BIOS when I set the mode to "ExpressCharge", > > > > > > > > this what I pulled for each token location: > > > > > > > > > > > > > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > > > > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > > > > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > > > > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > > > > > > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > > > > > > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > > > > > > > When I set it from linux as well, it changed all location values. > > > > > > > > > > > > > > Interesting... Anyway, I still think that the API could be similar to > > > > > > > what is used in keyboard backlight. > > > > > > > > > > > > > > Could you please dump all information about each token? They are in > > > > > > > struct calling_interface_token returned by dell_smbios_find_token. > > > > > > > > > > > > > > I'm interesting in tokenID, location and value. > > > > > > > > > > > > > > Ideally to compare what is in token->value and then in buffer.output[1] > > > > > > > (in case dell_send_request does not fail). > > > > > > > > > > > > > > > > > > Alright, here's what I see: > > > > > > > > > > > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > > > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 > > > > > > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > > > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 > > > > > > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > > > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 > > > > > > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > > > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 > > > > > > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > > > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 > > > > > > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > > > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > > > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > > > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > > > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > > > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN? > > > > > > > > Here's Express: > > > > > > > > [ 5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > [ 5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > [ 5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > [ 5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > [ 5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > [ 5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > [ 5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > [ 5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2 > > > > [ 5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > [ 5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > [ 5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > [ 5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > [ 5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > [ 5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > [ 5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > [ 5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > [ 5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > And here's Adaptive: > > > > > > > > [ 5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > [ 5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4 > > > > [ 5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > [ 5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4 > > > > [ 5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > [ 5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4 > > > > [ 5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > [ 5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4 > > > > [ 5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > [ 5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > [ 5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4 > > > > [ 5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > [ 5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > [ 5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > [ 5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > [ 5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > [ 5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > > > > > > > -- > > > > I'm available for contract & employment work, see: > > > > https://spindle.queued.net/~dilinger/resume-tech.pdf > > > > > > Nice! So it is exactly same as API of keyboard backlight tokens. Thanks. > > > > > > In dell_battery_write_req function you can drop second "val" argument > > > and replace it by token->value. So the dell_fill_request call in that > > > function would look like: > > > > > > dell_fill_request(&buffer, token->location, token->value, 0, 0); > > > > > > Well, except that we use dell_battery_write_req for writing the charge > > start/end values as well (in dell_battery_custom_set). Those can't be > > obtained from token->value. > > > > We could have two separate functions for that, or set 'val' to a > > sentinel value (0) that, if detected, we set val=token->value. I'm > > still not really understanding the point, though. > > I think that two separate functions would be needed. One which set > battery mode (enum) and which set custom thresholds. > > > > > > > And then you can mimic the usage as it is done in keyboard backlight > > > functions (kbd_get_first_active_token_bit). > > > > > > If you do not know what I mean then later (today or tomorrow) I can > > > write code example of the functionality. > > > > Sorry, I still don't understand what the goal is here. Is the goal to > > not pull from a random location to determine the current charging mode? > > Is the goal to determine what charging modes are currently supported > > (and if so, I don't see how)? Is the goal to avoid having the kernel > > hardcode a list of enums that the BIOS might have different values > > for? Is the goal to merge the keyboard backlight and battery setting > > functions? > > Avoid having the kernel hardcoded values for enums which SMBIOS > provides. Future (or maybe also older) modes may have different enum > values. So we should use what SMBIOS provides to us. > > Also to determinate which charging modes are supported by the current HW > configuration. If BIOS does not support some mode or does not allow to > set some mode, kernel should not export this as supported option. > > If you do not see how to do it, please give me some time, I will send > you an example. Going to look at it right now. > > Merging keyboard backlight and battery code is bonus, not required. > But I thought that it would be easier to build a new code from common > blocks. Here is very quick & hacky example of what I mean (completely untested): --- dell-laptop.c +++ dell-laptop.c @@ -353,6 +353,105 @@ static const struct dmi_system_id dell_q { } }; +static int dell_read_token_value(u16 tokenid, u32 *value) +{ + struct calling_interface_buffer buffer; + struct calling_interface_token *token; + int ret; + + token = dell_smbios_find_token(tokenid); + if (!token) + return -ENODEV; + + dell_fill_request(&buffer, token->location, 0, 0, 0); + ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); + if (ret) + return ret; + + *value = buffer.output[1]; + return 0; +} + +static int dell_write_token_value(u16 tokenid, u32 value) +{ + struct calling_interface_buffer buffer; + struct calling_interface_token *token; + + token = dell_smbios_find_token(type); + if (!token) + return -ENODEV; + + dell_fill_request(&buffer, token->location, value, 0, 0); + return dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); +} + +static int dell_is_enum_token_active(u16 tokenid) +{ + struct calling_interface_buffer buffer; + struct calling_interface_token *token; + int ret; + + token = dell_smbios_find_token(tokenid); + if (!token) + return -EINVAL; + + if (token->value == (u16)-1) + return -EINVAL; + + dell_fill_request(&buffer, token->location, 0, 0, 0); + ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); + if (ret) + return ret; + + return (buffer.output[1] == token->value); +} + +static int dell_activate_enum_token(u16 tokenid) +{ + struct calling_interface_buffer buffer; + struct calling_interface_token *token; + int ret; + + token = dell_smbios_find_token(tokenid); + if (!token) + return -EINVAL; + + if (token->value == (u16)-1) + return -EINVAL; + + dell_fill_request(&buffer, token->location, token->value, 0, 0); + return dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); +} + +static u32 dell_get_supported_enum_tokens(const u16 *tokenids, u32 count) +{ + u32 supported_mask = 0; + u32 i; + + for (i = 0; i < count; i++) { + if (dell_smbios_find_token(tokenids[i])) + supported_mask |= BIT(i); + } + + return supported_mask; +} + +static int dell_get_active_enum_token(const u16 *tokenids, u32 count, u32 supported_mask) +{ + int ret; + u32 i; + + for (i = 0; i < count; i++) { + if (!(supported_mask & BIT(i))) + continue; + ret = dell_is_enum_token_active(tokenids[i]); + if (ret == 1) + return i; + } + + return -EINVAL; +} + /* * Derived from information in smbios-wireless-ctl: * @@ -2183,6 +2282,144 @@ static struct led_classdev mute_led_cdev .default_trigger = "audio-mute", }; +static const u16 battery_mode_tokens[] = { + BAT_STANDARD_MODE_TOKEN, + BAT_EXPRESS_MODE_TOKEN, + BAT_PRI_AC_MODE_TOKEN, + BAT_ADAPTIVE_MODE_TOKEN, + BAT_CUSTOM_MODE_TOKEN, +}; + +static const char * const battery_mode_names[] = { + "standard", + "express", + "primarily_ac", + "adaptive", + "custom", +}; + +static u32 battery_mode_token_mask; + +static int dell_battery_read_custom_charge(u16 token) +{ + u32 value; + int ret; + + ret = dell_read_token_value(token, &value); + if (ret) + return ret; + if (value > 100) + return -EINVAL; + return value; +} + +#define CHARGE_START_MIN 50 +#define CHARGE_START_MAX 95 +#define CHARGE_END_MIN 55 +#define CHARGE_END_MAX 100 +#define CHARGE_MIN_DIFF 5 + +static int dell_battery_set_custom_charge_start(int val) +{ + int end; + + if (val < CHARGE_START_MIN) + val = CHARGE_START_MIN; + else if (val > CHARGE_START_MAX) + val = CHARGE_START_MAX; + + end = dell_battery_get_custom_charge(BAT_CUSTOM_CHARGE_END); + if (end < 0) + return end; + + if (end - val < CHARGE_MIN_DIFF) + val = end - CHARGE_MIN_DIFF; + + return dell_write_token_value(BAT_CUSTOM_CHARGE_START, val); +} + +static int dell_battery_set_custom_charge_end(int val) +{ + int start; + + if (val < CHARGE_END_MIN) + val = CHARGE_END_MIN; + else if (val > CHARGE_END_MAX) + val = CHARGE_END_MAX; + + start = dell_battery_get_custom_charge(BAT_CUSTOM_CHARGE_START); + if (start < 0) + return start; + + if (val - start < CHARGE_MIN_DIFF) + val = start + CHARGE_MIN_DIFF; + + return dell_write_token_value(BAT_CUSTOM_CHARGE_END, val); +} + +static ssize_t charge_type_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int active; + ssize_t count; + + active = dell_get_active_enum_token(battery_mode_tokens, ARRAY_SIZE(battery_mode_tokens), battery_mode_token_mask); + if (active < 0) + return ret; + + for (count = 0, i = 0; i < ARRAY_SIZE(battery_mode_names); i++) { + if (!(BIT(i) & battery_mode_token_mask)) + continue; + count += sysfs_emit_at(buf, count, i == active ? "[%s] " : "%s ", battery_mode_names[mode]); + } + + /* convert the last space to a newline */ + /* battery_mode_names is non-empty and battery_mode_token_mask is non-zero, so count is also non-zero */ + count--; + count += sysfs_emit_at(buf, count, "\n"); + + return count; +} + +static ssize_t charge_type_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + size_t i; + int ret; + + for (i = 0; i < ARRAY_SIZE(battery_mode_names); i++) { + if (sysfs_streq(battery_mode_names[i], buf)) + break; + } + + if (i >= ARRAY_SIZE(battery_mode_names)) + return -EINVAL; + + if (!(BIT(i) & battery_mode_token_mask)) + return -EINVAL; + + ret = dell_activate_enum_token(battery_mode_tokens[i]); + if (ret) + return ret; + + return size; +} + +static void __init dell_battery_init(struct device *dev) +{ + battery_mode_token_mask = dell_get_supported_enum_tokens(battery_mode_tokens, ARRAY_SIZE(battery_mode_tokens)); + if (battery_mode_token_mask != 0) + battery_hook_register(&dell_battery_hook); +} + +static void __exit dell_battery_exit(void) +{ + if (battery_mode_token_mask != 0) + battery_hook_unregister(&dell_battery_hook); +} + static int __init dell_init(void) { struct calling_interface_token *token;
On Mon, 22 Jul 2024 22:25:04 +0200 Pali Rohár <pali@kernel.org> wrote: > On Monday 22 July 2024 20:41:32 Pali Rohár wrote: > > On Monday 22 July 2024 14:34:32 Andres Salomon wrote: > > > On Mon, 22 Jul 2024 09:18:45 +0200 > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > On Sunday 21 July 2024 19:58:51 Andres Salomon wrote: > > > > > On Mon, 22 Jul 2024 01:40:37 +0200 > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote: > > > > > > > On Sun, 21 Jul 2024 11:02:38 +0200 > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > > > > > > > > On Sat, 20 Jul 2024 11:55:07 +0200 > > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > > > > > > > > Thanks for the quick feedback! Responses below. > > > > > > > > > > > > > > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > > > > > > > > +{ > > > > > > > > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > > > > > > > > + > > > > > > > > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > > > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > > > > > > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > > > > > > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > > > > > > > > value from Dell's API? > > > > > > > > > > > > > > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > > > > > > > > work, though. That is, current_mode ends up holding the correct value > > > > > > > > > > > based on what was previously set, even if the charging mode is set from > > > > > > > > > > > the BIOS. > > > > > > > > > > > > > > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > > > > > > > > it appears to loop through every charging mode to check if its active. > > > > > > > > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > > > > > > > > tests. > > > > > > > > > > > > > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > > > > > > > > this type scan. If I remember correctly, for every keyboard backlight > > > > > > > > > > token we just know the boolean value - if the token is set or not. > > > > > > > > > > > > > > > > > > > > It would really nice to see what (raw) value is returned by the > > > > > > > > > > dell_battery_read_req(token) function for every battery token and for > > > > > > > > > > every initial state. > > > > > > > > > > > > > > > > > > I checked this. The BIOS sets the mode value in every related token > > > > > > > > > location. I'm still not really sure what libsmbios is doing, but the > > > > > > > > > kernel code seems to arbitrarily choose one of the token locations > > > > > > > > > to read from. This makes sense to me now. > > > > > > > > > > > > > > > > > > In the BIOS when I set the mode to "ExpressCharge", > > > > > > > > > this what I pulled for each token location: > > > > > > > > > > > > > > > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > > > > > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > > > > > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > > > > > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > > > > > > > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > > > > > > > > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > > > > > > > > When I set it from linux as well, it changed all location values. > > > > > > > > > > > > > > > > Interesting... Anyway, I still think that the API could be similar to > > > > > > > > what is used in keyboard backlight. > > > > > > > > > > > > > > > > Could you please dump all information about each token? They are in > > > > > > > > struct calling_interface_token returned by dell_smbios_find_token. > > > > > > > > > > > > > > > > I'm interesting in tokenID, location and value. > > > > > > > > > > > > > > > > Ideally to compare what is in token->value and then in buffer.output[1] > > > > > > > > (in case dell_send_request does not fail). > > > > > > > > > > > > > > > > > > > > > Alright, here's what I see: > > > > > > > > > > > > > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > > > > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 > > > > > > > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > > > > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 > > > > > > > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > > > > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 > > > > > > > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > > > > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 > > > > > > > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > > > > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 > > > > > > > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > > > > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > > > > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > > > > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > > > > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > > > > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN? > > > > > > > > > > Here's Express: > > > > > > > > > > [ 5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > > [ 5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > [ 5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > > [ 5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > > [ 5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > [ 5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > > [ 5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > > [ 5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > [ 5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > > [ 5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > > [ 5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > [ 5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2 > > > > > [ 5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > > [ 5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > [ 5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > [ 5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > > [ 5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > > [ 5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > > [ 5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > > [ 5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > > [ 5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > And here's Adaptive: > > > > > > > > > > [ 5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > > [ 5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > [ 5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4 > > > > > [ 5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > > [ 5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > [ 5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4 > > > > > [ 5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > > [ 5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > [ 5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4 > > > > > [ 5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > > [ 5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > [ 5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4 > > > > > [ 5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > > [ 5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > [ 5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4 > > > > > [ 5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > > [ 5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > > [ 5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > > [ 5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > > [ 5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > > [ 5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > > > > > > > > > > > -- > > > > > I'm available for contract & employment work, see: > > > > > https://spindle.queued.net/~dilinger/resume-tech.pdf > > > > > > > > Nice! So it is exactly same as API of keyboard backlight tokens. Thanks. > > > > > > > > In dell_battery_write_req function you can drop second "val" argument > > > > and replace it by token->value. So the dell_fill_request call in that > > > > function would look like: > > > > > > > > dell_fill_request(&buffer, token->location, token->value, 0, 0); > > > > > > > > > Well, except that we use dell_battery_write_req for writing the charge > > > start/end values as well (in dell_battery_custom_set). Those can't be > > > obtained from token->value. > > > > > > We could have two separate functions for that, or set 'val' to a > > > sentinel value (0) that, if detected, we set val=token->value. I'm > > > still not really understanding the point, though. > > > > I think that two separate functions would be needed. One which set > > battery mode (enum) and which set custom thresholds. > > > > > > > > > > And then you can mimic the usage as it is done in keyboard backlight > > > > functions (kbd_get_first_active_token_bit). > > > > > > > > If you do not know what I mean then later (today or tomorrow) I can > > > > write code example of the functionality. > > > > > > Sorry, I still don't understand what the goal is here. Is the goal to > > > not pull from a random location to determine the current charging mode? > > > Is the goal to determine what charging modes are currently supported > > > (and if so, I don't see how)? Is the goal to avoid having the kernel > > > hardcode a list of enums that the BIOS might have different values > > > for? Is the goal to merge the keyboard backlight and battery setting > > > functions? > > > > Avoid having the kernel hardcoded values for enums which SMBIOS > > provides. Future (or maybe also older) modes may have different enum > > values. So we should use what SMBIOS provides to us. > > > > Also to determinate which charging modes are supported by the current HW > > configuration. If BIOS does not support some mode or does not allow to > > set some mode, kernel should not export this as supported option. > > > > If you do not see how to do it, please give me some time, I will send > > you an example. Going to look at it right now. > > > > Merging keyboard backlight and battery code is bonus, not required. > > But I thought that it would be easier to build a new code from common > > blocks. > > Here is very quick & hacky example of what I mean (completely untested): > > --- dell-laptop.c > +++ dell-laptop.c > @@ -353,6 +353,105 @@ static const struct dmi_system_id dell_q > { } > }; > > +static int dell_read_token_value(u16 tokenid, u32 *value) > +{ > + struct calling_interface_buffer buffer; > + struct calling_interface_token *token; > + int ret; > + > + token = dell_smbios_find_token(tokenid); > + if (!token) > + return -ENODEV; > + > + dell_fill_request(&buffer, token->location, 0, 0, 0); > + ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); > + if (ret) > + return ret; > + > + *value = buffer.output[1]; > + return 0; > +} > + > +static int dell_write_token_value(u16 tokenid, u32 value) > +{ > + struct calling_interface_buffer buffer; > + struct calling_interface_token *token; > + > + token = dell_smbios_find_token(type); > + if (!token) > + return -ENODEV; > + > + dell_fill_request(&buffer, token->location, value, 0, 0); > + return dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); > +} > + I did things a bit differently here. I created a function dell_send_request_by_token_loc() that is available to be reused by all the code that sends packets to token->location. This is available for kbd_{g,s}et_token_bit(), micmute_led_set(), and mute_led_set() to use, but I'd rather do that in a separate patch. +static int dell_send_request_by_token_loc(struct calling_interface_buffer *buffer, + u16 class, u16 select, int type, + int val) +{ + struct calling_interface_token *token; + + token = dell_smbios_find_token(type); + if (!token) + return -ENODEV; + + if (val <= 0) + val = token->value; + + dell_fill_request(buffer, token->location, val, 0, 0); + return dell_send_request(buffer, class, select); +} + +static int dell_battery_set_mode(const int type) +{ + struct calling_interface_buffer buffer; + + /* 0 means use the value from the token */ + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, + SELECT_TOKEN_STD, type, 0); +} + +static int dell_battery_read(const int type) +{ + struct calling_interface_buffer buffer; + int err; + + err = dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_READ, + SELECT_TOKEN_STD, type, 0); + if (err) + return err; + + return buffer.output[1]; +} > +static int dell_is_enum_token_active(u16 tokenid) > +{ > + struct calling_interface_buffer buffer; > + struct calling_interface_token *token; > + int ret; > + > + token = dell_smbios_find_token(tokenid); > + if (!token) > + return -EINVAL; > + > + if (token->value == (u16)-1) > + return -EINVAL; > + > + dell_fill_request(&buffer, token->location, 0, 0, 0); > + ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); > + if (ret) > + return ret; > + > + return (buffer.output[1] == token->value); > +} > + Okay, I incorporated this, but using the above helper functions: +static bool dell_battery_mode_is_active(const int type) +{ + struct calling_interface_token *token; + + token = dell_smbios_find_token(type); + if (!token) + return false; + + return token->value == dell_battery_read(type); +} It will look up the token twice, but this only happens once on it (and I'm realizing now that it can be marked __init). > +static int dell_activate_enum_token(u16 tokenid) > +{ > + struct calling_interface_buffer buffer; > + struct calling_interface_token *token; > + int ret; > + > + token = dell_smbios_find_token(tokenid); > + if (!token) > + return -EINVAL; > + > + if (token->value == (u16)-1) > + return -EINVAL; > + > + dell_fill_request(&buffer, token->location, token->value, 0, 0); > + return dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); > +} > + > +static u32 dell_get_supported_enum_tokens(const u16 *tokenids, u32 count) > +{ > + u32 supported_mask = 0; > + u32 i; > + > + for (i = 0; i < count; i++) { > + if (dell_smbios_find_token(tokenids[i])) > + supported_mask |= BIT(i); > + } > + > + return supported_mask; > +} > + > +static int dell_get_active_enum_token(const u16 *tokenids, u32 count, u32 supported_mask) > +{ > + int ret; > + u32 i; > + > + for (i = 0; i < count; i++) { > + if (!(supported_mask & BIT(i))) > + continue; > + ret = dell_is_enum_token_active(tokenids[i]); > + if (ret == 1) > + return i; > + } > + > + return -EINVAL; > +} Thanks, I incorporated and tested this code; though it only runs once during init, because I'm keeping the battery_cur_mode variable (see next section). +static u32 __init battery_get_supported_modes(void) +{ + u32 modes = 0; + int i; + + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { + if (dell_smbios_find_token(battery_modes[i].token)) + modes |= BIT(i); + } + + return modes; +} + +static int __init dell_battery_find_charging_mode(void) +{ + int i; + + battery_supported_modes = battery_get_supported_modes(); + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { + if (!(battery_supported_modes & BIT(i))) + continue; + if (dell_battery_mode_is_active(battery_modes[i].token)) + return battery_modes[i].token; + } + + return 0; +} > + > /* > * Derived from information in smbios-wireless-ctl: > * > @@ -2183,6 +2282,144 @@ static struct led_classdev mute_led_cdev > .default_trigger = "audio-mute", > }; > > +static const u16 battery_mode_tokens[] = { > + BAT_STANDARD_MODE_TOKEN, > + BAT_EXPRESS_MODE_TOKEN, > + BAT_PRI_AC_MODE_TOKEN, > + BAT_ADAPTIVE_MODE_TOKEN, > + BAT_CUSTOM_MODE_TOKEN, > +}; > + > +static const char * const battery_mode_names[] = { > + "standard", > + "express", > + "primarily_ac", > + "adaptive", > + "custom", > +}; > + > +static u32 battery_mode_token_mask; > + I merged the token and name lists. Also, is there any particular reason you dropped the current mode, preferring to read it from SMBIOS every time in the _show() callback? +static const struct { + int token; + const char *label; +} battery_modes[] = { + { BAT_STANDARD_MODE_TOKEN, "standard" }, + { BAT_EXPRESS_MODE_TOKEN, "express" }, + { BAT_PRI_AC_MODE_TOKEN, "primarily_ac" }, + { BAT_ADAPTIVE_MODE_TOKEN, "adaptive" }, + { BAT_CUSTOM_MODE_TOKEN, "custom" }, +}; +static u32 battery_supported_modes; +static int battery_cur_mode; + > +static int dell_battery_read_custom_charge(u16 token) > +{ > + u32 value; > + int ret; > + > + ret = dell_read_token_value(token, &value); > + if (ret) > + return ret; > + if (value > 100) > + return -EINVAL; > + return value; > +} > + > +#define CHARGE_START_MIN 50 > +#define CHARGE_START_MAX 95 > +#define CHARGE_END_MIN 55 > +#define CHARGE_END_MAX 100 > +#define CHARGE_MIN_DIFF 5 > + > +static int dell_battery_set_custom_charge_start(int val) > +{ > + int end; > + > + if (val < CHARGE_START_MIN) > + val = CHARGE_START_MIN; > + else if (val > CHARGE_START_MAX) > + val = CHARGE_START_MAX; > + > + end = dell_battery_get_custom_charge(BAT_CUSTOM_CHARGE_END); > + if (end < 0) > + return end; > + > + if (end - val < CHARGE_MIN_DIFF) > + val = end - CHARGE_MIN_DIFF; > + > + return dell_write_token_value(BAT_CUSTOM_CHARGE_START, val); > +} > + > +static int dell_battery_set_custom_charge_end(int val) > +{ > + int start; > + > + if (val < CHARGE_END_MIN) > + val = CHARGE_END_MIN; > + else if (val > CHARGE_END_MAX) > + val = CHARGE_END_MAX; > + > + start = dell_battery_get_custom_charge(BAT_CUSTOM_CHARGE_START); > + if (start < 0) > + return start; > + > + if (val - start < CHARGE_MIN_DIFF) > + val = start + CHARGE_MIN_DIFF; > + > + return dell_write_token_value(BAT_CUSTOM_CHARGE_END, val); > +} > + Good call, I split apart the start/end functions. It was originally much smaller and grew over time. :) > +static ssize_t charge_type_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int active; > + ssize_t count; > + > + active = dell_get_active_enum_token(battery_mode_tokens, ARRAY_SIZE(battery_mode_tokens), battery_mode_token_mask); > + if (active < 0) > + return ret; > + > + for (count = 0, i = 0; i < ARRAY_SIZE(battery_mode_names); i++) { > + if (!(BIT(i) & battery_mode_token_mask)) > + continue; > + count += sysfs_emit_at(buf, count, i == active ? "[%s] " : "%s ", battery_mode_names[mode]); > + } > + > + /* convert the last space to a newline */ > + /* battery_mode_names is non-empty and battery_mode_token_mask is non-zero, so count is also non-zero */ > + count--; > + count += sysfs_emit_at(buf, count, "\n"); > + > + return count; > +} > + Again, I personally prefer keeping the currently active charging mode around as a variable: +static ssize_t charge_type_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + ssize_t count = 0; + int i; + + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { + if (!(battery_supported_modes & BIT(i))) + continue; + + count += sysfs_emit_at(buf, count, + battery_modes[i].token == battery_cur_mode ? "[%s] " : "%s ", + battery_modes[i].label); + } + + /* convert the last space to a newline */ + if (count > 0) + count--; + count += sysfs_emit_at(buf, count, "\n"); + + return count; +} > +static ssize_t charge_type_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + size_t i; > + int ret; > + > + for (i = 0; i < ARRAY_SIZE(battery_mode_names); i++) { > + if (sysfs_streq(battery_mode_names[i], buf)) > + break; > + } > + > + if (i >= ARRAY_SIZE(battery_mode_names)) > + return -EINVAL; > + > + if (!(BIT(i) & battery_mode_token_mask)) > + return -EINVAL; > + > + ret = dell_activate_enum_token(battery_mode_tokens[i]); > + if (ret) > + return ret; > + > + return size; > +} > + > +static void __init dell_battery_init(struct device *dev) > +{ > + battery_mode_token_mask = dell_get_supported_enum_tokens(battery_mode_tokens, ARRAY_SIZE(battery_mode_tokens)); > + if (battery_mode_token_mask != 0) > + battery_hook_register(&dell_battery_hook); > +} > + > +static void __exit dell_battery_exit(void) > +{ > + if (battery_mode_token_mask != 0) > + battery_hook_unregister(&dell_battery_hook); > +} > + > static int __init dell_init(void) > { > struct calling_interface_token *token; Let me know your thoughts, and I can send a V2 patch if you're okay with the code snippets I sent.
On Tuesday 23 July 2024 00:36:10 Andres Salomon wrote: > On Mon, 22 Jul 2024 22:25:04 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > On Monday 22 July 2024 20:41:32 Pali Rohár wrote: > > > On Monday 22 July 2024 14:34:32 Andres Salomon wrote: > > > > On Mon, 22 Jul 2024 09:18:45 +0200 > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > On Sunday 21 July 2024 19:58:51 Andres Salomon wrote: > > > > > > On Mon, 22 Jul 2024 01:40:37 +0200 > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote: > > > > > > > > On Sun, 21 Jul 2024 11:02:38 +0200 > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote: > > > > > > > > > > On Sat, 20 Jul 2024 11:55:07 +0200 > > > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > > > > > > > > > > > > Thanks for the quick feedback! Responses below. > > > > > > > > > > > > > > > > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200 > > > > > > > > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +static void __init dell_battery_init(struct device *dev) > > > > > > > > > > > > > > +{ > > > > > > > > > > > > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > > > > > > > > > > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > > > > > > > > > > > > > > > > > > > > > > > I quite do not understand how is this code suppose to work. > > > > > > > > > > > > > > > > > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > > > > > > > > > > > > value from Dell's API? > > > > > > > > > > > > > > > > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does > > > > > > > > > > > > work, though. That is, current_mode ends up holding the correct value > > > > > > > > > > > > based on what was previously set, even if the charging mode is set from > > > > > > > > > > > > the BIOS. > > > > > > > > > > > > > > > > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and > > > > > > > > > > > > it appears to loop through every charging mode to check if its active. > > > > > > > > > > > > I'm not really sure that makes much more sense, so I'll try some more > > > > > > > > > > > > tests. > > > > > > > > > > > > > > > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also > > > > > > > > > > > this type scan. If I remember correctly, for every keyboard backlight > > > > > > > > > > > token we just know the boolean value - if the token is set or not. > > > > > > > > > > > > > > > > > > > > > > It would really nice to see what (raw) value is returned by the > > > > > > > > > > > dell_battery_read_req(token) function for every battery token and for > > > > > > > > > > > every initial state. > > > > > > > > > > > > > > > > > > > > I checked this. The BIOS sets the mode value in every related token > > > > > > > > > > location. I'm still not really sure what libsmbios is doing, but the > > > > > > > > > > kernel code seems to arbitrarily choose one of the token locations > > > > > > > > > > to read from. This makes sense to me now. > > > > > > > > > > > > > > > > > > > > In the BIOS when I set the mode to "ExpressCharge", > > > > > > > > > > this what I pulled for each token location: > > > > > > > > > > > > > > > > > > > > [ 5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > > > > > > > [ 5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > > > > > > > [ 5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > > > > > > > [ 5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2 > > > > > > > > > > [ 5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > > > > > > > > > > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1'). > > > > > > > > > > When I set it from linux as well, it changed all location values. > > > > > > > > > > > > > > > > > > Interesting... Anyway, I still think that the API could be similar to > > > > > > > > > what is used in keyboard backlight. > > > > > > > > > > > > > > > > > > Could you please dump all information about each token? They are in > > > > > > > > > struct calling_interface_token returned by dell_smbios_find_token. > > > > > > > > > > > > > > > > > > I'm interesting in tokenID, location and value. > > > > > > > > > > > > > > > > > > Ideally to compare what is in token->value and then in buffer.output[1] > > > > > > > > > (in case dell_send_request does not fail). > > > > > > > > > > > > > > > > > > > > > > > > Alright, here's what I see: > > > > > > > > > > > > > > > > [ 5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > > > > > [ 5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > > [ 5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3 > > > > > > > > [ 5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > > > > > [ 5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > > [ 5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3 > > > > > > > > [ 5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > > > > > [ 5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > > [ 5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3 > > > > > > > > [ 5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > > > > > [ 5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > > [ 5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3 > > > > > > > > [ 5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > > > > > [ 5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3 > > > > > > > > [ 5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3 > > > > > > > > [ 5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > > > > > [ 5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > > > > > [ 5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > > > > > [ 5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > > > > > [ 5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > > > > > [ 5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > > > > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN? > > > > > > > > > > > > Here's Express: > > > > > > > > > > > > [ 5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > > > [ 5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > > [ 5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2 > > > > > > [ 5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > > > [ 5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > > [ 5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2 > > > > > > [ 5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > > > [ 5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > > [ 5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2 > > > > > > [ 5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > > > [ 5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > > [ 5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2 > > > > > > [ 5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > > > [ 5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2 > > > > > > [ 5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2 > > > > > > [ 5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > > > [ 5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > > > [ 5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > > > [ 5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > > > [ 5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > > > [ 5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > > > And here's Adaptive: > > > > > > > > > > > > [ 5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5 > > > > > > [ 5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > > [ 5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4 > > > > > > [ 5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3 > > > > > > [ 5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > > [ 5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4 > > > > > > [ 5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4 > > > > > > [ 5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > > [ 5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4 > > > > > > [ 5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1 > > > > > > [ 5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > > [ 5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4 > > > > > > [ 5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2 > > > > > > [ 5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4 > > > > > > [ 5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4 > > > > > > [ 5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535 > > > > > > [ 5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65 > > > > > > [ 5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65 > > > > > > [ 5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535 > > > > > > [ 5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85 > > > > > > [ 5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85 > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > I'm available for contract & employment work, see: > > > > > > https://spindle.queued.net/~dilinger/resume-tech.pdf > > > > > > > > > > Nice! So it is exactly same as API of keyboard backlight tokens. Thanks. > > > > > > > > > > In dell_battery_write_req function you can drop second "val" argument > > > > > and replace it by token->value. So the dell_fill_request call in that > > > > > function would look like: > > > > > > > > > > dell_fill_request(&buffer, token->location, token->value, 0, 0); > > > > > > > > > > > > Well, except that we use dell_battery_write_req for writing the charge > > > > start/end values as well (in dell_battery_custom_set). Those can't be > > > > obtained from token->value. > > > > > > > > We could have two separate functions for that, or set 'val' to a > > > > sentinel value (0) that, if detected, we set val=token->value. I'm > > > > still not really understanding the point, though. > > > > > > I think that two separate functions would be needed. One which set > > > battery mode (enum) and which set custom thresholds. > > > > > > > > > > > > > And then you can mimic the usage as it is done in keyboard backlight > > > > > functions (kbd_get_first_active_token_bit). > > > > > > > > > > If you do not know what I mean then later (today or tomorrow) I can > > > > > write code example of the functionality. > > > > > > > > Sorry, I still don't understand what the goal is here. Is the goal to > > > > not pull from a random location to determine the current charging mode? > > > > Is the goal to determine what charging modes are currently supported > > > > (and if so, I don't see how)? Is the goal to avoid having the kernel > > > > hardcode a list of enums that the BIOS might have different values > > > > for? Is the goal to merge the keyboard backlight and battery setting > > > > functions? > > > > > > Avoid having the kernel hardcoded values for enums which SMBIOS > > > provides. Future (or maybe also older) modes may have different enum > > > values. So we should use what SMBIOS provides to us. > > > > > > Also to determinate which charging modes are supported by the current HW > > > configuration. If BIOS does not support some mode or does not allow to > > > set some mode, kernel should not export this as supported option. > > > > > > If you do not see how to do it, please give me some time, I will send > > > you an example. Going to look at it right now. > > > > > > Merging keyboard backlight and battery code is bonus, not required. > > > But I thought that it would be easier to build a new code from common > > > blocks. > > > > Here is very quick & hacky example of what I mean (completely untested): > > > > --- dell-laptop.c > > +++ dell-laptop.c > > @@ -353,6 +353,105 @@ static const struct dmi_system_id dell_q > > { } > > }; > > > > +static int dell_read_token_value(u16 tokenid, u32 *value) > > +{ > > + struct calling_interface_buffer buffer; > > + struct calling_interface_token *token; > > + int ret; > > + > > + token = dell_smbios_find_token(tokenid); > > + if (!token) > > + return -ENODEV; > > + > > + dell_fill_request(&buffer, token->location, 0, 0, 0); > > + ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); > > + if (ret) > > + return ret; > > + > > + *value = buffer.output[1]; > > + return 0; > > +} > > + > > +static int dell_write_token_value(u16 tokenid, u32 value) > > +{ > > + struct calling_interface_buffer buffer; > > + struct calling_interface_token *token; > > + > > + token = dell_smbios_find_token(type); > > + if (!token) > > + return -ENODEV; > > + > > + dell_fill_request(&buffer, token->location, value, 0, 0); > > + return dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); > > +} > > + > > I did things a bit differently here. I created a function > dell_send_request_by_token_loc() that is available to be reused by all > the code that sends packets to token->location. This is available for > kbd_{g,s}et_token_bit(), micmute_led_set(), and mute_led_set() to use, > but I'd rather do that in a separate patch. Ok, nice thing. > +static int dell_send_request_by_token_loc(struct calling_interface_buffer *buffer, > + u16 class, u16 select, int type, > + int val) > +{ > + struct calling_interface_token *token; > + > + token = dell_smbios_find_token(type); > + if (!token) > + return -ENODEV; > + > + if (val <= 0) I would be rather very careful here. Zero value can be valid value for some future call. It would be better to pass -1 or something like that. And IIRC dell_fill_request take u32 as val. So maybe it would be better to change "int val" to "u32 val" and interpret (u32)-1 as enum value from token->value? > + val = token->value; > + > + dell_fill_request(buffer, token->location, val, 0, 0); > + return dell_send_request(buffer, class, select); > +} > + > +static int dell_battery_set_mode(const int type) > +{ > + struct calling_interface_buffer buffer; > + > + /* 0 means use the value from the token */ > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > + SELECT_TOKEN_STD, type, 0); > +} > + > +static int dell_battery_read(const int type) > +{ > + struct calling_interface_buffer buffer; > + int err; > + > + err = dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_READ, > + SELECT_TOKEN_STD, type, 0); > + if (err) > + return err; > + > + return buffer.output[1]; > +} > > > > > +static int dell_is_enum_token_active(u16 tokenid) > > +{ > > + struct calling_interface_buffer buffer; > > + struct calling_interface_token *token; > > + int ret; > > + > > + token = dell_smbios_find_token(tokenid); > > + if (!token) > > + return -EINVAL; > > + > > + if (token->value == (u16)-1) > > + return -EINVAL; > > + > > + dell_fill_request(&buffer, token->location, 0, 0, 0); > > + ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); > > + if (ret) > > + return ret; > > + > > + return (buffer.output[1] == token->value); > > +} > > + > > Okay, I incorporated this, but using the above helper functions: > > +static bool dell_battery_mode_is_active(const int type) > +{ > + struct calling_interface_token *token; > + > + token = dell_smbios_find_token(type); > + if (!token) > + return false; > + > + return token->value == dell_battery_read(type); > +} > > It will look up the token twice, but this only happens once on it > (and I'm realizing now that it can be marked __init). Ok. Anyway if you are touching kbd code, exactly same function would be required for it. > > > +static int dell_activate_enum_token(u16 tokenid) > > +{ > > + struct calling_interface_buffer buffer; > > + struct calling_interface_token *token; > > + int ret; > > + > > + token = dell_smbios_find_token(tokenid); > > + if (!token) > > + return -EINVAL; > > + > > + if (token->value == (u16)-1) > > + return -EINVAL; > > + > > + dell_fill_request(&buffer, token->location, token->value, 0, 0); > > + return dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); > > +} > > + > > +static u32 dell_get_supported_enum_tokens(const u16 *tokenids, u32 count) > > +{ > > + u32 supported_mask = 0; > > + u32 i; > > + > > + for (i = 0; i < count; i++) { > > + if (dell_smbios_find_token(tokenids[i])) > > + supported_mask |= BIT(i); > > + } > > + > > + return supported_mask; > > +} > > + > > +static int dell_get_active_enum_token(const u16 *tokenids, u32 count, u32 supported_mask) > > +{ > > + int ret; > > + u32 i; > > + > > + for (i = 0; i < count; i++) { > > + if (!(supported_mask & BIT(i))) > > + continue; > > + ret = dell_is_enum_token_active(tokenids[i]); > > + if (ret == 1) > > + return i; > > + } > > + > > + return -EINVAL; > > +} > > > Thanks, I incorporated and tested this code; though it only runs once > during init, because I'm keeping the battery_cur_mode variable (see > next section). > > +static u32 __init battery_get_supported_modes(void) > +{ > + u32 modes = 0; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { > + if (dell_smbios_find_token(battery_modes[i].token)) > + modes |= BIT(i); > + } > + > + return modes; > +} > + > +static int __init dell_battery_find_charging_mode(void) > +{ > + int i; > + > + battery_supported_modes = battery_get_supported_modes(); > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { > + if (!(battery_supported_modes & BIT(i))) > + continue; > + if (dell_battery_mode_is_active(battery_modes[i].token)) > + return battery_modes[i].token; > + } > + > + return 0; > +} > > > > > + > > /* > > * Derived from information in smbios-wireless-ctl: > > * > > @@ -2183,6 +2282,144 @@ static struct led_classdev mute_led_cdev > > .default_trigger = "audio-mute", > > }; > > > > +static const u16 battery_mode_tokens[] = { > > + BAT_STANDARD_MODE_TOKEN, > > + BAT_EXPRESS_MODE_TOKEN, > > + BAT_PRI_AC_MODE_TOKEN, > > + BAT_ADAPTIVE_MODE_TOKEN, > > + BAT_CUSTOM_MODE_TOKEN, > > +}; > > + > > +static const char * const battery_mode_names[] = { > > + "standard", > > + "express", > > + "primarily_ac", > > + "adaptive", > > + "custom", > > +}; > > + > > +static u32 battery_mode_token_mask; > > + > > I merged the token and name lists. Also, is there any particular reason > you dropped the current mode, preferring to read it from SMBIOS every time > in the _show() callback? Yes, there is one important reason: You can change current mode from userspace. We had this problem also with other functionality in driver. So caching is not a good idea. There are/were enterprise Dell CCTK tools for managing configuration of Dell machines. And usage of these tools can break kernel driver. > +static const struct { > + int token; > + const char *label; > +} battery_modes[] = { > + { BAT_STANDARD_MODE_TOKEN, "standard" }, > + { BAT_EXPRESS_MODE_TOKEN, "express" }, > + { BAT_PRI_AC_MODE_TOKEN, "primarily_ac" }, > + { BAT_ADAPTIVE_MODE_TOKEN, "adaptive" }, > + { BAT_CUSTOM_MODE_TOKEN, "custom" }, > +}; > +static u32 battery_supported_modes; > +static int battery_cur_mode; > + > > > > > +static int dell_battery_read_custom_charge(u16 token) > > +{ > > + u32 value; > > + int ret; > > + > > + ret = dell_read_token_value(token, &value); > > + if (ret) > > + return ret; > > + if (value > 100) > > + return -EINVAL; > > + return value; > > +} > > + > > +#define CHARGE_START_MIN 50 > > +#define CHARGE_START_MAX 95 > > +#define CHARGE_END_MIN 55 > > +#define CHARGE_END_MAX 100 > > +#define CHARGE_MIN_DIFF 5 > > + > > +static int dell_battery_set_custom_charge_start(int val) > > +{ > > + int end; > > + > > + if (val < CHARGE_START_MIN) > > + val = CHARGE_START_MIN; > > + else if (val > CHARGE_START_MAX) > > + val = CHARGE_START_MAX; > > + > > + end = dell_battery_get_custom_charge(BAT_CUSTOM_CHARGE_END); > > + if (end < 0) > > + return end; > > + > > + if (end - val < CHARGE_MIN_DIFF) > > + val = end - CHARGE_MIN_DIFF; > > + > > + return dell_write_token_value(BAT_CUSTOM_CHARGE_START, val); > > +} > > + > > +static int dell_battery_set_custom_charge_end(int val) > > +{ > > + int start; > > + > > + if (val < CHARGE_END_MIN) > > + val = CHARGE_END_MIN; > > + else if (val > CHARGE_END_MAX) > > + val = CHARGE_END_MAX; > > + > > + start = dell_battery_get_custom_charge(BAT_CUSTOM_CHARGE_START); > > + if (start < 0) > > + return start; > > + > > + if (val - start < CHARGE_MIN_DIFF) > > + val = start + CHARGE_MIN_DIFF; > > + > > + return dell_write_token_value(BAT_CUSTOM_CHARGE_END, val); > > +} > > + > > > Good call, I split apart the start/end functions. It was originally > much smaller and grew over time. :) > > > > +static ssize_t charge_type_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + int active; > > + ssize_t count; > > + > > + active = dell_get_active_enum_token(battery_mode_tokens, ARRAY_SIZE(battery_mode_tokens), battery_mode_token_mask); > > + if (active < 0) > > + return ret; > > + > > + for (count = 0, i = 0; i < ARRAY_SIZE(battery_mode_names); i++) { > > + if (!(BIT(i) & battery_mode_token_mask)) > > + continue; > > + count += sysfs_emit_at(buf, count, i == active ? "[%s] " : "%s ", battery_mode_names[mode]); > > + } > > + > > + /* convert the last space to a newline */ > > + /* battery_mode_names is non-empty and battery_mode_token_mask is non-zero, so count is also non-zero */ > > + count--; > > + count += sysfs_emit_at(buf, count, "\n"); > > + > > + return count; > > +} > > + > > Again, I personally prefer keeping the currently active charging mode > around as a variable: > > > +static ssize_t charge_type_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + ssize_t count = 0; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { > + if (!(battery_supported_modes & BIT(i))) > + continue; > + > + count += sysfs_emit_at(buf, count, > + battery_modes[i].token == battery_cur_mode ? "[%s] " : "%s ", > + battery_modes[i].label); > + } > + > + /* convert the last space to a newline */ > + if (count > 0) > + count--; > + count += sysfs_emit_at(buf, count, "\n"); > + > + return count; > +} > > > > +static ssize_t charge_type_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t size) > > +{ > > + size_t i; > > + int ret; > > + > > + for (i = 0; i < ARRAY_SIZE(battery_mode_names); i++) { > > + if (sysfs_streq(battery_mode_names[i], buf)) > > + break; > > + } > > + > > + if (i >= ARRAY_SIZE(battery_mode_names)) > > + return -EINVAL; > > + > > + if (!(BIT(i) & battery_mode_token_mask)) > > + return -EINVAL; > > + > > + ret = dell_activate_enum_token(battery_mode_tokens[i]); > > + if (ret) > > + return ret; > > + > > + return size; > > +} > > + > > +static void __init dell_battery_init(struct device *dev) > > +{ > > + battery_mode_token_mask = dell_get_supported_enum_tokens(battery_mode_tokens, ARRAY_SIZE(battery_mode_tokens)); > > + if (battery_mode_token_mask != 0) > > + battery_hook_register(&dell_battery_hook); > > +} > > + > > +static void __exit dell_battery_exit(void) > > +{ > > + if (battery_mode_token_mask != 0) > > + battery_hook_unregister(&dell_battery_hook); > > +} > > + > > static int __init dell_init(void) > > { > > struct calling_interface_token *token; > > > Let me know your thoughts, and I can send a V2 patch if you're okay > with the code snippets I sent. > > -- > I'm available for contract & employment work, see: > https://spindle.queued.net/~dilinger/resume-tech.pdf
Hi, On 7/20/24 7:22 AM, Andres Salomon wrote: > > The Dell BIOS allows you to set custom charging modes, which is useful > in particular for extending battery life. This adds support for tweaking > those various settings from Linux via sysfs knobs. One might, for > example, have their laptop plugged into power at their desk the vast > majority of the time and choose fairly aggressive battery-saving > settings (only charging once the battery drops to 50% and only charging > up to 80%). When leaving for a trip, they might want to switch those > settings to charge to 100% and charge any time power is available; > rebooting into the BIOS to change those settings is a hassle. > > For the Custom charging type mode, we reuse the common > charge_control_{start,end}_threshold sysfs power_supply entries. The > BIOS also has a bunch of other charging modes with varying levels of > usefulness, so this adds a new Dell-specific sysfs entry called > 'charge_type' that's documented in sysfs-class-power-dell (and looks a > lot like sysfs-class-power-wilco). > > This work is based on a patch by Perry Yuan <perry_yuan@dell.com> and > Limonciello Mario <Mario_Limonciello@Dell.com> submitted back in 2020: > https://lore.kernel.org/all/20200729065424.12851-1-Perry_Yuan@Dell.com/ > Both of their email addresses bounce, so I'm assuming they're no longer > with the company. I've reworked most of the patch to make it smaller and > cleaner. > > Signed-off-by: Andres Salomon <dilinger@queued.net> Thank you for working on this and it is great to see the discussion to improve the code between you and Pali going on. One concern which I have is that work is underway for both upower and GNOME to add support for /sys/class/power_supply/*/charge_[start|stop]_threshold to gnome-control-center. But if I understand things correctly then these limits will only be honored when the charging-type is set to custom. So we need to do something to avoid userspace exposing those controls when the charging-type is not custom. I think it would be best to not have the charge_[start|stop]_threshold attributes visible when the charging mode is not custom. Regards, Hans > --- > .../ABI/testing/sysfs-class-power-dell | 31 +++ > drivers/platform/x86/dell/Kconfig | 1 + > drivers/platform/x86/dell/dell-laptop.c | 263 ++++++++++++++++++ > drivers/platform/x86/dell/dell-smbios.h | 17 ++ > 4 files changed, 312 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-power-dell > > diff --git a/Documentation/ABI/testing/sysfs-class-power-dell b/Documentation/ABI/testing/sysfs-class-power-dell > new file mode 100644 > index 000000000000..ef72c5f797ce > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-power-dell > @@ -0,0 +1,31 @@ > +What: /sys/class/power_supply/<supply_name>/charge_type > +Date: July 2024 > +KernelVersion: 6.11 > +Contact: linux-pm@vger.kernel.org > +Description: > + Select the charging algorithm to use for the (primary) > + battery. > + > + Standard: > + Fully charge the battery at a moderate rate. > + ExpressCharge™: > + Quickly charge the battery using fast-charge > + technology. This is harder on the battery than > + standard charging and may lower its lifespan. > + Primarily AC Use: > + Users who primarily operate the system while > + plugged into an external power source can extend > + battery life with this mode. > + Adaptive: > + Automatically optimize battery charge rate based > + on typical usage. > + Custom: > + Use the charge_control_* properties to determine > + when to start and stop charging. Advanced users > + can use this to drastically extend battery life. > + > + Access: Read, Write > + Valid values: > + "standard", "express", "primarily_ac", > + "adaptive", "custom" > + > diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig > index 85a78ef91182..02405793163c 100644 > --- a/drivers/platform/x86/dell/Kconfig > +++ b/drivers/platform/x86/dell/Kconfig > @@ -49,6 +49,7 @@ config DELL_LAPTOP > default m > depends on DMI > depends on BACKLIGHT_CLASS_DEVICE > + depends on ACPI_BATTERY > depends on ACPI_VIDEO || ACPI_VIDEO = n > depends on RFKILL || RFKILL = n > depends on DELL_WMI || DELL_WMI = n > diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c > index 6552dfe491c6..54f47b685a46 100644 > --- a/drivers/platform/x86/dell/dell-laptop.c > +++ b/drivers/platform/x86/dell/dell-laptop.c > @@ -22,11 +22,13 @@ > #include <linux/io.h> > #include <linux/rfkill.h> > #include <linux/power_supply.h> > +#include <linux/sysfs.h> > #include <linux/acpi.h> > #include <linux/mm.h> > #include <linux/i8042.h> > #include <linux/debugfs.h> > #include <linux/seq_file.h> > +#include <acpi/battery.h> > #include <acpi/video.h> > #include "dell-rbtn.h" > #include "dell-smbios.h" > @@ -98,6 +100,14 @@ static struct rfkill *wwan_rfkill; > static bool force_rfkill; > static bool micmute_led_registered; > static bool mute_led_registered; > +static enum battery_charging_mode bat_chg_current = DELL_BAT_MODE_NONE; > +static const char * const battery_state[DELL_BAT_MODE_MAX] = { > + [DELL_BAT_MODE_STANDARD] = "standard", > + [DELL_BAT_MODE_EXPRESS] = "express", > + [DELL_BAT_MODE_PRIMARILY_AC] = "primarily_ac", > + [DELL_BAT_MODE_ADAPTIVE] = "adaptive", > + [DELL_BAT_MODE_CUSTOM] = "custom", > +}; > > module_param(force_rfkill, bool, 0444); > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); > @@ -2183,6 +2193,256 @@ static struct led_classdev mute_led_cdev = { > .default_trigger = "audio-mute", > }; > > +static int dell_battery_read_req(const int type, int *val) > +{ > + struct calling_interface_buffer buffer; > + struct calling_interface_token *token; > + int err; > + > + token = dell_smbios_find_token(type); > + if (!token) > + return -ENODEV; > + > + dell_fill_request(&buffer, token->location, 0, 0, 0); > + err = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); > + if (!err) > + *val = buffer.output[1]; > + > + return err; > +} > + > +static int dell_battery_write_req(const int type, int val) > +{ > + struct calling_interface_buffer buffer; > + struct calling_interface_token *token; > + > + token = dell_smbios_find_token(type); > + if (!token) > + return -ENODEV; > + > + dell_fill_request(&buffer, token->location, val, 0, 0); > + return dell_send_request(&buffer, > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); > +} > + > +/* The rules: the minimum start charging value is 50%. The maximum > + * start charging value is 95%. The minimum end charging value is > + * 55%. The maximum end charging value is 100%. And finally, there > + * has to be at least a 5% difference between start & end values. > + */ > +#define CHARGE_START_MIN 50 > +#define CHARGE_START_MAX 95 > +#define CHARGE_END_MIN 55 > +#define CHARGE_END_MAX 100 > +#define CHARGE_MIN_DIFF 5 > + > +static int dell_battery_custom_set(const int type, int val) > +{ > + if (type == BAT_CUSTOM_CHARGE_START) { > + int end = CHARGE_END_MAX; > + > + if (val < CHARGE_START_MIN) > + val = CHARGE_START_MIN; > + else if (val > CHARGE_START_MAX) > + val = CHARGE_START_MAX; > + > + dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); > + if ((end - val) < CHARGE_MIN_DIFF) > + val = end - CHARGE_MIN_DIFF; > + } else if (type == BAT_CUSTOM_CHARGE_END) { > + int start = CHARGE_START_MIN; > + > + if (val < CHARGE_END_MIN) > + val = CHARGE_END_MIN; > + else if (val > CHARGE_END_MAX) > + val = CHARGE_END_MAX; > + > + dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); > + if ((val - start) < CHARGE_MIN_DIFF) > + val = start + CHARGE_MIN_DIFF; > + } > + > + return dell_battery_write_req(type, val); > +} > + > +static int battery_charging_mode_set(enum battery_charging_mode mode) > +{ > + int err; > + > + switch (mode) { > + case DELL_BAT_MODE_STANDARD: > + err = dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode); > + break; > + case DELL_BAT_MODE_EXPRESS: > + err = dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode); > + break; > + case DELL_BAT_MODE_PRIMARILY_AC: > + err = dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode); > + break; > + case DELL_BAT_MODE_ADAPTIVE: > + err = dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode); > + break; > + case DELL_BAT_MODE_CUSTOM: > + err = dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode); > + break; > + default: > + err = -EINVAL; > + } > + > + return err; > +} > + > +static ssize_t charge_type_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + enum battery_charging_mode mode; > + ssize_t count = 0; > + > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > + if (battery_state[mode]) { > + count += sysfs_emit_at(buf, count, > + mode == bat_chg_current ? "[%s] " : "%s ", > + battery_state[mode]); > + } > + } > + > + /* convert the last space to a newline */ > + count--; > + count += sysfs_emit_at(buf, count, "\n"); > + > + return count; > +} > + > +static ssize_t charge_type_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + enum battery_charging_mode mode; > + const char *label; > + int ret = -EINVAL; > + > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > + label = battery_state[mode]; > + if (label && sysfs_streq(label, buf)) > + break; > + } > + > + if (mode > DELL_BAT_MODE_NONE && mode < DELL_BAT_MODE_MAX) { > + ret = battery_charging_mode_set(mode); > + if (!ret) { > + bat_chg_current = mode; > + ret = size; > + } > + } > + > + return ret; > +} > + > +static ssize_t charge_control_start_threshold_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret, start; > + > + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); > + if (!ret) > + ret = sysfs_emit(buf, "%d\n", start); > + > + return ret; > +} > + > +static ssize_t charge_control_start_threshold_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + int ret, start; > + > + ret = kstrtoint(buf, 10, &start); > + if (!ret) > + ret = dell_battery_custom_set(BAT_CUSTOM_CHARGE_START, start); > + if (!ret) > + ret = size; > + > + return ret; > +} > + > +static ssize_t charge_control_end_threshold_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret, end; > + > + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); > + if (!ret) > + ret = sysfs_emit(buf, "%d\n", end); > + > + return ret; > +} > + > +static ssize_t charge_control_end_threshold_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + int ret, end; > + > + ret = kstrtouint(buf, 10, &end); > + if (!ret) > + ret = dell_battery_custom_set(BAT_CUSTOM_CHARGE_END, end); > + if (!ret) > + ret = size; > + > + return ret; > +} > + > +static DEVICE_ATTR_RW(charge_control_start_threshold); > +static DEVICE_ATTR_RW(charge_control_end_threshold); > +static DEVICE_ATTR_RW(charge_type); > + > +static struct attribute *dell_battery_attrs[] = { > + &dev_attr_charge_control_start_threshold.attr, > + &dev_attr_charge_control_end_threshold.attr, > + &dev_attr_charge_type.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(dell_battery); > + > +static int dell_battery_add(struct power_supply *battery, > + struct acpi_battery_hook *hook) > +{ > + return device_add_groups(&battery->dev, dell_battery_groups); > +} > + > +static int dell_battery_remove(struct power_supply *battery, > + struct acpi_battery_hook *hook) > +{ > + device_remove_groups(&battery->dev, dell_battery_groups); > + return 0; > +} > + > +static struct acpi_battery_hook dell_battery_hook = { > + .add_battery = dell_battery_add, > + .remove_battery = dell_battery_remove, > + .name = "Dell Primary Battery Extension", > +}; > + > +static void __init dell_battery_init(struct device *dev) > +{ > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > + > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > + if (current_mode != DELL_BAT_MODE_NONE) { > + bat_chg_current = current_mode; > + battery_hook_register(&dell_battery_hook); > + } > +} > + > +static void __exit dell_battery_exit(void) > +{ > + if (bat_chg_current != DELL_BAT_MODE_NONE) > + battery_hook_unregister(&dell_battery_hook); > +} > + > static int __init dell_init(void) > { > struct calling_interface_token *token; > @@ -2219,6 +2479,7 @@ static int __init dell_init(void) > touchpad_led_init(&platform_device->dev); > > kbd_led_init(&platform_device->dev); > + dell_battery_init(&platform_device->dev); > > dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL); > debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL, > @@ -2293,6 +2554,7 @@ static int __init dell_init(void) > if (mute_led_registered) > led_classdev_unregister(&mute_led_cdev); > fail_led: > + dell_battery_exit(); > dell_cleanup_rfkill(); > fail_rfkill: > platform_device_del(platform_device); > @@ -2311,6 +2573,7 @@ static void __exit dell_exit(void) > if (quirks && quirks->touchpad_led) > touchpad_led_exit(); > kbd_led_exit(); > + dell_battery_exit(); > backlight_device_unregister(dell_backlight_device); > if (micmute_led_registered) > led_classdev_unregister(&micmute_led_cdev); > diff --git a/drivers/platform/x86/dell/dell-smbios.h b/drivers/platform/x86/dell/dell-smbios.h > index ea0cc38642a2..f7c07b4d72e3 100644 > --- a/drivers/platform/x86/dell/dell-smbios.h > +++ b/drivers/platform/x86/dell/dell-smbios.h > @@ -33,11 +33,28 @@ > #define KBD_LED_AUTO_50_TOKEN 0x02EB > #define KBD_LED_AUTO_75_TOKEN 0x02EC > #define KBD_LED_AUTO_100_TOKEN 0x02F6 > +#define BAT_PRI_AC_MODE_TOKEN 0x0341 > +#define BAT_ADAPTIVE_MODE_TOKEN 0x0342 > +#define BAT_CUSTOM_MODE_TOKEN 0x0343 > +#define BAT_STANDARD_MODE_TOKEN 0x0346 > +#define BAT_EXPRESS_MODE_TOKEN 0x0347 > +#define BAT_CUSTOM_CHARGE_START 0x0349 > +#define BAT_CUSTOM_CHARGE_END 0x034A > #define GLOBAL_MIC_MUTE_ENABLE 0x0364 > #define GLOBAL_MIC_MUTE_DISABLE 0x0365 > #define GLOBAL_MUTE_ENABLE 0x058C > #define GLOBAL_MUTE_DISABLE 0x058D > > +enum battery_charging_mode { > + DELL_BAT_MODE_NONE = 0, > + DELL_BAT_MODE_STANDARD, > + DELL_BAT_MODE_EXPRESS, > + DELL_BAT_MODE_PRIMARILY_AC, > + DELL_BAT_MODE_ADAPTIVE, > + DELL_BAT_MODE_CUSTOM, > + DELL_BAT_MODE_MAX, > +}; > + > struct notifier_block; > > struct calling_interface_token {
Am 29.07.24 um 12:02 schrieb Hans de Goede: > Hi, > > On 7/20/24 7:22 AM, Andres Salomon wrote: >> The Dell BIOS allows you to set custom charging modes, which is useful >> in particular for extending battery life. This adds support for tweaking >> those various settings from Linux via sysfs knobs. One might, for >> example, have their laptop plugged into power at their desk the vast >> majority of the time and choose fairly aggressive battery-saving >> settings (only charging once the battery drops to 50% and only charging >> up to 80%). When leaving for a trip, they might want to switch those >> settings to charge to 100% and charge any time power is available; >> rebooting into the BIOS to change those settings is a hassle. >> >> For the Custom charging type mode, we reuse the common >> charge_control_{start,end}_threshold sysfs power_supply entries. The >> BIOS also has a bunch of other charging modes with varying levels of >> usefulness, so this adds a new Dell-specific sysfs entry called >> 'charge_type' that's documented in sysfs-class-power-dell (and looks a >> lot like sysfs-class-power-wilco). >> >> This work is based on a patch by Perry Yuan <perry_yuan@dell.com> and >> Limonciello Mario <Mario_Limonciello@Dell.com> submitted back in 2020: >> https://lore.kernel.org/all/20200729065424.12851-1-Perry_Yuan@Dell.com/ >> Both of their email addresses bounce, so I'm assuming they're no longer >> with the company. I've reworked most of the patch to make it smaller and >> cleaner. >> >> Signed-off-by: Andres Salomon <dilinger@queued.net> > Thank you for working on this and it is great to see the discussion > to improve the code between you and Pali going on. > > One concern which I have is that work is underway for both upower > and GNOME to add support for > /sys/class/power_supply/*/charge_[start|stop]_threshold > > to gnome-control-center. > > But if I understand things correctly then these limits will only > be honored when the charging-type is set to custom. > > So we need to do something to avoid userspace exposing those > controls when the charging-type is not custom. > > I think it would be best to not have the charge_[start|stop]_threshold > attributes visible when the charging mode is not custom. > > Regards, > > Hans Hi, the documentation of the "charge_type" sysfs attribute states that: "Custom" means that the charger uses the charge_control_* properties as configuration for some different algorithm. So i believe that "charge_[start|stop]_threshold" attributes should not be unregistered if "charge_type" is not "Custom" because: 1. The power supply subsystem does not allow that for power supplies, and i see little reason to deviate from this behavior here. 2. It is the responsibility of userspace to also set "charge_type" to "Custom" when using the "charge_[start|stop]_threshold" attributes. Maybe we could clarify what happens when "charge_[start|stop]_threshold" is written without "charge_type" being "Custom". This might help userspace to correctly switch to custom charging and set the charging thresholds. Basically, we could define that writes to "charge_[start|stop]_threshold" will get buffered by the driver if two conditions are met: 1. "charge_type" is not "Custom". 2. The hardware does not support setting the charging thresholds when "charge_type" is not "Custom". Userspace can then first set "charge_[start|stop]_threshold" and then switch to custom charging. This prevents any problems should the hardware have no default value for the charging thresholds when switching to custom charging for the first time. Thanks, Armin Wolf >> --- >> .../ABI/testing/sysfs-class-power-dell | 31 +++ >> drivers/platform/x86/dell/Kconfig | 1 + >> drivers/platform/x86/dell/dell-laptop.c | 263 ++++++++++++++++++ >> drivers/platform/x86/dell/dell-smbios.h | 17 ++ >> 4 files changed, 312 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-class-power-dell >> >> diff --git a/Documentation/ABI/testing/sysfs-class-power-dell b/Documentation/ABI/testing/sysfs-class-power-dell >> new file mode 100644 >> index 000000000000..ef72c5f797ce >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-class-power-dell >> @@ -0,0 +1,31 @@ >> +What: /sys/class/power_supply/<supply_name>/charge_type >> +Date: July 2024 >> +KernelVersion: 6.11 >> +Contact: linux-pm@vger.kernel.org >> +Description: >> + Select the charging algorithm to use for the (primary) >> + battery. >> + >> + Standard: >> + Fully charge the battery at a moderate rate. >> + ExpressCharge™: >> + Quickly charge the battery using fast-charge >> + technology. This is harder on the battery than >> + standard charging and may lower its lifespan. >> + Primarily AC Use: >> + Users who primarily operate the system while >> + plugged into an external power source can extend >> + battery life with this mode. >> + Adaptive: >> + Automatically optimize battery charge rate based >> + on typical usage. >> + Custom: >> + Use the charge_control_* properties to determine >> + when to start and stop charging. Advanced users >> + can use this to drastically extend battery life. >> + >> + Access: Read, Write >> + Valid values: >> + "standard", "express", "primarily_ac", >> + "adaptive", "custom" >> + >> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig >> index 85a78ef91182..02405793163c 100644 >> --- a/drivers/platform/x86/dell/Kconfig >> +++ b/drivers/platform/x86/dell/Kconfig >> @@ -49,6 +49,7 @@ config DELL_LAPTOP >> default m >> depends on DMI >> depends on BACKLIGHT_CLASS_DEVICE >> + depends on ACPI_BATTERY >> depends on ACPI_VIDEO || ACPI_VIDEO = n >> depends on RFKILL || RFKILL = n >> depends on DELL_WMI || DELL_WMI = n >> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c >> index 6552dfe491c6..54f47b685a46 100644 >> --- a/drivers/platform/x86/dell/dell-laptop.c >> +++ b/drivers/platform/x86/dell/dell-laptop.c >> @@ -22,11 +22,13 @@ >> #include <linux/io.h> >> #include <linux/rfkill.h> >> #include <linux/power_supply.h> >> +#include <linux/sysfs.h> >> #include <linux/acpi.h> >> #include <linux/mm.h> >> #include <linux/i8042.h> >> #include <linux/debugfs.h> >> #include <linux/seq_file.h> >> +#include <acpi/battery.h> >> #include <acpi/video.h> >> #include "dell-rbtn.h" >> #include "dell-smbios.h" >> @@ -98,6 +100,14 @@ static struct rfkill *wwan_rfkill; >> static bool force_rfkill; >> static bool micmute_led_registered; >> static bool mute_led_registered; >> +static enum battery_charging_mode bat_chg_current = DELL_BAT_MODE_NONE; >> +static const char * const battery_state[DELL_BAT_MODE_MAX] = { >> + [DELL_BAT_MODE_STANDARD] = "standard", >> + [DELL_BAT_MODE_EXPRESS] = "express", >> + [DELL_BAT_MODE_PRIMARILY_AC] = "primarily_ac", >> + [DELL_BAT_MODE_ADAPTIVE] = "adaptive", >> + [DELL_BAT_MODE_CUSTOM] = "custom", >> +}; >> >> module_param(force_rfkill, bool, 0444); >> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); >> @@ -2183,6 +2193,256 @@ static struct led_classdev mute_led_cdev = { >> .default_trigger = "audio-mute", >> }; >> >> +static int dell_battery_read_req(const int type, int *val) >> +{ >> + struct calling_interface_buffer buffer; >> + struct calling_interface_token *token; >> + int err; >> + >> + token = dell_smbios_find_token(type); >> + if (!token) >> + return -ENODEV; >> + >> + dell_fill_request(&buffer, token->location, 0, 0, 0); >> + err = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); >> + if (!err) >> + *val = buffer.output[1]; >> + >> + return err; >> +} >> + >> +static int dell_battery_write_req(const int type, int val) >> +{ >> + struct calling_interface_buffer buffer; >> + struct calling_interface_token *token; >> + >> + token = dell_smbios_find_token(type); >> + if (!token) >> + return -ENODEV; >> + >> + dell_fill_request(&buffer, token->location, val, 0, 0); >> + return dell_send_request(&buffer, >> + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); >> +} >> + >> +/* The rules: the minimum start charging value is 50%. The maximum >> + * start charging value is 95%. The minimum end charging value is >> + * 55%. The maximum end charging value is 100%. And finally, there >> + * has to be at least a 5% difference between start & end values. >> + */ >> +#define CHARGE_START_MIN 50 >> +#define CHARGE_START_MAX 95 >> +#define CHARGE_END_MIN 55 >> +#define CHARGE_END_MAX 100 >> +#define CHARGE_MIN_DIFF 5 >> + >> +static int dell_battery_custom_set(const int type, int val) >> +{ >> + if (type == BAT_CUSTOM_CHARGE_START) { >> + int end = CHARGE_END_MAX; >> + >> + if (val < CHARGE_START_MIN) >> + val = CHARGE_START_MIN; >> + else if (val > CHARGE_START_MAX) >> + val = CHARGE_START_MAX; >> + >> + dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); >> + if ((end - val) < CHARGE_MIN_DIFF) >> + val = end - CHARGE_MIN_DIFF; >> + } else if (type == BAT_CUSTOM_CHARGE_END) { >> + int start = CHARGE_START_MIN; >> + >> + if (val < CHARGE_END_MIN) >> + val = CHARGE_END_MIN; >> + else if (val > CHARGE_END_MAX) >> + val = CHARGE_END_MAX; >> + >> + dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); >> + if ((val - start) < CHARGE_MIN_DIFF) >> + val = start + CHARGE_MIN_DIFF; >> + } >> + >> + return dell_battery_write_req(type, val); >> +} >> + >> +static int battery_charging_mode_set(enum battery_charging_mode mode) >> +{ >> + int err; >> + >> + switch (mode) { >> + case DELL_BAT_MODE_STANDARD: >> + err = dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode); >> + break; >> + case DELL_BAT_MODE_EXPRESS: >> + err = dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode); >> + break; >> + case DELL_BAT_MODE_PRIMARILY_AC: >> + err = dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode); >> + break; >> + case DELL_BAT_MODE_ADAPTIVE: >> + err = dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode); >> + break; >> + case DELL_BAT_MODE_CUSTOM: >> + err = dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode); >> + break; >> + default: >> + err = -EINVAL; >> + } >> + >> + return err; >> +} >> + >> +static ssize_t charge_type_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + enum battery_charging_mode mode; >> + ssize_t count = 0; >> + >> + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { >> + if (battery_state[mode]) { >> + count += sysfs_emit_at(buf, count, >> + mode == bat_chg_current ? "[%s] " : "%s ", >> + battery_state[mode]); >> + } >> + } >> + >> + /* convert the last space to a newline */ >> + count--; >> + count += sysfs_emit_at(buf, count, "\n"); >> + >> + return count; >> +} >> + >> +static ssize_t charge_type_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + enum battery_charging_mode mode; >> + const char *label; >> + int ret = -EINVAL; >> + >> + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { >> + label = battery_state[mode]; >> + if (label && sysfs_streq(label, buf)) >> + break; >> + } >> + >> + if (mode > DELL_BAT_MODE_NONE && mode < DELL_BAT_MODE_MAX) { >> + ret = battery_charging_mode_set(mode); >> + if (!ret) { >> + bat_chg_current = mode; >> + ret = size; >> + } >> + } >> + >> + return ret; >> +} >> + >> +static ssize_t charge_control_start_threshold_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + int ret, start; >> + >> + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); >> + if (!ret) >> + ret = sysfs_emit(buf, "%d\n", start); >> + >> + return ret; >> +} >> + >> +static ssize_t charge_control_start_threshold_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + int ret, start; >> + >> + ret = kstrtoint(buf, 10, &start); >> + if (!ret) >> + ret = dell_battery_custom_set(BAT_CUSTOM_CHARGE_START, start); >> + if (!ret) >> + ret = size; >> + >> + return ret; >> +} >> + >> +static ssize_t charge_control_end_threshold_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + int ret, end; >> + >> + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); >> + if (!ret) >> + ret = sysfs_emit(buf, "%d\n", end); >> + >> + return ret; >> +} >> + >> +static ssize_t charge_control_end_threshold_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + int ret, end; >> + >> + ret = kstrtouint(buf, 10, &end); >> + if (!ret) >> + ret = dell_battery_custom_set(BAT_CUSTOM_CHARGE_END, end); >> + if (!ret) >> + ret = size; >> + >> + return ret; >> +} >> + >> +static DEVICE_ATTR_RW(charge_control_start_threshold); >> +static DEVICE_ATTR_RW(charge_control_end_threshold); >> +static DEVICE_ATTR_RW(charge_type); >> + >> +static struct attribute *dell_battery_attrs[] = { >> + &dev_attr_charge_control_start_threshold.attr, >> + &dev_attr_charge_control_end_threshold.attr, >> + &dev_attr_charge_type.attr, >> + NULL, >> +}; >> +ATTRIBUTE_GROUPS(dell_battery); >> + >> +static int dell_battery_add(struct power_supply *battery, >> + struct acpi_battery_hook *hook) >> +{ >> + return device_add_groups(&battery->dev, dell_battery_groups); >> +} >> + >> +static int dell_battery_remove(struct power_supply *battery, >> + struct acpi_battery_hook *hook) >> +{ >> + device_remove_groups(&battery->dev, dell_battery_groups); >> + return 0; >> +} >> + >> +static struct acpi_battery_hook dell_battery_hook = { >> + .add_battery = dell_battery_add, >> + .remove_battery = dell_battery_remove, >> + .name = "Dell Primary Battery Extension", >> +}; >> + >> +static void __init dell_battery_init(struct device *dev) >> +{ >> + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; >> + >> + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); >> + if (current_mode != DELL_BAT_MODE_NONE) { >> + bat_chg_current = current_mode; >> + battery_hook_register(&dell_battery_hook); >> + } >> +} >> + >> +static void __exit dell_battery_exit(void) >> +{ >> + if (bat_chg_current != DELL_BAT_MODE_NONE) >> + battery_hook_unregister(&dell_battery_hook); >> +} >> + >> static int __init dell_init(void) >> { >> struct calling_interface_token *token; >> @@ -2219,6 +2479,7 @@ static int __init dell_init(void) >> touchpad_led_init(&platform_device->dev); >> >> kbd_led_init(&platform_device->dev); >> + dell_battery_init(&platform_device->dev); >> >> dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL); >> debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL, >> @@ -2293,6 +2554,7 @@ static int __init dell_init(void) >> if (mute_led_registered) >> led_classdev_unregister(&mute_led_cdev); >> fail_led: >> + dell_battery_exit(); >> dell_cleanup_rfkill(); >> fail_rfkill: >> platform_device_del(platform_device); >> @@ -2311,6 +2573,7 @@ static void __exit dell_exit(void) >> if (quirks && quirks->touchpad_led) >> touchpad_led_exit(); >> kbd_led_exit(); >> + dell_battery_exit(); >> backlight_device_unregister(dell_backlight_device); >> if (micmute_led_registered) >> led_classdev_unregister(&micmute_led_cdev); >> diff --git a/drivers/platform/x86/dell/dell-smbios.h b/drivers/platform/x86/dell/dell-smbios.h >> index ea0cc38642a2..f7c07b4d72e3 100644 >> --- a/drivers/platform/x86/dell/dell-smbios.h >> +++ b/drivers/platform/x86/dell/dell-smbios.h >> @@ -33,11 +33,28 @@ >> #define KBD_LED_AUTO_50_TOKEN 0x02EB >> #define KBD_LED_AUTO_75_TOKEN 0x02EC >> #define KBD_LED_AUTO_100_TOKEN 0x02F6 >> +#define BAT_PRI_AC_MODE_TOKEN 0x0341 >> +#define BAT_ADAPTIVE_MODE_TOKEN 0x0342 >> +#define BAT_CUSTOM_MODE_TOKEN 0x0343 >> +#define BAT_STANDARD_MODE_TOKEN 0x0346 >> +#define BAT_EXPRESS_MODE_TOKEN 0x0347 >> +#define BAT_CUSTOM_CHARGE_START 0x0349 >> +#define BAT_CUSTOM_CHARGE_END 0x034A >> #define GLOBAL_MIC_MUTE_ENABLE 0x0364 >> #define GLOBAL_MIC_MUTE_DISABLE 0x0365 >> #define GLOBAL_MUTE_ENABLE 0x058C >> #define GLOBAL_MUTE_DISABLE 0x058D >> >> +enum battery_charging_mode { >> + DELL_BAT_MODE_NONE = 0, >> + DELL_BAT_MODE_STANDARD, >> + DELL_BAT_MODE_EXPRESS, >> + DELL_BAT_MODE_PRIMARILY_AC, >> + DELL_BAT_MODE_ADAPTIVE, >> + DELL_BAT_MODE_CUSTOM, >> + DELL_BAT_MODE_MAX, >> +}; >> + >> struct notifier_block; >> >> struct calling_interface_token { >
Hi, On 7/29/24 6:41 PM, Armin Wolf wrote: > Am 29.07.24 um 12:02 schrieb Hans de Goede: > >> Hi, >> >> On 7/20/24 7:22 AM, Andres Salomon wrote: >>> The Dell BIOS allows you to set custom charging modes, which is useful >>> in particular for extending battery life. This adds support for tweaking >>> those various settings from Linux via sysfs knobs. One might, for >>> example, have their laptop plugged into power at their desk the vast >>> majority of the time and choose fairly aggressive battery-saving >>> settings (only charging once the battery drops to 50% and only charging >>> up to 80%). When leaving for a trip, they might want to switch those >>> settings to charge to 100% and charge any time power is available; >>> rebooting into the BIOS to change those settings is a hassle. >>> >>> For the Custom charging type mode, we reuse the common >>> charge_control_{start,end}_threshold sysfs power_supply entries. The >>> BIOS also has a bunch of other charging modes with varying levels of >>> usefulness, so this adds a new Dell-specific sysfs entry called >>> 'charge_type' that's documented in sysfs-class-power-dell (and looks a >>> lot like sysfs-class-power-wilco). >>> >>> This work is based on a patch by Perry Yuan <perry_yuan@dell.com> and >>> Limonciello Mario <Mario_Limonciello@Dell.com> submitted back in 2020: >>> https://lore.kernel.org/all/20200729065424.12851-1-Perry_Yuan@Dell.com/ >>> Both of their email addresses bounce, so I'm assuming they're no longer >>> with the company. I've reworked most of the patch to make it smaller and >>> cleaner. >>> >>> Signed-off-by: Andres Salomon <dilinger@queued.net> >> Thank you for working on this and it is great to see the discussion >> to improve the code between you and Pali going on. >> >> One concern which I have is that work is underway for both upower >> and GNOME to add support for >> /sys/class/power_supply/*/charge_[start|stop]_threshold >> >> to gnome-control-center. >> >> But if I understand things correctly then these limits will only >> be honored when the charging-type is set to custom. >> >> So we need to do something to avoid userspace exposing those >> controls when the charging-type is not custom. >> >> I think it would be best to not have the charge_[start|stop]_threshold >> attributes visible when the charging mode is not custom. >> >> Regards, >> >> Hans > > Hi, > > the documentation of the "charge_type" sysfs attribute states that: > > "Custom" means that the charger uses the charge_control_* properties as > configuration for some different algorithm. > > So i believe that "charge_[start|stop]_threshold" attributes should not be unregistered > if "charge_type" is not "Custom" because: > > 1. The power supply subsystem does not allow that for power supplies, and i see little > reason to deviate from this behavior here. > > 2. It is the responsibility of userspace to also set "charge_type" to "Custom" when using > the "charge_[start|stop]_threshold" attributes. > > Maybe we could clarify what happens when "charge_[start|stop]_threshold" is written without > "charge_type" being "Custom". This might help userspace to correctly switch to custom charging > and set the charging thresholds. > > Basically, we could define that writes to "charge_[start|stop]_threshold" will get buffered by > the driver if two conditions are met: > > 1. "charge_type" is not "Custom". > > 2. The hardware does not support setting the charging thresholds when "charge_type" is not "Custom". > > Userspace can then first set "charge_[start|stop]_threshold" and then switch to custom charging. > This prevents any problems should the hardware have no default value for the charging thresholds > when switching to custom charging for the first time. Ah I did not realize this was already documented in this way. Yes if this is already documented this way then the driver does not have to do anything. Instead userspace consumers of the "charge_[start|stop]_threshold" user should check (and if necessary / if they want to set) "charge_type" = "Custom" on power_supply class devices which have a charge_type. Regards, Hans
diff --git a/Documentation/ABI/testing/sysfs-class-power-dell b/Documentation/ABI/testing/sysfs-class-power-dell new file mode 100644 index 000000000000..ef72c5f797ce --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-power-dell @@ -0,0 +1,31 @@ +What: /sys/class/power_supply/<supply_name>/charge_type +Date: July 2024 +KernelVersion: 6.11 +Contact: linux-pm@vger.kernel.org +Description: + Select the charging algorithm to use for the (primary) + battery. + + Standard: + Fully charge the battery at a moderate rate. + ExpressCharge™: + Quickly charge the battery using fast-charge + technology. This is harder on the battery than + standard charging and may lower its lifespan. + Primarily AC Use: + Users who primarily operate the system while + plugged into an external power source can extend + battery life with this mode. + Adaptive: + Automatically optimize battery charge rate based + on typical usage. + Custom: + Use the charge_control_* properties to determine + when to start and stop charging. Advanced users + can use this to drastically extend battery life. + + Access: Read, Write + Valid values: + "standard", "express", "primarily_ac", + "adaptive", "custom" + diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig index 85a78ef91182..02405793163c 100644 --- a/drivers/platform/x86/dell/Kconfig +++ b/drivers/platform/x86/dell/Kconfig @@ -49,6 +49,7 @@ config DELL_LAPTOP default m depends on DMI depends on BACKLIGHT_CLASS_DEVICE + depends on ACPI_BATTERY depends on ACPI_VIDEO || ACPI_VIDEO = n depends on RFKILL || RFKILL = n depends on DELL_WMI || DELL_WMI = n diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c index 6552dfe491c6..54f47b685a46 100644 --- a/drivers/platform/x86/dell/dell-laptop.c +++ b/drivers/platform/x86/dell/dell-laptop.c @@ -22,11 +22,13 @@ #include <linux/io.h> #include <linux/rfkill.h> #include <linux/power_supply.h> +#include <linux/sysfs.h> #include <linux/acpi.h> #include <linux/mm.h> #include <linux/i8042.h> #include <linux/debugfs.h> #include <linux/seq_file.h> +#include <acpi/battery.h> #include <acpi/video.h> #include "dell-rbtn.h" #include "dell-smbios.h" @@ -98,6 +100,14 @@ static struct rfkill *wwan_rfkill; static bool force_rfkill; static bool micmute_led_registered; static bool mute_led_registered; +static enum battery_charging_mode bat_chg_current = DELL_BAT_MODE_NONE; +static const char * const battery_state[DELL_BAT_MODE_MAX] = { + [DELL_BAT_MODE_STANDARD] = "standard", + [DELL_BAT_MODE_EXPRESS] = "express", + [DELL_BAT_MODE_PRIMARILY_AC] = "primarily_ac", + [DELL_BAT_MODE_ADAPTIVE] = "adaptive", + [DELL_BAT_MODE_CUSTOM] = "custom", +}; module_param(force_rfkill, bool, 0444); MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); @@ -2183,6 +2193,256 @@ static struct led_classdev mute_led_cdev = { .default_trigger = "audio-mute", }; +static int dell_battery_read_req(const int type, int *val) +{ + struct calling_interface_buffer buffer; + struct calling_interface_token *token; + int err; + + token = dell_smbios_find_token(type); + if (!token) + return -ENODEV; + + dell_fill_request(&buffer, token->location, 0, 0, 0); + err = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); + if (!err) + *val = buffer.output[1]; + + return err; +} + +static int dell_battery_write_req(const int type, int val) +{ + struct calling_interface_buffer buffer; + struct calling_interface_token *token; + + token = dell_smbios_find_token(type); + if (!token) + return -ENODEV; + + dell_fill_request(&buffer, token->location, val, 0, 0); + return dell_send_request(&buffer, + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); +} + +/* The rules: the minimum start charging value is 50%. The maximum + * start charging value is 95%. The minimum end charging value is + * 55%. The maximum end charging value is 100%. And finally, there + * has to be at least a 5% difference between start & end values. + */ +#define CHARGE_START_MIN 50 +#define CHARGE_START_MAX 95 +#define CHARGE_END_MIN 55 +#define CHARGE_END_MAX 100 +#define CHARGE_MIN_DIFF 5 + +static int dell_battery_custom_set(const int type, int val) +{ + if (type == BAT_CUSTOM_CHARGE_START) { + int end = CHARGE_END_MAX; + + if (val < CHARGE_START_MIN) + val = CHARGE_START_MIN; + else if (val > CHARGE_START_MAX) + val = CHARGE_START_MAX; + + dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); + if ((end - val) < CHARGE_MIN_DIFF) + val = end - CHARGE_MIN_DIFF; + } else if (type == BAT_CUSTOM_CHARGE_END) { + int start = CHARGE_START_MIN; + + if (val < CHARGE_END_MIN) + val = CHARGE_END_MIN; + else if (val > CHARGE_END_MAX) + val = CHARGE_END_MAX; + + dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); + if ((val - start) < CHARGE_MIN_DIFF) + val = start + CHARGE_MIN_DIFF; + } + + return dell_battery_write_req(type, val); +} + +static int battery_charging_mode_set(enum battery_charging_mode mode) +{ + int err; + + switch (mode) { + case DELL_BAT_MODE_STANDARD: + err = dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode); + break; + case DELL_BAT_MODE_EXPRESS: + err = dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode); + break; + case DELL_BAT_MODE_PRIMARILY_AC: + err = dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode); + break; + case DELL_BAT_MODE_ADAPTIVE: + err = dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode); + break; + case DELL_BAT_MODE_CUSTOM: + err = dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode); + break; + default: + err = -EINVAL; + } + + return err; +} + +static ssize_t charge_type_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + enum battery_charging_mode mode; + ssize_t count = 0; + + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { + if (battery_state[mode]) { + count += sysfs_emit_at(buf, count, + mode == bat_chg_current ? "[%s] " : "%s ", + battery_state[mode]); + } + } + + /* convert the last space to a newline */ + count--; + count += sysfs_emit_at(buf, count, "\n"); + + return count; +} + +static ssize_t charge_type_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + enum battery_charging_mode mode; + const char *label; + int ret = -EINVAL; + + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { + label = battery_state[mode]; + if (label && sysfs_streq(label, buf)) + break; + } + + if (mode > DELL_BAT_MODE_NONE && mode < DELL_BAT_MODE_MAX) { + ret = battery_charging_mode_set(mode); + if (!ret) { + bat_chg_current = mode; + ret = size; + } + } + + return ret; +} + +static ssize_t charge_control_start_threshold_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int ret, start; + + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); + if (!ret) + ret = sysfs_emit(buf, "%d\n", start); + + return ret; +} + +static ssize_t charge_control_start_threshold_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + int ret, start; + + ret = kstrtoint(buf, 10, &start); + if (!ret) + ret = dell_battery_custom_set(BAT_CUSTOM_CHARGE_START, start); + if (!ret) + ret = size; + + return ret; +} + +static ssize_t charge_control_end_threshold_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int ret, end; + + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); + if (!ret) + ret = sysfs_emit(buf, "%d\n", end); + + return ret; +} + +static ssize_t charge_control_end_threshold_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + int ret, end; + + ret = kstrtouint(buf, 10, &end); + if (!ret) + ret = dell_battery_custom_set(BAT_CUSTOM_CHARGE_END, end); + if (!ret) + ret = size; + + return ret; +} + +static DEVICE_ATTR_RW(charge_control_start_threshold); +static DEVICE_ATTR_RW(charge_control_end_threshold); +static DEVICE_ATTR_RW(charge_type); + +static struct attribute *dell_battery_attrs[] = { + &dev_attr_charge_control_start_threshold.attr, + &dev_attr_charge_control_end_threshold.attr, + &dev_attr_charge_type.attr, + NULL, +}; +ATTRIBUTE_GROUPS(dell_battery); + +static int dell_battery_add(struct power_supply *battery, + struct acpi_battery_hook *hook) +{ + return device_add_groups(&battery->dev, dell_battery_groups); +} + +static int dell_battery_remove(struct power_supply *battery, + struct acpi_battery_hook *hook) +{ + device_remove_groups(&battery->dev, dell_battery_groups); + return 0; +} + +static struct acpi_battery_hook dell_battery_hook = { + .add_battery = dell_battery_add, + .remove_battery = dell_battery_remove, + .name = "Dell Primary Battery Extension", +}; + +static void __init dell_battery_init(struct device *dev) +{ + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; + + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); + if (current_mode != DELL_BAT_MODE_NONE) { + bat_chg_current = current_mode; + battery_hook_register(&dell_battery_hook); + } +} + +static void __exit dell_battery_exit(void) +{ + if (bat_chg_current != DELL_BAT_MODE_NONE) + battery_hook_unregister(&dell_battery_hook); +} + static int __init dell_init(void) { struct calling_interface_token *token; @@ -2219,6 +2479,7 @@ static int __init dell_init(void) touchpad_led_init(&platform_device->dev); kbd_led_init(&platform_device->dev); + dell_battery_init(&platform_device->dev); dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL); debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL, @@ -2293,6 +2554,7 @@ static int __init dell_init(void) if (mute_led_registered) led_classdev_unregister(&mute_led_cdev); fail_led: + dell_battery_exit(); dell_cleanup_rfkill(); fail_rfkill: platform_device_del(platform_device); @@ -2311,6 +2573,7 @@ static void __exit dell_exit(void) if (quirks && quirks->touchpad_led) touchpad_led_exit(); kbd_led_exit(); + dell_battery_exit(); backlight_device_unregister(dell_backlight_device); if (micmute_led_registered) led_classdev_unregister(&micmute_led_cdev); diff --git a/drivers/platform/x86/dell/dell-smbios.h b/drivers/platform/x86/dell/dell-smbios.h index ea0cc38642a2..f7c07b4d72e3 100644 --- a/drivers/platform/x86/dell/dell-smbios.h +++ b/drivers/platform/x86/dell/dell-smbios.h @@ -33,11 +33,28 @@ #define KBD_LED_AUTO_50_TOKEN 0x02EB #define KBD_LED_AUTO_75_TOKEN 0x02EC #define KBD_LED_AUTO_100_TOKEN 0x02F6 +#define BAT_PRI_AC_MODE_TOKEN 0x0341 +#define BAT_ADAPTIVE_MODE_TOKEN 0x0342 +#define BAT_CUSTOM_MODE_TOKEN 0x0343 +#define BAT_STANDARD_MODE_TOKEN 0x0346 +#define BAT_EXPRESS_MODE_TOKEN 0x0347 +#define BAT_CUSTOM_CHARGE_START 0x0349 +#define BAT_CUSTOM_CHARGE_END 0x034A #define GLOBAL_MIC_MUTE_ENABLE 0x0364 #define GLOBAL_MIC_MUTE_DISABLE 0x0365 #define GLOBAL_MUTE_ENABLE 0x058C #define GLOBAL_MUTE_DISABLE 0x058D +enum battery_charging_mode { + DELL_BAT_MODE_NONE = 0, + DELL_BAT_MODE_STANDARD, + DELL_BAT_MODE_EXPRESS, + DELL_BAT_MODE_PRIMARILY_AC, + DELL_BAT_MODE_ADAPTIVE, + DELL_BAT_MODE_CUSTOM, + DELL_BAT_MODE_MAX, +}; + struct notifier_block; struct calling_interface_token {
The Dell BIOS allows you to set custom charging modes, which is useful in particular for extending battery life. This adds support for tweaking those various settings from Linux via sysfs knobs. One might, for example, have their laptop plugged into power at their desk the vast majority of the time and choose fairly aggressive battery-saving settings (only charging once the battery drops to 50% and only charging up to 80%). When leaving for a trip, they might want to switch those settings to charge to 100% and charge any time power is available; rebooting into the BIOS to change those settings is a hassle. For the Custom charging type mode, we reuse the common charge_control_{start,end}_threshold sysfs power_supply entries. The BIOS also has a bunch of other charging modes with varying levels of usefulness, so this adds a new Dell-specific sysfs entry called 'charge_type' that's documented in sysfs-class-power-dell (and looks a lot like sysfs-class-power-wilco). This work is based on a patch by Perry Yuan <perry_yuan@dell.com> and Limonciello Mario <Mario_Limonciello@Dell.com> submitted back in 2020: https://lore.kernel.org/all/20200729065424.12851-1-Perry_Yuan@Dell.com/ Both of their email addresses bounce, so I'm assuming they're no longer with the company. I've reworked most of the patch to make it smaller and cleaner. Signed-off-by: Andres Salomon <dilinger@queued.net> --- .../ABI/testing/sysfs-class-power-dell | 31 +++ drivers/platform/x86/dell/Kconfig | 1 + drivers/platform/x86/dell/dell-laptop.c | 263 ++++++++++++++++++ drivers/platform/x86/dell/dell-smbios.h | 17 ++ 4 files changed, 312 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-power-dell