| Message ID | 20240723220502.77cb0401@5400 (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/2] platform/x86:dell-laptop: Add knobs to change battery charge settings | expand |
Hello, the driver change looks good. I have just few minor comments for this change below. Anyway, if there is somebody on the list with Dell laptop with 2 or 3 batteries, see below, it would be nice to check how such laptop would behave with this patch. It does not seem that patch should cause regression but always it is better to do testing if it is possible. On Tuesday 23 July 2024 22:05:02 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> > --- > Changes in v2, based on extensive feedback from Pali Rohár <pali@kernel.org>: > - code style changes > - change battery write API to use token->value instead of passed value > - stop caching current mode, instead querying SMBIOS as needed > - drop the separate list of charging modes enum > - rework the list of charging mode strings > - query SMBIOS for supported charging modes > - split dell_battery_custom_set() up > --- > .../ABI/testing/sysfs-class-power-dell | 31 ++ > drivers/platform/x86/dell/Kconfig | 1 + > drivers/platform/x86/dell/dell-laptop.c | 288 ++++++++++++++++++ > drivers/platform/x86/dell/dell-smbios.h | 7 + > 4 files changed, 327 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..aae9a95fb188 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" > @@ -99,6 +101,18 @@ static bool force_rfkill; > static bool micmute_led_registered; > static bool mute_led_registered; > > +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; > + > module_param(force_rfkill, bool, 0444); > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); > > @@ -2183,6 +2197,277 @@ static struct led_classdev mute_led_cdev = { > .default_trigger = "audio-mute", > }; > > +static int dell_send_request_by_token_loc(struct calling_interface_buffer *buffer, > + u16 class, u16 select, int type, > + u32 val) "int type" argument is not type, but tokenid. tokenid is 16-bit unsigned integer, so it would be better to use "u16 tokenid" instead of "int type". And same applies for all functions below too. And I think that better name for this function could be "dell_set_token_value" or something like that. "_by_token_loc" sounds strange, this function is setting smbios token to some value. > +{ > + struct calling_interface_token *token; > + > + token = dell_smbios_find_token(type); > + if (!token) > + return -ENODEV; > + > + /* -1 is a sentinel value, telling us to use token->value */ > + if (val == (u32)-1) > + 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; > + > + /* -1 means use the value from the token */ > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > + SELECT_TOKEN_STD, type, -1); > +} > + > +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]; buffer.output[1] is of type u32. So theoretically it can contain value above 2^31. For safety it would be better to check if the output[1] value fits into INT_MAX and if not then return something like -ERANGE (or some other better errno code). > +} > + > +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; This check is redundant and can be dropped. dell_battery_read() returns -ENODEV when token (type) does not exist. So you could treat negative return value from dell_battery_read() as "return false". > + > + return token->value == dell_battery_read(type); token->value is of unsigned u16 type and dell_battery_read() returns signed int. So maybe some explicit cast could useful for documentation purposes. > +} > + > +/* The rules: the minimum start charging value is 50%. The maximum nit: Multiline non-network comment starts with "/*" on separate line and comment text continue on next line (not the same line). > + * 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_set_custom_charge_start(int start) > +{ > + struct calling_interface_buffer buffer; > + int end; > + > + if (start < CHARGE_START_MIN) > + start = CHARGE_START_MIN; > + else if (start > CHARGE_START_MAX) > + start = CHARGE_START_MAX; > + > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > + /* a failed read is okay */ Why is failed read okay? It sounds strange if we ignore firmware errors. I think that if reading the custom charge value is failing we should not continue and trying to set/change custom charge value. Also if the returned value is above 100 (%), should be continue? > + if (end < 0) > + end = CHARGE_END_MAX; > + if ((end - start) < CHARGE_MIN_DIFF) nit: I'm not sure what is the correct coding style for kernel drivers but I thought that parenthesis around (end - start) are not being written. > + start = end - CHARGE_MIN_DIFF; > + > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_START, start); > +} > + > +static int dell_battery_set_custom_charge_end(int end) > +{ > + struct calling_interface_buffer buffer; > + int start; > + > + if (end < CHARGE_END_MIN) > + end = CHARGE_END_MIN; > + else if (end > CHARGE_END_MAX) > + end = CHARGE_END_MAX; > + > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > + /* a failed read is okay */ > + if (start < 0) > + start = CHARGE_START_MIN; > + if ((end - start) < CHARGE_MIN_DIFF) > + end = start + CHARGE_MIN_DIFF; > + > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_END, end); > +} > + > +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++) { > + bool active; > + > + if (!(battery_supported_modes & BIT(i))) > + continue; > + > + active = dell_battery_mode_is_active(battery_modes[i].token); > + count += sysfs_emit_at(buf, count, active ? "[%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) > +{ > + bool matched = false; > + int err, i; > + > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { nit: Personally I would put the "if (!(battery_supported_modes & BIT(i)))" check here with continue, to have same pattern in _show and _store functions. And also if we want to support battery mode which will have different tokens on different machines (see below for possibility). > + if (sysfs_streq(battery_modes[i].label, buf)) { > + matched = true; > + break; > + } > + } > + if (!matched || !(battery_supported_modes & BIT(i))) > + return -EINVAL; > + > + err = dell_battery_set_mode(battery_modes[i].token); > + if (err) > + return err; > + > + return size; > +} > + > +static ssize_t charge_control_start_threshold_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int start; > + > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > + if (start < 0) > + return start; IIRC the value is in percentage. So we should also check that returned value is not above 100 (and return some error in case). > + > + 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) > + return ret; I think that there should be some sanity validation. If format is percentage then we should not accept from userspace value outside of [0, 100] range. > + > + ret = dell_battery_set_custom_charge_start(start); > + if (ret) > + return ret; > + > + return size; > +} > + > +static ssize_t charge_control_end_threshold_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int end; > + > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > + if (end < 0) > + return end; > + > + return sysfs_emit(buf, "%d\n", end); > +} > + > +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) > + return ret; > + > + ret = dell_battery_set_custom_charge_end(end); > + if (ret) > + return ret; > + > + return size; > +} > + > +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 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 void __init dell_battery_init(struct device *dev) > +{ > + battery_supported_modes = battery_get_supported_modes(); > + > + if (battery_supported_modes != 0) > + battery_hook_register(&dell_battery_hook); Anyway, how is this battery_hook_register() suppose to work on systems with multiple batteries? The provided API is only for the primary battery, but on older Dell laptop it was possible to connect up to 3 batteries. Would not this case some issues? I have one Dell laptop which has an option to connect secondary battery, but I do not have the compatible secondary battery to test it. Has somebody Dell laptop with 2 or 3 batteries? It would be really good to test this patch how it would behave... > +} > + > +static void __exit dell_battery_exit(void) > +{ > + if (battery_supported_modes != 0) > + battery_hook_unregister(&dell_battery_hook); > +} > + > static int __init dell_init(void) > { > struct calling_interface_token *token; > @@ -2219,6 +2504,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 +2579,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 +2598,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..77baa15eb523 100644 > --- a/drivers/platform/x86/dell/dell-smbios.h > +++ b/drivers/platform/x86/dell/dell-smbios.h > @@ -33,6 +33,13 @@ > #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 There is defined also tokenid 0x0348 which is per dell libsmbios's token_list.csv documentation also for custom mode. It is marked as deprecated, so seems that it is for old laptops, which do not support tokenid 0x0343. What about defining '#define BAT_CUSTOM_OLD_MODE_TOKEN 0x0348', then adding '{ BAT_CUSTOM_OLD_MODE_TOKEN, "custom" }' into battery_modes and changing battery_get_supported_modes() to mask out custom_old bit when the normal custom is supported? Seems that it would be OK, just the charge_type_store() needs modification as written above. Just an idea. What do you think? > +#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 > -- > 2.39.2 > > > > > -- > I'm available for contract & employment work, see: > https://spindle.queued.net/~dilinger/resume-tech.pdf
On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote: > Hello, the driver change looks good. I have just few minor comments for > this change below. > > Anyway, if there is somebody on the list with Dell laptop with 2 or 3 > batteries, see below, it would be nice to check how such laptop would > behave with this patch. It does not seem that patch should cause > regression but always it is better to do testing if it is possible. > > On Tuesday 23 July 2024 22:05:02 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> > > --- > > Changes in v2, based on extensive feedback from Pali Rohár <pali@kernel.org>: > > - code style changes > > - change battery write API to use token->value instead of passed value > > - stop caching current mode, instead querying SMBIOS as needed > > - drop the separate list of charging modes enum > > - rework the list of charging mode strings > > - query SMBIOS for supported charging modes > > - split dell_battery_custom_set() up > > --- > > .../ABI/testing/sysfs-class-power-dell | 31 ++ > > drivers/platform/x86/dell/Kconfig | 1 + > > drivers/platform/x86/dell/dell-laptop.c | 288 ++++++++++++++++++ > > drivers/platform/x86/dell/dell-smbios.h | 7 + > > 4 files changed, 327 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..aae9a95fb188 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" > > @@ -99,6 +101,18 @@ static bool force_rfkill; > > static bool micmute_led_registered; > > static bool mute_led_registered; > > > > +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; > > + > > module_param(force_rfkill, bool, 0444); > > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); > > > > @@ -2183,6 +2197,277 @@ static struct led_classdev mute_led_cdev = { > > .default_trigger = "audio-mute", > > }; > > > > +static int dell_send_request_by_token_loc(struct calling_interface_buffer *buffer, > > + u16 class, u16 select, int type, > > + u32 val) > > "int type" argument is not type, but tokenid. tokenid is 16-bit unsigned > integer, so it would be better to use "u16 tokenid" instead of "int type". > And same applies for all functions below too. > > And I think that better name for this function could be > "dell_set_token_value" or something like that. "_by_token_loc" sounds > strange, this function is setting smbios token to some value. Ou. Just now I figured out that this function is universal and is doing both SET and GET, based on the "u16 select" parameter. It is not SET-ONLY as I originally thought. So what about rather name "dell_send_request_for_tokenid"? And because CLASS_TOKEN_WRITE is being repeated, what about defining something like this? static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value) { dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value); } Just an idea. Do you think that it could be useful in second patch? > > +{ > > + struct calling_interface_token *token; > > + > > + token = dell_smbios_find_token(type); > > + if (!token) > > + return -ENODEV; > > + > > + /* -1 is a sentinel value, telling us to use token->value */ > > + if (val == (u32)-1) > > + 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; > > + > > + /* -1 means use the value from the token */ > > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > > + SELECT_TOKEN_STD, type, -1); > > +} > > + > > +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]; > > buffer.output[1] is of type u32. So theoretically it can contain value > above 2^31. For safety it would be better to check if the output[1] > value fits into INT_MAX and if not then return something like -ERANGE > (or some other better errno code). > > > +} > > + > > +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; > > This check is redundant and can be dropped. > > dell_battery_read() returns -ENODEV when token (type) does not exist. > So you could treat negative return value from dell_battery_read() as > "return false". > > > + > > + return token->value == dell_battery_read(type); > > token->value is of unsigned u16 type and dell_battery_read() returns > signed int. So maybe some explicit cast could useful for documentation > purposes. > > > +} > > + > > +/* The rules: the minimum start charging value is 50%. The maximum > > nit: Multiline non-network comment starts with "/*" on separate line and > comment text continue on next line (not the same line). > > > + * 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_set_custom_charge_start(int start) > > +{ > > + struct calling_interface_buffer buffer; > > + int end; > > + > > + if (start < CHARGE_START_MIN) > > + start = CHARGE_START_MIN; > > + else if (start > CHARGE_START_MAX) > > + start = CHARGE_START_MAX; > > + > > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > > + /* a failed read is okay */ > > Why is failed read okay? It sounds strange if we ignore firmware errors. > I think that if reading the custom charge value is failing we should not > continue and trying to set/change custom charge value. > > Also if the returned value is above 100 (%), should be continue? > > > + if (end < 0) > > + end = CHARGE_END_MAX; > > + if ((end - start) < CHARGE_MIN_DIFF) > > nit: I'm not sure what is the correct coding style for kernel drivers > but I thought that parenthesis around (end - start) are not being > written. > > > + start = end - CHARGE_MIN_DIFF; > > + > > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_START, start); > > +} > > + > > +static int dell_battery_set_custom_charge_end(int end) > > +{ > > + struct calling_interface_buffer buffer; > > + int start; > > + > > + if (end < CHARGE_END_MIN) > > + end = CHARGE_END_MIN; > > + else if (end > CHARGE_END_MAX) > > + end = CHARGE_END_MAX; > > + > > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > > + /* a failed read is okay */ > > + if (start < 0) > > + start = CHARGE_START_MIN; > > + if ((end - start) < CHARGE_MIN_DIFF) > > + end = start + CHARGE_MIN_DIFF; > > + > > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_END, end); > > +} > > + > > +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++) { > > + bool active; > > + > > + if (!(battery_supported_modes & BIT(i))) > > + continue; > > + > > + active = dell_battery_mode_is_active(battery_modes[i].token); > > + count += sysfs_emit_at(buf, count, active ? "[%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) > > +{ > > + bool matched = false; > > + int err, i; > > + > > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { > > nit: Personally I would put the "if (!(battery_supported_modes & BIT(i)))" > check here with continue, to have same pattern in _show and _store > functions. And also if we want to support battery mode which will have > different tokens on different machines (see below for possibility). > > > + if (sysfs_streq(battery_modes[i].label, buf)) { > > + matched = true; > > + break; > > + } > > + } > > + if (!matched || !(battery_supported_modes & BIT(i))) > > + return -EINVAL; > > + > > + err = dell_battery_set_mode(battery_modes[i].token); > > + if (err) > > + return err; > > + > > + return size; > > +} > > + > > +static ssize_t charge_control_start_threshold_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + int start; > > + > > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > > + if (start < 0) > > + return start; > > IIRC the value is in percentage. So we should also check that returned > value is not above 100 (and return some error in case). > > > + > > + 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) > > + return ret; > > I think that there should be some sanity validation. If format is > percentage then we should not accept from userspace value outside of > [0, 100] range. > > > + > > + ret = dell_battery_set_custom_charge_start(start); > > + if (ret) > > + return ret; > > + > > + return size; > > +} > > + > > +static ssize_t charge_control_end_threshold_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + int end; > > + > > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > > + if (end < 0) > > + return end; > > + > > + return sysfs_emit(buf, "%d\n", end); > > +} > > + > > +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) > > + return ret; > > + > > + ret = dell_battery_set_custom_charge_end(end); > > + if (ret) > > + return ret; > > + > > + return size; > > +} > > + > > +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 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 void __init dell_battery_init(struct device *dev) > > +{ > > + battery_supported_modes = battery_get_supported_modes(); > > + > > + if (battery_supported_modes != 0) > > + battery_hook_register(&dell_battery_hook); > > Anyway, how is this battery_hook_register() suppose to work on systems > with multiple batteries? The provided API is only for the primary > battery, but on older Dell laptop it was possible to connect up to > 3 batteries. Would not this case some issues? > > I have one Dell laptop which has an option to connect secondary > battery, but I do not have the compatible secondary battery to test it. > > Has somebody Dell laptop with 2 or 3 batteries? It would be really good > to test this patch how it would behave... > > > +} > > + > > +static void __exit dell_battery_exit(void) > > +{ > > + if (battery_supported_modes != 0) > > + battery_hook_unregister(&dell_battery_hook); > > +} > > + > > static int __init dell_init(void) > > { > > struct calling_interface_token *token; > > @@ -2219,6 +2504,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 +2579,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 +2598,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..77baa15eb523 100644 > > --- a/drivers/platform/x86/dell/dell-smbios.h > > +++ b/drivers/platform/x86/dell/dell-smbios.h > > @@ -33,6 +33,13 @@ > > #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 > > There is defined also tokenid 0x0348 which is per dell libsmbios's > token_list.csv documentation also for custom mode. It is marked as > deprecated, so seems that it is for old laptops, which do not support > tokenid 0x0343. > > What about defining '#define BAT_CUSTOM_OLD_MODE_TOKEN 0x0348', then > adding '{ BAT_CUSTOM_OLD_MODE_TOKEN, "custom" }' into battery_modes and > changing battery_get_supported_modes() to mask out custom_old bit when > the normal custom is supported? Seems that it would be OK, just the > charge_type_store() needs modification as written above. > > Just an idea. What do you think? > > > +#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 > > -- > > 2.39.2 > > > > > > > > > > -- > > I'm available for contract & employment work, see: > > https://spindle.queued.net/~dilinger/resume-tech.pdf
On Wed, 24 Jul 2024 22:45:23 +0200 Pali Rohár <pali@kernel.org> wrote: > On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote: > > Hello, the driver change looks good. I have just few minor comments for > > this change below. > > > > Anyway, if there is somebody on the list with Dell laptop with 2 or 3 > > batteries, see below, it would be nice to check how such laptop would > > behave with this patch. It does not seem that patch should cause > > regression but always it is better to do testing if it is possible. > > > > On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote: [...] > And because CLASS_TOKEN_WRITE is being repeated, what about defining > something like this? > > static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value) > { > dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value); > } > > Just an idea. Do you think that it could be useful in second patch? > Let me implement the other changes first and then take a look. > > > + > > > +static int dell_battery_set_custom_charge_start(int start) > > > +{ > > > + struct calling_interface_buffer buffer; > > > + int end; > > > + > > > + if (start < CHARGE_START_MIN) > > > + start = CHARGE_START_MIN; > > > + else if (start > CHARGE_START_MAX) > > > + start = CHARGE_START_MAX; > > > + > > > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > > > + /* a failed read is okay */ > > > > Why is failed read okay? It sounds strange if we ignore firmware errors. > > I think that if reading the custom charge value is failing we should not > > continue and trying to set/change custom charge value. Because the check itself is simply a sanity check to ensure that the start value is not larger than then end value. I thought that if it fails, it's not a big deal; the sanity check originally wasn't even there. However, you're right that if we're failing to communicate w/ SMBIOS, that likely indicates a bigger problem and we probably shouldn't continue. > > > > Also if the returned value is above 100 (%), should be continue? Right, I'd forgotten that we shouldn't trust a BIOS. :) > > > > > + if (end < 0) > > > + end = CHARGE_END_MAX; > > > + if ((end - start) < CHARGE_MIN_DIFF) > > > > nit: I'm not sure what is the correct coding style for kernel drivers > > but I thought that parenthesis around (end - start) are not being > > written. > > > > > + start = end - CHARGE_MIN_DIFF; > > > + > > > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > > > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_START, start); > > > +} > > > + > > > +static int dell_battery_set_custom_charge_end(int end) > > > +{ > > > + struct calling_interface_buffer buffer; > > > + int start; > > > + > > > + if (end < CHARGE_END_MIN) > > > + end = CHARGE_END_MIN; > > > + else if (end > CHARGE_END_MAX) > > > + end = CHARGE_END_MAX; > > > + > > > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > > > + /* a failed read is okay */ > > > + if (start < 0) > > > + start = CHARGE_START_MIN; > > > + if ((end - start) < CHARGE_MIN_DIFF) > > > + end = start + CHARGE_MIN_DIFF; > > > + > > > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > > > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_END, end); > > > +} > > > + > > > +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++) { > > > + bool active; > > > + > > > + if (!(battery_supported_modes & BIT(i))) > > > + continue; > > > + > > > + active = dell_battery_mode_is_active(battery_modes[i].token); > > > + count += sysfs_emit_at(buf, count, active ? "[%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) > > > +{ > > > + bool matched = false; > > > + int err, i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { > > > > nit: Personally I would put the "if (!(battery_supported_modes & BIT(i)))" > > check here with continue, to have same pattern in _show and _store > > functions. And also if we want to support battery mode which will have > > different tokens on different machines (see below for possibility). > > Certainly for possible future cases; yeah, it's smarter to handle modes with conflicting names. > > > + if (sysfs_streq(battery_modes[i].label, buf)) { > > > + matched = true; > > > + break; > > > + } > > > + } > > > + if (!matched || !(battery_supported_modes & BIT(i))) > > > + return -EINVAL; > > > + > > > + err = dell_battery_set_mode(battery_modes[i].token); > > > + if (err) > > > + return err; > > > + > > > + return size; > > > +} > > > + > > > +static ssize_t charge_control_start_threshold_show(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + int start; > > > + > > > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > > > + if (start < 0) > > > + return start; > > > > IIRC the value is in percentage. So we should also check that returned > > value is not above 100 (and return some error in case). > > > > > + > > > + 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) > > > + return ret; > > > > I think that there should be some sanity validation. If format is > > percentage then we should not accept from userspace value outside of > > [0, 100] range. > > dell_battery_set_custom_charge_start() does validate that the userspace values are correct. It will actually round up or down rather than returning -EINVAL. So if userspace sends the "150" for start charge, it will round it down to 95% (CHARGE_START_MAX) and then round it down further to the end charge value minus 5% (CHARGE_MIN_DIFF). This behavior matches what's documented in Documentation/ABI/testing/sysfs-class-power > > > + > > > + ret = dell_battery_set_custom_charge_start(start); > > > + if (ret) > > > + return ret; > > > + > > > + return size; > > > +} > > > + > > > +static ssize_t charge_control_end_threshold_show(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + int end; > > > + > > > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > > > + if (end < 0) > > > + return end; > > > + > > > + return sysfs_emit(buf, "%d\n", end); > > > +} > > > + > > > +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) > > > + return ret; > > > + > > > + ret = dell_battery_set_custom_charge_end(end); > > > + if (ret) > > > + return ret; > > > + > > > + return size; > > > +} > > > + > > > +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 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 void __init dell_battery_init(struct device *dev) > > > +{ > > > + battery_supported_modes = battery_get_supported_modes(); > > > + > > > + if (battery_supported_modes != 0) > > > + battery_hook_register(&dell_battery_hook); > > > > Anyway, how is this battery_hook_register() suppose to work on systems > > with multiple batteries? The provided API is only for the primary > > battery, but on older Dell laptop it was possible to connect up to > > 3 batteries. Would not this case some issues? This interface is _only_ for controlling charging of the primary battery. In theory, it should hopefully ignore any other batteries, which would need to have their settings changed in the BIOS or with a special tool or whatever. However, I'm basing that assumption on the SMBIOS interface. These tokens are marked "Primary Battery". There are separate tokens marked "Battery Slice", which from my understanding was an older type of battery that plugged into the docking port on old E* series Latitudes: https://dl.dell.com/manuals/all-products/esuprt_laptop/esuprt_latitude_laptop/latitude-e5420_user's%20guide_en-us.pdf There are also separate tokens marked "Dock Battery", which control charging behavior for batteries that are.. part of the dock? It's unclear, and I don't have the hardware. So I'm pretty comfortable with the assumption that this charging control interface will ignore all batteries other than the primary battery. Of course, more testing would be great, and I haven't personally dealt with multiple batteries. > > > > I have one Dell laptop which has an option to connect secondary > > battery, but I do not have the compatible secondary battery to test it. How does it connect? Is the interface one of the types that I mentioned above (slice & dock), or a different type? > > > > Has somebody Dell laptop with 2 or 3 batteries? It would be really good > > to test this patch how it would behave... Agreed. > > > > > +} > > > + > > > +static void __exit dell_battery_exit(void) > > > +{ > > > + if (battery_supported_modes != 0) > > > + battery_hook_unregister(&dell_battery_hook); > > > +} > > > + > > > static int __init dell_init(void) > > > { > > > struct calling_interface_token *token; > > > @@ -2219,6 +2504,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 +2579,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 +2598,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..77baa15eb523 100644 > > > --- a/drivers/platform/x86/dell/dell-smbios.h > > > +++ b/drivers/platform/x86/dell/dell-smbios.h > > > @@ -33,6 +33,13 @@ > > > #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 > > > > There is defined also tokenid 0x0348 which is per dell libsmbios's > > token_list.csv documentation also for custom mode. It is marked as > > deprecated, so seems that it is for old laptops, which do not support > > tokenid 0x0343. > > > > What about defining '#define BAT_CUSTOM_OLD_MODE_TOKEN 0x0348', then > > adding '{ BAT_CUSTOM_OLD_MODE_TOKEN, "custom" }' into battery_modes and > > changing battery_get_supported_modes() to mask out custom_old bit when > > the normal custom is supported? Seems that it would be OK, just the > > charge_type_store() needs modification as written above. > > > > Just an idea. What do you think? > > Generally, I'm nervous about touching stuff like that that I can't personally test. I don't know if 0348 works the same way as 0343, and the libsmbios code that added 0343 usage (<https://github.com/dell/libsmbios/commit/0eab0085e8f0db3c2a0f8ae0146d3352c40d785a>) never actually used 0348 previously. Or, it could be that 0348 was deprecated due to buggy semantics or implementation. I just don't know. If there's old code or documentation somewhere that shows how 0348 worked, and it turns out to be similar to 0343, then I wouldn't be opposed to adding BAT_CUSTOM_OLD_MODE_TOKEN and checking for it. However, without any kind of example, I'd prefer to just ignore it and only support custom mode charging on machines that have 0343. My 10yo Latitude has 0343, so I suspect that we're targeting the vast majority of (still working) hardware with 0343. > > > +#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 > > > -- > > > 2.39.2 > > > > > > > > > > > > > > > -- > > > I'm available for contract & employment work, see: > > > https://spindle.queued.net/~dilinger/resume-tech.pdf
On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: > On Wed, 24 Jul 2024 22:45:23 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote: > > > Hello, the driver change looks good. I have just few minor comments for > > > this change below. > > > > > > Anyway, if there is somebody on the list with Dell laptop with 2 or 3 > > > batteries, see below, it would be nice to check how such laptop would > > > behave with this patch. It does not seem that patch should cause > > > regression but always it is better to do testing if it is possible. > > > > > > On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote: > [...] > > And because CLASS_TOKEN_WRITE is being repeated, what about defining > > something like this? > > > > static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value) > > { > > dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value); > > } > > > > Just an idea. Do you think that it could be useful in second patch? > > > > Let me implement the other changes first and then take a look. Ok. > > > > + > > > > +static int dell_battery_set_custom_charge_start(int start) > > > > +{ > > > > + struct calling_interface_buffer buffer; > > > > + int end; > > > > + > > > > + if (start < CHARGE_START_MIN) > > > > + start = CHARGE_START_MIN; > > > > + else if (start > CHARGE_START_MAX) > > > > + start = CHARGE_START_MAX; > > > > + > > > > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > > > > + /* a failed read is okay */ > > > > > > Why is failed read okay? It sounds strange if we ignore firmware errors. > > > I think that if reading the custom charge value is failing we should not > > > continue and trying to set/change custom charge value. > > Because the check itself is simply a sanity check to ensure that the > start value is not larger than then end value. I thought that if it fails, > it's not a big deal; the sanity check originally wasn't even there. > > However, you're right that if we're failing to communicate w/ SMBIOS, > that likely indicates a bigger problem and we probably shouldn't > continue. > > > > > > > > > Also if the returned value is above 100 (%), should be continue? > > Right, I'd forgotten that we shouldn't trust a BIOS. :) For example in driver dell-smm-hwmon.c we have to validate what BIOS returns because sometimes it returns bogus value. This is not some hypothetical condition, but real problem confirmed by more users. So checking what Dell BIOS returns is really good idea. > > > > > > > > + if (end < 0) > > > > + end = CHARGE_END_MAX; > > > > + if ((end - start) < CHARGE_MIN_DIFF) > > > > > > nit: I'm not sure what is the correct coding style for kernel drivers > > > but I thought that parenthesis around (end - start) are not being > > > written. > > > > > > > + start = end - CHARGE_MIN_DIFF; > > > > + > > > > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > > > > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_START, start); > > > > +} > > > > + > > > > +static int dell_battery_set_custom_charge_end(int end) > > > > +{ > > > > + struct calling_interface_buffer buffer; > > > > + int start; > > > > + > > > > + if (end < CHARGE_END_MIN) > > > > + end = CHARGE_END_MIN; > > > > + else if (end > CHARGE_END_MAX) > > > > + end = CHARGE_END_MAX; > > > > + > > > > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > > > > + /* a failed read is okay */ > > > > + if (start < 0) > > > > + start = CHARGE_START_MIN; > > > > + if ((end - start) < CHARGE_MIN_DIFF) > > > > + end = start + CHARGE_MIN_DIFF; > > > > + > > > > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > > > > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_END, end); > > > > +} > > > > + > > > > +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++) { > > > > + bool active; > > > > + > > > > + if (!(battery_supported_modes & BIT(i))) > > > > + continue; > > > > + > > > > + active = dell_battery_mode_is_active(battery_modes[i].token); > > > > + count += sysfs_emit_at(buf, count, active ? "[%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) > > > > +{ > > > > + bool matched = false; > > > > + int err, i; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { > > > > > > nit: Personally I would put the "if (!(battery_supported_modes & BIT(i)))" > > > check here with continue, to have same pattern in _show and _store > > > functions. And also if we want to support battery mode which will have > > > different tokens on different machines (see below for possibility). > > > > > Certainly for possible future cases; yeah, it's smarter to handle modes with > conflicting names. > > > > > > + if (sysfs_streq(battery_modes[i].label, buf)) { > > > > + matched = true; > > > > + break; > > > > + } > > > > + } > > > > + if (!matched || !(battery_supported_modes & BIT(i))) > > > > + return -EINVAL; > > > > + > > > > + err = dell_battery_set_mode(battery_modes[i].token); > > > > + if (err) > > > > + return err; > > > > + > > > > + return size; > > > > +} > > > > + > > > > +static ssize_t charge_control_start_threshold_show(struct device *dev, > > > > + struct device_attribute *attr, > > > > + char *buf) > > > > +{ > > > > + int start; > > > > + > > > > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > > > > + if (start < 0) > > > > + return start; > > > > > > IIRC the value is in percentage. So we should also check that returned > > > value is not above 100 (and return some error in case). > > > > > > > + > > > > + 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) > > > > + return ret; > > > > > > I think that there should be some sanity validation. If format is > > > percentage then we should not accept from userspace value outside of > > > [0, 100] range. > > > > > dell_battery_set_custom_charge_start() does validate that the userspace > values are correct. It will actually round up or down rather than > returning -EINVAL. So if userspace sends the "150" for start charge, > it will round it down to 95% (CHARGE_START_MAX) and then round it down > further to the end charge value minus 5% (CHARGE_MIN_DIFF). This behavior > matches what's documented in Documentation/ABI/testing/sysfs-class-power Rounding value to the nearest supported value is correct (e.g. 99% to 95%) and documented in that file. But it is really correct to accept value -1% or 1000000%? Now I have just opened the first driver platform/x86/asus-wmi.c which implements this interface and it returns -EINVAL for value < 0 || value > 100. > > > > > + > > > > + ret = dell_battery_set_custom_charge_start(start); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + return size; > > > > +} > > > > + > > > > +static ssize_t charge_control_end_threshold_show(struct device *dev, > > > > + struct device_attribute *attr, > > > > + char *buf) > > > > +{ > > > > + int end; > > > > + > > > > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > > > > + if (end < 0) > > > > + return end; > > > > + > > > > + return sysfs_emit(buf, "%d\n", end); > > > > +} > > > > + > > > > +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) > > > > + return ret; > > > > + > > > > + ret = dell_battery_set_custom_charge_end(end); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + return size; > > > > +} > > > > + > > > > +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 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 void __init dell_battery_init(struct device *dev) > > > > +{ > > > > + battery_supported_modes = battery_get_supported_modes(); > > > > + > > > > + if (battery_supported_modes != 0) > > > > + battery_hook_register(&dell_battery_hook); > > > > > > Anyway, how is this battery_hook_register() suppose to work on systems > > > with multiple batteries? The provided API is only for the primary > > > battery, but on older Dell laptop it was possible to connect up to > > > 3 batteries. Would not this case some issues? > > This interface is _only_ for controlling charging of the primary battery. > In theory, it should hopefully ignore any other batteries, which would > need to have their settings changed in the BIOS or with a special tool or > whatever. That is OK. But where it is specified that the hook is being registered only for the primary battery? Because from the usage it looks like that the hook is applied either for all batteries present in the system or for some one arbitrary chosen battery. > However, I'm basing that assumption on the SMBIOS interface. These tokens > are marked "Primary Battery". There are separate tokens marked "Battery > Slice", which from my understanding was an older type of battery that From SMBIOS perspective it is clear, each battery seems to have its own tokens. The issue here is: how to tell kernel that the particular dell_battery_hook has to be bound with the primary battery? > plugged into the docking port on old E* series Latitudes: > https://dl.dell.com/manuals/all-products/esuprt_laptop/esuprt_latitude_laptop/latitude-e5420_user's%20guide_en-us.pdf > There are also separate tokens marked "Dock Battery", which control > charging behavior for batteries that are.. part of the dock? It's > unclear, and I don't have the hardware. IIRC both "Dock Battery" and "Slice Battery" means battery type connected to the connector on the bottom of the laptop. Same connector is used for connecting laptop to dock. As different models had different type of connector, probably also different name for battery were used. And there was also another "Media Bay" battery which was being connected to the slot where was normally either optical drive or second HDD. > So I'm pretty comfortable with the assumption that this charging > control interface will ignore all batteries other than the primary > battery. The SMBIOS code will ignore for sure. But would not the battery_hook_register() function hook the SMBIOS interface to the "wrong" battery? > Of course, more testing would be great, and I haven't personally > dealt with multiple batteries. > > > > > > > > > I have one Dell laptop which has an option to connect secondary > > > battery, but I do not have the compatible secondary battery to test it. > > How does it connect? Is the interface one of the types that I mentioned > above (slice & dock), or a different type? Via bottom connector. So it is either slice or dock, do not know exactly as I do not have that battery. > > > > > > > Has somebody Dell laptop with 2 or 3 batteries? It would be really good > > > to test this patch how it would behave... > > Agreed. > > > > > > > > > +} > > > > + > > > > +static void __exit dell_battery_exit(void) > > > > +{ > > > > + if (battery_supported_modes != 0) > > > > + battery_hook_unregister(&dell_battery_hook); > > > > +} > > > > + > > > > static int __init dell_init(void) > > > > { > > > > struct calling_interface_token *token; > > > > @@ -2219,6 +2504,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 +2579,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 +2598,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..77baa15eb523 100644 > > > > --- a/drivers/platform/x86/dell/dell-smbios.h > > > > +++ b/drivers/platform/x86/dell/dell-smbios.h > > > > @@ -33,6 +33,13 @@ > > > > #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 > > > > > > There is defined also tokenid 0x0348 which is per dell libsmbios's > > > token_list.csv documentation also for custom mode. It is marked as > > > deprecated, so seems that it is for old laptops, which do not support > > > tokenid 0x0343. > > > > > > What about defining '#define BAT_CUSTOM_OLD_MODE_TOKEN 0x0348', then > > > adding '{ BAT_CUSTOM_OLD_MODE_TOKEN, "custom" }' into battery_modes and > > > changing battery_get_supported_modes() to mask out custom_old bit when > > > the normal custom is supported? Seems that it would be OK, just the > > > charge_type_store() needs modification as written above. > > > > > > Just an idea. What do you think? > > > > > Generally, I'm nervous about touching stuff like that that I can't > personally test. I don't know if 0348 works the same way as 0343, and > the libsmbios code that added 0343 usage > (<https://github.com/dell/libsmbios/commit/0eab0085e8f0db3c2a0f8ae0146d3352c40d785a>) > never actually used 0348 previously. Or, it could be that 0348 was > deprecated due to buggy semantics or implementation. I just don't > know. > > If there's old code or documentation somewhere that shows how 0348 > worked, and it turns out to be similar to 0343, then I wouldn't be > opposed to adding BAT_CUSTOM_OLD_MODE_TOKEN and checking for it. > However, without any kind of example, I'd prefer to just ignore it > and only support custom mode charging on machines that have 0343. > My 10yo Latitude has 0343, so I suspect that we're targeting the vast > majority of (still working) hardware with 0343. Ok, I think that for now let it as is. It can be extended in the future after proper inspection. That mentioned dell commit does not contain any useful information about it. > > > > > +#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 > > > > -- > > > > 2.39.2 > > > > > > > > > > > > > > > > > > > > -- > > > > I'm available for contract & employment work, see: > > > > https://spindle.queued.net/~dilinger/resume-tech.pdf > > > > -- > I'm available for contract & employment work, see: > https://spindle.queued.net/~dilinger/resume-tech.pdf
On Thu, 25 Jul 2024 01:01:58 +0200 Pali Rohár <pali@kernel.org> wrote: > On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: > > On Wed, 24 Jul 2024 22:45:23 +0200 > > Pali Rohár <pali@kernel.org> wrote: > > > > > On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote: > > > > Hello, the driver change looks good. I have just few minor comments for > > > > this change below. > > > > > > > > Anyway, if there is somebody on the list with Dell laptop with 2 or 3 > > > > batteries, see below, it would be nice to check how such laptop would > > > > behave with this patch. It does not seem that patch should cause > > > > regression but always it is better to do testing if it is possible. > > > > > > > > On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote: > > [...] > > > And because CLASS_TOKEN_WRITE is being repeated, what about defining > > > something like this? > > > > > > static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value) > > > { > > > dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value); > > > } > > > > > > Just an idea. Do you think that it could be useful in second patch? > > > > > > > Let me implement the other changes first and then take a look. > > Ok. > For the helper function, I noticed that all of the CLASS_TOKEN_WRITEs also have SELECT_TOKEN_STD except for one (dell_send_intensity). So I think it makes sense to have the helper function also do that as well. Eg, static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer, u16 tokenid, u32 value) { dell_send_request_for_tokenid(buffer, SELECT_TOKEN_STD, CLASS_TOKEN_WRITE, tokenid, value); } I agree with your renaming to dell_send_request_for_tokenid, btw. > > > > > +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]; > > > > > > > > buffer.output[1] is of type u32. So theoretically it can contain value > > > > above 2^31. For safety it would be better to check if the output[1] > > > > value fits into INT_MAX and if not then return something like -ERANGE > > > > (or some other better errno code). I ended up returning -EIO here, with the logic that if we're getting some nonsense value from SMBIOS, it could either be junk in the stored values or communication corruption. Likewise, I used -EIO for the checks in charge_control_start_threshold_show and charge_control_end_threshold_show when SMBIOS returns values > 100%. > > > > > > > > > > > > + if (end < 0) > > > > > + end = CHARGE_END_MAX; > > > > > + if ((end - start) < CHARGE_MIN_DIFF) > > > > > > > > nit: I'm not sure what is the correct coding style for kernel drivers > > > > but I thought that parenthesis around (end - start) are not being > > > > written. > > > > I think it makes the comparison much easier to read. I'd prefer to keep it, unless the coding style specifically forbids it. > > > > > + > > > > > +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 void __init dell_battery_init(struct device *dev) > > > > > +{ > > > > > + battery_supported_modes = battery_get_supported_modes(); > > > > > + > > > > > + if (battery_supported_modes != 0) > > > > > + battery_hook_register(&dell_battery_hook); > > > > > > > > Anyway, how is this battery_hook_register() suppose to work on systems > > > > with multiple batteries? The provided API is only for the primary > > > > battery, but on older Dell laptop it was possible to connect up to > > > > 3 batteries. Would not this case some issues? > > > > This interface is _only_ for controlling charging of the primary battery. > > In theory, it should hopefully ignore any other batteries, which would > > need to have their settings changed in the BIOS or with a special tool or > > whatever. > > That is OK. But where it is specified that the hook is being registered > only for the primary battery? Because from the usage it looks like that > the hook is applied either for all batteries present in the system or > for some one arbitrary chosen battery. > > > However, I'm basing that assumption on the SMBIOS interface. These tokens > > are marked "Primary Battery". There are separate tokens marked "Battery > > Slice", which from my understanding was an older type of battery that > > From SMBIOS perspective it is clear, each battery seems to have its own > tokens. > > The issue here is: how to tell kernel that the particular > dell_battery_hook has to be bound with the primary battery? > So from userspace, we've got the expectation that multiple batteries would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1, and so on. The current BAT0 entry shows things like 'capacity' even without this patch, and we're just piggybacking off of that to add charge_type and other entries. So there shouldn't be any confusion there, agreed? In the kernel, we're registering the acpi_battery_hook as "Dell Primary Battery Extension". The functions set up by that acpi_battery_hook are the only ones using battery_support_modes. We could make it more explicit by: 1) renaming it to primary_battery_modes, along with dell_primary_battery_{init,exit} and/or 2) allocating the mode mask and strings dynamically, and storing that inside of the dev kobject. However, #2 seems overly complicated for what we're doing. In the circumstances that we want to add support for secondary batteries, we're going to need to query separate tokens, add another acpi_battery_hook, and also add a second mask. Whether that's a global variable or kept inside pdev seems like more of a style issue than anything else. #1 is easy enough to change, if you think that's necessary.
On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: > On Thu, 25 Jul 2024 01:01:58 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: > > > On Wed, 24 Jul 2024 22:45:23 +0200 > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote: > > > > > Hello, the driver change looks good. I have just few minor comments for > > > > > this change below. > > > > > > > > > > Anyway, if there is somebody on the list with Dell laptop with 2 or 3 > > > > > batteries, see below, it would be nice to check how such laptop would > > > > > behave with this patch. It does not seem that patch should cause > > > > > regression but always it is better to do testing if it is possible. > > > > > > > > > > On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote: > > > [...] > > > > And because CLASS_TOKEN_WRITE is being repeated, what about defining > > > > something like this? > > > > > > > > static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value) > > > > { > > > > dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value); > > > > } > > > > > > > > Just an idea. Do you think that it could be useful in second patch? > > > > > > > > > > Let me implement the other changes first and then take a look. > > > > Ok. > > > > For the helper function, I noticed that all of the CLASS_TOKEN_WRITEs > also have SELECT_TOKEN_STD except for one (dell_send_intensity). So I > think it makes sense to have the helper function also do that as well. > Eg, > > static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer, u16 tokenid, u32 value) > { > dell_send_request_for_tokenid(buffer, SELECT_TOKEN_STD, CLASS_TOKEN_WRITE, tokenid, value); > } > > I agree with your renaming to dell_send_request_for_tokenid, btw. > > > > > > > > +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]; > > > > > > > > > > buffer.output[1] is of type u32. So theoretically it can contain value > > > > > above 2^31. For safety it would be better to check if the output[1] > > > > > value fits into INT_MAX and if not then return something like -ERANGE > > > > > (or some other better errno code). > > > I ended up returning -EIO here, with the logic that if we're getting > some nonsense value from SMBIOS, it could either be junk in the stored > values or communication corruption. > > Likewise, I used -EIO for the checks in charge_control_start_threshold_show > and charge_control_end_threshold_show when SMBIOS returns values > 100%. > > > > > > > > > > > > > > > > > > > + if (end < 0) > > > > > > + end = CHARGE_END_MAX; > > > > > > + if ((end - start) < CHARGE_MIN_DIFF) > > > > > > > > > > nit: I'm not sure what is the correct coding style for kernel drivers > > > > > but I thought that parenthesis around (end - start) are not being > > > > > written. > > > > > > > I think it makes the comparison much easier to read. I'd prefer to > keep it, unless the coding style specifically forbids it. As I said I'm really not sure. So if nobody would complain then you can let it as is. You can use ./scripts/checkpatch.pl application which is in git tree, which does basic checks for code style. It cannot prove if something is really correct but it can prove if something is incorrect. > > > > > > > > > + > > > > > > +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 void __init dell_battery_init(struct device *dev) > > > > > > +{ > > > > > > + battery_supported_modes = battery_get_supported_modes(); > > > > > > + > > > > > > + if (battery_supported_modes != 0) > > > > > > + battery_hook_register(&dell_battery_hook); > > > > > > > > > > Anyway, how is this battery_hook_register() suppose to work on systems > > > > > with multiple batteries? The provided API is only for the primary > > > > > battery, but on older Dell laptop it was possible to connect up to > > > > > 3 batteries. Would not this case some issues? > > > > > > This interface is _only_ for controlling charging of the primary battery. > > > In theory, it should hopefully ignore any other batteries, which would > > > need to have their settings changed in the BIOS or with a special tool or > > > whatever. > > > > That is OK. But where it is specified that the hook is being registered > > only for the primary battery? Because from the usage it looks like that > > the hook is applied either for all batteries present in the system or > > for some one arbitrary chosen battery. > > > > > However, I'm basing that assumption on the SMBIOS interface. These tokens > > > are marked "Primary Battery". There are separate tokens marked "Battery > > > Slice", which from my understanding was an older type of battery that > > > > From SMBIOS perspective it is clear, each battery seems to have its own > > tokens. > > > > The issue here is: how to tell kernel that the particular > > dell_battery_hook has to be bound with the primary battery? > > > > So from userspace, we've got the expectation that multiple batteries > would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1, > and so on. Yes, I hope so. > The current BAT0 entry shows things like 'capacity' even without this > patch, and we're just piggybacking off of that to add charge_type and > other entries. So there shouldn't be any confusion there, agreed? I have not looked at the battery_hook_register() code yet (seems that I would have to properly read it and understand it). But does it mean that battery_hook_register() is adding hook just for "BAT0"? What I mean: cannot that hook be registered to "BAT1" too? Because if yes then we should prevent it. Otherwise this hook which is for "Dell Primary Battery" could be registered also for secondary battery "BAT1". (I hope that now it is more clear what I mean). > In the kernel, we're registering the acpi_battery_hook as "Dell Primary > Battery Extension". The functions set up by that acpi_battery_hook are > the only ones using battery_support_modes. We could make it more explicit > by: > 1) renaming it to primary_battery_modes, along with > dell_primary_battery_{init,exit} and/or > 2) allocating the mode mask and strings dynamically, and storing that > inside of the dev kobject. > > However, #2 seems overly complicated for what we're doing. In the > circumstances that we want to add support for secondary batteries, > we're going to need to query separate tokens, add another > acpi_battery_hook, and also add a second mask. Whether that's a global > variable or kept inside pdev seems like more of a style issue than > anything else. > > #1 is easy enough to change, if you think that's necessary. I think that "Dell Primary Battery Extension" is OK. All SMBIOS code is currently primary-battery only. The only my point is to prevent this &dell_battery_hook to be registered for non-primary battery.
Am 26.07.24 um 00:15 schrieb Pali Rohár: > On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: >> On Thu, 25 Jul 2024 01:01:58 +0200 >> Pali Rohár <pali@kernel.org> wrote: >> >>> On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: >>>> On Wed, 24 Jul 2024 22:45:23 +0200 >>>> Pali Rohár <pali@kernel.org> wrote: >>>> >>>>> On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote: >>>>>> Hello, the driver change looks good. I have just few minor comments for >>>>>> this change below. >>>>>> >>>>>> Anyway, if there is somebody on the list with Dell laptop with 2 or 3 >>>>>> batteries, see below, it would be nice to check how such laptop would >>>>>> behave with this patch. It does not seem that patch should cause >>>>>> regression but always it is better to do testing if it is possible. >>>>>> >>>>>> On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote: >>>> [...] >>>>> And because CLASS_TOKEN_WRITE is being repeated, what about defining >>>>> something like this? >>>>> >>>>> static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value) >>>>> { >>>>> dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value); >>>>> } >>>>> >>>>> Just an idea. Do you think that it could be useful in second patch? >>>>> >>>> Let me implement the other changes first and then take a look. >>> Ok. >>> >> For the helper function, I noticed that all of the CLASS_TOKEN_WRITEs >> also have SELECT_TOKEN_STD except for one (dell_send_intensity). So I >> think it makes sense to have the helper function also do that as well. >> Eg, >> >> static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer, u16 tokenid, u32 value) >> { >> dell_send_request_for_tokenid(buffer, SELECT_TOKEN_STD, CLASS_TOKEN_WRITE, tokenid, value); >> } >> >> I agree with your renaming to dell_send_request_for_tokenid, btw. >> >> >>>>>>> +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]; >>>>>> buffer.output[1] is of type u32. So theoretically it can contain value >>>>>> above 2^31. For safety it would be better to check if the output[1] >>>>>> value fits into INT_MAX and if not then return something like -ERANGE >>>>>> (or some other better errno code). >> >> I ended up returning -EIO here, with the logic that if we're getting >> some nonsense value from SMBIOS, it could either be junk in the stored >> values or communication corruption. >> >> Likewise, I used -EIO for the checks in charge_control_start_threshold_show >> and charge_control_end_threshold_show when SMBIOS returns values > 100%. >> >> >> >>>> >>>>>> >>>>>>> + if (end < 0) >>>>>>> + end = CHARGE_END_MAX; >>>>>>> + if ((end - start) < CHARGE_MIN_DIFF) >>>>>> nit: I'm not sure what is the correct coding style for kernel drivers >>>>>> but I thought that parenthesis around (end - start) are not being >>>>>> written. >>>>>> >> I think it makes the comparison much easier to read. I'd prefer to >> keep it, unless the coding style specifically forbids it. > As I said I'm really not sure. So if nobody would complain then you can > let it as is. > > You can use ./scripts/checkpatch.pl application which is in git tree, > which does basic checks for code style. It cannot prove if something is > really correct but it can prove if something is incorrect. > >> >> >>>>>>> + >>>>>>> +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 void __init dell_battery_init(struct device *dev) >>>>>>> +{ >>>>>>> + battery_supported_modes = battery_get_supported_modes(); >>>>>>> + >>>>>>> + if (battery_supported_modes != 0) >>>>>>> + battery_hook_register(&dell_battery_hook); >>>>>> Anyway, how is this battery_hook_register() suppose to work on systems >>>>>> with multiple batteries? The provided API is only for the primary >>>>>> battery, but on older Dell laptop it was possible to connect up to >>>>>> 3 batteries. Would not this case some issues? >>>> This interface is _only_ for controlling charging of the primary battery. >>>> In theory, it should hopefully ignore any other batteries, which would >>>> need to have their settings changed in the BIOS or with a special tool or >>>> whatever. >>> That is OK. But where it is specified that the hook is being registered >>> only for the primary battery? Because from the usage it looks like that >>> the hook is applied either for all batteries present in the system or >>> for some one arbitrary chosen battery. >>> >>>> However, I'm basing that assumption on the SMBIOS interface. These tokens >>>> are marked "Primary Battery". There are separate tokens marked "Battery >>>> Slice", which from my understanding was an older type of battery that >>> From SMBIOS perspective it is clear, each battery seems to have its own >>> tokens. >>> >>> The issue here is: how to tell kernel that the particular >>> dell_battery_hook has to be bound with the primary battery? >>> >> So from userspace, we've got the expectation that multiple batteries >> would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1, >> and so on. > Yes, I hope so. > >> The current BAT0 entry shows things like 'capacity' even without this >> patch, and we're just piggybacking off of that to add charge_type and >> other entries. So there shouldn't be any confusion there, agreed? > I have not looked at the battery_hook_register() code yet (seems that I > would have to properly read it and understand it). But does it mean that > battery_hook_register() is adding hook just for "BAT0"? > > What I mean: cannot that hook be registered to "BAT1" too? Because if > yes then we should prevent it. Otherwise this hook which is for "Dell > Primary Battery" could be registered also for secondary battery "BAT1". > (I hope that now it is more clear what I mean). Hi, the battery hook is being registered to all ACPI batteries present on a given system, so you need to do some manual filtering when .add_battery() is called. As a side note: i suspect that "newer" Dell machines use a different interface for controlling battery charging, since the Dell Power Manager software on my machine seems to provide some additional features not found in this token-based interface. Unfortunately i am not sure if reverse-engineering the Dell software is legal, does the kernel community have some helping guides in this area? If it is legal, then i would happily volunteer to do the reverse-engineering. Otherwise maybe someone at Dell can provide some clarifications if a different interface for controlling battery charging exists and how to use it? Thanks, Armin Wolf >> In the kernel, we're registering the acpi_battery_hook as "Dell Primary >> Battery Extension". The functions set up by that acpi_battery_hook are >> the only ones using battery_support_modes. We could make it more explicit >> by: >> 1) renaming it to primary_battery_modes, along with >> dell_primary_battery_{init,exit} and/or >> 2) allocating the mode mask and strings dynamically, and storing that >> inside of the dev kobject. >> >> However, #2 seems overly complicated for what we're doing. In the >> circumstances that we want to add support for secondary batteries, >> we're going to need to query separate tokens, add another >> acpi_battery_hook, and also add a second mask. Whether that's a global >> variable or kept inside pdev seems like more of a style issue than >> anything else. >> >> #1 is easy enough to change, if you think that's necessary. > I think that "Dell Primary Battery Extension" is OK. All SMBIOS code is > currently primary-battery only. > > The only my point is to prevent this &dell_battery_hook to be registered > for non-primary battery. >
On Friday 26 July 2024 01:48:50 Armin Wolf wrote: > Am 26.07.24 um 00:15 schrieb Pali Rohár: > > > On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: > > > On Thu, 25 Jul 2024 01:01:58 +0200 > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: > > > > > On Wed, 24 Jul 2024 22:45:23 +0200 > > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > > > On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote: > > > > > > > Hello, the driver change looks good. I have just few minor comments for > > > > > > > this change below. > > > > > > > > > > > > > > Anyway, if there is somebody on the list with Dell laptop with 2 or 3 > > > > > > > batteries, see below, it would be nice to check how such laptop would > > > > > > > behave with this patch. It does not seem that patch should cause > > > > > > > regression but always it is better to do testing if it is possible. > > > > > > > > > > > > > > On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote: > > > > > [...] > > > > > > And because CLASS_TOKEN_WRITE is being repeated, what about defining > > > > > > something like this? > > > > > > > > > > > > static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value) > > > > > > { > > > > > > dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value); > > > > > > } > > > > > > > > > > > > Just an idea. Do you think that it could be useful in second patch? > > > > > > > > > > > Let me implement the other changes first and then take a look. > > > > Ok. > > > > > > > For the helper function, I noticed that all of the CLASS_TOKEN_WRITEs > > > also have SELECT_TOKEN_STD except for one (dell_send_intensity). So I > > > think it makes sense to have the helper function also do that as well. > > > Eg, > > > > > > static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer, u16 tokenid, u32 value) > > > { > > > dell_send_request_for_tokenid(buffer, SELECT_TOKEN_STD, CLASS_TOKEN_WRITE, tokenid, value); > > > } > > > > > > I agree with your renaming to dell_send_request_for_tokenid, btw. > > > > > > > > > > > > > > +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]; > > > > > > > buffer.output[1] is of type u32. So theoretically it can contain value > > > > > > > above 2^31. For safety it would be better to check if the output[1] > > > > > > > value fits into INT_MAX and if not then return something like -ERANGE > > > > > > > (or some other better errno code). > > > > > > I ended up returning -EIO here, with the logic that if we're getting > > > some nonsense value from SMBIOS, it could either be junk in the stored > > > values or communication corruption. > > > > > > Likewise, I used -EIO for the checks in charge_control_start_threshold_show > > > and charge_control_end_threshold_show when SMBIOS returns values > 100%. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + if (end < 0) > > > > > > > > + end = CHARGE_END_MAX; > > > > > > > > + if ((end - start) < CHARGE_MIN_DIFF) > > > > > > > nit: I'm not sure what is the correct coding style for kernel drivers > > > > > > > but I thought that parenthesis around (end - start) are not being > > > > > > > written. > > > > > > > > > > I think it makes the comparison much easier to read. I'd prefer to > > > keep it, unless the coding style specifically forbids it. > > As I said I'm really not sure. So if nobody would complain then you can > > let it as is. > > > > You can use ./scripts/checkpatch.pl application which is in git tree, > > which does basic checks for code style. It cannot prove if something is > > really correct but it can prove if something is incorrect. > > > > > > > > > > > > > > > > + > > > > > > > > +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 void __init dell_battery_init(struct device *dev) > > > > > > > > +{ > > > > > > > > + battery_supported_modes = battery_get_supported_modes(); > > > > > > > > + > > > > > > > > + if (battery_supported_modes != 0) > > > > > > > > + battery_hook_register(&dell_battery_hook); > > > > > > > Anyway, how is this battery_hook_register() suppose to work on systems > > > > > > > with multiple batteries? The provided API is only for the primary > > > > > > > battery, but on older Dell laptop it was possible to connect up to > > > > > > > 3 batteries. Would not this case some issues? > > > > > This interface is _only_ for controlling charging of the primary battery. > > > > > In theory, it should hopefully ignore any other batteries, which would > > > > > need to have their settings changed in the BIOS or with a special tool or > > > > > whatever. > > > > That is OK. But where it is specified that the hook is being registered > > > > only for the primary battery? Because from the usage it looks like that > > > > the hook is applied either for all batteries present in the system or > > > > for some one arbitrary chosen battery. > > > > > > > > > However, I'm basing that assumption on the SMBIOS interface. These tokens > > > > > are marked "Primary Battery". There are separate tokens marked "Battery > > > > > Slice", which from my understanding was an older type of battery that > > > > From SMBIOS perspective it is clear, each battery seems to have its own > > > > tokens. > > > > > > > > The issue here is: how to tell kernel that the particular > > > > dell_battery_hook has to be bound with the primary battery? > > > > > > > So from userspace, we've got the expectation that multiple batteries > > > would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1, > > > and so on. > > Yes, I hope so. > > > > > The current BAT0 entry shows things like 'capacity' even without this > > > patch, and we're just piggybacking off of that to add charge_type and > > > other entries. So there shouldn't be any confusion there, agreed? > > I have not looked at the battery_hook_register() code yet (seems that I > > would have to properly read it and understand it). But does it mean that > > battery_hook_register() is adding hook just for "BAT0"? > > > > What I mean: cannot that hook be registered to "BAT1" too? Because if > > yes then we should prevent it. Otherwise this hook which is for "Dell > > Primary Battery" could be registered also for secondary battery "BAT1". > > (I hope that now it is more clear what I mean). > > Hi, > > the battery hook is being registered to all ACPI batteries present on a given system, > so you need to do some manual filtering when .add_battery() is called. Ok. So it means that the filtering based on the primary battery in add_battery callback is needed. > As a side note: i suspect that "newer" Dell machines use a different interface for controlling > battery charging, since the Dell Power Manager software on my machine seems to provide some > additional features not found in this token-based interface. Dell has released documentation of some other API, see the end of this file https://github.com/dell/libsmbios/blob/master/src/bin/smbios-battery-ctl > Unfortunately i am not sure if reverse-engineering the Dell software is legal, does the kernel > community have some helping guides in this area? If it is legal, then i would happily volunteer > to do the reverse-engineering. That is questionable. Some kernel drivers were written from reverse engineered data in past. Note that in some countries is reverse engineering legal if it is done for interoperability purposes (which this one could match). > Otherwise maybe someone at Dell can provide some clarifications if a different interface for controlling > battery charging exists and how to use it? Try to send an off-list/private email to Dell.Client.Kernel@dell.com with details for what you are asking. Maybe they would have access to some new documentation. > Thanks, > Armin Wolf > > > > In the kernel, we're registering the acpi_battery_hook as "Dell Primary > > > Battery Extension". The functions set up by that acpi_battery_hook are > > > the only ones using battery_support_modes. We could make it more explicit > > > by: > > > 1) renaming it to primary_battery_modes, along with > > > dell_primary_battery_{init,exit} and/or > > > 2) allocating the mode mask and strings dynamically, and storing that > > > inside of the dev kobject. > > > > > > However, #2 seems overly complicated for what we're doing. In the > > > circumstances that we want to add support for secondary batteries, > > > we're going to need to query separate tokens, add another > > > acpi_battery_hook, and also add a second mask. Whether that's a global > > > variable or kept inside pdev seems like more of a style issue than > > > anything else. > > > > > > #1 is easy enough to change, if you think that's necessary. > > I think that "Dell Primary Battery Extension" is OK. All SMBIOS code is > > currently primary-battery only. > > > > The only my point is to prevent this &dell_battery_hook to be registered > > for non-primary battery. > >
On Fri, 26 Jul 2024 02:04:09 +0200 Pali Rohár <pali@kernel.org> wrote: > On Friday 26 July 2024 01:48:50 Armin Wolf wrote: > > Am 26.07.24 um 00:15 schrieb Pali Rohár: > > > > > On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: > > > > On Thu, 25 Jul 2024 01:01:58 +0200 > > > > Pali Rohár <pali@kernel.org> wrote: > > > > > > > > > On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: [...] > > > > > > > > > > The issue here is: how to tell kernel that the particular > > > > > dell_battery_hook has to be bound with the primary battery? > > > > > > > > > So from userspace, we've got the expectation that multiple batteries > > > > would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1, > > > > and so on. > > > Yes, I hope so. > > > > > > > The current BAT0 entry shows things like 'capacity' even without this > > > > patch, and we're just piggybacking off of that to add charge_type and > > > > other entries. So there shouldn't be any confusion there, agreed? > > > I have not looked at the battery_hook_register() code yet (seems that I > > > would have to properly read it and understand it). But does it mean that > > > battery_hook_register() is adding hook just for "BAT0"? > > > > > > What I mean: cannot that hook be registered to "BAT1" too? Because if > > > yes then we should prevent it. Otherwise this hook which is for "Dell > > > Primary Battery" could be registered also for secondary battery "BAT1". > > > (I hope that now it is more clear what I mean). > > > > Hi, > > > > the battery hook is being registered to all ACPI batteries present on a given system, > > so you need to do some manual filtering when .add_battery() is called. > > Ok. So it means that the filtering based on the primary battery in > add_battery callback is needed. > Thanks for the explanations. Seems simple enough to fix that, as some of the other drivers are checking battery->desc->name for "BAT0". One thing that I keep coming back to, and was reinforced as I looked at include/linux/power_supply.h; the generic power supply charge_type has values that are very close to Dells, but with different names. I could shoehorn them in, though, with the following mappings: POWER_SUPPLY_CHARGE_TYPE_FAST, => "express" (aka ExpressCharge) POWER_SUPPLY_CHARGE_TYPE_STANDARD, => "standard" POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE, => "adaptive" POWER_SUPPLY_CHARGE_TYPE_CUSTOM, => "custom" POWER_SUPPLY_CHARGE_TYPE_LONGLIFE, => "primarily_ac" The main difference is that Primarily AC is described and documented as slightly different than Long Life, but I suspect the result is roughly the same thing. And the naming "Fast" and "Long Life" wouldn't match the BIOS naming of "ExpressCharge" and "Primarily AC". Until now I've opted to match the BIOS naming, but I'm curious what others think before I send V3 of the patches.
Am 26.07.24 um 06:25 schrieb Andres Salomon: > On Fri, 26 Jul 2024 02:04:09 +0200 > Pali Rohár <pali@kernel.org> wrote: > >> On Friday 26 July 2024 01:48:50 Armin Wolf wrote: >>> Am 26.07.24 um 00:15 schrieb Pali Rohár: >>> >>>> On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: >>>>> On Thu, 25 Jul 2024 01:01:58 +0200 >>>>> Pali Rohár <pali@kernel.org> wrote: >>>>> >>>>>> On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: > [...] >>>>>> The issue here is: how to tell kernel that the particular >>>>>> dell_battery_hook has to be bound with the primary battery? >>>>>> >>>>> So from userspace, we've got the expectation that multiple batteries >>>>> would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1, >>>>> and so on. >>>> Yes, I hope so. >>>> >>>>> The current BAT0 entry shows things like 'capacity' even without this >>>>> patch, and we're just piggybacking off of that to add charge_type and >>>>> other entries. So there shouldn't be any confusion there, agreed? >>>> I have not looked at the battery_hook_register() code yet (seems that I >>>> would have to properly read it and understand it). But does it mean that >>>> battery_hook_register() is adding hook just for "BAT0"? >>>> >>>> What I mean: cannot that hook be registered to "BAT1" too? Because if >>>> yes then we should prevent it. Otherwise this hook which is for "Dell >>>> Primary Battery" could be registered also for secondary battery "BAT1". >>>> (I hope that now it is more clear what I mean). >>> Hi, >>> >>> the battery hook is being registered to all ACPI batteries present on a given system, >>> so you need to do some manual filtering when .add_battery() is called. >> Ok. So it means that the filtering based on the primary battery in >> add_battery callback is needed. >> > Thanks for the explanations. Seems simple enough to fix that, as some of > the other drivers are checking battery->desc->name for "BAT0". > > > One thing that I keep coming back to, and was reinforced as I looked at > include/linux/power_supply.h; the generic power supply charge_type has > values that are very close to Dells, but with different names. I could > shoehorn them in, though, with the following mappings: > > POWER_SUPPLY_CHARGE_TYPE_FAST, => "express" (aka ExpressCharge) > POWER_SUPPLY_CHARGE_TYPE_STANDARD, => "standard" > POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE, => "adaptive" > POWER_SUPPLY_CHARGE_TYPE_CUSTOM, => "custom" > POWER_SUPPLY_CHARGE_TYPE_LONGLIFE, => "primarily_ac" > > The main difference is that Primarily AC is described and documented as > slightly different than Long Life, but I suspect the result is roughly > the same thing. And the naming "Fast" and "Long Life" wouldn't match the > BIOS naming of "ExpressCharge" and "Primarily AC". > > Until now I've opted to match the BIOS naming, but I'm curious what others > think before I send V3 of the patches. I agree that POWER_SUPPLY_CHARGE_TYPE_FAST should be mapped the ExpressCharge, but i think that "primarily_ac" should become a official power supply charging mode. The reason is that for example the wilco-charger driver also supports such a charging mode (currently reported as POWER_SUPPLY_CHARGE_TYPE_TRICKLE) and the charging mode seems to be both sufficiently different from POWER_SUPPLY_CHARGE_TYPE_LONGLIFE/POWER_SUPPLY_CHARGE_TYPE_TRICKLE and sufficiently generic to be supported by a wide array of devices. Thanks, Armin Wolf
Am 26.07.24 um 20:42 schrieb Armin Wolf: > Am 26.07.24 um 06:25 schrieb Andres Salomon: > >> On Fri, 26 Jul 2024 02:04:09 +0200 >> Pali Rohár <pali@kernel.org> wrote: >> >>> On Friday 26 July 2024 01:48:50 Armin Wolf wrote: >>>> Am 26.07.24 um 00:15 schrieb Pali Rohár: >>>> >>>>> On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: >>>>>> On Thu, 25 Jul 2024 01:01:58 +0200 >>>>>> Pali Rohár <pali@kernel.org> wrote: >>>>>> >>>>>>> On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: >> [...] >>>>>>> The issue here is: how to tell kernel that the particular >>>>>>> dell_battery_hook has to be bound with the primary battery? >>>>>>> >>>>>> So from userspace, we've got the expectation that multiple batteries >>>>>> would show up as /sys/class/power_supply/BAT0, >>>>>> /sys/class/power_supply/BAT1, >>>>>> and so on. >>>>> Yes, I hope so. >>>>> >>>>>> The current BAT0 entry shows things like 'capacity' even without >>>>>> this >>>>>> patch, and we're just piggybacking off of that to add charge_type >>>>>> and >>>>>> other entries. So there shouldn't be any confusion there, agreed? >>>>> I have not looked at the battery_hook_register() code yet (seems >>>>> that I >>>>> would have to properly read it and understand it). But does it >>>>> mean that >>>>> battery_hook_register() is adding hook just for "BAT0"? >>>>> >>>>> What I mean: cannot that hook be registered to "BAT1" too? Because if >>>>> yes then we should prevent it. Otherwise this hook which is for "Dell >>>>> Primary Battery" could be registered also for secondary battery >>>>> "BAT1". >>>>> (I hope that now it is more clear what I mean). >>>> Hi, >>>> >>>> the battery hook is being registered to all ACPI batteries present >>>> on a given system, >>>> so you need to do some manual filtering when .add_battery() is called. >>> Ok. So it means that the filtering based on the primary battery in >>> add_battery callback is needed. >>> >> Thanks for the explanations. Seems simple enough to fix that, as some of >> the other drivers are checking battery->desc->name for "BAT0". >> >> >> One thing that I keep coming back to, and was reinforced as I looked at >> include/linux/power_supply.h; the generic power supply charge_type has >> values that are very close to Dells, but with different names. I could >> shoehorn them in, though, with the following mappings: >> >> POWER_SUPPLY_CHARGE_TYPE_FAST, => "express" (aka ExpressCharge) >> POWER_SUPPLY_CHARGE_TYPE_STANDARD, => "standard" >> POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE, => "adaptive" >> POWER_SUPPLY_CHARGE_TYPE_CUSTOM, => "custom" >> POWER_SUPPLY_CHARGE_TYPE_LONGLIFE, => "primarily_ac" >> >> The main difference is that Primarily AC is described and documented as >> slightly different than Long Life, but I suspect the result is roughly >> the same thing. And the naming "Fast" and "Long Life" wouldn't match the >> BIOS naming of "ExpressCharge" and "Primarily AC". >> >> Until now I've opted to match the BIOS naming, but I'm curious what >> others >> think before I send V3 of the patches. > > I agree that POWER_SUPPLY_CHARGE_TYPE_FAST should be mapped the > ExpressCharge, > but i think that "primarily_ac" should become a official power supply > charging mode. > > The reason is that for example the wilco-charger driver also supports > such a charging mode > (currently reported as POWER_SUPPLY_CHARGE_TYPE_TRICKLE) and the > charging mode seems to be > both sufficiently different from > POWER_SUPPLY_CHARGE_TYPE_LONGLIFE/POWER_SUPPLY_CHARGE_TYPE_TRICKLE > and sufficiently generic to be supported by a wide array of devices. > > Thanks, > Armin Wolf > I just read the documentation regarding the charge_type sysfs attribute and it states that: Trickle: Extends battery lifespan, intended for users who primarily use their Chromebook while connected to AC. So i think that "primarily_ac" should be mapped to POWER_SUPPLY_CHARGE_TYPE_TRICKLE. Thanks, Armin Wolf
On Fri, 26 Jul 2024 20:46:22 +0200 Armin Wolf <W_Armin@gmx.de> wrote: > Am 26.07.24 um 20:42 schrieb Armin Wolf: > > > Am 26.07.24 um 06:25 schrieb Andres Salomon: > > > >> On Fri, 26 Jul 2024 02:04:09 +0200 > >> Pali Rohár <pali@kernel.org> wrote: > >> > >>> On Friday 26 July 2024 01:48:50 Armin Wolf wrote: > >>>> Am 26.07.24 um 00:15 schrieb Pali Rohár: > >>>> > >>>>> On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: > >>>>>> On Thu, 25 Jul 2024 01:01:58 +0200 > >>>>>> Pali Rohár <pali@kernel.org> wrote: > >>>>>> > >>>>>>> On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: > >> [...] > >>>>>>> The issue here is: how to tell kernel that the particular > >>>>>>> dell_battery_hook has to be bound with the primary battery? > >>>>>>> > >>>>>> So from userspace, we've got the expectation that multiple batteries > >>>>>> would show up as /sys/class/power_supply/BAT0, > >>>>>> /sys/class/power_supply/BAT1, > >>>>>> and so on. > >>>>> Yes, I hope so. > >>>>> > >>>>>> The current BAT0 entry shows things like 'capacity' even without > >>>>>> this > >>>>>> patch, and we're just piggybacking off of that to add charge_type > >>>>>> and > >>>>>> other entries. So there shouldn't be any confusion there, agreed? > >>>>> I have not looked at the battery_hook_register() code yet (seems > >>>>> that I > >>>>> would have to properly read it and understand it). But does it > >>>>> mean that > >>>>> battery_hook_register() is adding hook just for "BAT0"? > >>>>> > >>>>> What I mean: cannot that hook be registered to "BAT1" too? Because if > >>>>> yes then we should prevent it. Otherwise this hook which is for "Dell > >>>>> Primary Battery" could be registered also for secondary battery > >>>>> "BAT1". > >>>>> (I hope that now it is more clear what I mean). > >>>> Hi, > >>>> > >>>> the battery hook is being registered to all ACPI batteries present > >>>> on a given system, > >>>> so you need to do some manual filtering when .add_battery() is called. > >>> Ok. So it means that the filtering based on the primary battery in > >>> add_battery callback is needed. > >>> > >> Thanks for the explanations. Seems simple enough to fix that, as some of > >> the other drivers are checking battery->desc->name for "BAT0". > >> > >> > >> One thing that I keep coming back to, and was reinforced as I looked at > >> include/linux/power_supply.h; the generic power supply charge_type has > >> values that are very close to Dells, but with different names. I could > >> shoehorn them in, though, with the following mappings: > >> > >> POWER_SUPPLY_CHARGE_TYPE_FAST, => "express" (aka ExpressCharge) > >> POWER_SUPPLY_CHARGE_TYPE_STANDARD, => "standard" > >> POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE, => "adaptive" > >> POWER_SUPPLY_CHARGE_TYPE_CUSTOM, => "custom" > >> POWER_SUPPLY_CHARGE_TYPE_LONGLIFE, => "primarily_ac" > >> > >> The main difference is that Primarily AC is described and documented as > >> slightly different than Long Life, but I suspect the result is roughly > >> the same thing. And the naming "Fast" and "Long Life" wouldn't match the > >> BIOS naming of "ExpressCharge" and "Primarily AC". > >> > >> Until now I've opted to match the BIOS naming, but I'm curious what > >> others > >> think before I send V3 of the patches. > > > > I agree that POWER_SUPPLY_CHARGE_TYPE_FAST should be mapped the > > ExpressCharge, > > but i think that "primarily_ac" should become a official power supply > > charging mode. > > > > The reason is that for example the wilco-charger driver also supports > > such a charging mode > > (currently reported as POWER_SUPPLY_CHARGE_TYPE_TRICKLE) and the > > charging mode seems to be > > both sufficiently different from > > POWER_SUPPLY_CHARGE_TYPE_LONGLIFE/POWER_SUPPLY_CHARGE_TYPE_TRICKLE > > and sufficiently generic to be supported by a wide array of devices. > > > > Thanks, > > Armin Wolf > > > I just read the documentation regarding the charge_type sysfs attribute and it states that: > > Trickle: > Extends battery lifespan, intended for users who > primarily use their Chromebook while connected to AC. > > So i think that "primarily_ac" should be mapped to POWER_SUPPLY_CHARGE_TYPE_TRICKLE. Do you think it's worth keeping around the sysfs-class-power-dell file? At this point it's basically just documenting the slight naming differences: Standard: Fully charge the battery at a moderate rate. Fast: Quickly charge the battery using fast-charge technology. This is harder on the battery than standard charging and may lower its lifespan. The Dell BIOS calls this ExpressCharge™. Trickle: Users who primarily operate the system while plugged into an external power source can extend battery life with this mode. The Dell BIOS calls this "Primarily AC Use". Adaptive: Automatically optimize battery charge rate based on typical usage pattern. 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", "Fast", "Trickle", "Adaptive", "Custom"
On Wednesday 07 August 2024 17:28:32 Andres Salomon wrote: > On Fri, 26 Jul 2024 20:46:22 +0200 > Armin Wolf <W_Armin@gmx.de> wrote: > > > Am 26.07.24 um 20:42 schrieb Armin Wolf: > > > > > Am 26.07.24 um 06:25 schrieb Andres Salomon: > > > > > >> On Fri, 26 Jul 2024 02:04:09 +0200 > > >> Pali Rohár <pali@kernel.org> wrote: > > >> > > >>> On Friday 26 July 2024 01:48:50 Armin Wolf wrote: > > >>>> Am 26.07.24 um 00:15 schrieb Pali Rohár: > > >>>> > > >>>>> On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: > > >>>>>> On Thu, 25 Jul 2024 01:01:58 +0200 > > >>>>>> Pali Rohár <pali@kernel.org> wrote: > > >>>>>> > > >>>>>>> On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: > > >> [...] > > >>>>>>> The issue here is: how to tell kernel that the particular > > >>>>>>> dell_battery_hook has to be bound with the primary battery? > > >>>>>>> > > >>>>>> So from userspace, we've got the expectation that multiple batteries > > >>>>>> would show up as /sys/class/power_supply/BAT0, > > >>>>>> /sys/class/power_supply/BAT1, > > >>>>>> and so on. > > >>>>> Yes, I hope so. > > >>>>> > > >>>>>> The current BAT0 entry shows things like 'capacity' even without > > >>>>>> this > > >>>>>> patch, and we're just piggybacking off of that to add charge_type > > >>>>>> and > > >>>>>> other entries. So there shouldn't be any confusion there, agreed? > > >>>>> I have not looked at the battery_hook_register() code yet (seems > > >>>>> that I > > >>>>> would have to properly read it and understand it). But does it > > >>>>> mean that > > >>>>> battery_hook_register() is adding hook just for "BAT0"? > > >>>>> > > >>>>> What I mean: cannot that hook be registered to "BAT1" too? Because if > > >>>>> yes then we should prevent it. Otherwise this hook which is for "Dell > > >>>>> Primary Battery" could be registered also for secondary battery > > >>>>> "BAT1". > > >>>>> (I hope that now it is more clear what I mean). > > >>>> Hi, > > >>>> > > >>>> the battery hook is being registered to all ACPI batteries present > > >>>> on a given system, > > >>>> so you need to do some manual filtering when .add_battery() is called. > > >>> Ok. So it means that the filtering based on the primary battery in > > >>> add_battery callback is needed. > > >>> > > >> Thanks for the explanations. Seems simple enough to fix that, as some of > > >> the other drivers are checking battery->desc->name for "BAT0". > > >> > > >> > > >> One thing that I keep coming back to, and was reinforced as I looked at > > >> include/linux/power_supply.h; the generic power supply charge_type has > > >> values that are very close to Dells, but with different names. I could > > >> shoehorn them in, though, with the following mappings: > > >> > > >> POWER_SUPPLY_CHARGE_TYPE_FAST, => "express" (aka ExpressCharge) > > >> POWER_SUPPLY_CHARGE_TYPE_STANDARD, => "standard" > > >> POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE, => "adaptive" > > >> POWER_SUPPLY_CHARGE_TYPE_CUSTOM, => "custom" > > >> POWER_SUPPLY_CHARGE_TYPE_LONGLIFE, => "primarily_ac" > > >> > > >> The main difference is that Primarily AC is described and documented as > > >> slightly different than Long Life, but I suspect the result is roughly > > >> the same thing. And the naming "Fast" and "Long Life" wouldn't match the > > >> BIOS naming of "ExpressCharge" and "Primarily AC". > > >> > > >> Until now I've opted to match the BIOS naming, but I'm curious what > > >> others > > >> think before I send V3 of the patches. > > > > > > I agree that POWER_SUPPLY_CHARGE_TYPE_FAST should be mapped the > > > ExpressCharge, > > > but i think that "primarily_ac" should become a official power supply > > > charging mode. > > > > > > The reason is that for example the wilco-charger driver also supports > > > such a charging mode > > > (currently reported as POWER_SUPPLY_CHARGE_TYPE_TRICKLE) and the > > > charging mode seems to be > > > both sufficiently different from > > > POWER_SUPPLY_CHARGE_TYPE_LONGLIFE/POWER_SUPPLY_CHARGE_TYPE_TRICKLE > > > and sufficiently generic to be supported by a wide array of devices. > > > > > > Thanks, > > > Armin Wolf > > > > > I just read the documentation regarding the charge_type sysfs attribute and it states that: > > > > Trickle: > > Extends battery lifespan, intended for users who > > primarily use their Chromebook while connected to AC. > > > > So i think that "primarily_ac" should be mapped to POWER_SUPPLY_CHARGE_TYPE_TRICKLE. > > Do you think it's worth keeping around the sysfs-class-power-dell > file? At this point it's basically just documenting the slight naming > differences: > > > Standard: > Fully charge the battery at a moderate rate. > Fast: > Quickly charge the battery using fast-charge > technology. This is harder on the battery than > standard charging and may lower its lifespan. > The Dell BIOS calls this ExpressCharge™. > Trickle: > Users who primarily operate the system while > plugged into an external power source can extend > battery life with this mode. The Dell BIOS calls > this "Primarily AC Use". > Adaptive: > Automatically optimize battery charge rate based > on typical usage pattern. > 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", "Fast", "Trickle", > "Adaptive", "Custom" Another option could be to extend the description in sysfs-class-power file that "Trickle" covers "Dell's Primarily AC Use" and "Fast" covers "Dell's ExpressCharge". So if somebody is going to implement charge_type for some other Laptop vendor then this information can help.
On Wed, 7 Aug 2024 23:44:13 +0200 Pali Rohár <pali@kernel.org> wrote: > On Wednesday 07 August 2024 17:28:32 Andres Salomon wrote: > > On Fri, 26 Jul 2024 20:46:22 +0200 > > Armin Wolf <W_Armin@gmx.de> wrote: > > > > > Am 26.07.24 um 20:42 schrieb Armin Wolf: > > > > > > > Am 26.07.24 um 06:25 schrieb Andres Salomon: > > > > > > > >> On Fri, 26 Jul 2024 02:04:09 +0200 > > > >> Pali Rohár <pali@kernel.org> wrote: > > > >> > > > >>> On Friday 26 July 2024 01:48:50 Armin Wolf wrote: > > > >>>> Am 26.07.24 um 00:15 schrieb Pali Rohár: > > > >>>> > > > >>>>> On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: > > > >>>>>> On Thu, 25 Jul 2024 01:01:58 +0200 > > > >>>>>> Pali Rohár <pali@kernel.org> wrote: > > > >>>>>> > > > >>>>>>> On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: > > > >> [...] > > > >>>>>>> The issue here is: how to tell kernel that the particular > > > >>>>>>> dell_battery_hook has to be bound with the primary battery? > > > >>>>>>> > > > >>>>>> So from userspace, we've got the expectation that multiple batteries > > > >>>>>> would show up as /sys/class/power_supply/BAT0, > > > >>>>>> /sys/class/power_supply/BAT1, > > > >>>>>> and so on. > > > >>>>> Yes, I hope so. > > > >>>>> > > > >>>>>> The current BAT0 entry shows things like 'capacity' even without > > > >>>>>> this > > > >>>>>> patch, and we're just piggybacking off of that to add charge_type > > > >>>>>> and > > > >>>>>> other entries. So there shouldn't be any confusion there, agreed? > > > >>>>> I have not looked at the battery_hook_register() code yet (seems > > > >>>>> that I > > > >>>>> would have to properly read it and understand it). But does it > > > >>>>> mean that > > > >>>>> battery_hook_register() is adding hook just for "BAT0"? > > > >>>>> > > > >>>>> What I mean: cannot that hook be registered to "BAT1" too? Because if > > > >>>>> yes then we should prevent it. Otherwise this hook which is for "Dell > > > >>>>> Primary Battery" could be registered also for secondary battery > > > >>>>> "BAT1". > > > >>>>> (I hope that now it is more clear what I mean). > > > >>>> Hi, > > > >>>> > > > >>>> the battery hook is being registered to all ACPI batteries present > > > >>>> on a given system, > > > >>>> so you need to do some manual filtering when .add_battery() is called. > > > >>> Ok. So it means that the filtering based on the primary battery in > > > >>> add_battery callback is needed. > > > >>> > > > >> Thanks for the explanations. Seems simple enough to fix that, as some of > > > >> the other drivers are checking battery->desc->name for "BAT0". > > > >> > > > >> > > > >> One thing that I keep coming back to, and was reinforced as I looked at > > > >> include/linux/power_supply.h; the generic power supply charge_type has > > > >> values that are very close to Dells, but with different names. I could > > > >> shoehorn them in, though, with the following mappings: > > > >> > > > >> POWER_SUPPLY_CHARGE_TYPE_FAST, => "express" (aka ExpressCharge) > > > >> POWER_SUPPLY_CHARGE_TYPE_STANDARD, => "standard" > > > >> POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE, => "adaptive" > > > >> POWER_SUPPLY_CHARGE_TYPE_CUSTOM, => "custom" > > > >> POWER_SUPPLY_CHARGE_TYPE_LONGLIFE, => "primarily_ac" > > > >> > > > >> The main difference is that Primarily AC is described and documented as > > > >> slightly different than Long Life, but I suspect the result is roughly > > > >> the same thing. And the naming "Fast" and "Long Life" wouldn't match the > > > >> BIOS naming of "ExpressCharge" and "Primarily AC". > > > >> > > > >> Until now I've opted to match the BIOS naming, but I'm curious what > > > >> others > > > >> think before I send V3 of the patches. > > > > > > > > I agree that POWER_SUPPLY_CHARGE_TYPE_FAST should be mapped the > > > > ExpressCharge, > > > > but i think that "primarily_ac" should become a official power supply > > > > charging mode. > > > > > > > > The reason is that for example the wilco-charger driver also supports > > > > such a charging mode > > > > (currently reported as POWER_SUPPLY_CHARGE_TYPE_TRICKLE) and the > > > > charging mode seems to be > > > > both sufficiently different from > > > > POWER_SUPPLY_CHARGE_TYPE_LONGLIFE/POWER_SUPPLY_CHARGE_TYPE_TRICKLE > > > > and sufficiently generic to be supported by a wide array of devices. > > > > > > > > Thanks, > > > > Armin Wolf > > > > > > > I just read the documentation regarding the charge_type sysfs attribute and it states that: > > > > > > Trickle: > > > Extends battery lifespan, intended for users who > > > primarily use their Chromebook while connected to AC. > > > > > > So i think that "primarily_ac" should be mapped to POWER_SUPPLY_CHARGE_TYPE_TRICKLE. > > > > Do you think it's worth keeping around the sysfs-class-power-dell > > file? At this point it's basically just documenting the slight naming > > differences: > > > > > > Standard: > > Fully charge the battery at a moderate rate. > > Fast: > > Quickly charge the battery using fast-charge > > technology. This is harder on the battery than > > standard charging and may lower its lifespan. > > The Dell BIOS calls this ExpressCharge™. > > Trickle: > > Users who primarily operate the system while > > plugged into an external power source can extend > > battery life with this mode. The Dell BIOS calls > > this "Primarily AC Use". > > Adaptive: > > Automatically optimize battery charge rate based > > on typical usage pattern. > > 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", "Fast", "Trickle", > > "Adaptive", "Custom" > > Another option could be to extend the description in sysfs-class-power > file that "Trickle" covers "Dell's Primarily AC Use" and "Fast" covers > "Dell's ExpressCharge". > > So if somebody is going to implement charge_type for some other Laptop > vendor then this information can help. How's this? @@ -378,8 +378,10 @@ Date: July 2009 Contact: linux-pm@vger.kernel.org Description: Represents the type of charging currently being applied to the - battery. "Trickle", "Fast", and "Standard" all mean different - charging speeds. "Adaptive" means that the charger uses some + battery. "Fast" and "Standard" mean different charging speeds. + "Trickle" means a slow charging speed, or (depending on the + hardware) charging optimized for being mostly plugged into + wall power. "Adaptive" means that the charger uses some algorithm to adjust the charge rate dynamically, without any user configuration required. "Custom" means that the charger
Hi, On Wed, Aug 07, 2024 at 06:05:11PM GMT, Andres Salomon wrote: > > > > [...] > > > > > > Do you think it's worth keeping around the sysfs-class-power-dell > > > file? At this point it's basically just documenting the slight naming > > > differences: > > > > > > > > > Standard: > > > Fully charge the battery at a moderate rate. > > > Fast: > > > Quickly charge the battery using fast-charge > > > technology. This is harder on the battery than > > > standard charging and may lower its lifespan. > > > The Dell BIOS calls this ExpressCharge™. > > > Trickle: > > > Users who primarily operate the system while > > > plugged into an external power source can extend > > > battery life with this mode. The Dell BIOS calls > > > this "Primarily AC Use". > > > Adaptive: > > > Automatically optimize battery charge rate based > > > on typical usage pattern. > > > 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", "Fast", "Trickle", > > > "Adaptive", "Custom" > > > > Another option could be to extend the description in sysfs-class-power > > file that "Trickle" covers "Dell's Primarily AC Use" and "Fast" covers > > "Dell's ExpressCharge". > > > > So if somebody is going to implement charge_type for some other Laptop > > vendor then this information can help. > > How's this? > > @@ -378,8 +378,10 @@ Date: July 2009 > Contact: linux-pm@vger.kernel.org > Description: > Represents the type of charging currently being applied to the > - battery. "Trickle", "Fast", and "Standard" all mean different > - charging speeds. "Adaptive" means that the charger uses some > + battery. "Fast" and "Standard" mean different charging speeds. > + "Trickle" means a slow charging speed, or (depending on the > + hardware) charging optimized for being mostly plugged into > + wall power. "Adaptive" means that the charger uses some > algorithm to adjust the charge rate dynamically, without > any user configuration required. "Custom" means that the charger Looks good from my side. OTOH I would also be fine with using your Dell "specific" documentation as a replacement for the generic documentation. But then "The Dell BIOS calls this XYZ" should be replaced with "On Dell machines this mode is called XYZ". Greetings, -- Sebastian
Am 26.07.24 um 02:04 schrieb Pali Rohár: > On Friday 26 July 2024 01:48:50 Armin Wolf wrote: >> Am 26.07.24 um 00:15 schrieb Pali Rohár: >> >>> On Thursday 25 July 2024 16:24:57 Andres Salomon wrote: >>>> On Thu, 25 Jul 2024 01:01:58 +0200 >>>> Pali Rohár <pali@kernel.org> wrote: >>>> >>>>> On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote: >>>>>> On Wed, 24 Jul 2024 22:45:23 +0200 >>>>>> Pali Rohár <pali@kernel.org> wrote: >>>>>> >>>>>>> On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote: >>>>>>>> Hello, the driver change looks good. I have just few minor comments for >>>>>>>> this change below. >>>>>>>> >>>>>>>> Anyway, if there is somebody on the list with Dell laptop with 2 or 3 >>>>>>>> batteries, see below, it would be nice to check how such laptop would >>>>>>>> behave with this patch. It does not seem that patch should cause >>>>>>>> regression but always it is better to do testing if it is possible. >>>>>>>> >>>>>>>> On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote: >>>>>> [...] >>>>>>> And because CLASS_TOKEN_WRITE is being repeated, what about defining >>>>>>> something like this? >>>>>>> >>>>>>> static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value) >>>>>>> { >>>>>>> dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value); >>>>>>> } >>>>>>> >>>>>>> Just an idea. Do you think that it could be useful in second patch? >>>>>>> >>>>>> Let me implement the other changes first and then take a look. >>>>> Ok. >>>>> >>>> For the helper function, I noticed that all of the CLASS_TOKEN_WRITEs >>>> also have SELECT_TOKEN_STD except for one (dell_send_intensity). So I >>>> think it makes sense to have the helper function also do that as well. >>>> Eg, >>>> >>>> static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer, u16 tokenid, u32 value) >>>> { >>>> dell_send_request_for_tokenid(buffer, SELECT_TOKEN_STD, CLASS_TOKEN_WRITE, tokenid, value); >>>> } >>>> >>>> I agree with your renaming to dell_send_request_for_tokenid, btw. >>>> >>>> >>>>>>>>> +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]; >>>>>>>> buffer.output[1] is of type u32. So theoretically it can contain value >>>>>>>> above 2^31. For safety it would be better to check if the output[1] >>>>>>>> value fits into INT_MAX and if not then return something like -ERANGE >>>>>>>> (or some other better errno code). >>>> I ended up returning -EIO here, with the logic that if we're getting >>>> some nonsense value from SMBIOS, it could either be junk in the stored >>>> values or communication corruption. >>>> >>>> Likewise, I used -EIO for the checks in charge_control_start_threshold_show >>>> and charge_control_end_threshold_show when SMBIOS returns values > 100%. >>>> >>>> >>>> >>>>>>>>> + if (end < 0) >>>>>>>>> + end = CHARGE_END_MAX; >>>>>>>>> + if ((end - start) < CHARGE_MIN_DIFF) >>>>>>>> nit: I'm not sure what is the correct coding style for kernel drivers >>>>>>>> but I thought that parenthesis around (end - start) are not being >>>>>>>> written. >>>>>>>> >>>> I think it makes the comparison much easier to read. I'd prefer to >>>> keep it, unless the coding style specifically forbids it. >>> As I said I'm really not sure. So if nobody would complain then you can >>> let it as is. >>> >>> You can use ./scripts/checkpatch.pl application which is in git tree, >>> which does basic checks for code style. It cannot prove if something is >>> really correct but it can prove if something is incorrect. >>> >>>> >>>>>>>>> + >>>>>>>>> +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 void __init dell_battery_init(struct device *dev) >>>>>>>>> +{ >>>>>>>>> + battery_supported_modes = battery_get_supported_modes(); >>>>>>>>> + >>>>>>>>> + if (battery_supported_modes != 0) >>>>>>>>> + battery_hook_register(&dell_battery_hook); >>>>>>>> Anyway, how is this battery_hook_register() suppose to work on systems >>>>>>>> with multiple batteries? The provided API is only for the primary >>>>>>>> battery, but on older Dell laptop it was possible to connect up to >>>>>>>> 3 batteries. Would not this case some issues? >>>>>> This interface is _only_ for controlling charging of the primary battery. >>>>>> In theory, it should hopefully ignore any other batteries, which would >>>>>> need to have their settings changed in the BIOS or with a special tool or >>>>>> whatever. >>>>> That is OK. But where it is specified that the hook is being registered >>>>> only for the primary battery? Because from the usage it looks like that >>>>> the hook is applied either for all batteries present in the system or >>>>> for some one arbitrary chosen battery. >>>>> >>>>>> However, I'm basing that assumption on the SMBIOS interface. These tokens >>>>>> are marked "Primary Battery". There are separate tokens marked "Battery >>>>>> Slice", which from my understanding was an older type of battery that >>>>> From SMBIOS perspective it is clear, each battery seems to have its own >>>>> tokens. >>>>> >>>>> The issue here is: how to tell kernel that the particular >>>>> dell_battery_hook has to be bound with the primary battery? >>>>> >>>> So from userspace, we've got the expectation that multiple batteries >>>> would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1, >>>> and so on. >>> Yes, I hope so. >>> >>>> The current BAT0 entry shows things like 'capacity' even without this >>>> patch, and we're just piggybacking off of that to add charge_type and >>>> other entries. So there shouldn't be any confusion there, agreed? >>> I have not looked at the battery_hook_register() code yet (seems that I >>> would have to properly read it and understand it). But does it mean that >>> battery_hook_register() is adding hook just for "BAT0"? >>> >>> What I mean: cannot that hook be registered to "BAT1" too? Because if >>> yes then we should prevent it. Otherwise this hook which is for "Dell >>> Primary Battery" could be registered also for secondary battery "BAT1". >>> (I hope that now it is more clear what I mean). >> Hi, >> >> the battery hook is being registered to all ACPI batteries present on a given system, >> so you need to do some manual filtering when .add_battery() is called. > Ok. So it means that the filtering based on the primary battery in > add_battery callback is needed. > >> As a side note: i suspect that "newer" Dell machines use a different interface for controlling >> battery charging, since the Dell Power Manager software on my machine seems to provide some >> additional features not found in this token-based interface. > Dell has released documentation of some other API, see the end of this file > https://github.com/dell/libsmbios/blob/master/src/bin/smbios-battery-ctl > >> Unfortunately i am not sure if reverse-engineering the Dell software is legal, does the kernel >> community have some helping guides in this area? If it is legal, then i would happily volunteer >> to do the reverse-engineering. > That is questionable. Some kernel drivers were written from reverse > engineered data in past. > > Note that in some countries is reverse engineering legal if it is done > for interoperability purposes (which this one could match). > >> Otherwise maybe someone at Dell can provide some clarifications if a different interface for controlling >> battery charging exists and how to use it? > Try to send an off-list/private email to Dell.Client.Kernel@dell.com > with details for what you are asking. Maybe they would have access to > some new documentation. > I received no response to the email i send to Dell.Client.Kernel@dell.com, so i started reverse-engineering the Dell software which is used to control the battery charge settings. My findings (as for now) are: - there exists an extended battery control interface where the charge mode can be set for each individual battery - this extended interface uses the Dell SMBIOS calling interface together with some extensions - the interface has a design flaw which makes it impossible to reliably support hot-swap batteries (thanks Dell!) - the extended interface might only be supported on WMI-capable machines, but i need to verify this I will need some more time for reverse-engineering the whole interface, but i believe adding support for it should be possible. For now, i think this patch series is the way forward. Thanks, Armin Wolf >> Thanks, >> Armin Wolf >> >>>> In the kernel, we're registering the acpi_battery_hook as "Dell Primary >>>> Battery Extension". The functions set up by that acpi_battery_hook are >>>> the only ones using battery_support_modes. We could make it more explicit >>>> by: >>>> 1) renaming it to primary_battery_modes, along with >>>> dell_primary_battery_{init,exit} and/or >>>> 2) allocating the mode mask and strings dynamically, and storing that >>>> inside of the dev kobject. >>>> >>>> However, #2 seems overly complicated for what we're doing. In the >>>> circumstances that we want to add support for secondary batteries, >>>> we're going to need to query separate tokens, add another >>>> acpi_battery_hook, and also add a second mask. Whether that's a global >>>> variable or kept inside pdev seems like more of a style issue than >>>> anything else. >>>> >>>> #1 is easy enough to change, if you think that's necessary. >>> I think that "Dell Primary Battery Extension" is OK. All SMBIOS code is >>> currently primary-battery only. >>> >>> The only my point is to prevent this &dell_battery_hook to be registered >>> for non-primary battery. >>>
Hi Andres On 7/24/24 4:05 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> > --- > Changes in v2, based on extensive feedback from Pali Rohár <pali@kernel.org>: > - code style changes > - change battery write API to use token->value instead of passed value > - stop caching current mode, instead querying SMBIOS as needed > - drop the separate list of charging modes enum > - rework the list of charging mode strings > - query SMBIOS for supported charging modes > - split dell_battery_custom_set() up Thank you for your contribution, it will be great to have charge threshold support on Dell laptops available to end users. I see that there are some review-comments on this first patch of the series. Please submit a v3 addressing those and then this will hopefully be ready for merging. Regards, Hans > --- > .../ABI/testing/sysfs-class-power-dell | 31 ++ > drivers/platform/x86/dell/Kconfig | 1 + > drivers/platform/x86/dell/dell-laptop.c | 288 ++++++++++++++++++ > drivers/platform/x86/dell/dell-smbios.h | 7 + > 4 files changed, 327 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..aae9a95fb188 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" > @@ -99,6 +101,18 @@ static bool force_rfkill; > static bool micmute_led_registered; > static bool mute_led_registered; > > +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; > + > module_param(force_rfkill, bool, 0444); > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); > > @@ -2183,6 +2197,277 @@ static struct led_classdev mute_led_cdev = { > .default_trigger = "audio-mute", > }; > > +static int dell_send_request_by_token_loc(struct calling_interface_buffer *buffer, > + u16 class, u16 select, int type, > + u32 val) > +{ > + struct calling_interface_token *token; > + > + token = dell_smbios_find_token(type); > + if (!token) > + return -ENODEV; > + > + /* -1 is a sentinel value, telling us to use token->value */ > + if (val == (u32)-1) > + 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; > + > + /* -1 means use the value from the token */ > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > + SELECT_TOKEN_STD, type, -1); > +} > + > +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 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); > +} > + > +/* 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_set_custom_charge_start(int start) > +{ > + struct calling_interface_buffer buffer; > + int end; > + > + if (start < CHARGE_START_MIN) > + start = CHARGE_START_MIN; > + else if (start > CHARGE_START_MAX) > + start = CHARGE_START_MAX; > + > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > + /* a failed read is okay */ > + if (end < 0) > + end = CHARGE_END_MAX; > + if ((end - start) < CHARGE_MIN_DIFF) > + start = end - CHARGE_MIN_DIFF; > + > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_START, start); > +} > + > +static int dell_battery_set_custom_charge_end(int end) > +{ > + struct calling_interface_buffer buffer; > + int start; > + > + if (end < CHARGE_END_MIN) > + end = CHARGE_END_MIN; > + else if (end > CHARGE_END_MAX) > + end = CHARGE_END_MAX; > + > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > + /* a failed read is okay */ > + if (start < 0) > + start = CHARGE_START_MIN; > + if ((end - start) < CHARGE_MIN_DIFF) > + end = start + CHARGE_MIN_DIFF; > + > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_END, end); > +} > + > +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++) { > + bool active; > + > + if (!(battery_supported_modes & BIT(i))) > + continue; > + > + active = dell_battery_mode_is_active(battery_modes[i].token); > + count += sysfs_emit_at(buf, count, active ? "[%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) > +{ > + bool matched = false; > + int err, i; > + > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { > + if (sysfs_streq(battery_modes[i].label, buf)) { > + matched = true; > + break; > + } > + } > + if (!matched || !(battery_supported_modes & BIT(i))) > + return -EINVAL; > + > + err = dell_battery_set_mode(battery_modes[i].token); > + if (err) > + return err; > + > + return size; > +} > + > +static ssize_t charge_control_start_threshold_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int start; > + > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); > + if (start < 0) > + return start; > + > + 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) > + return ret; > + > + ret = dell_battery_set_custom_charge_start(start); > + if (ret) > + return ret; > + > + return size; > +} > + > +static ssize_t charge_control_end_threshold_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int end; > + > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); > + if (end < 0) > + return end; > + > + return sysfs_emit(buf, "%d\n", end); > +} > + > +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) > + return ret; > + > + ret = dell_battery_set_custom_charge_end(end); > + if (ret) > + return ret; > + > + return size; > +} > + > +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 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 void __init dell_battery_init(struct device *dev) > +{ > + battery_supported_modes = battery_get_supported_modes(); > + > + if (battery_supported_modes != 0) > + battery_hook_register(&dell_battery_hook); > +} > + > +static void __exit dell_battery_exit(void) > +{ > + if (battery_supported_modes != 0) > + battery_hook_unregister(&dell_battery_hook); > +} > + > static int __init dell_init(void) > { > struct calling_interface_token *token; > @@ -2219,6 +2504,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 +2579,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 +2598,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..77baa15eb523 100644 > --- a/drivers/platform/x86/dell/dell-smbios.h > +++ b/drivers/platform/x86/dell/dell-smbios.h > @@ -33,6 +33,13 @@ > #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
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..aae9a95fb188 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" @@ -99,6 +101,18 @@ static bool force_rfkill; static bool micmute_led_registered; static bool mute_led_registered; +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; + module_param(force_rfkill, bool, 0444); MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); @@ -2183,6 +2197,277 @@ static struct led_classdev mute_led_cdev = { .default_trigger = "audio-mute", }; +static int dell_send_request_by_token_loc(struct calling_interface_buffer *buffer, + u16 class, u16 select, int type, + u32 val) +{ + struct calling_interface_token *token; + + token = dell_smbios_find_token(type); + if (!token) + return -ENODEV; + + /* -1 is a sentinel value, telling us to use token->value */ + if (val == (u32)-1) + 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; + + /* -1 means use the value from the token */ + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, + SELECT_TOKEN_STD, type, -1); +} + +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 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); +} + +/* 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_set_custom_charge_start(int start) +{ + struct calling_interface_buffer buffer; + int end; + + if (start < CHARGE_START_MIN) + start = CHARGE_START_MIN; + else if (start > CHARGE_START_MAX) + start = CHARGE_START_MAX; + + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); + /* a failed read is okay */ + if (end < 0) + end = CHARGE_END_MAX; + if ((end - start) < CHARGE_MIN_DIFF) + start = end - CHARGE_MIN_DIFF; + + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_START, start); +} + +static int dell_battery_set_custom_charge_end(int end) +{ + struct calling_interface_buffer buffer; + int start; + + if (end < CHARGE_END_MIN) + end = CHARGE_END_MIN; + else if (end > CHARGE_END_MAX) + end = CHARGE_END_MAX; + + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); + /* a failed read is okay */ + if (start < 0) + start = CHARGE_START_MIN; + if ((end - start) < CHARGE_MIN_DIFF) + end = start + CHARGE_MIN_DIFF; + + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE, + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_END, end); +} + +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++) { + bool active; + + if (!(battery_supported_modes & BIT(i))) + continue; + + active = dell_battery_mode_is_active(battery_modes[i].token); + count += sysfs_emit_at(buf, count, active ? "[%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) +{ + bool matched = false; + int err, i; + + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { + if (sysfs_streq(battery_modes[i].label, buf)) { + matched = true; + break; + } + } + if (!matched || !(battery_supported_modes & BIT(i))) + return -EINVAL; + + err = dell_battery_set_mode(battery_modes[i].token); + if (err) + return err; + + return size; +} + +static ssize_t charge_control_start_threshold_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int start; + + start = dell_battery_read(BAT_CUSTOM_CHARGE_START); + if (start < 0) + return start; + + 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) + return ret; + + ret = dell_battery_set_custom_charge_start(start); + if (ret) + return ret; + + return size; +} + +static ssize_t charge_control_end_threshold_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int end; + + end = dell_battery_read(BAT_CUSTOM_CHARGE_END); + if (end < 0) + return end; + + return sysfs_emit(buf, "%d\n", end); +} + +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) + return ret; + + ret = dell_battery_set_custom_charge_end(end); + if (ret) + return ret; + + return size; +} + +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 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 void __init dell_battery_init(struct device *dev) +{ + battery_supported_modes = battery_get_supported_modes(); + + if (battery_supported_modes != 0) + battery_hook_register(&dell_battery_hook); +} + +static void __exit dell_battery_exit(void) +{ + if (battery_supported_modes != 0) + battery_hook_unregister(&dell_battery_hook); +} + static int __init dell_init(void) { struct calling_interface_token *token; @@ -2219,6 +2504,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 +2579,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 +2598,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..77baa15eb523 100644 --- a/drivers/platform/x86/dell/dell-smbios.h +++ b/drivers/platform/x86/dell/dell-smbios.h @@ -33,6 +33,13 @@ #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
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> --- Changes in v2, based on extensive feedback from Pali Rohár <pali@kernel.org>: - code style changes - change battery write API to use token->value instead of passed value - stop caching current mode, instead querying SMBIOS as needed - drop the separate list of charging modes enum - rework the list of charging mode strings - query SMBIOS for supported charging modes - split dell_battery_custom_set() up --- .../ABI/testing/sysfs-class-power-dell | 31 ++ drivers/platform/x86/dell/Kconfig | 1 + drivers/platform/x86/dell/dell-laptop.c | 288 ++++++++++++++++++ drivers/platform/x86/dell/dell-smbios.h | 7 + 4 files changed, 327 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-power-dell