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().
