On Fri, 16 Aug 2024 18:33:41 +0200
Pali Rohár <pali@xxxxxxxxxx> wrote:
> On Friday 16 August 2024 16:56:24 Ilpo Järvinen wrote:
> > On Thu, 15 Aug 2024, 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>
> > > ---
> > > 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"
> > > +
> > > 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[] = {
> >
> > Please don't try to do this in one go but split it into two (define and
> > then declaration of the variable).
>
> Why? Splitting definition of this anonymous structure and definition of
> variable would leak definition of anonymous structure of out the scope
> where it is used.
Also, it's two different arrays that we then have to keep synced as we
add new modes. That's the main reason I went for the combined struct.
>
> > > + { 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" },
> >
> > I suggest aligning the strings with tabs for better readability.
>
> For aligning something for better readability in "git diff" and
> "git show" (which includes also git format-patch and emails) is better
> to use spaces and not tabs. tab-alignment in git makes worse readability
> (due to name of the token).
>
Okay, so I'm hearing to align them, but to use spaces? I'll line everything
up with where "Standard" and "Adaptive" are.
[...]
> > > +
> > > +/*
> > > + * 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;
> >
> > We have clamp().
Thanks, I'll use that.
> >
> > > +
> > > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END);
> > > + if (end < 0)
> > > + return end;
> > > + if ((end - start) < CHARGE_MIN_DIFF)
> >
> > Extra parenthesis.
>
> I pointed about this in previous version and from the discussion the
> conclusion was that there is no reason to remove extra parenthesis.
>
> > > + start = end - CHARGE_MIN_DIFF;
> > > +
> > > + return dell_set_std_token_value(&buffer, 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;
> >
> > clamp.
> >
+1
> > > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START);
> > > + if (start < 0)
> > > + return start;
> > > + if ((end - start) < CHARGE_MIN_DIFF)
> >
> > Extra parenthesis.
> >
> > > + end = start + CHARGE_MIN_DIFF;
> > > +
> > > + return dell_set_std_token_value(&buffer, 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)))
> >
> > Why not store this supported information into battery_modes itself?
>
> Same style is already used in other parts in this driver / source file.
>
> > What's the benefit of obfuscation it with the extra variable & BIT()?
>
> In my opinion, this is not obfuscation but clear and common style how to
> check which values of some enumeration are supported.
>
> Storing this kind of information into battery_modes is not possible
> because battery_modes is constant array with constant data.
>
Yep. It's not a given that all modes will be supported by the BIOS.
--
I'm available for contract & employment work, please contact me if
interested.
