RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
> From: Andy Shevchenko
> Sent: Wednesday, July 29, 2020 3:32 PM
> To: Yuan, Perry
> Cc: Sebastian Reichel; Matthew Garrett; Pali Rohár; Darren Hart; Andy
> Shevchenko; Limonciello, Mario; Linux PM; Linux Kernel Mailing List; Platform
> Driver
> Subject: Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds
> and charging mode switch.
>
>
> [EXTERNAL EMAIL]
>
> On Wed, Jul 29, 2020 at 9:54 AM Perry Yuan wrote:
> >
> > From: perry_yuan
>
> Fix your name, please. Or do you have it spelled in the official documents
> like
> above?
>
> > The patch control battery charging thresholds when system is under
> > custom charging mode through smbios API and driver`s sys attributes.It
> > also set the percentage bounds for custom charge.
> > Start value must lie in the range [50, 95],End value must lie in the
> > range [55, 100],END must be at least (START + 5).
> >
> > The patch also add the battery charging modes switch support.User can
> > switch the battery charging mode through the new sysfs entry.
>
> The commit message needs rewording. I would recommend reading [1] and [2].
>
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> (section 2)
> [2]: https://chris.beams.io/posts/git-commit/
>
> > Primary battery charging modes valid choices are:
> > ['primarily_ac', 'adaptive', 'custom', 'standard', 'express']
>
> This and documentation are different (at least in terms of case).
>
> Looks like this is something that should be agreed upon on the format and gets
> supported by Power Supply framework in the first place,
>
> > Documentation/ABI/testing/sysfs-class-power | 23 ++
>
> In any case it should be a separate patch. It has nothing to do with Dell, on
> contrary it's a certain device which relies on generic attributes.
>
> ...
>
> > #include
> > #include
>
> > #include
> > +#include
>
> Keep it ordered.
>
> > +#include
>
> And this is a generic one, should be in generic headers block.
>
> > #include "dell-rbtn.h"
> > #include "dell-smbios.h"
>
> ...
>
> > +static int dell_battery_get(int *start, int *end) {
> > + struct calling_interface_buffer buffer;
> > + struct calling_interface_token *token;
> > + int ret;
> > +
>
> > + if (start) {
> > + token =
> dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_START);
> > + if (!token)
> > + return -ENODEV;
> > + dell_fill_request(, token->location, 0, 0, 0);
> > + ret = dell_send_request(,
> > + CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> > + *start = buffer.output[1];
> > + }
>
> (1)
>
> > + if (end) {
> > + token = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_END);
> > + if (!token)
> > + return -ENODEV;
> > + dell_fill_request(, token->location, 0, 0, 0);
> > + ret = dell_send_request(,
> > + CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> > + if (ret)
> > + return -EIO;
> > + *end = buffer.output[1];
> > + }
>
> (2)
>
> (1) and (2) look like twins. Care to provide a helper to simplify?
>
> > + return 0;
> > +}
>
> ...
>
> > + ret = dell_send_request(,
> > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > + if (ret)
>
> > + return -EIO;
>
> Why shadowing error code?
>
> ...
>
> > + ret = dell_send_request(,
> > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > + if (ret)
> > + return -EIO;
>
> Ditto.
>
> > + return ret;
>
> And here perhaps simple 'return dell_send_request();'?
>
> ...
>
> > + switch (mode) {
> > + case BAT_STANDARD_MODE:
> > + token =
> > + dell_smbios_find_token(BAT_STANDARD_MODE_TOKEN);
>
> > + if (!token)
> > + return -ENODEV;
>
> > + break;
> > + case BAT_EXPRESS_MODE:
> > + token =
> > + dell_smbios_find_token(BAT_EXPRESS_MODE_TOKEN);
>
> > + if (!token)
> > + return -ENODEV;
>
> > + break;
> > +
RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
> From: Matthew Garrett > Sent: Tuesday, August 4, 2020 1:54 PM > To: Yuan, Perry > Cc: kernel test robot; s...@kernel.org; p...@kernel.org; dvh...@infradead.org; > a...@infradead.org; Limonciello, Mario; kbuild-...@lists.01.org; linux- > p...@vger.kernel.org; linux-kernel@vger.kernel.org; platform-driver- > x...@vger.kernel.org > Subject: Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds > and charging mode switch. > > > [EXTERNAL EMAIL] > > On Tue, Aug 04, 2020 at 05:46:30AM +, Yuan, Perry wrote: > > > It is not patch issue, the kernel config needs to add > "CONFIG_ACPI_BATTERY=y" > > In that case you probably want to add a dependency to ACPI_BATTERY in the > DELL_LAPTOP Kconfig. > > -- > Matthew Garrett | mj...@srcf.ucam.org Thank you Matthew. I will add it to the Kconfig as DELL_LAPTOP dependency.
Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
On Tue, Aug 04, 2020 at 05:46:30AM +, Yuan, Perry wrote: > It is not patch issue, the kernel config needs to add "CONFIG_ACPI_BATTERY=y" In that case you probably want to add a dependency to ACPI_BATTERY in the DELL_LAPTOP Kconfig. -- Matthew Garrett | mj...@srcf.ucam.org
RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
> From: kernel test robot > Sent: Saturday, August 1, 2020 1:08 PM > To: Yuan, Perry; s...@kernel.org; mj...@srcf.ucam.org; p...@kernel.org; > dvh...@infradead.org; a...@infradead.org; Limonciello, Mario > Cc: kbuild-...@lists.01.org; linux...@vger.kernel.org; linux- > ker...@vger.kernel.org; platform-driver-...@vger.kernel.org; Yuan, Perry > Subject: Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds > and charging mode switch. > > > [EXTERNAL EMAIL] > > Hi Perry, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on power-supply/for-next] [also build test ERROR on > linux/master linus/master v5.8-rc7 next-20200731] [If your patch is applied to > the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url:https://github.com/0day-ci/linux/commits/Perry-Yuan/platform-x86- > dell-laptop-Add-battery-charging-thresholds-and-charging-mode- > switch/20200729-150347 > base: https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power- > supply.git for-next > config: i386-randconfig-a005-20200731 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 > reproduce (this is a W=1 build): > # save the attached .config to linux build tree > make W=1 ARCH=i386 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > All errors (new ones prefixed by >>, old ones prefixed by <<): > > >> ERROR: modpost: "battery_hook_unregister" [drivers/platform/x86/dell- > laptop.ko] undefined! > >> ERROR: modpost: "battery_hook_register" [drivers/platform/x86/dell- > laptop.ko] undefined! > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org It is not patch issue, the kernel config needs to add "CONFIG_ACPI_BATTERY=y"
Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
Hi Perry, Thank you for the patch! Yet something to improve: [auto build test ERROR on power-supply/for-next] [also build test ERROR on linux/master linus/master v5.8-rc7 next-20200731] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Perry-Yuan/platform-x86-dell-laptop-Add-battery-charging-thresholds-and-charging-mode-switch/20200729-150347 base: https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next config: i386-randconfig-a005-20200731 (attached as .config) compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>, old ones prefixed by <<): >> ERROR: modpost: "battery_hook_unregister" >> [drivers/platform/x86/dell-laptop.ko] undefined! >> ERROR: modpost: "battery_hook_register" >> [drivers/platform/x86/dell-laptop.ko] undefined! --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
> The values here seem very Dell specific, but this is going into a > generic sysfs path. Really stuff here should be as vendor independent as > possible. If these values don't correspond to a wider industry > specification it probably makes sense to make this something Dell > specific. Worth pointing out that for wilco-ec they use this path: Documentation/ABI/testing/sysfs-class-power-wilco So that could be something good to model off for v2.
Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
On Tue, Jul 28, 2020 at 11:54:24PM -0700, Perry Yuan wrote: This seems extremely useful, but: > diff --git a/Documentation/ABI/testing/sysfs-class-power > b/Documentation/ABI/testing/sysfs-class-power > index bf3b48f022dc..a8adc3b0ca4b 100644 > --- a/Documentation/ABI/testing/sysfs-class-power > +++ b/Documentation/ABI/testing/sysfs-class-power > @@ -334,6 +334,29 @@ Description: > Access: Read > Valid values: Represented in microvolts > > +What: > /sys/class/power_supply//charge_control_charging_mode > +Date:March 2020 > +Contact: linux...@vger.kernel.org The values here seem very Dell specific, but this is going into a generic sysfs path. Really stuff here should be as vendor independent as possible. If these values don't correspond to a wider industry specification it probably makes sense to make this something Dell specific. -- Matthew Garrett | mj...@srcf.ucam.org
Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
On Wed, Jul 29, 2020 at 9:54 AM Perry Yuan wrote:
>
> From: perry_yuan
Fix your name, please. Or do you have it spelled in the official
documents like above?
> The patch control battery charging thresholds when system is under custom
> charging mode through smbios API and driver`s sys attributes.It also set the
> percentage bounds for custom charge.
> Start value must lie in the range [50, 95],End value must lie in the range
> [55, 100],END must be at least (START + 5).
>
> The patch also add the battery charging modes switch support.User can switch
> the battery charging mode through the new sysfs entry.
The commit message needs rewording. I would recommend reading [1] and [2].
[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html
(section 2)
[2]: https://chris.beams.io/posts/git-commit/
> Primary battery charging modes valid choices are:
> ['primarily_ac', 'adaptive', 'custom', 'standard', 'express']
This and documentation are different (at least in terms of case).
Looks like this is something that should be agreed upon on the format
and gets supported by Power Supply framework in the first place,
> Documentation/ABI/testing/sysfs-class-power | 23 ++
In any case it should be a separate patch. It has nothing to do with
Dell, on contrary it's a certain device which relies on generic
attributes.
...
> #include
> #include
> #include
> +#include
Keep it ordered.
> +#include
And this is a generic one, should be in generic headers block.
> #include "dell-rbtn.h"
> #include "dell-smbios.h"
...
> +static int dell_battery_get(int *start, int *end)
> +{
> + struct calling_interface_buffer buffer;
> + struct calling_interface_token *token;
> + int ret;
> +
> + if (start) {
> + token = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_START);
> + if (!token)
> + return -ENODEV;
> + dell_fill_request(, token->location, 0, 0, 0);
> + ret = dell_send_request(,
> + CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> + *start = buffer.output[1];
> + }
(1)
> + if (end) {
> + token = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_END);
> + if (!token)
> + return -ENODEV;
> + dell_fill_request(, token->location, 0, 0, 0);
> + ret = dell_send_request(,
> + CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> + if (ret)
> + return -EIO;
> + *end = buffer.output[1];
> + }
(2)
(1) and (2) look like twins. Care to provide a helper to simplify?
> + return 0;
> +}
...
> + ret = dell_send_request(,
> + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> + if (ret)
> + return -EIO;
Why shadowing error code?
...
> + ret = dell_send_request(,
> + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> + if (ret)
> + return -EIO;
Ditto.
> + return ret;
And here perhaps simple 'return dell_send_request();'?
...
> + switch (mode) {
> + case BAT_STANDARD_MODE:
> + token = dell_smbios_find_token(BAT_STANDARD_MODE_TOKEN);
> + if (!token)
> + return -ENODEV;
> + break;
> + case BAT_EXPRESS_MODE:
> + token = dell_smbios_find_token(BAT_EXPRESS_MODE_TOKEN);
> + if (!token)
> + return -ENODEV;
> + break;
> + case BAT_PRIMARILY_AC_MODE:
> + token = dell_smbios_find_token(BAT_PRIMARILY_AC_MODE_TOKEN);
> + if (!token)
> + return -ENODEV;
> + break;
> + case BAT_CUSTOM_MODE:
> + token = dell_smbios_find_token(BAT_CUSTOM_MODE_TOKEN);
> + if (!token)
> + return -ENODEV;
> + break;
> + case BAT_ADAPTIVE_MODE:
> + token = dell_smbios_find_token(BAT_ADAPTIVE_MODE_TOKEN);
> + if (!token)
> + return -ENODEV;
Five times the same lines. Please deduplicate them. One is enough.
> + break;
> + default:
> + pr_warn("unspported charging mode!\n");
When you have a device use dev_*() to print messages.
> + return -EINVAL;
> + }
...
> + ret = dell_send_request(, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> + if (ret)
> + return -EIO;
> +
> + return ret;
return dell_send_request(...);
...
> + ret = dell_send_request(, CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> + if (ret)
> + return -EIO;
Do not shadow error codes.
> + if (ret == 0)
What is the point?
> + *mode = buffer.output[1];
> + return ret;
Any difference to
[PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.
From: perry_yuan
The patch control battery charging thresholds when system is under custom
charging mode through smbios API and driver`s sys attributes.It also set the
percentage bounds for custom charge.
Start value must lie in the range [50, 95],End value must lie in the range
[55, 100],END must be at least (START + 5).
The patch also add the battery charging modes switch support.User can switch
the battery charging mode through the new sysfs entry.
Primary battery charging modes valid choices are:
['primarily_ac', 'adaptive', 'custom', 'standard', 'express']
Signed-off-by: Perry Yuan
Signed-off-by: Limonciello Mario
---
Documentation/ABI/testing/sysfs-class-power | 23 ++
drivers/platform/x86/dell-laptop.c | 344
drivers/platform/x86/dell-smbios.h | 26 ++
3 files changed, 393 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-power
b/Documentation/ABI/testing/sysfs-class-power
index bf3b48f022dc..a8adc3b0ca4b 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -334,6 +334,29 @@ Description:
Access: Read
Valid values: Represented in microvolts
+What:
/sys/class/power_supply//charge_control_charging_mode
+Date: March 2020
+Contact: linux...@vger.kernel.org
+Description:
+ Represents the type of charging modes currently being applied
to the
+ battery."Express", "Primarily_ac", "Adaptive", "Custom" and
+ "Standard" all mean different charging speeds.
+
+ 1: "Adaptive" means that the charger uses some
+ algorithm to adjust the charge rate dynamically, without
+ any user configuration required.
+ 2: "Custom" means that the charger uses the charge_control_*
+ properties to start and stop charging
+ based on user input.
+ 3: "Express" means the charger use fast charging technology
+ 4: "Primarily_ac" means that users who primarily operate the
system
+ while plugged into an external power source.
+ 5: "Standard" fully charges the battery at a moderate rate.
+
+ Access: Read, Write
+ Valid values: "Express", "Primarily_ac", "Standard",
+ "Adaptive", "Custom"
+
= USB Properties =
What: /sys/class/power_supply//current_avg
diff --git a/drivers/platform/x86/dell-laptop.c
b/drivers/platform/x86/dell-laptop.c
index 74e988f839e8..8e45ce92a2d9 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -28,6 +28,8 @@
#include
#include
#include
+#include
+#include
#include "dell-rbtn.h"
#include "dell-smbios.h"
@@ -90,6 +92,14 @@ static struct rfkill *wifi_rfkill;
static struct rfkill *bluetooth_rfkill;
static struct rfkill *wwan_rfkill;
static bool force_rfkill;
+static enum battery_charging_mode bat_chg_current = BAT_NONE_MODE;
+static const char * const battery_state[BAT_MAX_MODE] = {
+ [BAT_PRIMARILY_AC_MODE] = "primarily_ac",
+ [BAT_ADAPTIVE_MODE] = "adaptive",
+ [BAT_CUSTOM_MODE] = "custom",
+ [BAT_STANDARD_MODE] = "standard",
+ [BAT_EXPRESS_MODE] = "express",
+};
module_param(force_rfkill, bool, 0444);
MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
@@ -2161,6 +2171,338 @@ static struct led_classdev micmute_led_cdev = {
.default_trigger = "audio-micmute",
};
+static int dell_battery_get(int *start, int *end)
+{
+ struct calling_interface_buffer buffer;
+ struct calling_interface_token *token;
+ int ret;
+
+ if (start) {
+ token = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_START);
+ if (!token)
+ return -ENODEV;
+ dell_fill_request(, token->location, 0, 0, 0);
+ ret = dell_send_request(,
+ CLASS_TOKEN_READ, SELECT_TOKEN_STD);
+ *start = buffer.output[1];
+ }
+
+ if (end) {
+ token = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_END);
+ if (!token)
+ return -ENODEV;
+ dell_fill_request(, token->location, 0, 0, 0);
+ ret = dell_send_request(,
+ CLASS_TOKEN_READ, SELECT_TOKEN_STD);
+ if (ret)
+ return -EIO;
+ *end = buffer.output[1];
+ }
+
+ return 0;
+}
+
+static int dell_battery_set(int start, int end)
+{
+ struct calling_interface_buffer buffer;
+ struct calling_interface_token *token;
+ int ret;
+
+ if (start < CHARGE_START_MIN || end < CHARGE_START_MAX ||
+ start > CHARGE_END_MIN || end > CHARGE_END_MAX)
+ return -EINVAL;
+
+ token =
