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

[Tzung-Bi Shih]

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