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 <[email protected]>
---
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 <[email protected]>
+S: Maintained
+F: drivers/leds/leds-cros_ec.c
+
CHROMEOS EC SUBDRIVERS
M: Benson Leung <[email protected]>
R: Guenter Roeck <[email protected]>
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 <[email protected]>
+ */
+
+#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 <[email protected]");
+MODULE_LICENSE("GPL");
--
2.45.2
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 <[email protected]>
> ---
> 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 <[email protected]>
> +S: Maintained
> +F: drivers/leds/leds-cros_ec.c
> +
> CHROMEOS EC SUBDRIVERS
> M: Benson Leung <[email protected]>
> R: Guenter Roeck <[email protected]>
> 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 <[email protected]>
> + */
> +
> +#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 <[email protected]");
> +MODULE_LICENSE("GPL");
>
> --
> 2.45.2
>
--
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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > > +S: Maintained
> > > +F: drivers/leds/leds-cros_ec.c
> > > +
> > > CHROMEOS EC SUBDRIVERS
> > > M: Benson Leung <[email protected]>
> > > R: Guenter Roeck <[email protected]>
> > > 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 <[email protected]>
> > > + */
> > > +
> > > +#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 <[email protected]");
> > > +MODULE_LICENSE("GPL");
--
Lee Jones [李琼斯]
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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > +S: Maintained
> > +F: drivers/leds/leds-cros_ec.c
> > +
> > CHROMEOS EC SUBDRIVERS
> > M: Benson Leung <[email protected]>
> > R: Guenter Roeck <[email protected]>
> > 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 <[email protected]>
> > + */
> > +
> > +#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 <[email protected]");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.45.2
> >
>
> --
> Lee Jones [李琼斯]