[PATCH v2 0/3] ChromeOS Embedded Controller charge control driver

[PATCH v2 0/3] ChromeOS Embedded Controller charge control driver

[Thomas Weißschuh]

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" [0], 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.

[0] https://lore.kernel.org/lkml/20240505-cros_ec-framework-v1-0-402662d6276b@weissschuh.net/

Signed-off-by: Thomas Weißschuh <linux@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 (3):
      platform/chrome: Update binary interface for EC-based charge control
      power: supply: add ChromeOS EC based charge control driver
      mfd: cros_ec: Register charge control subdevice

 MAINTAINERS                                    |   6 +
 drivers/mfd/cros_ec_dev.c                      |   1 +
 drivers/power/supply/Kconfig                   |  12 +
 drivers/power/supply/Makefile                  |   1 +
 drivers/power/supply/cros_charge-control.c     | 353 +++++++++++++++++++++++++
 include/linux/platform_data/cros_ec_commands.h |  49 +++-
 6 files changed, 420 insertions(+), 2 deletions(-)
---
base-commit: e0cce98fe279b64f4a7d81b7f5c3a23d80b92fbc
change-id: 20240506-cros_ec-charge-control-685617e8ed87

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

[PATCH v2 1/3] 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")

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 ecc47d5fe239..c35e534e3c3d 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -3809,16 +3809,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.1

Re: [PATCH v2 1/3] platform/chrome: Update binary interface for EC-based charge control

[Tzung-Bi Shih]

On Tue, May 28, 2024 at 10:04:10PM +0200, Thomas Weißschuh wrote:
> 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")
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Checked [1], the changes matched:
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

[1]: https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h

[PATCH v2 2/3] 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 | 353 +++++++++++++++++++++++++++++
 4 files changed, 372 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d6c90161c7bf..fc0fb3827163 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5135,11 +5135,17 @@ 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 SUBDRIVERS
 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..a2bed984163a
--- /dev/null
+++ b/drivers/power/supply/cros_charge-control.c
@@ -0,0 +1,353 @@
+// 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 device *dev, 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:
+		dev_warn_ratelimited(dev, "Unexpected behaviour %d", priv->current_behaviour);
+		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 *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(dev, 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 = 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 = 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 = 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 = 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 = 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 = 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(dev, 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 = 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 ec_params_get_cmd_versions req = {};
+	struct ec_response_get_cmd_versions resp;
+	struct cros_chctl_priv *priv;
+	size_t i;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	req.cmd = EC_CMD_CHARGE_CONTROL;
+	ret = cros_ec_cmd(cros_ec, 0, EC_CMD_GET_CMD_VERSIONS,
+			  &req, sizeof(req), &resp, sizeof(resp));
+	if (ret < 0)
+		return ret;
+	else if (resp.version_mask & EC_VER_MASK(3))
+		priv->cmd_version = 3;
+	else if (resp.version_mask & EC_VER_MASK(2))
+		priv->cmd_version = 2;
+	else if (resp.version_mask & 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++)
+		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(dev, priv);
+	if (ret == -EOPNOTSUPP)
+		return -ENODEV;
+	else if (ret < 0)
+		return ret;
+
+	battery_hook_register(&priv->battery_hook);
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int cros_chctl_remove(struct platform_device *pdev)
+{
+	struct cros_chctl_priv *priv = platform_get_drvdata(pdev);
+
+	battery_hook_unregister(&priv->battery_hook);
+
+	return 0;
+}
+
+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,
+	.remove		= cros_chctl_remove,
+	.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.1

Re: [PATCH v2 2/3] power: supply: add ChromeOS EC based charge control driver

[Sebastian Reichel]

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

Hi,

On Tue, May 28, 2024 at 10:04:11PM +0200, Thomas Weißschuh wrote:
> +	battery_hook_register(&priv->battery_hook);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}
> +
> +static int cros_chctl_remove(struct platform_device *pdev)
> +{
> +	struct cros_chctl_priv *priv = platform_get_drvdata(pdev);
> +
> +	battery_hook_unregister(&priv->battery_hook);
> +
> +	return 0;
> +}

Please use devm_add_action_or_reset() or introduce
devm_battery_hook_register(), which could also be used by
dell-wmi-ddv. Otherwise LGTM.

-- Sebastian

> +
> +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,
> +	.remove		= cros_chctl_remove,
> +	.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.1
> 

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

[PATCH v2 3/3] mfd: cros_ec: Register charge control subdevice

[Thomas Weißschuh]

Add ChromeOS EC-based charge control as EC subdevice.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/mfd/cros_ec_dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index a52d59cc2b1e..dab2bec39dcd 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -87,6 +87,7 @@ static const struct mfd_cell cros_ec_sensorhub_cells[] = {
 };
 
 static const struct mfd_cell cros_usbpd_charger_cells[] = {
+	{ .name = "cros-charge-control", },
 	{ .name = "cros-usbpd-charger", },
 	{ .name = "cros-usbpd-logger", },
 };

-- 
2.45.1

Re: (subset) [PATCH v2 3/3] mfd: cros_ec: Register charge control subdevice

[Lee Jones]

On Tue, 28 May 2024 22:04:12 +0200, Thomas Weißschuh wrote:
> Add ChromeOS EC-based charge control as EC subdevice.
> 
> 

Applied, thanks!

[3/3] mfd: cros_ec: Register charge control subdevice
      commit: 08dbad2c7c3275e0e79190dca139bc65ce775a92

--
Lee Jones [李琼斯]

Re: (subset) [PATCH v2 3/3] mfd: cros_ec: Register charge control subdevice

[Lee Jones]

On Tue, 28 May 2024 22:04:12 +0200, Thomas Weißschuh wrote:
> Add ChromeOS EC-based charge control as EC subdevice.
> 
> 

Applied, thanks!

[3/3] mfd: cros_ec: Register charge control subdevice
      commit: c353be492fbfe7d1c77af6e18891183141c36f98

--
Lee Jones [李琼斯]

Re: [PATCH v2 0/3] ChromeOS Embedded Controller charge control driver

[Dustin Howett]

On Tue, May 28, 2024 at 3:05 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> 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" [0], 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.
>

I've tested this out on the Framework Laptop 13, 11th gen intel core
and AMD Ryzen 7040 editions.

The problem is that the AMD framework laptop *reports* support for the
CrOS charge controller, but it does not truly support it.
As with the 11th Gen Intel Core (and by proxy the 12th, 13th) it still
does require the OEM-specific command.

This is evinced by a mismatch between the firmware-configured value
and the value reported by the charge control subsystem through this
driver.

$ cat /sys/class/power_supply/BAT1/charge_control_end_threshold
100

$ ectool raw 0x3E03 b8 # OEM command 0x3E03 with BIT(3) in the payload
is Framework's charge limit query host command
Read 2 bytes
 50 00                                           |P.              |
(in my case, 80 in decimal)

The charge limit is managed at [1], and it does not appear to
integrate with the standard charge control machinery.

I'll pursue getting this board not to report support for CrOS charge
control. This driver is still entirely fit for purpose, just not for
this board.

Cheers,
d

Re: [PATCH v2 0/3] ChromeOS Embedded Controller charge control driver

[Thomas Weißschuh]

On 2024-06-02 18:40:18+0000, Dustin Howett wrote:
> On Tue, May 28, 2024 at 3:05 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > 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" [0], 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.
> >
> 
> I've tested this out on the Framework Laptop 13, 11th gen intel core
> and AMD Ryzen 7040 editions.

Thanks!

> The problem is that the AMD framework laptop *reports* support for the
> CrOS charge controller, but it does not truly support it.
> As with the 11th Gen Intel Core (and by proxy the 12th, 13th) it still
> does require the OEM-specific command.

This is surpising, it works on my machine, which is also a AMD 7040.

> This is evinced by a mismatch between the firmware-configured value
> and the value reported by the charge control subsystem through this
> driver.
> 
> $ cat /sys/class/power_supply/BAT1/charge_control_end_threshold
> 100
> 
> $ ectool raw 0x3E03 b8 # OEM command 0x3E03 with BIT(3) in the payload
> is Framework's charge limit query host command
> Read 2 bytes
>  50 00                                           |P.              |
> (in my case, 80 in decimal)
> 
> The charge limit is managed at [1], and it does not appear to
> integrate with the standard charge control machinery.
> 
> I'll pursue getting this board not to report support for CrOS charge
> control. This driver is still entirely fit for purpose, just not for
> this board.

Can you try disabling all of the Framework-specific charge control
settings and test again?
Probably the different, disparate logics in the Framework ECs are
conflicting with each other.

Thomas

Re: [PATCH v2 0/3] ChromeOS Embedded Controller charge control driver

[Dustin Howett]

On Mon, Jun 3, 2024 at 3:59 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> Can you try disabling all of the Framework-specific charge control
> settings and test again?
> Probably the different, disparate logics in the Framework ECs are
> conflicting with each other.

Fascinating! This board does indeed support charge limiting through
both interfaces. It looks like the most recently set one wins for a
time.

The UEFI setup utility only sets the framework-specific charge limit value.

We should probably find some way to converge them, for all of the
supported Framework Laptop programs.

> Thomas

Re: [PATCH v2 0/3] ChromeOS Embedded Controller charge control driver

[Thomas Weißschuh]

On 2024-06-04 20:27:57+0000, Dustin Howett wrote:
> On Mon, Jun 3, 2024 at 3:59 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > Can you try disabling all of the Framework-specific charge control
> > settings and test again?
> > Probably the different, disparate logics in the Framework ECs are
> > conflicting with each other.
> 
> Fascinating! This board does indeed support charge limiting through
> both interfaces. It looks like the most recently set one wins for a
> time.

If it is the most recent one, shouldn't the driver have worked?
What does "for a time" mean?
I'm using only the upstream EC command and that seems to work fine.

> The UEFI setup utility only sets the framework-specific charge limit value.
> 
> We should probably find some way to converge them, for all of the
> supported Framework Laptop programs.

In the long term, Framework should align their implementation with
upstream CrOS EC and either drop their custom command or make it a thin
wrapper around the normal the upstream command.

(As you are familiar with EC programming maybe you want to tackle this?)

Until then I think we can detect at probe-time if the Framework APIs are
available and use them to disable the Framework-specific mechanism.
Then the CrOS EC commands should be usable.

The drawback is, that userspace using the Framework APIs will break
the driver. That userspace would need to migrate to the standard UAPI.

Also the settings set in the firmware would be ignored at that point.

I don't want to use the functionality of the Framework command because
it's less featureful and I really hope it will go away at some point.

Re: [PATCH v2 0/3] ChromeOS Embedded Controller charge control driver

[Mario Limonciello]

On 6/5/2024 04:33, Thomas Weißschuh wrote:
> On 2024-06-04 20:27:57+0000, Dustin Howett wrote:
>> On Mon, Jun 3, 2024 at 3:59 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>>>
>>> Can you try disabling all of the Framework-specific charge control
>>> settings and test again?
>>> Probably the different, disparate logics in the Framework ECs are
>>> conflicting with each other.
>>
>> Fascinating! This board does indeed support charge limiting through
>> both interfaces. It looks like the most recently set one wins for a
>> time.
> 
> If it is the most recent one, shouldn't the driver have worked?
> What does "for a time" mean?
> I'm using only the upstream EC command and that seems to work fine.
> 
>> The UEFI setup utility only sets the framework-specific charge limit value.
>>
>> We should probably find some way to converge them, for all of the
>> supported Framework Laptop programs.
> 
> In the long term, Framework should align their implementation with
> upstream CrOS EC and either drop their custom command or make it a thin
> wrapper around the normal the upstream command.
> 
> (As you are familiar with EC programming maybe you want to tackle this?)
> 
> Until then I think we can detect at probe-time if the Framework APIs are
> available and use them to disable the Framework-specific mechanism.
> Then the CrOS EC commands should be usable.
> 
> The drawback is, that userspace using the Framework APIs will break
> the driver. That userspace would need to migrate to the standard UAPI.

How does userspace access the Framework APIs?  Surely it needs to go 
through the kernel?  Could you "filter" the userspace calls to block them?

For example this is something that currently happens in the dell-pc 
driver to block userspace from doing thermal calls and instead guide 
people to the proper API that the driver exports.

> 
> Also the settings set in the firmware would be ignored at that point.
> 
> I don't want to use the functionality of the Framework command because
> it's less featureful and I really hope it will go away at some point.

Re: [PATCH v2 0/3] ChromeOS Embedded Controller charge control driver

[Thomas Weißschuh]

+Matt, the Linux support lead for Framework.

Hi Matt,

below we are discussing on how to implement charge controls for ChromeOS
EC devices including Framework laptops in mainline Linux.
Some feedback would be great.

On 2024-06-05 15:32:33+0000, Mario Limonciello wrote:
> On 6/5/2024 04:33, Thomas Weißschuh wrote:
> > On 2024-06-04 20:27:57+0000, Dustin Howett wrote:
> > > On Mon, Jun 3, 2024 at 3:59 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > > > 
> > > > Can you try disabling all of the Framework-specific charge control
> > > > settings and test again?
> > > > Probably the different, disparate logics in the Framework ECs are
> > > > conflicting with each other.
> > > 
> > > Fascinating! This board does indeed support charge limiting through
> > > both interfaces. It looks like the most recently set one wins for a
> > > time.
> > 
> > If it is the most recent one, shouldn't the driver have worked?
> > What does "for a time" mean?
> > I'm using only the upstream EC command and that seems to work fine.
> > 
> > > The UEFI setup utility only sets the framework-specific charge limit value.
> > > 
> > > We should probably find some way to converge them, for all of the
> > > supported Framework Laptop programs.
> > 
> > In the long term, Framework should align their implementation with
> > upstream CrOS EC and either drop their custom command or make it a thin
> > wrapper around the normal the upstream command.
> > 
> > (As you are familiar with EC programming maybe you want to tackle this?)
> > 
> > Until then I think we can detect at probe-time if the Framework APIs are
> > available and use them to disable the Framework-specific mechanism.
> > Then the CrOS EC commands should be usable.
> > 
> > The drawback is, that userspace using the Framework APIs will break
> > the driver. That userspace would need to migrate to the standard UAPI.
> 
> How does userspace access the Framework APIs?  Surely it needs to go through
> the kernel?  Could you "filter" the userspace calls to block them?
> 
> For example this is something that currently happens in the dell-pc driver
> to block userspace from doing thermal calls and instead guide people to the
> proper API that the driver exports.

This would work when userspace uses /dev/cros_ec.
But the EC can also used via raw port IO which wouldn't be covered.
Given that /dev/cros_ec wasn't usable on Framework AMD until v6.9 it's
not unlikely users are using that.

And technically both aproaches would break userspace.

Another aproach would be to not load the module on Framework devices
which implement their custom command (overwritable by module parameter).

Framework unifies the implementation of their command with the core
CrOS EC logic so both commands work on the same data.
The custom command is adapted to also implement a new command version.
This is completely transparent as the old version will continue to work.

We update the Linux driver to recognize that new command version, know
that they are now compatible and probe the driver.

Newer devices could also drop the custom command and the driver would
start working.

This scheme requires some cooperation from Framework, though.

> > 
> > Also the settings set in the firmware would be ignored at that point.
> > 
> > I don't want to use the functionality of the Framework command because
> > it's less featureful and I really hope it will go away at some point.
>