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 35ab336a6093..8f487c2f68cd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5097,6 +5097,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..f87ee9c4a29a
--- /dev/null
+++ b/drivers/leds/leds-cros_ec.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  ChromesOS 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 = "cros_ec-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_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 if (led_mc_cdev->led_cdev.max_brightness == 1)
+		color = LED_COLOR_ID_MULTI;
+	else
+		color = LED_COLOR_ID_RGB;
+
+	return led_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];
+		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->hw_control_trigger = cros_ec_led_trigger.name;
+
+	led_cdev->name = devm_kasprintf(dev, GFP_KERNEL, "cros_ec:%s:%s",
+					cros_ec_led_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;
+	size_t i;
+	int ret;
+
+	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.1

On Mon, May 20, 2024 at 12:00:32PM +0200, Thomas Weißschuh wrote:
> diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c
[...]
> + *  ChromesOS EC LED Driver

s/ChromesOS/ChromeOS/.

> +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 = { };

To be neat, { } -> {}.

> +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 = { };

Ditto.

> +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;
> +		}

What if the array is [0, 1, 1]?

> +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 = { };

Ditto.

> +static int cros_ec_led_probe(struct platform_device *pdev)
> +{
[...]
> +	int ret;
> +
> +	for (i = 0; i < EC_LED_ID_COUNT; i++) {
> +		ret = cros_ec_led_probe_led(dev, cros_ec, i);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;

`ret` should be initialized in case EC_LED_ID_COUNT would be somehow 0.

> +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);

I wonder it could use module_led_trigger() and module_platform_driver().

On 2024-05-28 05:09:29+0000, Tzung-Bi Shih wrote:
> On Mon, May 20, 2024 at 12:00:32PM +0200, Thomas Weißschuh wrote:
> > diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c
> [...]
> > + *  ChromesOS EC LED Driver
> 
> s/ChromesOS/ChromeOS/.

Ack.

> > +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 = { };
> 
> To be neat, { } -> {}.

Ack.
 
> > +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 = { };
> 
> Ditto.
> 
> > +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;
> > +		}
> 
> What if the array is [0, 1, 1]?

The "0" will be skipped by 

if (!range)
	continue;

And the two "1" will pass the check.

> 
> > +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 = { };
> 
> Ditto.
> 
> > +static int cros_ec_led_probe(struct platform_device *pdev)
> > +{
> [...]
> > +	int ret;
> > +
> > +	for (i = 0; i < EC_LED_ID_COUNT; i++) {
> > +		ret = cros_ec_led_probe_led(dev, cros_ec, i);
> > +		if (ret)
> > +			break;
> > +	}
> > +
> > +	return ret;
> 
> `ret` should be initialized in case EC_LED_ID_COUNT would be somehow 0.

As that's a constant define, this should never happen.
But after thinking about it, it seems a bit clearer.
The compiler should figure out that it's redundant anyways.

> > +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);
> 
> I wonder it could use module_led_trigger() and module_platform_driver().

This won't compile as the macros generate various duplicate symbols.

Also the order is important, so I think the explicit logic is clearer.


Thomas

On Tue, May 28, 2024 at 07:25:07AM +0200, Thomas Weißschuh wrote:
> On 2024-05-28 05:09:29+0000, Tzung-Bi Shih wrote:
> > On Mon, May 20, 2024 at 12:00:32PM +0200, Thomas Weißschuh wrote:
> > > +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;
> > > +		}
> > 
> > What if the array is [0, 1, 1]?
> 
> The "0" will be skipped by 
> 
> if (!range)
> 	continue;
> 
> And the two "1" will pass the check.

Ack.

> > > +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);
> > 
> > I wonder it could use module_led_trigger() and module_platform_driver().
> 
> This won't compile as the macros generate various duplicate symbols.
> 
> Also the order is important, so I think the explicit logic is clearer.

I'm not sure if it is feasible by separating the trigger part to
drivers/leds/trigger/ and specify it in `default_trigger`.

On 2024-05-28 07:15:07+0000, Tzung-Bi Shih wrote:
> On Tue, May 28, 2024 at 07:25:07AM +0200, Thomas Weißschuh wrote:
> > On 2024-05-28 05:09:29+0000, Tzung-Bi Shih wrote:
> > > On Mon, May 20, 2024 at 12:00:32PM +0200, Thomas Weißschuh wrote:
> > > > +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);
> > > 
> > > I wonder it could use module_led_trigger() and module_platform_driver().
> > 
> > This won't compile as the macros generate various duplicate symbols.
> > 
> > Also the order is important, so I think the explicit logic is clearer.
> 
> I'm not sure if it is feasible by separating the trigger part to
> drivers/leds/trigger/ and specify it in `default_trigger`.

I don't think so.

The trigger is a private one and can only ever used with those LEDs.
(through cros_ec_led_trigger_type)

If we want to split it out we would need to export at least
cros_ec_led_trigger_type, cros_ec_led_cdev_to_priv,
cros_ec_led_cmd_arg_data, cros_ec_led_priv and more from leds-cros_ec to
the trigger.