[PATCH 0/2] platform/chrome: cros_ec_framework_laptop: new driver

[PATCH 0/2] platform/chrome: cros_ec_framework_laptop: new driver

[Thomas Weißschuh]

Framework Laptops are using embedded controller firmware based on the
ChromeOS EC project.
In addition to the standard upstream commands some vendor-specific
commands are implemented.

Add a driver that implements battery charge thresholds using these
custom commands.

Patch 1 adds the general scaffolding and device binding.
Patch 2 implements the battery charge thresholds.

This series is based on
https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (2):
      platform/chrome: cros_ec_framework_laptop: introduce driver
      platform/chrome: cros_ec_framework_laptop: implement battery charge thresholds

 MAINTAINERS                                        |   5 +
 drivers/mfd/cros_ec_dev.c                          |  13 ++
 drivers/platform/chrome/Kconfig                    |  11 ++
 drivers/platform/chrome/Makefile                   |   1 +
 drivers/platform/chrome/cros_ec_framework_laptop.c | 173 +++++++++++++++++++++
 5 files changed, 203 insertions(+)
---
base-commit: 2fbe479c0024e1c6b992184a799055e19932aa48
change-id: 20240505-cros_ec-framework-10e627c46a0a

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

[PATCH 1/2] platform/chrome: cros_ec_framework_laptop: introduce driver

[Thomas Weißschuh]

Framework Laptops are using embedded controller firmware based on the
ChromeOS EC project.
In addition to the standard upstream commands, some vendor-specific
ones are implemented.

Add a driver for those custom EC commands.

At first, provide an empty driver that only takes care of scaffolding and
device binding.
Further patches will add functionality to the driver.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 MAINTAINERS                                        |  5 ++
 drivers/mfd/cros_ec_dev.c                          | 13 ++++++
 drivers/platform/chrome/Kconfig                    | 11 +++++
 drivers/platform/chrome/Makefile                   |  1 +
 drivers/platform/chrome/cros_ec_framework_laptop.c | 53 ++++++++++++++++++++++
 5 files changed, 83 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c23fda1aa1f0..60699c289757 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4988,6 +4988,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
 F:	sound/soc/codecs/cros_ec_codec.*
 
+CHROMEOS EC FRAMEWORK LAPTOP EXTENSIONS
+M:	Thomas Weißschuh <linux@weissschuh.net>
+S:	Maintained
+F:	drivers/platform/chrome/cros_ec_framework_laptop.c
+
 CHROMEOS EC SUBDRIVERS
 M:	Benson Leung <bleung@chromium.org>
 R:	Guenter Roeck <groeck@chromium.org>
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index a52d59cc2b1e..0a36e77e5039 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -145,6 +145,10 @@ static const struct mfd_cell cros_ec_vbc_cells[] = {
 	{ .name = "cros-ec-vbc", }
 };
 
+static const struct mfd_cell cros_ec_framework_cells[] = {
+	{ .name = "cros-ec-framework", }
+};
+
 static void cros_ec_class_release(struct device *dev)
 {
 	kfree(to_cros_ec_dev(dev));
@@ -299,6 +303,15 @@ static int ec_device_probe(struct platform_device *pdev)
 				 retval);
 	}
 
+	 /* The EC on Framework laptops implements some nonstandard features */
+	if (dmi_match(DMI_SYS_VENDOR, "Framework")) {
+		retval = mfd_add_hotplug_devices(ec->dev, cros_ec_framework_cells,
+						 ARRAY_SIZE(cros_ec_framework_cells));
+		if (retval)
+			dev_warn(ec->dev, "failed to add framework laptop devices: %d\n",
+				 retval);
+	}
+
 	return 0;
 
 failed:
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 073616b5b5a0..ff69ee226606 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -239,6 +239,17 @@ config CROS_EC_TYPEC
 	  To compile this driver as a module, choose M here: the module will be
 	  called cros-ec-typec.
 
+config CROS_EC_FRAMEWORK_LAPTOP
+	tristate "ChromeOS EC Framework Laptop extensions"
+	depends on MFD_CROS_EC_DEV
+	default MFD_CROS_EC_DEV
+	help
+	  If you say Y here, you get support for using Framework Laptop-specific extensions
+	  of the Chrome OS EC.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called cros_ec_framework_laptop.
+
 config CROS_HPS_I2C
 	tristate "ChromeOS HPS device"
 	depends on HID && I2C && PM
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 2dcc6ccc2302..ce6aac620086 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_CHROMEOS_PRIVACY_SCREEN)	+= chromeos_privacy_screen.o
 obj-$(CONFIG_CHROMEOS_PSTORE)		+= chromeos_pstore.o
 obj-$(CONFIG_CHROMEOS_TBMC)		+= chromeos_tbmc.o
 obj-$(CONFIG_CROS_EC)			+= cros_ec.o
+obj-$(CONFIG_CROS_EC_FRAMEWORK_LAPTOP)	+= cros_ec_framework_laptop.o
 obj-$(CONFIG_CROS_EC_I2C)		+= cros_ec_i2c.o
 obj-$(CONFIG_CROS_EC_ISHTP)		+= cros_ec_ishtp.o
 obj-$(CONFIG_CROS_TYPEC_SWITCH)		+= cros_typec_switch.o
diff --git a/drivers/platform/chrome/cros_ec_framework_laptop.c b/drivers/platform/chrome/cros_ec_framework_laptop.c
new file mode 100644
index 000000000000..8a8bf039fa9c
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_framework_laptop.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  ChromesOS EC driver for Framework laptop
+ *
+ *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
+ */
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME	"cros-ec-framework"
+
+struct cros_fwk_priv {
+	struct cros_ec_device *cros_ec;
+};
+
+static int cros_fwk_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_fwk_priv *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->cros_ec = cros_ec;
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static const struct platform_device_id cros_fwk_id[] = {
+	{ DRV_NAME, 0 },
+	{ }
+};
+
+static struct platform_driver cros_fwk_driver = {
+	.driver.name	= DRV_NAME,
+	.probe		= cros_fwk_probe,
+	.id_table	= cros_fwk_id,
+};
+
+module_platform_driver(cros_fwk_driver);
+
+MODULE_DEVICE_TABLE(platform, cros_fwk_id);
+MODULE_DESCRIPTION("ChromeOS EC Framework Laptop extensions");
+MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net");
+MODULE_LICENSE("GPL");

-- 
2.45.0

[PATCH 2/2] platform/chrome: cros_ec_framework_laptop: implement battery charge thresholds

[Thomas Weißschuh]

Make use of the non-standard EC command FW_EC_CMD_CHARGE_LIMIT_CONTROL
to implement the standard sysfs attribute
"charge_control_end_threshold".

Tested on a Framework 13 AMD with firmware version 3.05.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/chrome/cros_ec_framework_laptop.c | 120 +++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_framework_laptop.c b/drivers/platform/chrome/cros_ec_framework_laptop.c
index 8a8bf039fa9c..2693a80df38f 100644
--- a/drivers/platform/chrome/cros_ec_framework_laptop.c
+++ b/drivers/platform/chrome/cros_ec_framework_laptop.c
@@ -4,18 +4,119 @@
  *
  *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
  */
+#include <acpi/battery.h>
 #include <linux/kernel.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>
 
 #define DRV_NAME	"cros-ec-framework"
 
+#define FW_EC_CMD_CHARGE_LIMIT_CONTROL	0x3E03
+
+#define FW_EC_CHG_LIMIT_DISABLE		BIT(0)
+#define FW_EC_CHG_LIMIT_SET		BIT(1)
+#define FW_EC_CHG_LIMIT_GET		BIT(3)
+#define FW_EC_CHG_LIMIT_OVERRIDE	BIT(7)
+
 struct cros_fwk_priv {
 	struct cros_ec_device *cros_ec;
+	struct acpi_battery_hook battery_hook;
+	struct device_attribute end_threshold;
+};
+
+union cros_fwk_data {
+	struct {
+		u8 modes;
+		u8 max_percentage;
+		u8 min_percentage; /* not implemented in the EC */
+	} __packed req;
+	struct {
+		u8 max_percentage;
+		u8 min_percentage;
+	} __packed resp;
 };
 
+static int cros_fwk_send_cmd(struct cros_ec_device *cros_ec, union cros_fwk_data *arg)
+{
+	int ret;
+	struct {
+		struct cros_ec_command msg;
+		union cros_fwk_data data;
+	} __packed buf = {
+		.msg = {
+			.version = 0,
+			.command = FW_EC_CMD_CHARGE_LIMIT_CONTROL,
+			.insize  = sizeof(arg->resp),
+			.outsize = sizeof(arg->req),
+		},
+		.data.req = arg->req
+	};
+
+	ret = cros_ec_cmd_xfer_status(cros_ec, &buf.msg);
+	if (ret < 0)
+		return ret;
+
+	arg->resp = buf.data.resp;
+
+	return 0;
+}
+
+static ssize_t charge_control_end_threshold_show(struct device *dev, struct device_attribute *attr,
+						 char *buf)
+{
+	struct cros_fwk_priv *priv = container_of(attr, struct cros_fwk_priv, end_threshold);
+	union cros_fwk_data arg = { };
+	int ret;
+
+	arg.req.modes = FW_EC_CHG_LIMIT_GET;
+	ret = cros_fwk_send_cmd(priv->cros_ec, &arg);
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", (unsigned int)arg.resp.max_percentage);
+}
+
+static ssize_t charge_control_end_threshold_store(struct device *dev, struct device_attribute *attr,
+						  const char *buf, size_t count)
+{
+	struct cros_fwk_priv *priv = container_of(attr, struct cros_fwk_priv, end_threshold);
+	union cros_fwk_data arg = { };
+	int ret, val;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+	if (val < 1 || val > 100)
+		return -EINVAL;
+
+	arg.req.modes = FW_EC_CHG_LIMIT_SET;
+	arg.req.max_percentage = val;
+	ret = cros_fwk_send_cmd(priv->cros_ec, &arg);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static int cros_fwk_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	struct cros_fwk_priv *priv = container_of(hook, struct cros_fwk_priv, battery_hook);
+
+	return device_create_file(&battery->dev, &priv->end_threshold);
+}
+
+static int cros_fwk_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	struct cros_fwk_priv *priv = container_of(hook, struct cros_fwk_priv, battery_hook);
+
+	device_remove_file(&battery->dev, &priv->end_threshold);
+
+	return 0;
+}
+
 static int cros_fwk_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -28,6 +129,24 @@ static int cros_fwk_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->cros_ec = cros_ec;
+	priv->end_threshold = (struct device_attribute)__ATTR_RW(charge_control_end_threshold);
+
+	priv->battery_hook.name = dev_name(dev),
+	priv->battery_hook.add_battery = cros_fwk_add_battery,
+	priv->battery_hook.remove_battery = cros_fwk_remove_battery,
+
+	battery_hook_register(&priv->battery_hook);
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int cros_fwk_remove(struct platform_device *pdev)
+{
+	struct cros_fwk_priv *priv = platform_get_drvdata(pdev);
+
+	battery_hook_unregister(&priv->battery_hook);
 
 	platform_set_drvdata(pdev, priv);
 
@@ -42,6 +161,7 @@ static const struct platform_device_id cros_fwk_id[] = {
 static struct platform_driver cros_fwk_driver = {
 	.driver.name	= DRV_NAME,
 	.probe		= cros_fwk_probe,
+	.remove		= cros_fwk_remove,
 	.id_table	= cros_fwk_id,
 };
 

-- 
2.45.0

Re: [PATCH 0/2] platform/chrome: cros_ec_framework_laptop: new driver

[Thomas Weißschuh]

On 2024-05-05 22:56:33+0000, Thomas Weißschuh wrote:
> Framework Laptops are using embedded controller firmware based on the
> ChromeOS EC project.
> In addition to the standard upstream commands some vendor-specific
> commands are implemented.
> 
> Add a driver that implements battery charge thresholds using these
> custom commands.

It turns out that standard ChromesOS EC defines EC_CMD_CHARGE_CONTROL.
The kernel headers however only define v1 of the protocol, which is very
limited.

But in the upstream firmware repo there is a v3 which is much better.

The Framework laptop only implements v2 which is also fine.
Given that v3 was only introduced late last year, it seems better to
stick to v2 anyways for now.

So please disregard Patch 2, I'll see on how to use this via a normal
cros_ec driver.

There are some other Framework-only features that will use Patch 1,
so feedback for that would still be good.

> Patch 1 adds the general scaffolding and device binding.
> Patch 2 implements the battery charge thresholds.
> 
> This series is based on
> https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Thomas Weißschuh (2):
>       platform/chrome: cros_ec_framework_laptop: introduce driver
>       platform/chrome: cros_ec_framework_laptop: implement battery charge thresholds
> 
>  MAINTAINERS                                        |   5 +
>  drivers/mfd/cros_ec_dev.c                          |  13 ++
>  drivers/platform/chrome/Kconfig                    |  11 ++
>  drivers/platform/chrome/Makefile                   |   1 +
>  drivers/platform/chrome/cros_ec_framework_laptop.c | 173 +++++++++++++++++++++
>  5 files changed, 203 insertions(+)
> ---
> base-commit: 2fbe479c0024e1c6b992184a799055e19932aa48
> change-id: 20240505-cros_ec-framework-10e627c46a0a
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@weissschuh.net>
> 

Re: [PATCH 0/2] platform/chrome: cros_ec_framework_laptop: new driver

[Limonciello, Mario]

On 5/6/2024 1:09 AM, Thomas Weißschuh wrote:
> On 2024-05-05 22:56:33+0000, Thomas Weißschuh wrote:
>> Framework Laptops are using embedded controller firmware based on the
>> ChromeOS EC project.
>> In addition to the standard upstream commands some vendor-specific
>> commands are implemented.
>>
>> Add a driver that implements battery charge thresholds using these
>> custom commands.
> 
> It turns out that standard ChromesOS EC defines EC_CMD_CHARGE_CONTROL.
> The kernel headers however only define v1 of the protocol, which is very
> limited.
> 
> But in the upstream firmware repo there is a v3 which is much better.
> 
> The Framework laptop only implements v2 which is also fine.
> Given that v3 was only introduced late last year, it seems better to
> stick to v2 anyways for now.
> 
> So please disregard Patch 2, I'll see on how to use this via a normal
> cros_ec driver.
> 
> There are some other Framework-only features that will use Patch 1,
> so feedback for that would still be good.

What other kinds of features do you have in mind?

Considering your above finding I think it's better to put patch 1 into 
the series of "other framework only features" that will use it so it's 
clearer if it's the best way or not.

> 
>> Patch 1 adds the general scaffolding and device binding.
>> Patch 2 implements the battery charge thresholds.
>>
>> This series is based on
>> https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
>>
>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>> ---
>> Thomas Weißschuh (2):
>>        platform/chrome: cros_ec_framework_laptop: introduce driver
>>        platform/chrome: cros_ec_framework_laptop: implement battery charge thresholds
>>
>>   MAINTAINERS                                        |   5 +
>>   drivers/mfd/cros_ec_dev.c                          |  13 ++
>>   drivers/platform/chrome/Kconfig                    |  11 ++
>>   drivers/platform/chrome/Makefile                   |   1 +
>>   drivers/platform/chrome/cros_ec_framework_laptop.c | 173 +++++++++++++++++++++
>>   5 files changed, 203 insertions(+)
>> ---
>> base-commit: 2fbe479c0024e1c6b992184a799055e19932aa48
>> change-id: 20240505-cros_ec-framework-10e627c46a0a
>>
>> Best regards,
>> -- 
>> Thomas Weißschuh <linux@weissschuh.net>
>>

Re: [PATCH 1/2] platform/chrome: cros_ec_framework_laptop: introduce driver

[Limonciello, Mario]

On 5/5/2024 3:56 PM, Thomas Weißschuh wrote:
> Framework Laptops are using embedded controller firmware based on the
> ChromeOS EC project.
> In addition to the standard upstream commands, some vendor-specific
> ones are implemented.
> 
> Add a driver for those custom EC commands.
> 
> At first, provide an empty driver that only takes care of scaffolding and
> device binding.
> Further patches will add functionality to the driver.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   MAINTAINERS                                        |  5 ++
>   drivers/mfd/cros_ec_dev.c                          | 13 ++++++
>   drivers/platform/chrome/Kconfig                    | 11 +++++
>   drivers/platform/chrome/Makefile                   |  1 +
>   drivers/platform/chrome/cros_ec_framework_laptop.c | 53 ++++++++++++++++++++++
>   5 files changed, 83 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c23fda1aa1f0..60699c289757 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4988,6 +4988,11 @@ S:	Maintained
>   F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
>   F:	sound/soc/codecs/cros_ec_codec.*
>   
> +CHROMEOS EC FRAMEWORK LAPTOP EXTENSIONS
> +M:	Thomas Weißschuh <linux@weissschuh.net>
> +S:	Maintained
> +F:	drivers/platform/chrome/cros_ec_framework_laptop.c
> +
>   CHROMEOS EC SUBDRIVERS
>   M:	Benson Leung <bleung@chromium.org>
>   R:	Guenter Roeck <groeck@chromium.org>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index a52d59cc2b1e..0a36e77e5039 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -145,6 +145,10 @@ static const struct mfd_cell cros_ec_vbc_cells[] = {
>   	{ .name = "cros-ec-vbc", }
>   };
>   
> +static const struct mfd_cell cros_ec_framework_cells[] = {
> +	{ .name = "cros-ec-framework", }
> +};
> +
>   static void cros_ec_class_release(struct device *dev)
>   {
>   	kfree(to_cros_ec_dev(dev));
> @@ -299,6 +303,15 @@ static int ec_device_probe(struct platform_device *pdev)
>   				 retval);
>   	}
>   
> +	 /* The EC on Framework laptops implements some nonstandard features */

I don't think there is a spec really for cros_ec is there?  I think it 
will depend upon what features you're talking about if this is the right 
way to go.

The reason I say this is that maybe some of the same kinds of features 
will make sense for chromebooks that use cros_ec in the future and thus 
they should be "generic" cros_ec mfd cells to probe for in some way.

> +	if (dmi_match(DMI_SYS_VENDOR, "Framework")) {
> +		retval = mfd_add_hotplug_devices(ec->dev, cros_ec_framework_cells,
> +						 ARRAY_SIZE(cros_ec_framework_cells));
> +		if (retval)
> +			dev_warn(ec->dev, "failed to add framework laptop devices: %d\n",
> +				 retval);
> +	}
> +
>   	return 0;
>   
>   failed:
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 073616b5b5a0..ff69ee226606 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -239,6 +239,17 @@ config CROS_EC_TYPEC
>   	  To compile this driver as a module, choose M here: the module will be
>   	  called cros-ec-typec.
>   
> +config CROS_EC_FRAMEWORK_LAPTOP
> +	tristate "ChromeOS EC Framework Laptop extensions"
> +	depends on MFD_CROS_EC_DEV
> +	default MFD_CROS_EC_DEV
> +	help
> +	  If you say Y here, you get support for using Framework Laptop-specific extensions
> +	  of the Chrome OS EC.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called cros_ec_framework_laptop.
> +
>   config CROS_HPS_I2C
>   	tristate "ChromeOS HPS device"
>   	depends on HID && I2C && PM
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 2dcc6ccc2302..ce6aac620086 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_CHROMEOS_PRIVACY_SCREEN)	+= chromeos_privacy_screen.o
>   obj-$(CONFIG_CHROMEOS_PSTORE)		+= chromeos_pstore.o
>   obj-$(CONFIG_CHROMEOS_TBMC)		+= chromeos_tbmc.o
>   obj-$(CONFIG_CROS_EC)			+= cros_ec.o
> +obj-$(CONFIG_CROS_EC_FRAMEWORK_LAPTOP)	+= cros_ec_framework_laptop.o
>   obj-$(CONFIG_CROS_EC_I2C)		+= cros_ec_i2c.o
>   obj-$(CONFIG_CROS_EC_ISHTP)		+= cros_ec_ishtp.o
>   obj-$(CONFIG_CROS_TYPEC_SWITCH)		+= cros_typec_switch.o
> diff --git a/drivers/platform/chrome/cros_ec_framework_laptop.c b/drivers/platform/chrome/cros_ec_framework_laptop.c
> new file mode 100644
> index 000000000000..8a8bf039fa9c
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_framework_laptop.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  ChromesOS EC driver for Framework laptop
> + *
> + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> + */
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME	"cros-ec-framework"
> +
> +struct cros_fwk_priv {
> +	struct cros_ec_device *cros_ec;
> +};
> +
> +static int cros_fwk_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_fwk_priv *priv;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->cros_ec = cros_ec;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id cros_fwk_id[] = {
> +	{ DRV_NAME, 0 },
> +	{ }
> +};
> +
> +static struct platform_driver cros_fwk_driver = {
> +	.driver.name	= DRV_NAME,
> +	.probe		= cros_fwk_probe,
> +	.id_table	= cros_fwk_id,
> +};
> +
> +module_platform_driver(cros_fwk_driver);
> +
> +MODULE_DEVICE_TABLE(platform, cros_fwk_id);
> +MODULE_DESCRIPTION("ChromeOS EC Framework Laptop extensions");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 0/2] platform/chrome: cros_ec_framework_laptop: new driver

[Thomas Weißschuh]

On 2024-05-06 08:09:07+0000, Limonciello, Mario wrote:
> 
> 
> On 5/6/2024 1:09 AM, Thomas Weißschuh wrote:
> > On 2024-05-05 22:56:33+0000, Thomas Weißschuh wrote:
> > > Framework Laptops are using embedded controller firmware based on the
> > > ChromeOS EC project.
> > > In addition to the standard upstream commands some vendor-specific
> > > commands are implemented.
> > > 
> > > Add a driver that implements battery charge thresholds using these
> > > custom commands.
> > 
> > It turns out that standard ChromesOS EC defines EC_CMD_CHARGE_CONTROL.
> > The kernel headers however only define v1 of the protocol, which is very
> > limited.
> > 
> > But in the upstream firmware repo there is a v3 which is much better.
> > 
> > The Framework laptop only implements v2 which is also fine.
> > Given that v3 was only introduced late last year, it seems better to
> > stick to v2 anyways for now.
> > 
> > So please disregard Patch 2, I'll see on how to use this via a normal
> > cros_ec driver.
> > 
> > There are some other Framework-only features that will use Patch 1,
> > so feedback for that would still be good.
> 
> What other kinds of features do you have in mind?

There are at least fan and privacy switch reporting in which im interested.
But fan reporting also has a standard command which I'll try to use first.

And then there are many others I'm personally not interested in:
Fingerprint LED configuration, Intrusion Detection, "Standalone mode",
some diagnostics, etc.

Especially as they can also be handled from userspace.

> Considering your above finding I think it's better to put patch 1 into the
> series of "other framework only features" that will use it so it's clearer
> if it's the best way or not.

Agreed, that is what I tried to express.

> [..]

Thomas

Re: [PATCH 1/2] platform/chrome: cros_ec_framework_laptop: introduce driver

[Thomas Weißschuh]

On 2024-05-06 08:10:58+0000, Limonciello, Mario wrote:
> 
> 
> On 5/5/2024 3:56 PM, Thomas Weißschuh wrote:
> > Framework Laptops are using embedded controller firmware based on the
> > ChromeOS EC project.
> > In addition to the standard upstream commands, some vendor-specific
> > ones are implemented.
> > 
> > Add a driver for those custom EC commands.
> > 
> > At first, provide an empty driver that only takes care of scaffolding and
> > device binding.
> > Further patches will add functionality to the driver.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >   MAINTAINERS                                        |  5 ++
> >   drivers/mfd/cros_ec_dev.c                          | 13 ++++++
> >   drivers/platform/chrome/Kconfig                    | 11 +++++
> >   drivers/platform/chrome/Makefile                   |  1 +
> >   drivers/platform/chrome/cros_ec_framework_laptop.c | 53 ++++++++++++++++++++++
> >   5 files changed, 83 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c23fda1aa1f0..60699c289757 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4988,6 +4988,11 @@ S:	Maintained
> >   F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
> >   F:	sound/soc/codecs/cros_ec_codec.*
> > +CHROMEOS EC FRAMEWORK LAPTOP EXTENSIONS
> > +M:	Thomas Weißschuh <linux@weissschuh.net>
> > +S:	Maintained
> > +F:	drivers/platform/chrome/cros_ec_framework_laptop.c
> > +
> >   CHROMEOS EC SUBDRIVERS
> >   M:	Benson Leung <bleung@chromium.org>
> >   R:	Guenter Roeck <groeck@chromium.org>
> > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > index a52d59cc2b1e..0a36e77e5039 100644
> > --- a/drivers/mfd/cros_ec_dev.c
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -145,6 +145,10 @@ static const struct mfd_cell cros_ec_vbc_cells[] = {
> >   	{ .name = "cros-ec-vbc", }
> >   };
> > +static const struct mfd_cell cros_ec_framework_cells[] = {
> > +	{ .name = "cros-ec-framework", }
> > +};
> > +
> >   static void cros_ec_class_release(struct device *dev)
> >   {
> >   	kfree(to_cros_ec_dev(dev));
> > @@ -299,6 +303,15 @@ static int ec_device_probe(struct platform_device *pdev)
> >   				 retval);
> >   	}
> > +	 /* The EC on Framework laptops implements some nonstandard features */
> 
> I don't think there is a spec really for cros_ec is there?  I think it will
> depend upon what features you're talking about if this is the right way to
> go.

I equate "standard" with "mainline".

> The reason I say this is that maybe some of the same kinds of features will
> make sense for chromebooks that use cros_ec in the future and thus they
> should be "generic" cros_ec mfd cells to probe for in some way.

The commands identifiers are just numbers.
So if multiple vendors use the same numbers there could be collisions.
By loading the subdriver only for the correct vendors devices we avoid those.

If other vendors or mainline CrOS EC implements the same commands we can
revisit this and move the functionality to a generic mfd cell.

But after the discussions on the cover letter, let's first see if a
vendor-specific driver is needed at all.

Thomas

Re: [PATCH 0/2] platform/chrome: cros_ec_framework_laptop: new driver

[Dustin Howett]

On Mon, May 6, 2024 at 12:43 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> On 2024-05-06 08:09:07+0000, Limonciello, Mario wrote:
> >
> >
> > On 5/6/2024 1:09 AM, Thomas Weißschuh wrote:
> > > On 2024-05-05 22:56:33+0000, Thomas Weißschuh wrote:
> > > > Framework Laptops are using embedded controller firmware based on the
> > > > ChromeOS EC project.
> > > > In addition to the standard upstream commands some vendor-specific
> > > > commands are implemented.
> > > >
> > > > Add a driver that implements battery charge thresholds using these
> > > > custom commands.
> > >
> > > It turns out that standard ChromesOS EC defines EC_CMD_CHARGE_CONTROL.
> > > The kernel headers however only define v1 of the protocol, which is very
> > > limited.
> > >
> > > But in the upstream firmware repo there is a v3 which is much better.
> > >
> > > The Framework laptop only implements v2 which is also fine.
> > > Given that v3 was only introduced late last year, it seems better to
> > > stick to v2 anyways for now.
> > >
> > > So please disregard Patch 2, I'll see on how to use this via a normal
> > > cros_ec driver.
> > >
> > > There are some other Framework-only features that will use Patch 1,
> > > so feedback for that would still be good.
> >
> > What other kinds of features do you have in mind?
>

Definitely privacy switch reporting belongs in a driver like this.

Overall, I'm not sure about making it a subjugate driver under the
cros_ec_mfd virtual "bus"... even though a lot of the features take a
dependency on cros_ec.
Doing so centralizes the work in the platform-chrome tree and may
serve as a guidepost for any future laptop OEMs that derive their
embedded controller firmware from ChromeOS's.
If the owners of this tree sign off on that, that's awesome! I'd be
concerned about making it all their responsibility.

I may be a bit biased, as I have been working on a driver of my own[1]
for this purpose. It currently supports battery charge limiting[3],
reporting fan speed via hwmon, the keyboard backlight[2], and has an
open pull request that exposes the status of the privacy switches.

It is destined--once I find the time to clean it up--for
drivers\platforms\x86 instead of ...\chrome.

This may be a good place for us to combine our efforts!
d

[1] https://github.com/DHowett/framework-laptop-kmod
[2] I found that the Azalea did not report its keyboard backlight
values through the standard cros ec KBLIGHT interface like hx20/30
did, so the driver as it stands implements a fallback that uses the
raw PWM state. I'm sure that you'd've noticed this if it was still
true... so I am always happy to drop an unnecessary workaround. :)
[3] Which I believe still requires a special host command and is not
integrated into the charge manager, at least as of Azalea/Lotus and
_definitely_ not as of hx20/30!

> [...]
>
> > [..]
>
> Thomas

Re: [PATCH 0/2] platform/chrome: cros_ec_framework_laptop: new driver

[Thomas Weißschuh]

Hi Dustin,

On 2024-05-06 13:29:32+0000, Dustin Howett wrote:
> On Mon, May 6, 2024 at 12:43 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > On 2024-05-06 08:09:07+0000, Limonciello, Mario wrote:
> > > On 5/6/2024 1:09 AM, Thomas Weißschuh wrote:
> > > > On 2024-05-05 22:56:33+0000, Thomas Weißschuh wrote:
> > > > > Framework Laptops are using embedded controller firmware based on the
> > > > > ChromeOS EC project.
> > > > > In addition to the standard upstream commands some vendor-specific
> > > > > commands are implemented.
> > > > >
> > > > > Add a driver that implements battery charge thresholds using these
> > > > > custom commands.
> > > >
> > > > It turns out that standard ChromesOS EC defines EC_CMD_CHARGE_CONTROL.
> > > > The kernel headers however only define v1 of the protocol, which is very
> > > > limited.
> > > >
> > > > But in the upstream firmware repo there is a v3 which is much better.
> > > >
> > > > The Framework laptop only implements v2 which is also fine.
> > > > Given that v3 was only introduced late last year, it seems better to
> > > > stick to v2 anyways for now.
> > > >
> > > > So please disregard Patch 2, I'll see on how to use this via a normal
> > > > cros_ec driver.
> > > >
> > > > There are some other Framework-only features that will use Patch 1,
> > > > so feedback for that would still be good.
> > >
> > > What other kinds of features do you have in mind?
> >
> 
> Definitely privacy switch reporting belongs in a driver like this.

If it can't be done via one of the upstream CrOS EC commands, surely.

> Overall, I'm not sure about making it a subjugate driver under the
> cros_ec_mfd virtual "bus"... even though a lot of the features take a
> dependency on cros_ec.
> Doing so centralizes the work in the platform-chrome tree and may
> serve as a guidepost for any future laptop OEMs that derive their
> embedded controller firmware from ChromeOS's.
> If the owners of this tree sign off on that, that's awesome! I'd be
> concerned about making it all their responsibility.

Yes, some guidance from the maintainers will be great.

> I may be a bit biased, as I have been working on a driver of my own[1]
> for this purpose. It currently supports battery charge limiting[3],
> reporting fan speed via hwmon, the keyboard backlight[2], and has an
> open pull request that exposes the status of the privacy switches.

I have taken a look at that driver but wasn't fond of the fact that it
is not using cros_ec mfd. Taking a reference on a completely different
device looks iffy to me and in violation of the device hierarchy.

FYI I have completely non-Framework-specific implementations for
keyboard backlight [0], charge limiting [1] and hwmon [2].
(I didn't look at the privacy switches yet, maybe there is a generic
solution)
(I'm currently polishing [1] and [2], any feedback already would also be
much appreciated)

All of them work correctly on my Framework 13 AMD, Firmware 3.05.
These standard APIs are more powerful than the Framework-only ones.

Charge control can do start_threshold, stop_threshold and
charge_behaviour. Hwmon can do fans and temperature sensors.

Keyboard backlight just reuses the existing mainline driver.

> It is destined--once I find the time to clean it up--for
> drivers\platforms\x86 instead of ...\chrome.
> 
> This may be a good place for us to combine our efforts!

Surely!

Personally I only have the AMD 13 device (Azalea),
so I can't test anything else.
And I'd like to focus on the mainline-compatible APIs (first).

Feel free to contact me (privately?) if you have any suggestions.

> [1] https://github.com/DHowett/framework-laptop-kmod
> [2] I found that the Azalea did not report its keyboard backlight
> values through the standard cros ec KBLIGHT interface like hx20/30
> did, so the driver as it stands implements a fallback that uses the
> raw PWM state. I'm sure that you'd've noticed this if it was still
> true... so I am always happy to drop an unnecessary workaround. :)

For me the posted driver under [0] works as expected.

> [3] Which I believe still requires a special host command and is not
> integrated into the charge manager, at least as of Azalea/Lotus and
> _definitely_ not as of hx20/30!

This also works for me correctly with [1].
Do you know if there are plans by Framework to move the older devices to
a newer firmware?
This would also make their own maintenance work easier in the future,
especially considering their commitment to software longevity[3].

> [..]

Thomas

[0] https://lore.kernel.org/lkml/20240505-cros_ec-kbd-led-framework-v1-1-bfcca69013d2@weissschuh.net/
[1] https://git.sr.ht/~t-8ch/linux/tree/b4/cros_ec-charge-control
[2] https://git.sr.ht/~t-8ch/linux/tree/b4/cros_ec-hwmon
[3] https://frame.work/de/en/blog/enabling-software-longevity

Re: [PATCH 1/2] platform/chrome: cros_ec_framework_laptop: introduce driver

[Lee Jones]

On Sun, 05 May 2024, Thomas Weißschuh wrote:

> Framework Laptops are using embedded controller firmware based on the
> ChromeOS EC project.
> In addition to the standard upstream commands, some vendor-specific
> ones are implemented.
> 
> Add a driver for those custom EC commands.
> 
> At first, provide an empty driver that only takes care of scaffolding and
> device binding.
> Further patches will add functionality to the driver.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  MAINTAINERS                                        |  5 ++
>  drivers/mfd/cros_ec_dev.c                          | 13 ++++++

I do not see any build-time deps here.

Please split this change out.

>  drivers/platform/chrome/Kconfig                    | 11 +++++
>  drivers/platform/chrome/Makefile                   |  1 +
>  drivers/platform/chrome/cros_ec_framework_laptop.c | 53 ++++++++++++++++++++++
>  5 files changed, 83 insertions(+)

-- 
Lee Jones [李琼斯]