[PATCH v4 0/5] ChromeOS Embedded Controller charge control driver

[PATCH v4 0/5] ChromeOS Embedded Controller charge control driver

[Thomas Weißschuh]

Note:
To be useful, this series depends on
commit 08dbad2c7c32 ("mfd: cros_ec: Register charge control subdevice")
from the MFD tree [0].

Add a power supply driver that supports charge thresholds and behaviour
configuration.

This is a complete rework of
"platform/chrome: cros_ec_framework_laptop: new driver" [1], which used
Framework specific EC commands.

The driver propsed in this series only uses upstream CrOS functionality.

Tested on a Framework 13 AMD, Firmware 3.05.

This driver should go through the chrome-platform tree.
Otherwise "platform/chrome: cros_ec_proto: Introduce cros_ec_cmd_versions()"
will conflict with
commit a14a569a9918 ("platform/chrome: cros_ec_proto: Introduce cros_ec_cmd_readmem()")
from chrome-platform/for-next.

The patch
"platform/chrome: cros_ec_proto: Introduce cros_ec_cmd_versions()"
is also part of my series
"hwmon: (cros_ec): fan target, fan pwm control and temperature thresholds"[2].

Based on chrome-platform/for-next.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next&id=08dbad2c7c3275e0e79190dca139bc65ce775a92
[1] https://lore.kernel.org/lkml/20240505-cros_ec-framework-v1-0-402662d6276b@weissschuh.net/
[2] https://lore.kernel.org/lkml/20240608-cros_ec-hwmon-pwm-v1-0-d29dfc26fbc3@weissschuh.net/

Changes in v4:
- Rename cros_ec_cmd_versions() to cros_ec_get_cmd_versions()
- Use EC_CMD_GET_CMD_VERSIONS v0 in cros_ec_get_cmd_versions() if possible
- Drop now unnecessary EOPNOTSUPP check after initial configuration
- Collect review tags
- Link to v3: https://lore.kernel.org/r/20240610-cros_ec-charge-control-v3-0-135e37252094@weissschuh.net

Changes in v3:
- Drop MFD patch that was already applied
  (therefore drop Lee from series recipients)
- Introduce and use devm_battery_hook_register()
- Note that the driver should go through the chrome-platform tree
- Introduce and use cros_ec_cmd_versions()
- Drop logging about impossible charge behaviours
- Use sysfs_attr_init()
- Disable probing on incompatible Framework devices by default
- Link to v2: https://lore.kernel.org/r/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@weissschuh.net

Changes in v2:
- Accept "0" as charge_start_threshold
- Don't include linux/kernel.h
- Only bind to the first found battery
- Import EC_CMD_CHARGE_CONTROL v3 headers
- Add support for v1 and v3 commands
- Sort mfd cell entry alphabetically
- Link to v1: https://lore.kernel.org/r/20240519-cros_ec-charge-control-v1-0-baf305dc79b8@weissschuh.net

---
Thomas Weißschuh (5):
      ACPI: battery: add devm_battery_hook_register()
      platform/chrome: Update binary interface for EC-based charge control
      platform/chrome: cros_ec_proto: Introduce cros_ec_get_cmd_versions()
      power: supply: add ChromeOS EC based charge control driver
      power: supply: cros_charge-control: don't load if Framework control is present

 MAINTAINERS                                    |   6 +
 drivers/acpi/battery.c                         |  15 ++
 drivers/platform/chrome/cros_ec_proto.c        |  35 +++
 drivers/power/supply/Kconfig                   |  12 +
 drivers/power/supply/Makefile                  |   1 +
 drivers/power/supply/cros_charge-control.c     | 357 +++++++++++++++++++++++++
 include/acpi/battery.h                         |   2 +
 include/linux/platform_data/cros_ec_commands.h |  49 +++-
 include/linux/platform_data/cros_ec_proto.h    |   2 +
 9 files changed, 477 insertions(+), 2 deletions(-)
---
base-commit: b57cd5703a1618e87772094ac12c5ee7d6c35e2f
change-id: 20240506-cros_ec-charge-control-685617e8ed87

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v4 1/5] ACPI: battery: add devm_battery_hook_register()

[Thomas Weißschuh]

Add a utility function for device-managed registration of battery hooks.
The function makes it easier to manage the lifecycle of a hook.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/acpi/battery.c | 15 +++++++++++++++
 include/acpi/battery.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index b379401ff1c2..6ea979f76f84 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -756,6 +756,21 @@ void battery_hook_register(struct acpi_battery_hook *hook)
 }
 EXPORT_SYMBOL_GPL(battery_hook_register);
 
+static void devm_battery_hook_unregister(void *data)
+{
+	struct acpi_battery_hook *hook = data;
+
+	battery_hook_unregister(hook);
+}
+
+int devm_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook)
+{
+	battery_hook_register(hook);
+
+	return devm_add_action_or_reset(dev, devm_battery_hook_unregister, hook);
+}
+EXPORT_SYMBOL_GPL(devm_battery_hook_register);
+
 /*
  * This function gets called right after the battery sysfs
  * attributes have been added, so that the drivers that
diff --git a/include/acpi/battery.h b/include/acpi/battery.h
index 611a2561a014..c93f16dfb944 100644
--- a/include/acpi/battery.h
+++ b/include/acpi/battery.h
@@ -2,6 +2,7 @@
 #ifndef __ACPI_BATTERY_H
 #define __ACPI_BATTERY_H
 
+#include <linux/device.h>
 #include <linux/power_supply.h>
 
 #define ACPI_BATTERY_CLASS "battery"
@@ -19,5 +20,6 @@ struct acpi_battery_hook {
 
 void battery_hook_register(struct acpi_battery_hook *hook);
 void battery_hook_unregister(struct acpi_battery_hook *hook);
+int devm_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
 
 #endif

-- 
2.45.2

[PATCH v4 2/5] platform/chrome: Update binary interface for EC-based charge control

[Thomas Weißschuh]

The charge-control command v2/v3 is more featureful than v1, it
additionally supports charge thresholds.

The definitions were imported from ChromeOS EC commit
32870d602317 ("squirtle: modify motionsense rotation matrix")

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/platform_data/cros_ec_commands.h | 49 ++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index ec598057d1da..e574b790be6f 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -3843,16 +3843,61 @@ struct ec_params_i2c_write {
  * discharge the battery.
  */
 #define EC_CMD_CHARGE_CONTROL 0x0096
-#define EC_VER_CHARGE_CONTROL 1
+#define EC_VER_CHARGE_CONTROL 3
 
 enum ec_charge_control_mode {
 	CHARGE_CONTROL_NORMAL = 0,
 	CHARGE_CONTROL_IDLE,
 	CHARGE_CONTROL_DISCHARGE,
+	/* Add no more entry below. */
+	CHARGE_CONTROL_COUNT,
+};
+
+#define EC_CHARGE_MODE_TEXT                               \
+	{                                                 \
+		[CHARGE_CONTROL_NORMAL] = "NORMAL",       \
+		[CHARGE_CONTROL_IDLE] = "IDLE",           \
+		[CHARGE_CONTROL_DISCHARGE] = "DISCHARGE", \
+	}
+
+enum ec_charge_control_cmd {
+	EC_CHARGE_CONTROL_CMD_SET = 0,
+	EC_CHARGE_CONTROL_CMD_GET,
+};
+
+enum ec_charge_control_flag {
+	EC_CHARGE_CONTROL_FLAG_NO_IDLE = BIT(0),
 };
 
 struct ec_params_charge_control {
-	uint32_t mode;  /* enum charge_control_mode */
+	uint32_t mode; /* enum charge_control_mode */
+
+	/* Below are the fields added in V2. */
+	uint8_t cmd; /* enum ec_charge_control_cmd. */
+	uint8_t flags; /* enum ec_charge_control_flag (v3+) */
+	/*
+	 * Lower and upper thresholds for battery sustainer. This struct isn't
+	 * named to avoid tainting foreign projects' name spaces.
+	 *
+	 * If charge mode is explicitly set (e.g. DISCHARGE), battery sustainer
+	 * will be disabled. To disable battery sustainer, set mode=NORMAL,
+	 * lower=-1, upper=-1.
+	 */
+	struct {
+		int8_t lower; /* Display SoC in percentage. */
+		int8_t upper; /* Display SoC in percentage. */
+	} sustain_soc;
+} __ec_align4;
+
+/* Added in v2 */
+struct ec_response_charge_control {
+	uint32_t mode; /* enum charge_control_mode */
+	struct { /* Battery sustainer thresholds */
+		int8_t lower;
+		int8_t upper;
+	} sustain_soc;
+	uint8_t flags; /* enum ec_charge_control_flag (v3+) */
+	uint8_t reserved;
 } __ec_align4;
 
 /*****************************************************************************/

-- 
2.45.2

[PATCH v4 3/5] platform/chrome: cros_ec_proto: Introduce cros_ec_get_cmd_versions()

[Thomas Weißschuh]

Retrieving the supported versions of a command is a fairly common
operation. Provide a helper for it.

If the command is not supported at all the EC returns
-EINVAL/EC_RES_INVALID_PARAMS.

This error is translated into an empty version mask as that is easier to
handle for callers and they don't need to know about the error details.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/chrome/cros_ec_proto.c     | 35 +++++++++++++++++++++++++++++
 include/linux/platform_data/cros_ec_proto.h |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index fe68be66ee98..f776fd42244f 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -5,6 +5,7 @@
 
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/limits.h>
 #include <linux/module.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_ec_proto.h>
@@ -1069,3 +1070,37 @@ int cros_ec_cmd_readmem(struct cros_ec_device *ec_dev, u8 offset, u8 size, void
 			   &params, sizeof(params), dest, size);
 }
 EXPORT_SYMBOL_GPL(cros_ec_cmd_readmem);
+
+/**
+ * cros_ec_get_cmd_versions - Get supported version mask.
+ *
+ * @ec_dev: EC device
+ * @cmd: Command to test
+ *
+ * Return: version mask on success, negative error number on failure.
+ */
+int cros_ec_get_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd)
+{
+	struct ec_params_get_cmd_versions req_v0;
+	struct ec_params_get_cmd_versions_v1 req_v1;
+	struct ec_response_get_cmd_versions resp;
+	int ret;
+
+	if (cmd <= U8_MAX) {
+		req_v0.cmd = cmd;
+		ret = cros_ec_cmd(ec_dev, 0, EC_CMD_GET_CMD_VERSIONS,
+				  &req_v0, sizeof(req_v0), &resp, sizeof(resp));
+	} else {
+		req_v1.cmd = cmd;
+		ret = cros_ec_cmd(ec_dev, 1, EC_CMD_GET_CMD_VERSIONS,
+				  &req_v1, sizeof(req_v1), &resp, sizeof(resp));
+	}
+
+	if (ret == -EINVAL)
+		return 0; /* Command not implemented */
+	else if (ret < 0)
+		return ret;
+	else
+		return resp.version_mask;
+}
+EXPORT_SYMBOL_GPL(cros_ec_get_cmd_versions);
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 6e9225bdf903..b34ed0cc1f8d 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -263,6 +263,8 @@ int cros_ec_cmd(struct cros_ec_device *ec_dev, unsigned int version, int command
 
 int cros_ec_cmd_readmem(struct cros_ec_device *ec_dev, u8 offset, u8 size, void *dest);
 
+int cros_ec_get_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd);
+
 /**
  * cros_ec_get_time_ns() - Return time in ns.
  *

-- 
2.45.2

Re: [PATCH v4 3/5] platform/chrome: cros_ec_proto: Introduce cros_ec_get_cmd_versions()

[Tzung-Bi Shih]

On Sun, Jun 16, 2024 at 09:03:31PM +0200, Thomas Weißschuh wrote:
> Retrieving the supported versions of a command is a fairly common
> operation. Provide a helper for it.
> 
> If the command is not supported at all the EC returns
> -EINVAL/EC_RES_INVALID_PARAMS.
> 
> This error is translated into an empty version mask as that is easier to
> handle for callers and they don't need to know about the error details.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

In case of the series wouldn't go through chrome-platform tree:
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

[PATCH v4 4/5] power: supply: add ChromeOS EC based charge control driver

[Thomas Weißschuh]

The ChromeOS Embedded Controller implements a command to control charge
thresholds and behaviour.

Use it to implement the standard Linux charge_control_start_threshold,
charge_control_end_threshold and charge_behaviour sysfs UAPIs.

The driver is designed to be probed via the cros_ec mfd device.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 MAINTAINERS                                |   6 +
 drivers/power/supply/Kconfig               |  12 ++
 drivers/power/supply/Makefile              |   1 +
 drivers/power/supply/cros_charge-control.c | 335 +++++++++++++++++++++++++++++
 4 files changed, 354 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e4e6aad46668..8101cd0df305 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5135,6 +5135,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
 F:	sound/soc/codecs/cros_ec_codec.*
 
+CHROMEOS EC CHARGE CONTROL
+M:	Thomas Weißschuh <thomas@weissschuh.net>
+S:	Maintained
+F:	drivers/power/supply/cros_charge-control.c
+
 CHROMEOS EC HARDWARE MONITORING
 M:	Thomas Weißschuh <thomas@weissschuh.net>
 L:	chrome-platform@lists.linux.dev
@@ -5148,6 +5153,7 @@ M:	Benson Leung <bleung@chromium.org>
 R:	Guenter Roeck <groeck@chromium.org>
 L:	chrome-platform@lists.linux.dev
 S:	Maintained
+F:	drivers/power/supply/cros_charge-control.c
 F:	drivers/power/supply/cros_usbpd-charger.c
 N:	cros_ec
 N:	cros-ec
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 3e31375491d5..f6321a42aa53 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -860,6 +860,18 @@ config CHARGER_CROS_PCHG
 	  the peripheral charge ports from the EC and converts that into
 	  power_supply properties.
 
+config CHARGER_CROS_CONTROL
+	tristate "ChromeOS EC based charge control"
+	depends on MFD_CROS_EC_DEV
+	depends on ACPI_BATTERY
+	default MFD_CROS_EC_DEV
+	help
+	  Say Y here to enable ChromeOS EC based battery charge control.
+	  This driver can manage charge thresholds and behaviour.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called cros_charge-control.
+
 config CHARGER_SC2731
 	tristate "Spreadtrum SC2731 charger driver"
 	depends on MFD_SC27XX_PMIC || COMPILE_TEST
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 58b567278034..31ca6653a564 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -100,6 +100,7 @@ obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
 obj-$(CONFIG_CHARGER_TPS65217)	+= tps65217_charger.o
 obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
 obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
+obj-$(CONFIG_CHARGER_CROS_CONTROL)	+= cros_charge-control.o
 obj-$(CONFIG_CHARGER_CROS_USBPD)	+= cros_usbpd-charger.o
 obj-$(CONFIG_CHARGER_CROS_PCHG)	+= cros_peripheral_charger.o
 obj-$(CONFIG_CHARGER_SC2731)	+= sc2731_charger.o
diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
new file mode 100644
index 000000000000..e1317a6f23ca
--- /dev/null
+++ b/drivers/power/supply/cros_charge-control.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  ChromeOS EC driver for charge control
+ *
+ *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
+ */
+#include <acpi/battery.h>
+#include <linux/container_of.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#define DRV_NAME	"cros-charge-control"
+
+#define EC_CHARGE_CONTROL_BEHAVIOURS	(BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)             | \
+					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)   | \
+					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE))
+
+enum CROS_CHCTL_ATTR {
+	CROS_CHCTL_ATTR_START_THRESHOLD,
+	CROS_CHCTL_ATTR_END_THRESHOLD,
+	CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR,
+
+	_CROS_CHCTL_ATTR_COUNT,
+};
+
+/*
+ * Semantics of data *returned* from the EC API and Linux sysfs differ
+ * slightly, also the v1 API can not return any data.
+ * To match the expected sysfs API, data is never read back from the EC but
+ * cached in the driver.
+ *
+ * Changes to the EC bypassing the driver will not be reflected in sysfs.
+ * Any change to "charge_behaviour" will synchronize the EC with the driver state.
+ */
+
+struct cros_chctl_priv {
+	struct cros_ec_device *cros_ec;
+	struct acpi_battery_hook battery_hook;
+	struct power_supply *hooked_battery;
+	u8 cmd_version;
+
+	/* The callbacks need to access this priv structure.
+	 * As neither the struct device nor power_supply are under the drivers
+	 * control, embed the attributes within priv to use with container_of().
+	 */
+	struct device_attribute device_attrs[_CROS_CHCTL_ATTR_COUNT];
+	struct attribute *attributes[_CROS_CHCTL_ATTR_COUNT];
+	struct attribute_group group;
+
+	enum power_supply_charge_behaviour current_behaviour;
+	u8 current_start_threshold, current_end_threshold;
+};
+
+static int cros_chctl_send_charge_control_cmd(struct cros_ec_device *cros_ec,
+					      u8 cmd_version, struct ec_params_charge_control *req)
+{
+	static const u8 outsizes[] = {
+		[1] = offsetof(struct ec_params_charge_control, cmd),
+		[2] = sizeof(struct ec_params_charge_control),
+		[3] = sizeof(struct ec_params_charge_control),
+	};
+
+	struct {
+		struct cros_ec_command msg;
+		union {
+			struct ec_params_charge_control req;
+			struct ec_response_charge_control resp;
+		} __packed data;
+	} __packed buf = {
+		.msg = {
+			.command = EC_CMD_CHARGE_CONTROL,
+			.version = cmd_version,
+			.insize  = 0,
+			.outsize = outsizes[cmd_version],
+		},
+		.data.req = *req,
+	};
+
+	return cros_ec_cmd_xfer_status(cros_ec, &buf.msg);
+}
+
+static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
+{
+	struct ec_params_charge_control req = { };
+
+	req.cmd = EC_CHARGE_CONTROL_CMD_SET;
+
+	switch (priv->current_behaviour) {
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
+		req.mode = CHARGE_CONTROL_NORMAL;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
+		req.mode = CHARGE_CONTROL_IDLE;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
+		req.mode = CHARGE_CONTROL_DISCHARGE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (priv->current_behaviour == POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO &&
+	    !(priv->current_start_threshold == 0 && priv->current_end_threshold == 100)) {
+		req.sustain_soc.lower = priv->current_start_threshold;
+		req.sustain_soc.upper = priv->current_end_threshold;
+	} else {
+		/* Disable charging limits */
+		req.sustain_soc.lower = -1;
+		req.sustain_soc.upper = -1;
+	}
+
+	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
+}
+
+static struct cros_chctl_priv *cros_chctl_attr_to_priv(struct attribute *attr,
+						       enum CROS_CHCTL_ATTR idx)
+{
+	struct device_attribute *dev_attr = container_of(attr, struct device_attribute, attr);
+
+	return container_of(dev_attr, struct cros_chctl_priv, device_attrs[idx]);
+}
+
+static ssize_t cros_chctl_store_threshold(struct device *dev, struct cros_chctl_priv *priv,
+					  int is_end_threshold, const char *buf, size_t count)
+{
+	int ret, val;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+	if (val < 0 || val > 100)
+		return -EINVAL;
+
+	if (is_end_threshold) {
+		if (val <= priv->current_start_threshold)
+			return -EINVAL;
+		priv->current_end_threshold = val;
+	} else {
+		if (val >= priv->current_end_threshold)
+			return -EINVAL;
+		priv->current_start_threshold = val;
+	}
+
+	if (priv->current_behaviour == POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) {
+		ret = cros_chctl_configure_ec(priv);
+		if (ret < 0)
+			return ret;
+	}
+
+	return count;
+}
+
+static ssize_t charge_control_start_threshold_show(struct device *dev,
+						   struct device_attribute *attr,
+						   char *buf)
+{
+	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
+							       CROS_CHCTL_ATTR_START_THRESHOLD);
+
+	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_start_threshold);
+}
+
+static ssize_t charge_control_start_threshold_store(struct device *dev,
+						    struct device_attribute *attr,
+						    const char *buf, size_t count)
+{
+	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
+							       CROS_CHCTL_ATTR_START_THRESHOLD);
+
+	return cros_chctl_store_threshold(dev, priv, 0, buf, count);
+}
+
+static ssize_t charge_control_end_threshold_show(struct device *dev, struct device_attribute *attr,
+						 char *buf)
+{
+	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
+							       CROS_CHCTL_ATTR_END_THRESHOLD);
+
+	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_end_threshold);
+}
+
+static ssize_t charge_control_end_threshold_store(struct device *dev, struct device_attribute *attr,
+						  const char *buf, size_t count)
+{
+	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
+							       CROS_CHCTL_ATTR_END_THRESHOLD);
+
+	return cros_chctl_store_threshold(dev, priv, 1, buf, count);
+}
+
+static ssize_t charge_behaviour_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
+							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
+
+	return power_supply_charge_behaviour_show(dev, EC_CHARGE_CONTROL_BEHAVIOURS,
+						  priv->current_behaviour, buf);
+}
+
+static ssize_t charge_behaviour_store(struct device *dev, struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
+							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
+	enum power_supply_charge_behaviour behaviour;
+	int ret;
+
+	behaviour = power_supply_charge_behaviour_parse(EC_CHARGE_CONTROL_BEHAVIOURS, buf);
+	if (behaviour < 0)
+		return behaviour;
+
+	priv->current_behaviour = behaviour;
+
+	ret = cros_chctl_configure_ec(priv);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static umode_t cros_chtl_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(attr, n);
+
+	if (priv->cmd_version < 2) {
+		if (n == CROS_CHCTL_ATTR_START_THRESHOLD)
+			return 0;
+		if (n == CROS_CHCTL_ATTR_END_THRESHOLD)
+			return 0;
+	}
+
+	return attr->mode;
+}
+
+static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	struct cros_chctl_priv *priv = container_of(hook, struct cros_chctl_priv, battery_hook);
+
+	if (priv->hooked_battery)
+		return 0;
+
+	priv->hooked_battery = battery;
+	return device_add_group(&battery->dev, &priv->group);
+}
+
+static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	struct cros_chctl_priv *priv = container_of(hook, struct cros_chctl_priv, battery_hook);
+
+	if (priv->hooked_battery == battery) {
+		device_remove_group(&battery->dev, &priv->group);
+		priv->hooked_battery = NULL;
+	}
+
+	return 0;
+}
+
+static int cros_chctl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
+	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
+	struct cros_chctl_priv *priv;
+	size_t i;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = cros_ec_get_cmd_versions(cros_ec, EC_CMD_CHARGE_CONTROL);
+	if (ret < 0)
+		return ret;
+	else if (ret & EC_VER_MASK(3))
+		priv->cmd_version = 3;
+	else if (ret & EC_VER_MASK(2))
+		priv->cmd_version = 2;
+	else if (ret & EC_VER_MASK(1))
+		priv->cmd_version = 1;
+	else
+		return -ENODEV;
+
+	dev_dbg(dev, "Command version: %u\n", (unsigned int)priv->cmd_version);
+
+	priv->cros_ec = cros_ec;
+	priv->device_attrs[CROS_CHCTL_ATTR_START_THRESHOLD] =
+		(struct device_attribute)__ATTR_RW(charge_control_start_threshold);
+	priv->device_attrs[CROS_CHCTL_ATTR_END_THRESHOLD] =
+		(struct device_attribute)__ATTR_RW(charge_control_end_threshold);
+	priv->device_attrs[CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR] =
+		(struct device_attribute)__ATTR_RW(charge_behaviour);
+	for (i = 0; i < _CROS_CHCTL_ATTR_COUNT; i++) {
+		sysfs_attr_init(&priv->device_attrs[i].attr);
+		priv->attributes[i] = &priv->device_attrs[i].attr;
+	}
+	priv->attributes[_CROS_CHCTL_ATTR_COUNT] = NULL;
+	priv->group.is_visible = cros_chtl_attr_is_visible;
+	priv->group.attrs = priv->attributes;
+
+	priv->battery_hook.name = dev_name(dev),
+	priv->battery_hook.add_battery = cros_chctl_add_battery,
+	priv->battery_hook.remove_battery = cros_chctl_remove_battery,
+
+	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+	priv->current_start_threshold = 0;
+	priv->current_end_threshold = 100;
+
+	/* Bring EC into well-known state */
+	ret = cros_chctl_configure_ec(priv);
+	if (ret < 0)
+		return ret;
+
+	return devm_battery_hook_register(dev, &priv->battery_hook);
+}
+
+static const struct platform_device_id cros_chctl_id[] = {
+	{ DRV_NAME, 0 },
+	{ }
+};
+
+static struct platform_driver cros_chctl_driver = {
+	.driver.name	= DRV_NAME,
+	.probe		= cros_chctl_probe,
+	.id_table	= cros_chctl_id,
+};
+module_platform_driver(cros_chctl_driver);
+
+MODULE_DEVICE_TABLE(platform, cros_chctl_id);
+MODULE_DESCRIPTION("ChromeOS EC charge control");
+MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
+MODULE_LICENSE("GPL");

-- 
2.45.2

Re: [PATCH v4 4/5] power: supply: add ChromeOS EC based charge control driver

[Sebastian Reichel]

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

Hi,

On Sun, Jun 16, 2024 at 09:03:32PM GMT, Thomas Weißschuh wrote:
> The ChromeOS Embedded Controller implements a command to control charge
> thresholds and behaviour.
> 
> Use it to implement the standard Linux charge_control_start_threshold,
> charge_control_end_threshold and charge_behaviour sysfs UAPIs.
> 
> The driver is designed to be probed via the cros_ec mfd device.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  MAINTAINERS                                |   6 +
>  drivers/power/supply/Kconfig               |  12 ++
>  drivers/power/supply/Makefile              |   1 +
>  drivers/power/supply/cros_charge-control.c | 335 +++++++++++++++++++++++++++++
>  4 files changed, 354 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e4e6aad46668..8101cd0df305 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5135,6 +5135,11 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
>  F:	sound/soc/codecs/cros_ec_codec.*
>  
> +CHROMEOS EC CHARGE CONTROL
> +M:	Thomas Weißschuh <thomas@weissschuh.net>
> +S:	Maintained
> +F:	drivers/power/supply/cros_charge-control.c
> +
>  CHROMEOS EC HARDWARE MONITORING
>  M:	Thomas Weißschuh <thomas@weissschuh.net>
>  L:	chrome-platform@lists.linux.dev
> @@ -5148,6 +5153,7 @@ M:	Benson Leung <bleung@chromium.org>
>  R:	Guenter Roeck <groeck@chromium.org>
>  L:	chrome-platform@lists.linux.dev
>  S:	Maintained
> +F:	drivers/power/supply/cros_charge-control.c
>  F:	drivers/power/supply/cros_usbpd-charger.c
>  N:	cros_ec
>  N:	cros-ec
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 3e31375491d5..f6321a42aa53 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -860,6 +860,18 @@ config CHARGER_CROS_PCHG
>  	  the peripheral charge ports from the EC and converts that into
>  	  power_supply properties.
>  
> +config CHARGER_CROS_CONTROL
> +	tristate "ChromeOS EC based charge control"
> +	depends on MFD_CROS_EC_DEV
> +	depends on ACPI_BATTERY
> +	default MFD_CROS_EC_DEV
> +	help
> +	  Say Y here to enable ChromeOS EC based battery charge control.
> +	  This driver can manage charge thresholds and behaviour.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called cros_charge-control.
> +
>  config CHARGER_SC2731
>  	tristate "Spreadtrum SC2731 charger driver"
>  	depends on MFD_SC27XX_PMIC || COMPILE_TEST
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 58b567278034..31ca6653a564 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -100,6 +100,7 @@ obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
>  obj-$(CONFIG_CHARGER_TPS65217)	+= tps65217_charger.o
>  obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>  obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
> +obj-$(CONFIG_CHARGER_CROS_CONTROL)	+= cros_charge-control.o
>  obj-$(CONFIG_CHARGER_CROS_USBPD)	+= cros_usbpd-charger.o
>  obj-$(CONFIG_CHARGER_CROS_PCHG)	+= cros_peripheral_charger.o
>  obj-$(CONFIG_CHARGER_SC2731)	+= sc2731_charger.o
> diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
> new file mode 100644
> index 000000000000..e1317a6f23ca
> --- /dev/null
> +++ b/drivers/power/supply/cros_charge-control.c
> @@ -0,0 +1,335 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  ChromeOS EC driver for charge control
> + *
> + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> + */
> +#include <acpi/battery.h>
> +#include <linux/container_of.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#define DRV_NAME	"cros-charge-control"
> +
> +#define EC_CHARGE_CONTROL_BEHAVIOURS	(BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)             | \
> +					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)   | \
> +					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE))
> +
> +enum CROS_CHCTL_ATTR {
> +	CROS_CHCTL_ATTR_START_THRESHOLD,
> +	CROS_CHCTL_ATTR_END_THRESHOLD,
> +	CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR,
> +
> +	_CROS_CHCTL_ATTR_COUNT,
> +};
> +
> +/*
> + * Semantics of data *returned* from the EC API and Linux sysfs differ
> + * slightly, also the v1 API can not return any data.
> + * To match the expected sysfs API, data is never read back from the EC but
> + * cached in the driver.
> + *
> + * Changes to the EC bypassing the driver will not be reflected in sysfs.
> + * Any change to "charge_behaviour" will synchronize the EC with the driver state.
> + */
> +
> +struct cros_chctl_priv {
> +	struct cros_ec_device *cros_ec;
> +	struct acpi_battery_hook battery_hook;
> +	struct power_supply *hooked_battery;
> +	u8 cmd_version;
> +
> +	/* The callbacks need to access this priv structure.
> +	 * As neither the struct device nor power_supply are under the drivers
> +	 * control, embed the attributes within priv to use with container_of().
> +	 */
> +	struct device_attribute device_attrs[_CROS_CHCTL_ATTR_COUNT];
> +	struct attribute *attributes[_CROS_CHCTL_ATTR_COUNT];
> +	struct attribute_group group;
> +
> +	enum power_supply_charge_behaviour current_behaviour;
> +	u8 current_start_threshold, current_end_threshold;
> +};
> +
> +static int cros_chctl_send_charge_control_cmd(struct cros_ec_device *cros_ec,
> +					      u8 cmd_version, struct ec_params_charge_control *req)
> +{
> +	static const u8 outsizes[] = {
> +		[1] = offsetof(struct ec_params_charge_control, cmd),
> +		[2] = sizeof(struct ec_params_charge_control),
> +		[3] = sizeof(struct ec_params_charge_control),
> +	};
> +
> +	struct {
> +		struct cros_ec_command msg;
> +		union {
> +			struct ec_params_charge_control req;
> +			struct ec_response_charge_control resp;
> +		} __packed data;
> +	} __packed buf = {
> +		.msg = {
> +			.command = EC_CMD_CHARGE_CONTROL,
> +			.version = cmd_version,
> +			.insize  = 0,
> +			.outsize = outsizes[cmd_version],
> +		},
> +		.data.req = *req,
> +	};
> +
> +	return cros_ec_cmd_xfer_status(cros_ec, &buf.msg);
> +}
> +
> +static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
> +{
> +	struct ec_params_charge_control req = { };
> +
> +	req.cmd = EC_CHARGE_CONTROL_CMD_SET;
> +
> +	switch (priv->current_behaviour) {
> +	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
> +		req.mode = CHARGE_CONTROL_NORMAL;
> +		break;
> +	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
> +		req.mode = CHARGE_CONTROL_IDLE;
> +		break;
> +	case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
> +		req.mode = CHARGE_CONTROL_DISCHARGE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (priv->current_behaviour == POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO &&
> +	    !(priv->current_start_threshold == 0 && priv->current_end_threshold == 100)) {
> +		req.sustain_soc.lower = priv->current_start_threshold;
> +		req.sustain_soc.upper = priv->current_end_threshold;
> +	} else {
> +		/* Disable charging limits */
> +		req.sustain_soc.lower = -1;
> +		req.sustain_soc.upper = -1;
> +	}
> +
> +	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
> +}
> +
> +static struct cros_chctl_priv *cros_chctl_attr_to_priv(struct attribute *attr,
> +						       enum CROS_CHCTL_ATTR idx)
> +{
> +	struct device_attribute *dev_attr = container_of(attr, struct device_attribute, attr);
> +
> +	return container_of(dev_attr, struct cros_chctl_priv, device_attrs[idx]);
> +}
> +
> +static ssize_t cros_chctl_store_threshold(struct device *dev, struct cros_chctl_priv *priv,
> +					  int is_end_threshold, const char *buf, size_t count)
> +{
> +	int ret, val;
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;
> +	if (val < 0 || val > 100)
> +		return -EINVAL;
> +
> +	if (is_end_threshold) {
> +		if (val <= priv->current_start_threshold)
> +			return -EINVAL;
> +		priv->current_end_threshold = val;
> +	} else {
> +		if (val >= priv->current_end_threshold)
> +			return -EINVAL;
> +		priv->current_start_threshold = val;
> +	}
> +
> +	if (priv->current_behaviour == POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) {
> +		ret = cros_chctl_configure_ec(priv);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return count;
> +}
> +
> +static ssize_t charge_control_start_threshold_show(struct device *dev,
> +						   struct device_attribute *attr,
> +						   char *buf)
> +{
> +	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
> +							       CROS_CHCTL_ATTR_START_THRESHOLD);
> +
> +	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_start_threshold);
> +}
> +
> +static ssize_t charge_control_start_threshold_store(struct device *dev,
> +						    struct device_attribute *attr,
> +						    const char *buf, size_t count)
> +{
> +	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
> +							       CROS_CHCTL_ATTR_START_THRESHOLD);
> +
> +	return cros_chctl_store_threshold(dev, priv, 0, buf, count);
> +}
> +
> +static ssize_t charge_control_end_threshold_show(struct device *dev, struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
> +							       CROS_CHCTL_ATTR_END_THRESHOLD);
> +
> +	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_end_threshold);
> +}
> +
> +static ssize_t charge_control_end_threshold_store(struct device *dev, struct device_attribute *attr,
> +						  const char *buf, size_t count)
> +{
> +	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
> +							       CROS_CHCTL_ATTR_END_THRESHOLD);
> +
> +	return cros_chctl_store_threshold(dev, priv, 1, buf, count);
> +}
> +
> +static ssize_t charge_behaviour_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
> +							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
> +
> +	return power_supply_charge_behaviour_show(dev, EC_CHARGE_CONTROL_BEHAVIOURS,
> +						  priv->current_behaviour, buf);
> +}
> +
> +static ssize_t charge_behaviour_store(struct device *dev, struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
> +							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
> +	enum power_supply_charge_behaviour behaviour;
> +	int ret;
> +
> +	behaviour = power_supply_charge_behaviour_parse(EC_CHARGE_CONTROL_BEHAVIOURS, buf);
> +	if (behaviour < 0)
> +		return behaviour;
> +
> +	priv->current_behaviour = behaviour;
> +
> +	ret = cros_chctl_configure_ec(priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static umode_t cros_chtl_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
> +{
> +	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(attr, n);
> +
> +	if (priv->cmd_version < 2) {
> +		if (n == CROS_CHCTL_ATTR_START_THRESHOLD)
> +			return 0;
> +		if (n == CROS_CHCTL_ATTR_END_THRESHOLD)
> +			return 0;
> +	}
> +
> +	return attr->mode;
> +}
> +
> +static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	struct cros_chctl_priv *priv = container_of(hook, struct cros_chctl_priv, battery_hook);
> +
> +	if (priv->hooked_battery)
> +		return 0;
> +
> +	priv->hooked_battery = battery;
> +	return device_add_group(&battery->dev, &priv->group);
> +}
> +
> +static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	struct cros_chctl_priv *priv = container_of(hook, struct cros_chctl_priv, battery_hook);
> +
> +	if (priv->hooked_battery == battery) {
> +		device_remove_group(&battery->dev, &priv->group);
> +		priv->hooked_battery = NULL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_chctl_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> +	struct cros_chctl_priv *priv;
> +	size_t i;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ret = cros_ec_get_cmd_versions(cros_ec, EC_CMD_CHARGE_CONTROL);
> +	if (ret < 0)
> +		return ret;
> +	else if (ret & EC_VER_MASK(3))
> +		priv->cmd_version = 3;
> +	else if (ret & EC_VER_MASK(2))
> +		priv->cmd_version = 2;
> +	else if (ret & EC_VER_MASK(1))
> +		priv->cmd_version = 1;
> +	else
> +		return -ENODEV;
> +
> +	dev_dbg(dev, "Command version: %u\n", (unsigned int)priv->cmd_version);
> +
> +	priv->cros_ec = cros_ec;
> +	priv->device_attrs[CROS_CHCTL_ATTR_START_THRESHOLD] =
> +		(struct device_attribute)__ATTR_RW(charge_control_start_threshold);
> +	priv->device_attrs[CROS_CHCTL_ATTR_END_THRESHOLD] =
> +		(struct device_attribute)__ATTR_RW(charge_control_end_threshold);
> +	priv->device_attrs[CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR] =
> +		(struct device_attribute)__ATTR_RW(charge_behaviour);
> +	for (i = 0; i < _CROS_CHCTL_ATTR_COUNT; i++) {
> +		sysfs_attr_init(&priv->device_attrs[i].attr);
> +		priv->attributes[i] = &priv->device_attrs[i].attr;
> +	}
> +	priv->attributes[_CROS_CHCTL_ATTR_COUNT] = NULL;
> +	priv->group.is_visible = cros_chtl_attr_is_visible;
> +	priv->group.attrs = priv->attributes;
> +
> +	priv->battery_hook.name = dev_name(dev),
> +	priv->battery_hook.add_battery = cros_chctl_add_battery,
> +	priv->battery_hook.remove_battery = cros_chctl_remove_battery,
> +
> +	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +	priv->current_start_threshold = 0;
> +	priv->current_end_threshold = 100;
> +
> +	/* Bring EC into well-known state */
> +	ret = cros_chctl_configure_ec(priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_battery_hook_register(dev, &priv->battery_hook);
> +}
> +
> +static const struct platform_device_id cros_chctl_id[] = {
> +	{ DRV_NAME, 0 },
> +	{ }
> +};
> +
> +static struct platform_driver cros_chctl_driver = {
> +	.driver.name	= DRV_NAME,
> +	.probe		= cros_chctl_probe,
> +	.id_table	= cros_chctl_id,
> +};
> +module_platform_driver(cros_chctl_driver);
> +
> +MODULE_DEVICE_TABLE(platform, cros_chctl_id);
> +MODULE_DESCRIPTION("ChromeOS EC charge control");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 4/5] power: supply: add ChromeOS EC based charge control driver

[Thomas Weißschuh]

Hi Sebastian,

On 2024-06-20 00:24:23+0000, Sebastian Reichel wrote:
> On Sun, Jun 16, 2024 at 09:03:32PM GMT, Thomas Weißschuh wrote:
> > The ChromeOS Embedded Controller implements a command to control charge
> > thresholds and behaviour.
> > 
> > Use it to implement the standard Linux charge_control_start_threshold,
> > charge_control_end_threshold and charge_behaviour sysfs UAPIs.
> > 
> > The driver is designed to be probed via the cros_ec mfd device.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> 
> Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Thanks!

<snip>

Would you also take a look at patch 5,
"power: supply: cros_charge-control: don't load if Framework control is present"?

I'm still wondering what the best solution for the two different EC APIs
would be. Maybe you have an idea?

In short:

Framework laptops have a downstream charge control API in their EC.
This drivers binds to the upstream CrOS EC APIs, which does work on
Framework laptops.
If the downstream API is used, it overrides the functionality of the
upstream API.

Choices I see:
* Ignore the incompatibility and just load the driver (maybe log something)
* Detect if the downstream API is actively used during probing
  (If enabled by UEFI) and either
  * don't load the driver
  * disable the current downstream API configuration, so the driver works
  * disable the downstream API and take over its configuration into the
    upstream driver
* Detect if the downstream API is present at all and don't load the
  driver (currently implemented)

The problem is that the downstream API is still usable and its usage
would break this driver.
On the other hand, disabling the driver forces users to manually specify
a kernel commandline, preventing most users from actually making use of it.

(For the future I plan on adapting the Framework EC firmware to avoid
this issue, but there is no telling how long it will take and if it is
accepted at all)

Thanks for any ideas,
Thomas

[PATCH v4 5/5] power: supply: cros_charge-control: don't load if Framework control is present

[Thomas Weißschuh]

Framework laptops implement a custom charge control EC command.
The upstream CrOS EC command is also present and functional but can get
overridden by the custom one.

Until Framework make both commands compatible or remove their custom
one, don't load the driver on those machines.

If the user knows they are not going to use the custom command they can
use a module parameter to load cros_charge-control anyways.

Note that the UEFI setup configuration for battery control also uses
their custom command.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/cros_charge-control.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
index e1317a6f23ca..0aec3142054b 100644
--- a/drivers/power/supply/cros_charge-control.c
+++ b/drivers/power/supply/cros_charge-control.c
@@ -6,6 +6,7 @@
  */
 #include <acpi/battery.h>
 #include <linux/container_of.h>
+#include <linux/dmi.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_data/cros_ec_commands.h>
@@ -259,6 +260,19 @@ static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_b
 	return 0;
 }
 
+static bool probe_with_fwk_charge_control;
+module_param(probe_with_fwk_charge_control, bool, 0644);
+MODULE_PARM_DESC(probe_with_fwk_charge_control,
+		 "Probe the driver in the presence of the custom Framework EC charge control");
+
+static int cros_chctl_fwk_charge_control_versions(struct cros_ec_device *cros_ec)
+{
+	if (!dmi_match(DMI_SYS_VENDOR, "Framework"))
+		return 0;
+
+	return cros_ec_get_cmd_versions(cros_ec, 0x3E03 /* FW_EC_CMD_CHARGE_LIMIT */);
+}
+
 static int cros_chctl_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -268,6 +282,14 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	size_t i;
 	int ret;
 
+	ret = cros_chctl_fwk_charge_control_versions(cros_ec);
+	if (ret < 0)
+		return ret;
+	if (ret > 0 && !probe_with_fwk_charge_control) {
+		dev_info(dev, "Framework charge control detected, preventing load\n");
+		return -ENODEV;
+	}
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;

-- 
2.45.2