[PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.

[PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.

[Perry Yuan]

From: perry_yuan <perry_yuan@dell.com>

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 <perry_yuan@dell.com>
Signed-off-by: Limonciello Mario <Mario_Limonciello@Dell.com>
---
 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/<supply_name>/charge_control_charging_mode
+Date:		March 2020
+Contact:	linux-pm@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/<supply_name>/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 <linux/debugfs.h>
 #include <linux/seq_file.h>
 #include <acpi/video.h>
+#include <acpi/battery.h>
+#include <linux/string.h>
 #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(&buffer, token->location, 0, 0, 0);
+		ret = dell_send_request(&buffer,
+					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(&buffer, token->location, 0, 0, 0);
+		ret = dell_send_request(&buffer,
+					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 = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_START);
+	if (!token)
+		return -ENODEV;
+
+	dell_fill_request(&buffer, token->location, start, 0, 0);
+	ret = dell_send_request(&buffer,
+				CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
+	if (ret)
+		return -EIO;
+
+	token = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_END);
+	if (!token)
+		return -ENODEV;
+
+	dell_fill_request(&buffer, token->location, end, 0, 0);
+	ret = dell_send_request(&buffer,
+				CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
+	if (ret)
+		return -EIO;
+
+	return ret;
+}
+
+static int battery_charging_mode_set(enum battery_charging_mode mode)
+{
+	struct calling_interface_buffer buffer;
+	struct calling_interface_token *token;
+	int ret;
+
+	if (mode <= BAT_NONE_MODE || mode >= BAT_MAX_MODE)
+		return -EINVAL;
+
+	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;
+		break;
+	default:
+		pr_warn("unspported charging mode!\n");
+		return -EINVAL;
+	}
+
+	dell_fill_request(&buffer, token->location, mode, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
+	if (ret)
+		return -EIO;
+
+	return ret;
+}
+
+static int battery_charging_mode_get(enum battery_charging_mode *mode)
+{
+	struct calling_interface_buffer buffer;
+	struct calling_interface_token *token;
+	int ret;
+
+	token = dell_smbios_find_token(BAT_CUSTOM_MODE_TOKEN);
+	if (!token)
+		return -ENODEV;
+	dell_fill_request(&buffer, token->location, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD);
+	if (ret)
+		return -EIO;
+	if (ret == 0)
+		*mode = buffer.output[1];
+
+	return ret;
+}
+
+static ssize_t charge_control_charging_mode_show(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	enum battery_charging_mode mode;
+	char *s = buf;
+
+	for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++) {
+		if (battery_state[mode]) {
+			if (mode == bat_chg_current)
+				s += sprintf(s, "[%s] ", battery_state[mode]);
+			else
+				s += sprintf(s, "%s ", battery_state[mode]);
+		}
+	}
+	if (s != buf)
+		/* convert the last space to a newline */
+		*(s-1) = '\n';
+	return (s - buf);
+}
+
+static ssize_t charge_control_charging_mode_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t size)
+{
+	int err;
+	enum battery_charging_mode mode;
+	char *p;
+	int len;
+	const char *label;
+
+	p = memchr(buf, '\n', size);
+	len = p ? p - buf : size;
+
+	for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++) {
+		label = battery_state[mode];
+		if (label && len == strlen(label) &&
+			!strncmp(buf, label, len)) {
+			bat_chg_current = mode;
+			break;
+		}
+	}
+	if (mode > BAT_NONE_MODE && mode < BAT_MAX_MODE)
+		err = battery_charging_mode_set(mode);
+	else
+		err = -EINVAL;
+
+	return err ? err : size;
+}
+
+static ssize_t charge_control_start_threshold_show(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	int err, start;
+
+	err = dell_battery_get(&start, NULL);
+	if (err)
+		return err;
+
+	return sprintf(buf, "%d\n", start);
+}
+
+static ssize_t charge_control_start_threshold_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t size)
+{
+	int err, start, end;
+
+	err = dell_battery_get(NULL, &end);
+	if (err)
+		return err;
+	err = kstrtoint(buf, 10, &start);
+	if (err)
+		return err;
+	err = dell_battery_set(start, end);
+	if (err)
+		return err;
+
+	return size;
+}
+
+static ssize_t charge_control_end_threshold_show(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	int err, end;
+
+	err = dell_battery_get(NULL, &end);
+	if (err)
+		return err;
+
+	return sprintf(buf, "%d\n", end);
+}
+
+static ssize_t charge_control_end_threshold_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t size)
+{
+	int err, start, end;
+
+	err = dell_battery_get(&start, NULL);
+	if (err)
+		return err;
+	err = kstrtouint(buf, 10, &end);
+	if (err)
+		return err;
+	err = dell_battery_set(start, end);
+	if (err)
+		return err;
+
+	return size;
+}
+
+static ssize_t charge_control_thresholds_show(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	int err, start, end;
+
+	err = dell_battery_get(&start, &end);
+	if (err)
+		return err;
+
+	return sprintf(buf, "%d %d\n", start, end);
+}
+
+static ssize_t charge_control_thresholds_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t size)
+{
+	int err, start, end;
+
+	if (sscanf(buf, "%d %d", &start, &end) != 2)
+		return -EINVAL;
+
+	err = dell_battery_set(start, end);
+	if (err)
+		return err;
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(charge_control_start_threshold);
+static DEVICE_ATTR_RW(charge_control_end_threshold);
+static DEVICE_ATTR_RW(charge_control_thresholds);
+static DEVICE_ATTR_RW(charge_control_charging_mode);
+
+static int dell_battery_add(struct power_supply *battery)
+{
+	device_create_file(&battery->dev,
+		&dev_attr_charge_control_start_threshold);
+	device_create_file(&battery->dev,
+		&dev_attr_charge_control_end_threshold);
+	device_create_file(&battery->dev,
+		&dev_attr_charge_control_charging_mode);
+
+	return 0;
+}
+
+static int dell_battery_remove(struct power_supply *battery)
+{
+	device_remove_file(&battery->dev,
+		&dev_attr_charge_control_start_threshold);
+	device_remove_file(&battery->dev,
+		&dev_attr_charge_control_end_threshold);
+	device_remove_file(&battery->dev,
+		&dev_attr_charge_control_charging_mode);
+
+	return 0;
+}
+
+static struct acpi_battery_hook dell_battery_hook = {
+	.add_battery = dell_battery_add,
+	.remove_battery = dell_battery_remove,
+	.name = "Dell Battery Extension"
+};
+
+static void dell_battery_setup(struct device *dev)
+{
+	enum battery_charging_mode current_mode = BAT_NONE_MODE;
+
+	battery_charging_mode_get(&current_mode);
+	if (current_mode) {
+		bat_chg_current = current_mode;
+		pr_debug("battery is present\n");
+	} else {
+		pr_debug("battery is not present\n");
+	}
+	battery_hook_register(&dell_battery_hook);
+	device_create_file(dev, &dev_attr_charge_control_thresholds);
+}
+
+static void dell_battery_exit(struct device *dev)
+{
+	if (bat_chg_current != BAT_NONE_MODE) {
+		battery_hook_unregister(&dell_battery_hook);
+		device_remove_file(dev, &dev_attr_charge_control_thresholds);
+	}
+}
+
 static int __init dell_init(void)
 {
 	struct calling_interface_token *token;
@@ -2197,6 +2539,7 @@ static int __init dell_init(void)
 		touchpad_led_init(&platform_device->dev);
 
 	kbd_led_init(&platform_device->dev);
+	dell_battery_setup(&platform_device->dev);
 
 	dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL);
 	debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
@@ -2281,6 +2624,7 @@ static void __exit dell_exit(void)
 		platform_device_unregister(platform_device);
 		platform_driver_unregister(&platform_driver);
 	}
+	dell_battery_exit(&platform_device->dev);
 }
 
 /* dell-rbtn.c driver export functions which will not work correctly (and could
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index a7ff9803f41a..36e6b06a0f47 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,6 +35,32 @@
 #define GLOBAL_MIC_MUTE_ENABLE	0x0364
 #define GLOBAL_MIC_MUTE_DISABLE	0x0365
 
+/*Battery Charging Modes Tokens*/
+#define BAT_CUSTOM_MODE_TOKEN		0x343
+#define BAT_PRIMARILY_AC_MODE_TOKEN	0x0341
+#define BAT_ADAPTIVE_MODE_TOKEN		0x0342
+#define BAT_STANDARD_MODE_TOKEN		0x0346
+#define BAT_EXPRESS_MODE_TOKEN		0x0347
+#define BATTERY_CUSTOM_CHARGE_START	0x0349
+#define BATTERY_CUSTOM_CHARGE_END	0x034A
+
+/* percentage bounds for custom charge */
+#define CHARGE_START_MIN	50
+#define CHARGE_START_MAX	95
+#define CHARGE_END_MIN		55
+#define CHARGE_END_MAX		100
+
+/*Battery Charging Modes */
+enum battery_charging_mode {
+	BAT_NONE_MODE = 0,
+	BAT_STANDARD_MODE,
+	BAT_EXPRESS_MODE,
+	BAT_PRIMARILY_AC_MODE,
+	BAT_ADAPTIVE_MODE,
+	BAT_CUSTOM_MODE,
+	BAT_MAX_MODE,
+};
+
 struct notifier_block;
 
 struct calling_interface_token {
-- 
2.27.0

Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.

[Matthew Garrett]

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/<supply_name>/charge_control_charging_mode
> +Date:		March 2020
> +Contact:	linux-pm@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 | mjg59@srcf.ucam.org

Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.

[Limonciello, Mario]

> 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.

[Andy Shevchenko]

On Wed, Jul 29, 2020 at 9:54 AM Perry Yuan <Perry.Yuan@dell.com> wrote:
>
> From: perry_yuan <perry_yuan@dell.com>

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 <linux/debugfs.h>
>  #include <linux/seq_file.h>

>  #include <acpi/video.h>
> +#include <acpi/battery.h>

Keep it ordered.

> +#include <linux/string.h>

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(&buffer, token->location, 0, 0, 0);
> +               ret = dell_send_request(&buffer,
> +                                       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(&buffer, token->location, 0, 0, 0);
> +               ret = dell_send_request(&buffer,
> +                                       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(&buffer,
> +                               CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> +       if (ret)

> +               return -EIO;

Why shadowing error code?

...

> +       ret = dell_send_request(&buffer,
> +                               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(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> +       if (ret)
> +               return -EIO;
> +
> +       return ret;

return dell_send_request(...);

...

> +       ret = dell_send_request(&buffer, 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 return 0; ?

...

> +static ssize_t charge_control_charging_mode_show(struct device *dev,
> +               struct device_attribute *attr,
> +               char *buf)
> +{
> +       enum battery_charging_mode mode;
> +       char *s = buf;
> +
> +       for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++) {
> +               if (battery_state[mode]) {
> +                       if (mode == bat_chg_current)
> +                               s += sprintf(s, "[%s] ", battery_state[mode]);
> +                       else
> +                               s += sprintf(s, "%s ", battery_state[mode]);

You have to control buffer boundaries.

> +               }
> +       }

> +       if (s != buf)

if (s == buf)
  return 0;

> +               /* convert the last space to a newline */
> +               *(s-1) = '\n';
> +       return (s - buf);
> +}

...

> +static ssize_t charge_control_charging_mode_store(struct device *dev,
> +               struct device_attribute *attr,
> +               const char *buf, size_t size)
> +{
> +       int err;
> +       enum battery_charging_mode mode;
> +       char *p;
> +       int len;
> +       const char *label;

> +       p = memchr(buf, '\n', size);
> +       len = p ? p - buf : size;
> +
> +       for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++) {
> +               label = battery_state[mode];
> +               if (label && len == strlen(label) &&
> +                       !strncmp(buf, label, len)) {
> +                       bat_chg_current = mode;
> +                       break;
> +               }
> +       }

sysfs_match_string()

> +       if (mode > BAT_NONE_MODE && mode < BAT_MAX_MODE)
> +               err = battery_charging_mode_set(mode);
> +       else
> +               err = -EINVAL;
> +
> +       return err ? err : size;
> +}

...

> +static ssize_t charge_control_thresholds_store(struct device *dev,
> +               struct device_attribute *attr,
> +               const char *buf, size_t size)
> +{
> +       int err, start, end;

> +       if (sscanf(buf, "%d %d", &start, &end) != 2)
> +               return -EINVAL;

It's bad. The rule of thumb is one node per one property.
On top of that sscanf() is not good, it doesn't check for overflow,
use one of kstrto*().

> +       err = dell_battery_set(start, end);
> +       if (err)
> +               return err;
> +
> +       return size;
> +}

...

> +static int dell_battery_add(struct power_supply *battery)
> +{
> +       device_create_file(&battery->dev,
> +               &dev_attr_charge_control_start_threshold);
> +       device_create_file(&battery->dev,
> +               &dev_attr_charge_control_end_threshold);
> +       device_create_file(&battery->dev,
> +               &dev_attr_charge_control_charging_mode);

Can it be an attribute group?

> +}

...

> +static struct acpi_battery_hook dell_battery_hook = {
> +       .add_battery = dell_battery_add,
> +       .remove_battery = dell_battery_remove,
> +       .name = "Dell Battery Extension"

Missed comma.

> +};

...

> +static void dell_battery_setup(struct device *dev)
> +{
> +       enum battery_charging_mode current_mode = BAT_NONE_MODE;
> +
> +       battery_charging_mode_get(&current_mode);
> +       if (current_mode) {
> +               bat_chg_current = current_mode;

> +               pr_debug("battery is present\n");
> +       } else {
> +               pr_debug("battery is not present\n");
> +       }

Why not dev_dbg()? Why do we have these messages at all?

> +       battery_hook_register(&dell_battery_hook);

> +       device_create_file(dev, &dev_attr_charge_control_thresholds);

Hmm...

> +}
> +
> +static void dell_battery_exit(struct device *dev)
> +{
> +       if (bat_chg_current != BAT_NONE_MODE) {
> +               battery_hook_unregister(&dell_battery_hook);
> +               device_remove_file(dev, &dev_attr_charge_control_thresholds);
> +       }
> +}

...

> +/*Battery Charging Modes */

Indentation issues.
Same for many other comments in this change.


--
With Best Regards,
Andy Shevchenko

RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.

[Yuan, Perry]

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> 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 <Perry.Yuan@dell.com> wrote:
> >
> > From: perry_yuan <perry_yuan@dell.com>
> 
> 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 <linux/debugfs.h>
> >  #include <linux/seq_file.h>
> 
> >  #include <acpi/video.h>
> > +#include <acpi/battery.h>
> 
> Keep it ordered.
> 
> > +#include <linux/string.h>
> 
> 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(&buffer, token->location, 0, 0, 0);
> > +               ret = dell_send_request(&buffer,
> > +                                       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(&buffer, token->location, 0, 0, 0);
> > +               ret = dell_send_request(&buffer,
> > +                                       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(&buffer,
> > +                               CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > +       if (ret)
> 
> > +               return -EIO;
> 
> Why shadowing error code?
> 
> ...
> 
> > +       ret = dell_send_request(&buffer,
> > +                               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(&buffer, CLASS_TOKEN_WRITE,
> SELECT_TOKEN_STD);
> > +       if (ret)
> > +               return -EIO;
> > +
> > +       return ret;
> 
> return dell_send_request(...);
> 
> ...
> 
> > +       ret = dell_send_request(&buffer, 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 return 0; ?
> 
> ...
> 
> > +static ssize_t charge_control_charging_mode_show(struct device *dev,
> > +               struct device_attribute *attr,
> > +               char *buf)
> > +{
> > +       enum battery_charging_mode mode;
> > +       char *s = buf;
> > +
> > +       for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++)
> {
> > +               if (battery_state[mode]) {
> > +                       if (mode == bat_chg_current)
> > +                               s += sprintf(s, "[%s] ", battery_state[mode]);
> > +                       else
> > +                               s += sprintf(s, "%s ",
> > + battery_state[mode]);
> 
> You have to control buffer boundaries.
> 
> > +               }
> > +       }
> 
> > +       if (s != buf)
> 
> if (s == buf)
>   return 0;
> 
> > +               /* convert the last space to a newline */
> > +               *(s-1) = '\n';
> > +       return (s - buf);
> > +}
> 
> ...
> 
> > +static ssize_t charge_control_charging_mode_store(struct device *dev,
> > +               struct device_attribute *attr,
> > +               const char *buf, size_t size) {
> > +       int err;
> > +       enum battery_charging_mode mode;
> > +       char *p;
> > +       int len;
> > +       const char *label;
> 
> > +       p = memchr(buf, '\n', size);
> > +       len = p ? p - buf : size;
> > +
> > +       for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++)
> {
> > +               label = battery_state[mode];
> > +               if (label && len == strlen(label) &&
> > +                       !strncmp(buf, label, len)) {
> > +                       bat_chg_current = mode;
> > +                       break;
> > +               }
> > +       }
> 
> sysfs_match_string()
> 
> > +       if (mode > BAT_NONE_MODE && mode < BAT_MAX_MODE)
> > +               err = battery_charging_mode_set(mode);
> > +       else
> > +               err = -EINVAL;
> > +
> > +       return err ? err : size;
> > +}
> 
> ...
> 
> > +static ssize_t charge_control_thresholds_store(struct device *dev,
> > +               struct device_attribute *attr,
> > +               const char *buf, size_t size) {
> > +       int err, start, end;
> 
> > +       if (sscanf(buf, "%d %d", &start, &end) != 2)
> > +               return -EINVAL;
> 
> It's bad. The rule of thumb is one node per one property.
> On top of that sscanf() is not good, it doesn't check for overflow, use one of
> kstrto*().
> 
> > +       err = dell_battery_set(start, end);
> > +       if (err)
> > +               return err;
> > +
> > +       return size;
> > +}
> 
> ...
> 
> > +static int dell_battery_add(struct power_supply *battery) {
> > +       device_create_file(&battery->dev,
> > +               &dev_attr_charge_control_start_threshold);
> > +       device_create_file(&battery->dev,
> > +               &dev_attr_charge_control_end_threshold);
> > +       device_create_file(&battery->dev,
> > +               &dev_attr_charge_control_charging_mode);
> 
> Can it be an attribute group?
> 
> > +}
> 
> ...
> 
> > +static struct acpi_battery_hook dell_battery_hook = {
> > +       .add_battery = dell_battery_add,
> > +       .remove_battery = dell_battery_remove,
> > +       .name = "Dell Battery Extension"
> 
> Missed comma.
> 
> > +};
> 
> ...
> 
> > +static void dell_battery_setup(struct device *dev) {
> > +       enum battery_charging_mode current_mode = BAT_NONE_MODE;
> > +
> > +       battery_charging_mode_get(&current_mode);
> > +       if (current_mode) {
> > +               bat_chg_current = current_mode;
> 
> > +               pr_debug("battery is present\n");
> > +       } else {
> > +               pr_debug("battery is not present\n");
> > +       }
> 
> Why not dev_dbg()? Why do we have these messages at all?
> 
> > +       battery_hook_register(&dell_battery_hook);
> 
> > +       device_create_file(dev, &dev_attr_charge_control_thresholds);
> 
> Hmm...
> 
> > +}
> > +
> > +static void dell_battery_exit(struct device *dev) {
> > +       if (bat_chg_current != BAT_NONE_MODE) {
> > +               battery_hook_unregister(&dell_battery_hook);
> > +               device_remove_file(dev, &dev_attr_charge_control_thresholds);
> > +       }
> > +}
> 
> ...
> 
> > +/*Battery Charging Modes */
> 
> Indentation issues.
> Same for many other comments in this change.
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

Andy .
Thanks a lot for your review, I am working on the V2 to drive your feedback.

Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

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 <lkp@intel.com>

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-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34004 bytes --]

RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.

[Yuan, Perry]

> From: kernel test robot <lkp@intel.com>
> Sent: Saturday, August 1, 2020 1:08 PM
> To: Yuan, Perry; sre@kernel.org; mjg59@srcf.ucam.org; pali@kernel.org;
> dvhart@infradead.org; andy@infradead.org; Limonciello, Mario
> Cc: kbuild-all@lists.01.org; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org; platform-driver-x86@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 <lkp@intel.com>
> 
> 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-all@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.

[Matthew Garrett]

On Tue, Aug 04, 2020 at 05:46:30AM +0000, 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 | mjg59@srcf.ucam.org

RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.

[Yuan, Perry]

> From: Matthew Garrett <mjg59@srcf.ucam.org>
> Sent: Tuesday, August 4, 2020 1:54 PM
> To: Yuan, Perry
> Cc: kernel test robot; sre@kernel.org; pali@kernel.org; dvhart@infradead.org;
> andy@infradead.org; Limonciello, Mario; kbuild-all@lists.01.org; linux-
> pm@vger.kernel.org; linux-kernel@vger.kernel.org; platform-driver-
> x86@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 +0000, 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 | mjg59@srcf.ucam.org

Thank you Matthew.
I will add it to the Kconfig as DELL_LAPTOP dependency.