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/[email protected]/
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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/[email protected]
---
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 <[email protected]>
Add ChromeOS EC-based charge control as EC subdevice.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
+S: Maintained
+F: drivers/power/supply/cros_charge-control.c
+
CHROMEOS EC SUBDRIVERS
M: Benson Leung <[email protected]>
R: Guenter Roeck <[email protected]>
L: [email protected]
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 <[email protected]>
+ */
+#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 <[email protected]>");
+MODULE_LICENSE("GPL");
--
2.45.1
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 [李琼斯]
On Tue, May 28, 2024 at 3:05 PM Thomas Weißschuh <[email protected]> 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
On 2024-06-02 18:40:18+0000, Dustin Howett wrote:
> On Tue, May 28, 2024 at 3:05 PM Thomas Weißschuh <[email protected]> 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
On Mon, Jun 3, 2024 at 3:59 PM Thomas Weißschuh <[email protected]> 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
On 2024-06-04 20:27:57+0000, Dustin Howett wrote:
> On Mon, Jun 3, 2024 at 3:59 PM Thomas Weißschuh <[email protected]> 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.
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 <[email protected]> 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.
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 <[email protected]>");
> +MODULE_LICENSE("GPL");
>
> --
> 2.45.1
>
+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 <[email protected]> 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.
>
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 [李琼斯]