Hi,
On 8/16/24 1:28 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 (eg, only charging once the battery drops below 50% and only
> charging up to 80%). When leaving for a trip, it would be more useful
> to instead switch to a standard charging mode (top off at 100%, charge
> any time power is available). Rebooting into the BIOS to change the
> charging mode 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 also adds a 'charge_type' sysfs entry that maps the
> standard values to Dell-specific ones and documents those mappings in
> sysfs-class-power-dell.
>
> This work is based on a patch by Perry Yuan <perry_yuan@xxxxxxxx> and
> Limonciello Mario <Mario_Limonciello@xxxxxxxx> submitted back in 2020:
> https://lore.kernel.org/all/20200729065424.12851-1-Perry_Yuan@xxxxxxxx/
> 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@xxxxxxxxxx>
Thank you for your patch, some small comments below. For the next
version please also address Pali's and Ilpo's remarks.
> ---
> Changes in v3:
> - switch tokenid and class types
> - be stricter with results from both userspace and BIOS
> - no longer allow failed BIOS reads
> - rename/move dell_send_request_by_token_loc, and add helper function
> - only allow registration for BAT0
> - rename charge_type modes to match power_supply names
> Changes in v2, based on extensive feedback from Pali Rohár <pali@xxxxxxxxxx>:
> - 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 | 33 ++
> drivers/platform/x86/dell/Kconfig | 1 +
> drivers/platform/x86/dell/dell-laptop.c | 316 ++++++++++++++++++
> drivers/platform/x86/dell/dell-smbios.h | 7 +
> 4 files changed, 357 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..d8c542177558
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-power-dell
> @@ -0,0 +1,33 @@
> +What: /sys/class/power_supply/<supply_name>/charge_type
> +Date: August 2024
> +KernelVersion: 6.12
> +Contact: linux-pm@xxxxxxxxxxxxxxx
> +Description:
> + Select the charging algorithm to use for the (primary)
> + battery.
> +
> + 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"
> +
As the kernel test robot reports:
Warning: /sys/class/power_supply/<supply_name>/charge_type is defined 2 times: ./Documentation/ABI/testing/sysfs-class-power-dell:0 ./Documentation/ABI/testing/sysfs-class-power:375
So please drop this. What you could do is instead submit (as a separate) patch
an update to Documentation/ABI/testing/sysfs-class-power:375 to use your IMHO
more readable version.
And when doing so I think it would best to replace "The Dell BIOS calls this ..."
with "In vendor tooling this may also be called ...".
> 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..8cc05f0fab91 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, "Fast" },
> + { BAT_PRI_AC_MODE_TOKEN, "Trickle" },
> + { BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" },
> + { BAT_CUSTOM_MODE_TOKEN, "Custom" },
> +};
So Ilpo suggested to split this (first declare the struct type and then
declare an array of that type) and Pali suggested to keep this as is.
> +static u32 battery_supported_modes;
I see there also is some disagreement on keeping battery_modes[]
const vs adding a flag for this in the struct.
IMHO in both cases either option is fine, so you (Andres) get
to choose which solution you prefer in either case.
I do see there is some confusion about Ilpo's split suggestion,
to clarify that, what I believe is Ilpo meant doing something
like this:
struct battery_mode_info {
int token;
const char *label;
};
static const struct battery_mode_info battery_modes[] = {
{ BAT_STANDARD_MODE_TOKEN, "Standard" },
{ BAT_EXPRESS_MODE_TOKEN, "Fast" },
{ BAT_PRI_AC_MODE_TOKEN, "Trickle" },
{ BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" },
{ BAT_CUSTOM_MODE_TOKEN, "Custom" },
};
Also whatever option you chose please align the second column using
spaces as done above.
<snip>
> +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);
> +}
> +
You cannot mark this __exit and also call it from the __init
dell_init() function to cleanup on errors, this lead to
the following error reported by the kernel test robot:
WARNING: modpost: drivers/platform/x86/dell/dell-laptop: section mismatch in reference: dell_init+0x637 (section: .init.text) -> dell_battery_exit (section: .exit.text)
to fix this please drop the __exit marking from this function.
Regards,
Hans
