[PATCH v3 0/5] ChromeOS Embedded Controller LED driver

[PATCH v3 0/5] ChromeOS Embedded Controller LED driver

[Thomas Weißschuh]

Add a LED driver that supports the LED devices exposed by the
ChromeOS Embedded Controller.

Patch 1-3 add a utility function to the led subsystem.
Patch 4 introduces the actual driver.
Patch 5 registers the driver through the cros_ec mfd devices.

Currently the driver introduces some non-standard LED functions.
(See "cros_ec_led_functions")

Tested on a Framework 13 AMD, Firmware 3.05.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v3:
- Set default_trigger explicitly as the LED core doesn't do this anymore
- Only set intensity for first subled by default
- Link to v2: https://lore.kernel.org/r/20240531-cros_ec-led-v2-0-6cc34408b40d@weissschuh.net

Changes in v2:
- Cosmetic cleanups (Tzung-Bi)
- Add trailing comma to MFD cell array
- Rename LEDs and trigger to "chromeos" prefix, to align with kbd
  backlight driver
- Don't use type "rgb" anymore, they are only "multicolor"
- Align commit messages and subject to subsystem standards (Lee)
- Rename led_color_name() to led_get_color_name()
- The same for cros_ec_led_color_name()
- Link to v1: https://lore.kernel.org/r/20240520-cros_ec-led-v1-0-4068fc5c051a@weissschuh.net

---
Thomas Weißschuh (5):
      leds: core: Introduce led_get_color_name() function
      leds: multicolor: Use led_get_color_name() function
      leds: core: Unexport led_colors[] array
      leds: Add ChromeOS EC driver
      mfd: cros_ec: Register LED subdevice

 MAINTAINERS                         |   5 +
 drivers/leds/Kconfig                |  15 ++
 drivers/leds/Makefile               |   1 +
 drivers/leds/led-class-multicolor.c |   2 +-
 drivers/leds/led-core.c             |  12 +-
 drivers/leds/leds-cros_ec.c         | 299 ++++++++++++++++++++++++++++++++++++
 drivers/leds/leds.h                 |   1 -
 drivers/mfd/cros_ec_dev.c           |   9 ++
 include/linux/leds.h                |  10 ++
 9 files changed, 350 insertions(+), 4 deletions(-)
---
base-commit: 2ccbdf43d5e758f8493a95252073cf9078a5fea5
change-id: 20240519-cros_ec-led-3efa24e3991e

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

[PATCH v3 1/5] leds: core: Introduce led_get_color_name() function

[Thomas Weißschuh]

This is similar to the existing led_colors[] array but is safer to use and
usable by everyone.

Getting string representations of color ids is useful for drivers
which are handling color IDs anyways, for example for the multicolor API.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/leds/led-core.c |  9 +++++++++
 include/linux/leds.h    | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 89c9806cc97f..e0dd2284e84a 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -534,6 +534,15 @@ int led_compose_name(struct device *dev, struct led_init_data *init_data,
 }
 EXPORT_SYMBOL_GPL(led_compose_name);
 
+const char *led_get_color_name(u8 color_id)
+{
+	if (color_id >= ARRAY_SIZE(led_colors))
+		return NULL;
+
+	return led_colors[color_id];
+}
+EXPORT_SYMBOL_GPL(led_get_color_name);
+
 enum led_default_state led_init_default_state_get(struct fwnode_handle *fwnode)
 {
 	const char *state = NULL;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6300313c46b7..dedea965afbf 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -427,6 +427,16 @@ void led_sysfs_enable(struct led_classdev *led_cdev);
 int led_compose_name(struct device *dev, struct led_init_data *init_data,
 		     char *led_classdev_name);
 
+/**
+ * led_get_color_name - get string representation of color ID
+ * @color_id: The LED_COLOR_ID_* constant
+ *
+ * Get the string name of a LED_COLOR_ID_* constant.
+ *
+ * Returns: A string constant or NULL on an invalid ID.
+ */
+const char *led_get_color_name(u8 color_id);
+
 /**
  * led_sysfs_is_disabled - check if LED sysfs interface is disabled
  * @led_cdev: the LED to query

-- 
2.45.2

[PATCH v3 2/5] leds: multicolor: Use led_get_color_name() function

[Thomas Weißschuh]

led_get_color_name() is a safer alternative to led_colors.

led-class-multicolor.c is the only external user of led_colors and its
removal allows unexporting the array.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/leds/led-class-multicolor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index ec62a4811613..584e3786a1e7 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -101,7 +101,7 @@ static ssize_t multi_index_show(struct device *dev,
 
 	for (i = 0; i < mcled_cdev->num_colors; i++) {
 		index = mcled_cdev->subled_info[i].color_index;
-		len += sprintf(buf + len, "%s", led_colors[index]);
+		len += sprintf(buf + len, "%s", led_get_color_name(index));
 		if (i < mcled_cdev->num_colors - 1)
 			len += sprintf(buf + len, " ");
 	}

-- 
2.45.2

[PATCH v3 3/5] leds: core: Unexport led_colors[] array

[Thomas Weißschuh]

There are no external users left, make the array static.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/leds/led-core.c | 3 +--
 drivers/leds/leds.h     | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index e0dd2284e84a..f2cea4e094f6 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -25,7 +25,7 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
 LIST_HEAD(leds_list);
 EXPORT_SYMBOL_GPL(leds_list);
 
-const char * const led_colors[LED_COLOR_ID_MAX] = {
+static const char * const led_colors[LED_COLOR_ID_MAX] = {
 	[LED_COLOR_ID_WHITE] = "white",
 	[LED_COLOR_ID_RED] = "red",
 	[LED_COLOR_ID_GREEN] = "green",
@@ -42,7 +42,6 @@ const char * const led_colors[LED_COLOR_ID_MAX] = {
 	[LED_COLOR_ID_CYAN] = "cyan",
 	[LED_COLOR_ID_LIME] = "lime",
 };
-EXPORT_SYMBOL_GPL(led_colors);
 
 static int __led_set_brightness(struct led_classdev *led_cdev, unsigned int value)
 {
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 1138e2ab82e5..d7999e7372a4 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -30,6 +30,5 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
-extern const char * const led_colors[LED_COLOR_ID_MAX];
 
 #endif	/* __LEDS_H_INCLUDED */

-- 
2.45.2

[PATCH v3 4/5] leds: Add ChromeOS EC driver

[Thomas Weißschuh]

The ChromeOS Embedded Controller exposes an LED control command.
Expose its functionality through the leds subsystem.

The LEDs are exposed as multicolor devices.
A hardware trigger, which is active by default, is provided to let the
EC itself take over control over the LED.

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

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 MAINTAINERS                 |   5 +
 drivers/leds/Kconfig        |  15 +++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-cros_ec.c | 299 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 320 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aacccb376c28..8bc3491a08af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5135,6 +5135,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
 F:	sound/soc/codecs/cros_ec_codec.*
 
+CHROMEOS EC LED DRIVER
+M:	Thomas Weißschuh <thomas@weissschuh.net>
+S:	Maintained
+F:	drivers/leds/leds-cros_ec.c
+
 CHROMEOS EC SUBDRIVERS
 M:	Benson Leung <bleung@chromium.org>
 R:	Guenter Roeck <groeck@chromium.org>
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 05e6af88b88c..aa2fec9a34ed 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -179,6 +179,21 @@ config LEDS_CR0014114
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-cr0014114.
 
+config LEDS_CROS_EC
+	tristate "LED Support for ChromeOS EC"
+	depends on MFD_CROS_EC_DEV
+	depends on LEDS_CLASS_MULTICOLOR
+	select LEDS_TRIGGERS
+	default MFD_CROS_EC_DEV
+	help
+	  This option enables support for LEDs managed by ChromeOS ECs.
+	  All LEDs exposed by the EC are supported in multicolor mode.
+	  A hardware trigger to switch back to the automatic behaviour is
+	  provided.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-cros_ec.
+
 config LEDS_EL15203000
 	tristate "LED Support for Crane EL15203000"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index effdfc6f1e95..3491904e13f7 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
 obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
 obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
 obj-$(CONFIG_LEDS_CPCAP)		+= leds-cpcap.o
+obj-$(CONFIG_LEDS_CROS_EC)		+= leds-cros_ec.o
 obj-$(CONFIG_LEDS_DA903X)		+= leds-da903x.o
 obj-$(CONFIG_LEDS_DA9052)		+= leds-da9052.o
 obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c
new file mode 100644
index 000000000000..7bb21a587713
--- /dev/null
+++ b/drivers/leds/leds-cros_ec.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  ChromeOS EC LED Driver
+ *
+ *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
+ */
+
+#include <linux/device.h>
+#include <linux/leds.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+
+#define DRV_NAME	"cros-ec-led"
+
+static const char * const cros_ec_led_functions[] = {
+	[EC_LED_ID_BATTERY_LED]            = LED_FUNCTION_CHARGING,
+	[EC_LED_ID_POWER_LED]              = LED_FUNCTION_POWER,
+	[EC_LED_ID_ADAPTER_LED]            = "adapter",
+	[EC_LED_ID_LEFT_LED]               = "left",
+	[EC_LED_ID_RIGHT_LED]              = "right",
+	[EC_LED_ID_RECOVERY_HW_REINIT_LED] = "recovery-hw-reinit",
+	[EC_LED_ID_SYSRQ_DEBUG_LED]        = "sysrq-debug",
+};
+
+static_assert(ARRAY_SIZE(cros_ec_led_functions) == EC_LED_ID_COUNT);
+
+static const int cros_ec_led_to_linux_id[] = {
+	[EC_LED_COLOR_RED]    = LED_COLOR_ID_RED,
+	[EC_LED_COLOR_GREEN]  = LED_COLOR_ID_GREEN,
+	[EC_LED_COLOR_BLUE]   = LED_COLOR_ID_BLUE,
+	[EC_LED_COLOR_YELLOW] = LED_COLOR_ID_YELLOW,
+	[EC_LED_COLOR_WHITE]  = LED_COLOR_ID_WHITE,
+	[EC_LED_COLOR_AMBER]  = LED_COLOR_ID_AMBER,
+};
+
+static_assert(ARRAY_SIZE(cros_ec_led_to_linux_id) == EC_LED_COLOR_COUNT);
+
+static const int cros_ec_linux_to_ec_id[] = {
+	[LED_COLOR_ID_RED]    = EC_LED_COLOR_RED,
+	[LED_COLOR_ID_GREEN]  = EC_LED_COLOR_GREEN,
+	[LED_COLOR_ID_BLUE]   = EC_LED_COLOR_BLUE,
+	[LED_COLOR_ID_YELLOW] = EC_LED_COLOR_YELLOW,
+	[LED_COLOR_ID_WHITE]  = EC_LED_COLOR_WHITE,
+	[LED_COLOR_ID_AMBER]  = EC_LED_COLOR_AMBER,
+};
+
+struct cros_ec_led_priv {
+	struct led_classdev_mc led_mc_cdev;
+	struct cros_ec_device *cros_ec;
+	enum ec_led_id led_id;
+};
+
+static inline struct cros_ec_led_priv *cros_ec_led_cdev_to_priv(struct led_classdev *led_cdev)
+{
+	return container_of(lcdev_to_mccdev(led_cdev), struct cros_ec_led_priv, led_mc_cdev);
+}
+
+union cros_ec_led_cmd_data {
+	struct ec_params_led_control req;
+	struct ec_response_led_control resp;
+} __packed;
+
+static int cros_ec_led_send_cmd(struct cros_ec_device *cros_ec,
+				union cros_ec_led_cmd_data *arg)
+{
+	int ret;
+	struct {
+		struct cros_ec_command msg;
+		union cros_ec_led_cmd_data data;
+	} __packed buf = {
+		.msg = {
+			.version = 1,
+			.command = EC_CMD_LED_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 int cros_ec_led_trigger_activate(struct led_classdev *led_cdev)
+{
+	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
+	union cros_ec_led_cmd_data arg = {};
+
+	arg.req.led_id = priv->led_id;
+	arg.req.flags = EC_LED_FLAGS_AUTO;
+
+	return cros_ec_led_send_cmd(priv->cros_ec, &arg);
+}
+
+static struct led_hw_trigger_type cros_ec_led_trigger_type;
+
+static struct led_trigger cros_ec_led_trigger = {
+	.name = "chromeos-auto",
+	.trigger_type = &cros_ec_led_trigger_type,
+	.activate = cros_ec_led_trigger_activate,
+};
+
+static int cros_ec_led_brightness_set_blocking(struct led_classdev *led_cdev,
+					       enum led_brightness brightness)
+{
+	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
+	union cros_ec_led_cmd_data arg = {};
+	enum ec_led_colors led_color;
+	struct mc_subled *subled;
+	size_t i;
+
+	led_mc_calc_color_components(&priv->led_mc_cdev, brightness);
+
+	arg.req.led_id = priv->led_id;
+
+	for (i = 0; i < priv->led_mc_cdev.num_colors; i++) {
+		subled = &priv->led_mc_cdev.subled_info[i];
+		led_color = cros_ec_linux_to_ec_id[subled->color_index];
+		arg.req.brightness[led_color] = subled->brightness;
+	}
+
+	return cros_ec_led_send_cmd(priv->cros_ec, &arg);
+}
+
+static int cros_ec_led_count_subleds(struct device *dev,
+				     struct ec_response_led_control *resp,
+				     unsigned int *max_brightness)
+{
+	unsigned int range, common_range = 0;
+	int num_subleds = 0;
+	size_t i;
+
+	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
+		range = resp->brightness_range[i];
+
+		if (!range)
+			continue;
+
+		num_subleds++;
+
+		if (!common_range)
+			common_range = range;
+
+		if (common_range != range) {
+			/* The multicolor LED API expects a uniform max_brightness */
+			dev_warn(dev, "Inconsistent LED brightness values\n");
+			return -EINVAL;
+		}
+	}
+
+	if (!num_subleds)
+		return -EINVAL;
+
+	*max_brightness = common_range;
+	return num_subleds;
+}
+
+static const char *cros_ec_led_get_color_name(struct led_classdev_mc *led_mc_cdev)
+{
+	int color;
+
+	if (led_mc_cdev->num_colors == 1)
+		color = led_mc_cdev->subled_info[0].color_index;
+	else
+		color = LED_COLOR_ID_MULTI;
+
+	return led_get_color_name(color);
+}
+
+static int cros_ec_led_probe_led(struct device *dev, struct cros_ec_device *cros_ec,
+				 enum ec_led_id id)
+{
+	union cros_ec_led_cmd_data arg = {};
+	struct cros_ec_led_priv *priv;
+	struct led_classdev *led_cdev;
+	struct mc_subled *subleds;
+	int ret, num_subleds;
+	size_t i, subled;
+
+	arg.req.led_id = id;
+	arg.req.flags = EC_LED_FLAGS_QUERY;
+	ret = cros_ec_led_send_cmd(cros_ec, &arg);
+	/* Unknown LED, skip */
+	if (ret == -EINVAL)
+		return 0;
+	if (ret == -EOPNOTSUPP)
+		return -ENODEV;
+	if (ret < 0)
+		return ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	num_subleds = cros_ec_led_count_subleds(dev, &arg.resp,
+						&priv->led_mc_cdev.led_cdev.max_brightness);
+	if (num_subleds < 0)
+		return num_subleds;
+
+	priv->cros_ec = cros_ec;
+	priv->led_id = id;
+
+	subleds = devm_kcalloc(dev, num_subleds, sizeof(*subleds), GFP_KERNEL);
+	if (!subleds)
+		return -ENOMEM;
+
+	subled = 0;
+	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
+		if (!arg.resp.brightness_range[i])
+			continue;
+
+		subleds[subled].color_index = cros_ec_led_to_linux_id[i];
+		if (subled == 0)
+			subleds[subled].intensity = 100;
+		subled++;
+	}
+
+	priv->led_mc_cdev.subled_info = subleds;
+	priv->led_mc_cdev.num_colors = num_subleds;
+
+	led_cdev = &priv->led_mc_cdev.led_cdev;
+	led_cdev->brightness_set_blocking = cros_ec_led_brightness_set_blocking;
+	led_cdev->trigger_type = &cros_ec_led_trigger_type;
+	led_cdev->default_trigger = cros_ec_led_trigger.name;
+	led_cdev->hw_control_trigger = cros_ec_led_trigger.name;
+
+	led_cdev->name = devm_kasprintf(dev, GFP_KERNEL, "chromeos:%s:%s",
+					cros_ec_led_get_color_name(&priv->led_mc_cdev),
+					cros_ec_led_functions[id]);
+	if (!led_cdev->name)
+		return -ENOMEM;
+
+	return devm_led_classdev_multicolor_register(dev, &priv->led_mc_cdev);
+}
+
+static int cros_ec_led_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;
+	int ret = 0;
+	size_t i;
+
+	for (i = 0; i < EC_LED_ID_COUNT; i++) {
+		ret = cros_ec_led_probe_led(dev, cros_ec, i);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static const struct platform_device_id cros_ec_led_id[] = {
+	{ DRV_NAME, 0 },
+	{}
+};
+
+static struct platform_driver cros_ec_led_driver = {
+	.driver.name	= DRV_NAME,
+	.probe		= cros_ec_led_probe,
+	.id_table	= cros_ec_led_id,
+};
+
+static int __init cros_ec_led_init(void)
+{
+	int ret;
+
+	ret = led_trigger_register(&cros_ec_led_trigger);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&cros_ec_led_driver);
+	if (ret)
+		led_trigger_unregister(&cros_ec_led_trigger);
+
+	return ret;
+};
+module_init(cros_ec_led_init);
+
+static void __exit cros_ec_led_exit(void)
+{
+	platform_driver_unregister(&cros_ec_led_driver);
+	led_trigger_unregister(&cros_ec_led_trigger);
+};
+module_exit(cros_ec_led_exit);
+
+MODULE_DEVICE_TABLE(platform, cros_ec_led_id);
+MODULE_DESCRIPTION("ChromeOS EC LED Driver");
+MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net");
+MODULE_LICENSE("GPL");

-- 
2.45.2

Re: [PATCH v3 4/5] leds: Add ChromeOS EC driver

[Lee Jones]

On Thu, 13 Jun 2024, Thomas Weißschuh wrote:

> The ChromeOS Embedded Controller exposes an LED control command.
> Expose its functionality through the leds subsystem.
> 
> The LEDs are exposed as multicolor devices.
> A hardware trigger, which is active by default, is provided to let the
> EC itself take over control over the LED.
> 
> The driver is designed to be probed via the cros_ec mfd device.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  MAINTAINERS                 |   5 +
>  drivers/leds/Kconfig        |  15 +++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-cros_ec.c | 299 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 320 insertions(+)

Mostly fine.  Couple of points.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index aacccb376c28..8bc3491a08af 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5135,6 +5135,11 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
>  F:	sound/soc/codecs/cros_ec_codec.*
>  
> +CHROMEOS EC LED DRIVER
> +M:	Thomas Weißschuh <thomas@weissschuh.net>
> +S:	Maintained
> +F:	drivers/leds/leds-cros_ec.c
> +
>  CHROMEOS EC SUBDRIVERS
>  M:	Benson Leung <bleung@chromium.org>
>  R:	Guenter Roeck <groeck@chromium.org>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 05e6af88b88c..aa2fec9a34ed 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -179,6 +179,21 @@ config LEDS_CR0014114
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-cr0014114.
>  
> +config LEDS_CROS_EC
> +	tristate "LED Support for ChromeOS EC"
> +	depends on MFD_CROS_EC_DEV
> +	depends on LEDS_CLASS_MULTICOLOR
> +	select LEDS_TRIGGERS
> +	default MFD_CROS_EC_DEV
> +	help
> +	  This option enables support for LEDs managed by ChromeOS ECs.
> +	  All LEDs exposed by the EC are supported in multicolor mode.
> +	  A hardware trigger to switch back to the automatic behaviour is
> +	  provided.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-cros_ec.
> +
>  config LEDS_EL15203000
>  	tristate "LED Support for Crane EL15203000"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index effdfc6f1e95..3491904e13f7 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
>  obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
>  obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
>  obj-$(CONFIG_LEDS_CPCAP)		+= leds-cpcap.o
> +obj-$(CONFIG_LEDS_CROS_EC)		+= leds-cros_ec.o
>  obj-$(CONFIG_LEDS_DA903X)		+= leds-da903x.o
>  obj-$(CONFIG_LEDS_DA9052)		+= leds-da9052.o
>  obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
> diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c
> new file mode 100644
> index 000000000000..7bb21a587713
> --- /dev/null
> +++ b/drivers/leds/leds-cros_ec.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  ChromeOS EC LED Driver
> + *
> + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/leds.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +
> +#define DRV_NAME	"cros-ec-led"

Please refrain from defining device names.  Use the string in-place.

> +static const char * const cros_ec_led_functions[] = {
> +	[EC_LED_ID_BATTERY_LED]            = LED_FUNCTION_CHARGING,
> +	[EC_LED_ID_POWER_LED]              = LED_FUNCTION_POWER,
> +	[EC_LED_ID_ADAPTER_LED]            = "adapter",
> +	[EC_LED_ID_LEFT_LED]               = "left",
> +	[EC_LED_ID_RIGHT_LED]              = "right",
> +	[EC_LED_ID_RECOVERY_HW_REINIT_LED] = "recovery-hw-reinit",
> +	[EC_LED_ID_SYSRQ_DEBUG_LED]        = "sysrq-debug",
> +};
> +
> +static_assert(ARRAY_SIZE(cros_ec_led_functions) == EC_LED_ID_COUNT);

What does this do?  Result in a build failure?

> +static const int cros_ec_led_to_linux_id[] = {
> +	[EC_LED_COLOR_RED]    = LED_COLOR_ID_RED,
> +	[EC_LED_COLOR_GREEN]  = LED_COLOR_ID_GREEN,
> +	[EC_LED_COLOR_BLUE]   = LED_COLOR_ID_BLUE,
> +	[EC_LED_COLOR_YELLOW] = LED_COLOR_ID_YELLOW,
> +	[EC_LED_COLOR_WHITE]  = LED_COLOR_ID_WHITE,
> +	[EC_LED_COLOR_AMBER]  = LED_COLOR_ID_AMBER,
> +};
> +
> +static_assert(ARRAY_SIZE(cros_ec_led_to_linux_id) == EC_LED_COLOR_COUNT);
> +
> +static const int cros_ec_linux_to_ec_id[] = {
> +	[LED_COLOR_ID_RED]    = EC_LED_COLOR_RED,
> +	[LED_COLOR_ID_GREEN]  = EC_LED_COLOR_GREEN,
> +	[LED_COLOR_ID_BLUE]   = EC_LED_COLOR_BLUE,
> +	[LED_COLOR_ID_YELLOW] = EC_LED_COLOR_YELLOW,
> +	[LED_COLOR_ID_WHITE]  = EC_LED_COLOR_WHITE,
> +	[LED_COLOR_ID_AMBER]  = EC_LED_COLOR_AMBER,
> +};
> +
> +struct cros_ec_led_priv {
> +	struct led_classdev_mc led_mc_cdev;
> +	struct cros_ec_device *cros_ec;
> +	enum ec_led_id led_id;
> +};
> +
> +static inline struct cros_ec_led_priv *cros_ec_led_cdev_to_priv(struct led_classdev *led_cdev)
> +{
> +	return container_of(lcdev_to_mccdev(led_cdev), struct cros_ec_led_priv, led_mc_cdev);
> +}
> +
> +union cros_ec_led_cmd_data {
> +	struct ec_params_led_control req;
> +	struct ec_response_led_control resp;
> +} __packed;
> +
> +static int cros_ec_led_send_cmd(struct cros_ec_device *cros_ec,
> +				union cros_ec_led_cmd_data *arg)
> +{
> +	int ret;
> +	struct {
> +		struct cros_ec_command msg;
> +		union cros_ec_led_cmd_data data;
> +	} __packed buf = {
> +		.msg = {
> +			.version = 1,
> +			.command = EC_CMD_LED_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 int cros_ec_led_trigger_activate(struct led_classdev *led_cdev)
> +{
> +	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
> +	union cros_ec_led_cmd_data arg = {};
> +
> +	arg.req.led_id = priv->led_id;
> +	arg.req.flags = EC_LED_FLAGS_AUTO;
> +
> +	return cros_ec_led_send_cmd(priv->cros_ec, &arg);
> +}
> +
> +static struct led_hw_trigger_type cros_ec_led_trigger_type;
> +
> +static struct led_trigger cros_ec_led_trigger = {
> +	.name = "chromeos-auto",
> +	.trigger_type = &cros_ec_led_trigger_type,
> +	.activate = cros_ec_led_trigger_activate,
> +};
> +
> +static int cros_ec_led_brightness_set_blocking(struct led_classdev *led_cdev,
> +					       enum led_brightness brightness)
> +{
> +	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
> +	union cros_ec_led_cmd_data arg = {};
> +	enum ec_led_colors led_color;
> +	struct mc_subled *subled;
> +	size_t i;
> +
> +	led_mc_calc_color_components(&priv->led_mc_cdev, brightness);
> +
> +	arg.req.led_id = priv->led_id;
> +
> +	for (i = 0; i < priv->led_mc_cdev.num_colors; i++) {
> +		subled = &priv->led_mc_cdev.subled_info[i];
> +		led_color = cros_ec_linux_to_ec_id[subled->color_index];
> +		arg.req.brightness[led_color] = subled->brightness;
> +	}
> +
> +	return cros_ec_led_send_cmd(priv->cros_ec, &arg);
> +}
> +
> +static int cros_ec_led_count_subleds(struct device *dev,
> +				     struct ec_response_led_control *resp,
> +				     unsigned int *max_brightness)
> +{
> +	unsigned int range, common_range = 0;
> +	int num_subleds = 0;
> +	size_t i;
> +
> +	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
> +		range = resp->brightness_range[i];
> +
> +		if (!range)
> +			continue;
> +
> +		num_subleds++;
> +
> +		if (!common_range)
> +			common_range = range;
> +
> +		if (common_range != range) {
> +			/* The multicolor LED API expects a uniform max_brightness */
> +			dev_warn(dev, "Inconsistent LED brightness values\n");

You shouldn't print a warning then return an error.

Please upgrade to dev_err().

> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (!num_subleds)
> +		return -EINVAL;
> +
> +	*max_brightness = common_range;
> +	return num_subleds;
> +}
> +
> +static const char *cros_ec_led_get_color_name(struct led_classdev_mc *led_mc_cdev)
> +{
> +	int color;
> +
> +	if (led_mc_cdev->num_colors == 1)
> +		color = led_mc_cdev->subled_info[0].color_index;
> +	else
> +		color = LED_COLOR_ID_MULTI;
> +
> +	return led_get_color_name(color);
> +}
> +
> +static int cros_ec_led_probe_led(struct device *dev, struct cros_ec_device *cros_ec,

Odd naming choice.

How about cros_ec_led_probe_one() or cros_ec_led_init()?

> +				 enum ec_led_id id)
> +{
> +	union cros_ec_led_cmd_data arg = {};
> +	struct cros_ec_led_priv *priv;
> +	struct led_classdev *led_cdev;
> +	struct mc_subled *subleds;
> +	int ret, num_subleds;
> +	size_t i, subled;

Why size_t for the iterator?

> +	arg.req.led_id = id;
> +	arg.req.flags = EC_LED_FLAGS_QUERY;
> +	ret = cros_ec_led_send_cmd(cros_ec, &arg);
> +	/* Unknown LED, skip */

Place the comment inside the if() or next to the return.


> +	if (ret == -EINVAL)
> +		return 0;
> +	if (ret == -EOPNOTSUPP)
> +		return -ENODEV;
> +	if (ret < 0)
> +		return ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	num_subleds = cros_ec_led_count_subleds(dev, &arg.resp,
> +						&priv->led_mc_cdev.led_cdev.max_brightness);
> +	if (num_subleds < 0)
> +		return num_subleds;
> +
> +	priv->cros_ec = cros_ec;
> +	priv->led_id = id;
> +
> +	subleds = devm_kcalloc(dev, num_subleds, sizeof(*subleds), GFP_KERNEL);
> +	if (!subleds)
> +		return -ENOMEM;
> +
> +	subled = 0;
> +	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
> +		if (!arg.resp.brightness_range[i])
> +			continue;
> +
> +		subleds[subled].color_index = cros_ec_led_to_linux_id[i];
> +		if (subled == 0)
> +			subleds[subled].intensity = 100;
> +		subled++;
> +	}
> +
> +	priv->led_mc_cdev.subled_info = subleds;
> +	priv->led_mc_cdev.num_colors = num_subleds;
> +
> +	led_cdev = &priv->led_mc_cdev.led_cdev;
> +	led_cdev->brightness_set_blocking = cros_ec_led_brightness_set_blocking;
> +	led_cdev->trigger_type = &cros_ec_led_trigger_type;
> +	led_cdev->default_trigger = cros_ec_led_trigger.name;
> +	led_cdev->hw_control_trigger = cros_ec_led_trigger.name;
> +
> +	led_cdev->name = devm_kasprintf(dev, GFP_KERNEL, "chromeos:%s:%s",
> +					cros_ec_led_get_color_name(&priv->led_mc_cdev),
> +					cros_ec_led_functions[id]);
> +	if (!led_cdev->name)
> +		return -ENOMEM;
> +
> +	return devm_led_classdev_multicolor_register(dev, &priv->led_mc_cdev);
> +}
> +
> +static int cros_ec_led_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;
> +	int ret = 0;
> +	size_t i;
> +
> +	for (i = 0; i < EC_LED_ID_COUNT; i++) {
> +		ret = cros_ec_led_probe_led(dev, cros_ec, i);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct platform_device_id cros_ec_led_id[] = {
> +	{ DRV_NAME, 0 },
> +	{}
> +};
> +
> +static struct platform_driver cros_ec_led_driver = {
> +	.driver.name	= DRV_NAME,
> +	.probe		= cros_ec_led_probe,
> +	.id_table	= cros_ec_led_id,
> +};
> +
> +static int __init cros_ec_led_init(void)
> +{
> +	int ret;
> +
> +	ret = led_trigger_register(&cros_ec_led_trigger);
> +	if (ret)
> +		return ret;

This has to be done before probe?

> +	ret = platform_driver_register(&cros_ec_led_driver);
> +	if (ret)
> +		led_trigger_unregister(&cros_ec_led_trigger);
> +
> +	return ret;
> +};
> +module_init(cros_ec_led_init);
> +
> +static void __exit cros_ec_led_exit(void)
> +{
> +	platform_driver_unregister(&cros_ec_led_driver);
> +	led_trigger_unregister(&cros_ec_led_trigger);
> +};
> +module_exit(cros_ec_led_exit);
> +
> +MODULE_DEVICE_TABLE(platform, cros_ec_led_id);
> +MODULE_DESCRIPTION("ChromeOS EC LED Driver");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.45.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 4/5] leds: Add ChromeOS EC driver

[Thomas Weißschuh]

On 2024-06-14 10:02:19+0000, Lee Jones wrote:
> On Thu, 13 Jun 2024, Thomas Weißschuh wrote:
> 
> > The ChromeOS Embedded Controller exposes an LED control command.
> > Expose its functionality through the leds subsystem.
> > 
> > The LEDs are exposed as multicolor devices.
> > A hardware trigger, which is active by default, is provided to let the
> > EC itself take over control over the LED.
> > 
> > The driver is designed to be probed via the cros_ec mfd device.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  MAINTAINERS                 |   5 +
> >  drivers/leds/Kconfig        |  15 +++
> >  drivers/leds/Makefile       |   1 +
> >  drivers/leds/leds-cros_ec.c | 299 ++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 320 insertions(+)
> 
> Mostly fine.  Couple of points.
> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index aacccb376c28..8bc3491a08af 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5135,6 +5135,11 @@ S:	Maintained
> >  F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
> >  F:	sound/soc/codecs/cros_ec_codec.*
> >  
> > +CHROMEOS EC LED DRIVER
> > +M:	Thomas Weißschuh <thomas@weissschuh.net>
> > +S:	Maintained
> > +F:	drivers/leds/leds-cros_ec.c
> > +
> >  CHROMEOS EC SUBDRIVERS
> >  M:	Benson Leung <bleung@chromium.org>
> >  R:	Guenter Roeck <groeck@chromium.org>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 05e6af88b88c..aa2fec9a34ed 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -179,6 +179,21 @@ config LEDS_CR0014114
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called leds-cr0014114.
> >  
> > +config LEDS_CROS_EC
> > +	tristate "LED Support for ChromeOS EC"
> > +	depends on MFD_CROS_EC_DEV
> > +	depends on LEDS_CLASS_MULTICOLOR
> > +	select LEDS_TRIGGERS
> > +	default MFD_CROS_EC_DEV
> > +	help
> > +	  This option enables support for LEDs managed by ChromeOS ECs.
> > +	  All LEDs exposed by the EC are supported in multicolor mode.
> > +	  A hardware trigger to switch back to the automatic behaviour is
> > +	  provided.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called leds-cros_ec.
> > +
> >  config LEDS_EL15203000
> >  	tristate "LED Support for Crane EL15203000"
> >  	depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index effdfc6f1e95..3491904e13f7 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -26,6 +26,7 @@ obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
> >  obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
> >  obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
> >  obj-$(CONFIG_LEDS_CPCAP)		+= leds-cpcap.o
> > +obj-$(CONFIG_LEDS_CROS_EC)		+= leds-cros_ec.o
> >  obj-$(CONFIG_LEDS_DA903X)		+= leds-da903x.o
> >  obj-$(CONFIG_LEDS_DA9052)		+= leds-da9052.o
> >  obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
> > diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c
> > new file mode 100644
> > index 000000000000..7bb21a587713
> > --- /dev/null
> > +++ b/drivers/leds/leds-cros_ec.c
> > @@ -0,0 +1,299 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *  ChromeOS EC LED Driver
> > + *
> > + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/leds.h>
> > +#include <linux/led-class-multicolor.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +
> > +#define DRV_NAME	"cros-ec-led"
> 
> Please refrain from defining device names.  Use the string in-place.

This is the common pattern used for other drivers using the cros_ec MFD
device. I'll change it, though.

> > +static const char * const cros_ec_led_functions[] = {
> > +	[EC_LED_ID_BATTERY_LED]            = LED_FUNCTION_CHARGING,
> > +	[EC_LED_ID_POWER_LED]              = LED_FUNCTION_POWER,
> > +	[EC_LED_ID_ADAPTER_LED]            = "adapter",
> > +	[EC_LED_ID_LEFT_LED]               = "left",
> > +	[EC_LED_ID_RIGHT_LED]              = "right",
> > +	[EC_LED_ID_RECOVERY_HW_REINIT_LED] = "recovery-hw-reinit",
> > +	[EC_LED_ID_SYSRQ_DEBUG_LED]        = "sysrq-debug",
> > +};
> > +
> > +static_assert(ARRAY_SIZE(cros_ec_led_functions) == EC_LED_ID_COUNT);
> 
> What does this do?  Result in a build failure?

Yes. It's the standard C version of BUILD_BUG_ON().
It can be used in all contexts, in contrast to BUILD_BUG_ON) and is
already widely used within the tree.

The goal is to make sure that additions to "enum ec_led_id" do not
inadvertedly lead to out-of-bounds accesses in those arrays.

> > +static const int cros_ec_led_to_linux_id[] = {
> > +	[EC_LED_COLOR_RED]    = LED_COLOR_ID_RED,
> > +	[EC_LED_COLOR_GREEN]  = LED_COLOR_ID_GREEN,
> > +	[EC_LED_COLOR_BLUE]   = LED_COLOR_ID_BLUE,
> > +	[EC_LED_COLOR_YELLOW] = LED_COLOR_ID_YELLOW,
> > +	[EC_LED_COLOR_WHITE]  = LED_COLOR_ID_WHITE,
> > +	[EC_LED_COLOR_AMBER]  = LED_COLOR_ID_AMBER,
> > +};
> > +
> > +static_assert(ARRAY_SIZE(cros_ec_led_to_linux_id) == EC_LED_COLOR_COUNT);
> > +
> > +static const int cros_ec_linux_to_ec_id[] = {
> > +	[LED_COLOR_ID_RED]    = EC_LED_COLOR_RED,
> > +	[LED_COLOR_ID_GREEN]  = EC_LED_COLOR_GREEN,
> > +	[LED_COLOR_ID_BLUE]   = EC_LED_COLOR_BLUE,
> > +	[LED_COLOR_ID_YELLOW] = EC_LED_COLOR_YELLOW,
> > +	[LED_COLOR_ID_WHITE]  = EC_LED_COLOR_WHITE,
> > +	[LED_COLOR_ID_AMBER]  = EC_LED_COLOR_AMBER,
> > +};
> > +
> > +struct cros_ec_led_priv {
> > +	struct led_classdev_mc led_mc_cdev;
> > +	struct cros_ec_device *cros_ec;
> > +	enum ec_led_id led_id;
> > +};
> > +
> > +static inline struct cros_ec_led_priv *cros_ec_led_cdev_to_priv(struct led_classdev *led_cdev)
> > +{
> > +	return container_of(lcdev_to_mccdev(led_cdev), struct cros_ec_led_priv, led_mc_cdev);
> > +}
> > +
> > +union cros_ec_led_cmd_data {
> > +	struct ec_params_led_control req;
> > +	struct ec_response_led_control resp;
> > +} __packed;
> > +
> > +static int cros_ec_led_send_cmd(struct cros_ec_device *cros_ec,
> > +				union cros_ec_led_cmd_data *arg)
> > +{
> > +	int ret;
> > +	struct {
> > +		struct cros_ec_command msg;
> > +		union cros_ec_led_cmd_data data;
> > +	} __packed buf = {
> > +		.msg = {
> > +			.version = 1,
> > +			.command = EC_CMD_LED_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 int cros_ec_led_trigger_activate(struct led_classdev *led_cdev)
> > +{
> > +	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
> > +	union cros_ec_led_cmd_data arg = {};
> > +
> > +	arg.req.led_id = priv->led_id;
> > +	arg.req.flags = EC_LED_FLAGS_AUTO;
> > +
> > +	return cros_ec_led_send_cmd(priv->cros_ec, &arg);
> > +}
> > +
> > +static struct led_hw_trigger_type cros_ec_led_trigger_type;
> > +
> > +static struct led_trigger cros_ec_led_trigger = {
> > +	.name = "chromeos-auto",
> > +	.trigger_type = &cros_ec_led_trigger_type,
> > +	.activate = cros_ec_led_trigger_activate,
> > +};
> > +
> > +static int cros_ec_led_brightness_set_blocking(struct led_classdev *led_cdev,
> > +					       enum led_brightness brightness)
> > +{
> > +	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
> > +	union cros_ec_led_cmd_data arg = {};
> > +	enum ec_led_colors led_color;
> > +	struct mc_subled *subled;
> > +	size_t i;
> > +
> > +	led_mc_calc_color_components(&priv->led_mc_cdev, brightness);
> > +
> > +	arg.req.led_id = priv->led_id;
> > +
> > +	for (i = 0; i < priv->led_mc_cdev.num_colors; i++) {
> > +		subled = &priv->led_mc_cdev.subled_info[i];
> > +		led_color = cros_ec_linux_to_ec_id[subled->color_index];
> > +		arg.req.brightness[led_color] = subled->brightness;
> > +	}
> > +
> > +	return cros_ec_led_send_cmd(priv->cros_ec, &arg);
> > +}
> > +
> > +static int cros_ec_led_count_subleds(struct device *dev,
> > +				     struct ec_response_led_control *resp,
> > +				     unsigned int *max_brightness)
> > +{
> > +	unsigned int range, common_range = 0;
> > +	int num_subleds = 0;
> > +	size_t i;
> > +
> > +	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
> > +		range = resp->brightness_range[i];
> > +
> > +		if (!range)
> > +			continue;
> > +
> > +		num_subleds++;
> > +
> > +		if (!common_range)
> > +			common_range = range;
> > +
> > +		if (common_range != range) {
> > +			/* The multicolor LED API expects a uniform max_brightness */
> > +			dev_warn(dev, "Inconsistent LED brightness values\n");
> 
> You shouldn't print a warning then return an error.
> 
> Please upgrade to dev_err().

Ack.

> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	if (!num_subleds)
> > +		return -EINVAL;
> > +
> > +	*max_brightness = common_range;
> > +	return num_subleds;
> > +}
> > +
> > +static const char *cros_ec_led_get_color_name(struct led_classdev_mc *led_mc_cdev)
> > +{
> > +	int color;
> > +
> > +	if (led_mc_cdev->num_colors == 1)
> > +		color = led_mc_cdev->subled_info[0].color_index;
> > +	else
> > +		color = LED_COLOR_ID_MULTI;
> > +
> > +	return led_get_color_name(color);
> > +}
> > +
> > +static int cros_ec_led_probe_led(struct device *dev, struct cros_ec_device *cros_ec,
> 
> Odd naming choice.
> 
> How about cros_ec_led_probe_one() or cros_ec_led_init()?

Ack.

> > +				 enum ec_led_id id)
> > +{
> > +	union cros_ec_led_cmd_data arg = {};
> > +	struct cros_ec_led_priv *priv;
> > +	struct led_classdev *led_cdev;
> > +	struct mc_subled *subleds;
> > +	int ret, num_subleds;
> > +	size_t i, subled;
> 
> Why size_t for the iterator?

Habit, because ARRAY_SIZE() returns size_t.
Will change to int.

> > +	arg.req.led_id = id;
> > +	arg.req.flags = EC_LED_FLAGS_QUERY;
> > +	ret = cros_ec_led_send_cmd(cros_ec, &arg);
> > +	/* Unknown LED, skip */
> 
> Place the comment inside the if() or next to the return.

Ack.
 
> > +	if (ret == -EINVAL)
> > +		return 0;
> > +	if (ret == -EOPNOTSUPP)
> > +		return -ENODEV;
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	num_subleds = cros_ec_led_count_subleds(dev, &arg.resp,
> > +						&priv->led_mc_cdev.led_cdev.max_brightness);
> > +	if (num_subleds < 0)
> > +		return num_subleds;
> > +
> > +	priv->cros_ec = cros_ec;
> > +	priv->led_id = id;
> > +
> > +	subleds = devm_kcalloc(dev, num_subleds, sizeof(*subleds), GFP_KERNEL);
> > +	if (!subleds)
> > +		return -ENOMEM;
> > +
> > +	subled = 0;
> > +	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
> > +		if (!arg.resp.brightness_range[i])
> > +			continue;
> > +
> > +		subleds[subled].color_index = cros_ec_led_to_linux_id[i];
> > +		if (subled == 0)
> > +			subleds[subled].intensity = 100;
> > +		subled++;
> > +	}
> > +
> > +	priv->led_mc_cdev.subled_info = subleds;
> > +	priv->led_mc_cdev.num_colors = num_subleds;
> > +
> > +	led_cdev = &priv->led_mc_cdev.led_cdev;
> > +	led_cdev->brightness_set_blocking = cros_ec_led_brightness_set_blocking;
> > +	led_cdev->trigger_type = &cros_ec_led_trigger_type;
> > +	led_cdev->default_trigger = cros_ec_led_trigger.name;
> > +	led_cdev->hw_control_trigger = cros_ec_led_trigger.name;
> > +
> > +	led_cdev->name = devm_kasprintf(dev, GFP_KERNEL, "chromeos:%s:%s",
> > +					cros_ec_led_get_color_name(&priv->led_mc_cdev),
> > +					cros_ec_led_functions[id]);
> > +	if (!led_cdev->name)
> > +		return -ENOMEM;
> > +
> > +	return devm_led_classdev_multicolor_register(dev, &priv->led_mc_cdev);
> > +}
> > +
> > +static int cros_ec_led_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;
> > +	int ret = 0;
> > +	size_t i;
> > +
> > +	for (i = 0; i < EC_LED_ID_COUNT; i++) {
> > +		ret = cros_ec_led_probe_led(dev, cros_ec, i);
> > +		if (ret)
> > +			break;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct platform_device_id cros_ec_led_id[] = {
> > +	{ DRV_NAME, 0 },
> > +	{}
> > +};
> > +
> > +static struct platform_driver cros_ec_led_driver = {
> > +	.driver.name	= DRV_NAME,
> > +	.probe		= cros_ec_led_probe,
> > +	.id_table	= cros_ec_led_id,
> > +};
> > +
> > +static int __init cros_ec_led_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = led_trigger_register(&cros_ec_led_trigger);
> > +	if (ret)
> > +		return ret;
> 
> This has to be done before probe?

Nope, I think the can be moved into probe.
It makes everything easier.

> > +	ret = platform_driver_register(&cros_ec_led_driver);
> > +	if (ret)
> > +		led_trigger_unregister(&cros_ec_led_trigger);
> > +
> > +	return ret;
> > +};
> > +module_init(cros_ec_led_init);
> > +
> > +static void __exit cros_ec_led_exit(void)
> > +{
> > +	platform_driver_unregister(&cros_ec_led_driver);
> > +	led_trigger_unregister(&cros_ec_led_trigger);
> > +};
> > +module_exit(cros_ec_led_exit);
> > +
> > +MODULE_DEVICE_TABLE(platform, cros_ec_led_id);
> > +MODULE_DESCRIPTION("ChromeOS EC LED Driver");
> > +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net");
> > +MODULE_LICENSE("GPL");
> > 
> > -- 
> > 2.45.2
> > 
> 
> -- 
> Lee Jones [李琼斯]

Re: [PATCH v3 4/5] leds: Add ChromeOS EC driver

[Lee Jones]

On Fri, 14 Jun 2024, Thomas Weißschuh wrote:

> On 2024-06-14 10:02:19+0000, Lee Jones wrote:
> > On Thu, 13 Jun 2024, Thomas Weißschuh wrote:
> > 
> > > The ChromeOS Embedded Controller exposes an LED control command.
> > > Expose its functionality through the leds subsystem.
> > > 
> > > The LEDs are exposed as multicolor devices.
> > > A hardware trigger, which is active by default, is provided to let the
> > > EC itself take over control over the LED.
> > > 
> > > The driver is designed to be probed via the cros_ec mfd device.
> > > 
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > >  MAINTAINERS                 |   5 +
> > >  drivers/leds/Kconfig        |  15 +++
> > >  drivers/leds/Makefile       |   1 +
> > >  drivers/leds/leds-cros_ec.c | 299 ++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 320 insertions(+)
> > 
> > Mostly fine.  Couple of points.
> > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index aacccb376c28..8bc3491a08af 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -5135,6 +5135,11 @@ S:	Maintained
> > >  F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
> > >  F:	sound/soc/codecs/cros_ec_codec.*
> > >  
> > > +CHROMEOS EC LED DRIVER
> > > +M:	Thomas Weißschuh <thomas@weissschuh.net>
> > > +S:	Maintained
> > > +F:	drivers/leds/leds-cros_ec.c
> > > +
> > >  CHROMEOS EC SUBDRIVERS
> > >  M:	Benson Leung <bleung@chromium.org>
> > >  R:	Guenter Roeck <groeck@chromium.org>
> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > index 05e6af88b88c..aa2fec9a34ed 100644
> > > --- a/drivers/leds/Kconfig
> > > +++ b/drivers/leds/Kconfig
> > > @@ -179,6 +179,21 @@ config LEDS_CR0014114
> > >  	  To compile this driver as a module, choose M here: the module
> > >  	  will be called leds-cr0014114.
> > >  
> > > +config LEDS_CROS_EC
> > > +	tristate "LED Support for ChromeOS EC"
> > > +	depends on MFD_CROS_EC_DEV
> > > +	depends on LEDS_CLASS_MULTICOLOR
> > > +	select LEDS_TRIGGERS
> > > +	default MFD_CROS_EC_DEV
> > > +	help
> > > +	  This option enables support for LEDs managed by ChromeOS ECs.
> > > +	  All LEDs exposed by the EC are supported in multicolor mode.
> > > +	  A hardware trigger to switch back to the automatic behaviour is
> > > +	  provided.
> > > +
> > > +	  To compile this driver as a module, choose M here: the module
> > > +	  will be called leds-cros_ec.
> > > +
> > >  config LEDS_EL15203000
> > >  	tristate "LED Support for Crane EL15203000"
> > >  	depends on LEDS_CLASS
> > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > index effdfc6f1e95..3491904e13f7 100644
> > > --- a/drivers/leds/Makefile
> > > +++ b/drivers/leds/Makefile
> > > @@ -26,6 +26,7 @@ obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
> > >  obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
> > >  obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
> > >  obj-$(CONFIG_LEDS_CPCAP)		+= leds-cpcap.o
> > > +obj-$(CONFIG_LEDS_CROS_EC)		+= leds-cros_ec.o
> > >  obj-$(CONFIG_LEDS_DA903X)		+= leds-da903x.o
> > >  obj-$(CONFIG_LEDS_DA9052)		+= leds-da9052.o
> > >  obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
> > > diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c
> > > new file mode 100644
> > > index 000000000000..7bb21a587713
> > > --- /dev/null
> > > +++ b/drivers/leds/leds-cros_ec.c
> > > @@ -0,0 +1,299 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + *  ChromeOS EC LED Driver
> > > + *
> > > + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/leds.h>
> > > +#include <linux/led-class-multicolor.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/platform_data/cros_ec_commands.h>
> > > +#include <linux/platform_data/cros_ec_proto.h>
> > > +
> > > +#define DRV_NAME	"cros-ec-led"
> > 
> > Please refrain from defining device names.  Use the string in-place.
> 
> This is the common pattern used for other drivers using the cros_ec MFD
> device. I'll change it, though.

And the others ideally. ;)

> > > +static const char * const cros_ec_led_functions[] = {
> > > +	[EC_LED_ID_BATTERY_LED]            = LED_FUNCTION_CHARGING,
> > > +	[EC_LED_ID_POWER_LED]              = LED_FUNCTION_POWER,
> > > +	[EC_LED_ID_ADAPTER_LED]            = "adapter",
> > > +	[EC_LED_ID_LEFT_LED]               = "left",
> > > +	[EC_LED_ID_RIGHT_LED]              = "right",
> > > +	[EC_LED_ID_RECOVERY_HW_REINIT_LED] = "recovery-hw-reinit",
> > > +	[EC_LED_ID_SYSRQ_DEBUG_LED]        = "sysrq-debug",
> > > +};
> > > +
> > > +static_assert(ARRAY_SIZE(cros_ec_led_functions) == EC_LED_ID_COUNT);
> > 
> > What does this do?  Result in a build failure?
> 
> Yes. It's the standard C version of BUILD_BUG_ON().
> It can be used in all contexts, in contrast to BUILD_BUG_ON) and is
> already widely used within the tree.

I see.  Thanks for the explanation.

> The goal is to make sure that additions to "enum ec_led_id" do not
> inadvertedly lead to out-of-bounds accesses in those arrays.
> 
> > > +static const int cros_ec_led_to_linux_id[] = {
> > > +	[EC_LED_COLOR_RED]    = LED_COLOR_ID_RED,
> > > +	[EC_LED_COLOR_GREEN]  = LED_COLOR_ID_GREEN,
> > > +	[EC_LED_COLOR_BLUE]   = LED_COLOR_ID_BLUE,
> > > +	[EC_LED_COLOR_YELLOW] = LED_COLOR_ID_YELLOW,
> > > +	[EC_LED_COLOR_WHITE]  = LED_COLOR_ID_WHITE,
> > > +	[EC_LED_COLOR_AMBER]  = LED_COLOR_ID_AMBER,
> > > +};
> > > +
> > > +static_assert(ARRAY_SIZE(cros_ec_led_to_linux_id) == EC_LED_COLOR_COUNT);
> > > +
> > > +static const int cros_ec_linux_to_ec_id[] = {
> > > +	[LED_COLOR_ID_RED]    = EC_LED_COLOR_RED,
> > > +	[LED_COLOR_ID_GREEN]  = EC_LED_COLOR_GREEN,
> > > +	[LED_COLOR_ID_BLUE]   = EC_LED_COLOR_BLUE,
> > > +	[LED_COLOR_ID_YELLOW] = EC_LED_COLOR_YELLOW,
> > > +	[LED_COLOR_ID_WHITE]  = EC_LED_COLOR_WHITE,
> > > +	[LED_COLOR_ID_AMBER]  = EC_LED_COLOR_AMBER,
> > > +};
> > > +
> > > +struct cros_ec_led_priv {
> > > +	struct led_classdev_mc led_mc_cdev;
> > > +	struct cros_ec_device *cros_ec;
> > > +	enum ec_led_id led_id;
> > > +};
> > > +
> > > +static inline struct cros_ec_led_priv *cros_ec_led_cdev_to_priv(struct led_classdev *led_cdev)
> > > +{
> > > +	return container_of(lcdev_to_mccdev(led_cdev), struct cros_ec_led_priv, led_mc_cdev);
> > > +}
> > > +
> > > +union cros_ec_led_cmd_data {
> > > +	struct ec_params_led_control req;
> > > +	struct ec_response_led_control resp;
> > > +} __packed;
> > > +
> > > +static int cros_ec_led_send_cmd(struct cros_ec_device *cros_ec,
> > > +				union cros_ec_led_cmd_data *arg)
> > > +{
> > > +	int ret;
> > > +	struct {
> > > +		struct cros_ec_command msg;
> > > +		union cros_ec_led_cmd_data data;
> > > +	} __packed buf = {
> > > +		.msg = {
> > > +			.version = 1,
> > > +			.command = EC_CMD_LED_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 int cros_ec_led_trigger_activate(struct led_classdev *led_cdev)
> > > +{
> > > +	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
> > > +	union cros_ec_led_cmd_data arg = {};
> > > +
> > > +	arg.req.led_id = priv->led_id;
> > > +	arg.req.flags = EC_LED_FLAGS_AUTO;
> > > +
> > > +	return cros_ec_led_send_cmd(priv->cros_ec, &arg);
> > > +}
> > > +
> > > +static struct led_hw_trigger_type cros_ec_led_trigger_type;
> > > +
> > > +static struct led_trigger cros_ec_led_trigger = {
> > > +	.name = "chromeos-auto",
> > > +	.trigger_type = &cros_ec_led_trigger_type,
> > > +	.activate = cros_ec_led_trigger_activate,
> > > +};
> > > +
> > > +static int cros_ec_led_brightness_set_blocking(struct led_classdev *led_cdev,
> > > +					       enum led_brightness brightness)
> > > +{
> > > +	struct cros_ec_led_priv *priv = cros_ec_led_cdev_to_priv(led_cdev);
> > > +	union cros_ec_led_cmd_data arg = {};
> > > +	enum ec_led_colors led_color;
> > > +	struct mc_subled *subled;
> > > +	size_t i;
> > > +
> > > +	led_mc_calc_color_components(&priv->led_mc_cdev, brightness);
> > > +
> > > +	arg.req.led_id = priv->led_id;
> > > +
> > > +	for (i = 0; i < priv->led_mc_cdev.num_colors; i++) {
> > > +		subled = &priv->led_mc_cdev.subled_info[i];
> > > +		led_color = cros_ec_linux_to_ec_id[subled->color_index];
> > > +		arg.req.brightness[led_color] = subled->brightness;
> > > +	}
> > > +
> > > +	return cros_ec_led_send_cmd(priv->cros_ec, &arg);
> > > +}
> > > +
> > > +static int cros_ec_led_count_subleds(struct device *dev,
> > > +				     struct ec_response_led_control *resp,
> > > +				     unsigned int *max_brightness)
> > > +{
> > > +	unsigned int range, common_range = 0;
> > > +	int num_subleds = 0;
> > > +	size_t i;
> > > +
> > > +	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
> > > +		range = resp->brightness_range[i];
> > > +
> > > +		if (!range)
> > > +			continue;
> > > +
> > > +		num_subleds++;
> > > +
> > > +		if (!common_range)
> > > +			common_range = range;
> > > +
> > > +		if (common_range != range) {
> > > +			/* The multicolor LED API expects a uniform max_brightness */
> > > +			dev_warn(dev, "Inconsistent LED brightness values\n");
> > 
> > You shouldn't print a warning then return an error.
> > 
> > Please upgrade to dev_err().
> 
> Ack.
> 
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > > +
> > > +	if (!num_subleds)
> > > +		return -EINVAL;
> > > +
> > > +	*max_brightness = common_range;
> > > +	return num_subleds;
> > > +}
> > > +
> > > +static const char *cros_ec_led_get_color_name(struct led_classdev_mc *led_mc_cdev)
> > > +{
> > > +	int color;
> > > +
> > > +	if (led_mc_cdev->num_colors == 1)
> > > +		color = led_mc_cdev->subled_info[0].color_index;
> > > +	else
> > > +		color = LED_COLOR_ID_MULTI;
> > > +
> > > +	return led_get_color_name(color);
> > > +}
> > > +
> > > +static int cros_ec_led_probe_led(struct device *dev, struct cros_ec_device *cros_ec,
> > 
> > Odd naming choice.
> > 
> > How about cros_ec_led_probe_one() or cros_ec_led_init()?
> 
> Ack.
> 
> > > +				 enum ec_led_id id)
> > > +{
> > > +	union cros_ec_led_cmd_data arg = {};
> > > +	struct cros_ec_led_priv *priv;
> > > +	struct led_classdev *led_cdev;
> > > +	struct mc_subled *subleds;
> > > +	int ret, num_subleds;
> > > +	size_t i, subled;
> > 
> > Why size_t for the iterator?
> 
> Habit, because ARRAY_SIZE() returns size_t.
> Will change to int.

And the one below please.

> > > +	arg.req.led_id = id;
> > > +	arg.req.flags = EC_LED_FLAGS_QUERY;
> > > +	ret = cros_ec_led_send_cmd(cros_ec, &arg);
> > > +	/* Unknown LED, skip */
> > 
> > Place the comment inside the if() or next to the return.
> 
> Ack.
>  
> > > +	if (ret == -EINVAL)
> > > +		return 0;
> > > +	if (ret == -EOPNOTSUPP)
> > > +		return -ENODEV;
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	num_subleds = cros_ec_led_count_subleds(dev, &arg.resp,
> > > +						&priv->led_mc_cdev.led_cdev.max_brightness);
> > > +	if (num_subleds < 0)
> > > +		return num_subleds;
> > > +
> > > +	priv->cros_ec = cros_ec;
> > > +	priv->led_id = id;
> > > +
> > > +	subleds = devm_kcalloc(dev, num_subleds, sizeof(*subleds), GFP_KERNEL);
> > > +	if (!subleds)
> > > +		return -ENOMEM;
> > > +
> > > +	subled = 0;
> > > +	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
> > > +		if (!arg.resp.brightness_range[i])
> > > +			continue;
> > > +
> > > +		subleds[subled].color_index = cros_ec_led_to_linux_id[i];
> > > +		if (subled == 0)
> > > +			subleds[subled].intensity = 100;
> > > +		subled++;
> > > +	}
> > > +
> > > +	priv->led_mc_cdev.subled_info = subleds;
> > > +	priv->led_mc_cdev.num_colors = num_subleds;
> > > +
> > > +	led_cdev = &priv->led_mc_cdev.led_cdev;
> > > +	led_cdev->brightness_set_blocking = cros_ec_led_brightness_set_blocking;
> > > +	led_cdev->trigger_type = &cros_ec_led_trigger_type;
> > > +	led_cdev->default_trigger = cros_ec_led_trigger.name;
> > > +	led_cdev->hw_control_trigger = cros_ec_led_trigger.name;
> > > +
> > > +	led_cdev->name = devm_kasprintf(dev, GFP_KERNEL, "chromeos:%s:%s",
> > > +					cros_ec_led_get_color_name(&priv->led_mc_cdev),
> > > +					cros_ec_led_functions[id]);
> > > +	if (!led_cdev->name)
> > > +		return -ENOMEM;
> > > +
> > > +	return devm_led_classdev_multicolor_register(dev, &priv->led_mc_cdev);
> > > +}
> > > +
> > > +static int cros_ec_led_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;
> > > +	int ret = 0;
> > > +	size_t i;
> > > +
> > > +	for (i = 0; i < EC_LED_ID_COUNT; i++) {
> > > +		ret = cros_ec_led_probe_led(dev, cros_ec, i);
> > > +		if (ret)
> > > +			break;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct platform_device_id cros_ec_led_id[] = {
> > > +	{ DRV_NAME, 0 },
> > > +	{}
> > > +};
> > > +
> > > +static struct platform_driver cros_ec_led_driver = {
> > > +	.driver.name	= DRV_NAME,
> > > +	.probe		= cros_ec_led_probe,
> > > +	.id_table	= cros_ec_led_id,
> > > +};
> > > +
> > > +static int __init cros_ec_led_init(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = led_trigger_register(&cros_ec_led_trigger);
> > > +	if (ret)
> > > +		return ret;
> > 
> > This has to be done before probe?
> 
> Nope, I think the can be moved into probe.
> It makes everything easier.

Then you get rid of the boilerplate?

> > > +	ret = platform_driver_register(&cros_ec_led_driver);
> > > +	if (ret)
> > > +		led_trigger_unregister(&cros_ec_led_trigger);
> > > +
> > > +	return ret;
> > > +};
> > > +module_init(cros_ec_led_init);
> > > +
> > > +static void __exit cros_ec_led_exit(void)
> > > +{
> > > +	platform_driver_unregister(&cros_ec_led_driver);
> > > +	led_trigger_unregister(&cros_ec_led_trigger);
> > > +};
> > > +module_exit(cros_ec_led_exit);
> > > +
> > > +MODULE_DEVICE_TABLE(platform, cros_ec_led_id);
> > > +MODULE_DESCRIPTION("ChromeOS EC LED Driver");
> > > +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net");
> > > +MODULE_LICENSE("GPL");



-- 
Lee Jones [李琼斯]

[PATCH v3 5/5] mfd: cros_ec: Register LED subdevice

[Thomas Weißschuh]

Add ChromeOS EC-based LED control as EC subdevice.

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

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index a52d59cc2b1e..d8408054ba15 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -99,6 +99,10 @@ static const struct mfd_cell cros_ec_wdt_cells[] = {
 	{ .name = "cros-ec-wdt", }
 };
 
+static const struct mfd_cell cros_ec_led_cells[] = {
+	{ .name = "cros-ec-led", },
+};
+
 static const struct cros_feature_to_cells cros_subdevices[] = {
 	{
 		.id		= EC_FEATURE_CEC,
@@ -125,6 +129,11 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
 		.mfd_cells	= cros_ec_wdt_cells,
 		.num_cells	= ARRAY_SIZE(cros_ec_wdt_cells),
 	},
+	{
+		.id		= EC_FEATURE_LED,
+		.mfd_cells	= cros_ec_led_cells,
+		.num_cells	= ARRAY_SIZE(cros_ec_led_cells),
+	},
 };
 
 static const struct mfd_cell cros_ec_platform_cells[] = {

-- 
2.45.2

Re: [PATCH v3 0/5] ChromeOS Embedded Controller LED driver

[Lee Jones]

On Thu, 13 Jun 2024, Thomas Weißschuh wrote:

> Add a LED driver that supports the LED devices exposed by the
> ChromeOS Embedded Controller.
> 
> Patch 1-3 add a utility function to the led subsystem.
> Patch 4 introduces the actual driver.
> Patch 5 registers the driver through the cros_ec mfd devices.
> 
> Currently the driver introduces some non-standard LED functions.
> (See "cros_ec_led_functions")
> 
> Tested on a Framework 13 AMD, Firmware 3.05.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Changes in v3:
> - Set default_trigger explicitly as the LED core doesn't do this anymore
> - Only set intensity for first subled by default
> - Link to v2: https://lore.kernel.org/r/20240531-cros_ec-led-v2-0-6cc34408b40d@weissschuh.net
> 
> Changes in v2:
> - Cosmetic cleanups (Tzung-Bi)
> - Add trailing comma to MFD cell array
> - Rename LEDs and trigger to "chromeos" prefix, to align with kbd
>   backlight driver
> - Don't use type "rgb" anymore, they are only "multicolor"
> - Align commit messages and subject to subsystem standards (Lee)
> - Rename led_color_name() to led_get_color_name()
> - The same for cros_ec_led_color_name()
> - Link to v1: https://lore.kernel.org/r/20240520-cros_ec-led-v1-0-4068fc5c051a@weissschuh.net
> 
> ---
> Thomas Weißschuh (5):
>       leds: core: Introduce led_get_color_name() function
>       leds: multicolor: Use led_get_color_name() function
>       leds: core: Unexport led_colors[] array
>       leds: Add ChromeOS EC driver
>       mfd: cros_ec: Register LED subdevice
> 
>  MAINTAINERS                         |   5 +
>  drivers/leds/Kconfig                |  15 ++
>  drivers/leds/Makefile               |   1 +
>  drivers/leds/led-class-multicolor.c |   2 +-
>  drivers/leds/led-core.c             |  12 +-
>  drivers/leds/leds-cros_ec.c         | 299 ++++++++++++++++++++++++++++++++++++
>  drivers/leds/leds.h                 |   1 -
>  drivers/mfd/cros_ec_dev.c           |   9 ++
>  include/linux/leds.h                |  10 ++
>  9 files changed, 350 insertions(+), 4 deletions(-)
> ---
> base-commit: 2ccbdf43d5e758f8493a95252073cf9078a5fea5
> change-id: 20240519-cros_ec-led-3efa24e3991e

Applied and submitted for testing.

All being well, I'll follow-up with a cross-subsystem pull-request shortly

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 0/5] ChromeOS Embedded Controller LED driver

[Thomas Weißschuh]

On 2024-06-14 10:12:20+0000, Lee Jones wrote:
> On Thu, 13 Jun 2024, Thomas Weißschuh wrote:
> 
> > Add a LED driver that supports the LED devices exposed by the
> > ChromeOS Embedded Controller.
> > 
> > Patch 1-3 add a utility function to the led subsystem.
> > Patch 4 introduces the actual driver.
> > Patch 5 registers the driver through the cros_ec mfd devices.
> > 
> > Currently the driver introduces some non-standard LED functions.
> > (See "cros_ec_led_functions")
> > 
> > Tested on a Framework 13 AMD, Firmware 3.05.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> > Changes in v3:
> > - Set default_trigger explicitly as the LED core doesn't do this anymore
> > - Only set intensity for first subled by default
> > - Link to v2: https://lore.kernel.org/r/20240531-cros_ec-led-v2-0-6cc34408b40d@weissschuh.net
> > 
> > Changes in v2:
> > - Cosmetic cleanups (Tzung-Bi)
> > - Add trailing comma to MFD cell array
> > - Rename LEDs and trigger to "chromeos" prefix, to align with kbd
> >   backlight driver
> > - Don't use type "rgb" anymore, they are only "multicolor"
> > - Align commit messages and subject to subsystem standards (Lee)
> > - Rename led_color_name() to led_get_color_name()
> > - The same for cros_ec_led_color_name()
> > - Link to v1: https://lore.kernel.org/r/20240520-cros_ec-led-v1-0-4068fc5c051a@weissschuh.net
> > 
> > ---
> > Thomas Weißschuh (5):
> >       leds: core: Introduce led_get_color_name() function
> >       leds: multicolor: Use led_get_color_name() function
> >       leds: core: Unexport led_colors[] array
> >       leds: Add ChromeOS EC driver
> >       mfd: cros_ec: Register LED subdevice
> > 
> >  MAINTAINERS                         |   5 +
> >  drivers/leds/Kconfig                |  15 ++
> >  drivers/leds/Makefile               |   1 +
> >  drivers/leds/led-class-multicolor.c |   2 +-
> >  drivers/leds/led-core.c             |  12 +-
> >  drivers/leds/leds-cros_ec.c         | 299 ++++++++++++++++++++++++++++++++++++
> >  drivers/leds/leds.h                 |   1 -
> >  drivers/mfd/cros_ec_dev.c           |   9 ++
> >  include/linux/leds.h                |  10 ++
> >  9 files changed, 350 insertions(+), 4 deletions(-)
> > ---
> > base-commit: 2ccbdf43d5e758f8493a95252073cf9078a5fea5
> > change-id: 20240519-cros_ec-led-3efa24e3991e
> 
> Applied and submitted for testing.
> 
> All being well, I'll follow-up with a cross-subsystem pull-request shortly

Thanks!

I'm not sure which effect this application has on the review comments
you gave to patch 4 (thanks for those, too).

After implementing your requests, should I
* resubmit the whole series
* resubmit only patch 4
* send an incremental patch on top of the series
?


Thomas

Re: [PATCH v3 0/5] ChromeOS Embedded Controller LED driver

[Lee Jones]

On Fri, 14 Jun 2024, Thomas Weißschuh wrote:

> On 2024-06-14 10:12:20+0000, Lee Jones wrote:
> > On Thu, 13 Jun 2024, Thomas Weißschuh wrote:
> > 
> > > Add a LED driver that supports the LED devices exposed by the
> > > ChromeOS Embedded Controller.
> > > 
> > > Patch 1-3 add a utility function to the led subsystem.
> > > Patch 4 introduces the actual driver.
> > > Patch 5 registers the driver through the cros_ec mfd devices.
> > > 
> > > Currently the driver introduces some non-standard LED functions.
> > > (See "cros_ec_led_functions")
> > > 
> > > Tested on a Framework 13 AMD, Firmware 3.05.
> > > 
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > > Changes in v3:
> > > - Set default_trigger explicitly as the LED core doesn't do this anymore
> > > - Only set intensity for first subled by default
> > > - Link to v2: https://lore.kernel.org/r/20240531-cros_ec-led-v2-0-6cc34408b40d@weissschuh.net
> > > 
> > > Changes in v2:
> > > - Cosmetic cleanups (Tzung-Bi)
> > > - Add trailing comma to MFD cell array
> > > - Rename LEDs and trigger to "chromeos" prefix, to align with kbd
> > >   backlight driver
> > > - Don't use type "rgb" anymore, they are only "multicolor"
> > > - Align commit messages and subject to subsystem standards (Lee)
> > > - Rename led_color_name() to led_get_color_name()
> > > - The same for cros_ec_led_color_name()
> > > - Link to v1: https://lore.kernel.org/r/20240520-cros_ec-led-v1-0-4068fc5c051a@weissschuh.net
> > > 
> > > ---
> > > Thomas Weißschuh (5):
> > >       leds: core: Introduce led_get_color_name() function
> > >       leds: multicolor: Use led_get_color_name() function
> > >       leds: core: Unexport led_colors[] array
> > >       leds: Add ChromeOS EC driver
> > >       mfd: cros_ec: Register LED subdevice
> > > 
> > >  MAINTAINERS                         |   5 +
> > >  drivers/leds/Kconfig                |  15 ++
> > >  drivers/leds/Makefile               |   1 +
> > >  drivers/leds/led-class-multicolor.c |   2 +-
> > >  drivers/leds/led-core.c             |  12 +-
> > >  drivers/leds/leds-cros_ec.c         | 299 ++++++++++++++++++++++++++++++++++++
> > >  drivers/leds/leds.h                 |   1 -
> > >  drivers/mfd/cros_ec_dev.c           |   9 ++
> > >  include/linux/leds.h                |  10 ++
> > >  9 files changed, 350 insertions(+), 4 deletions(-)
> > > ---
> > > base-commit: 2ccbdf43d5e758f8493a95252073cf9078a5fea5
> > > change-id: 20240519-cros_ec-led-3efa24e3991e
> > 
> > Applied and submitted for testing.
> > 
> > All being well, I'll follow-up with a cross-subsystem pull-request shortly
> 
> Thanks!
> 
> I'm not sure which effect this application has on the review comments
> you gave to patch 4 (thanks for those, too).
> 
> After implementing your requests, should I
> * resubmit the whole series
> * resubmit only patch 4
> * send an incremental patch on top of the series

Incremental patch please.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 0/5] ChromeOS Embedded Controller LED driver

[Lee Jones]

On Thu, 13 Jun 2024 16:48:35 +0200, Thomas Weißschuh wrote:
> Add a LED driver that supports the LED devices exposed by the
> ChromeOS Embedded Controller.
> 
> Patch 1-3 add a utility function to the led subsystem.
> Patch 4 introduces the actual driver.
> Patch 5 registers the driver through the cros_ec mfd devices.
> 
> [...]

Applied, thanks!

[1/5] leds: core: Introduce led_get_color_name() function
      commit: ab52674e673ae3ecf457ee6036c24b2c5e0a2cca
[2/5] leds: multicolor: Use led_get_color_name() function
      commit: f5e22810770af92f69d266d4a7db505679264eab
[3/5] leds: core: Unexport led_colors[] array
      commit: e056183d2852d6e8cfd0da2a1e3eaf9c08801963
[4/5] leds: Add ChromeOS EC driver
      commit: 583745b475ace58b6223aee6d174ee46f04bce95
[5/5] mfd: cros_ec: Register LED subdevice
      commit: 2796819735adfc3cdf388696ab20a6780effb525

--
Lee Jones [李琼斯]

[GIT PULL] Immutable branch between LEDS and MFD due for the v6.11 merge window

[Lee Jones]

Enjoy!

The following changes since commit 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0:

  Linux 6.10-rc1 (2024-05-26 15:20:12 -0700)

are available in the Git repository at:

  ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/lee/leds.git tags/ib-leds-mfd-v6.11

for you to fetch changes up to b107093f433c3afb83a46af43c830e578e2ba54d:

  mfd: cros_ec: Register LED subdevice (2024-06-21 11:41:51 +0100)

----------------------------------------------------------------
Immutable branch between LEDS and MFD due for the v6.11 merge window

----------------------------------------------------------------
Thomas Weißschuh (5):
      leds: core: Introduce led_get_color_name() function
      leds: multicolor: Use led_get_color_name() function
      leds: core: Unexport led_colors[] array
      leds: Add ChromeOS EC driver
      mfd: cros_ec: Register LED subdevice

 MAINTAINERS                         |   5 +
 drivers/leds/Kconfig                |  15 ++
 drivers/leds/Makefile               |   1 +
 drivers/leds/led-class-multicolor.c |   2 +-
 drivers/leds/led-core.c             |  12 +-
 drivers/leds/leds-cros_ec.c         | 277 ++++++++++++++++++++++++++++++++++++
 drivers/leds/leds.h                 |   1 -
 drivers/mfd/cros_ec_dev.c           |   9 ++
 include/linux/leds.h                |  10 ++
 9 files changed, 328 insertions(+), 4 deletions(-)
 create mode 100644 drivers/leds/leds-cros_ec.c

-- 
Lee Jones [李琼斯]