On Fri, 16 Aug 2024, Pali Rohár 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.
As is, this splits "static const" from battery_modes. Naming anonymous
struct isn't as bad problem as you seem to suggest (and named structs
are also easier when grepping).
> > > +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.
If you refer to 'triggers', it is definitely not an example of something
that is easy to follow. With triggers, it seems that there's undocumented
array-index to hw-field bit mapping going on (or to be more precise, the
documentation for the hw field is in a far-away documentation block so
it's practically impossible to make the connection when reading the
struct).
> > 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.
Err, it's proposed to be const by this patch. Why that struct cannot be
changed to be non-const? Clearly there is battery mode related data that
is not const.
As a sidenote now that I had to look, this driver is doing other BIT(n)
obfusction too (and open-coded FIELD_PREP() seem to be plenty as well).
--
i.
