On Mon, 19 Aug 2024 15:59:45 +0200 Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > Hi, > > On 8/16/24 1:28 AM, Andres Salomon wrote: [...] > > > --- > > 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 ...". > > Is this what you had in mind? I don't see many users of charge_type, and I'm not sure whether the assumptions I'm making there are correct. https://lore.kernel.org/lkml/20240820041942.30ed42f3@5400/ > > 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: Thanks for clarify that, my fault for misreading what they'd written! -- I'm available for contract & employment work, please contact me if interested.
