Re: [PATCH] leds: cros_ec: Implement review comments from Lee

[Lee Jones <lee@kernel.org>]

Definitely not seen a commit message like that before

> Implement review comments from Lee as requested in [0] for
> "leds: Add ChromeOS EC driver".
> 
> Changes:
> * Inline DRV_NAME string constant.
> * Use dev_err() instead of dev_warn() to report errors.
> * Rename cros_ec_led_probe_led() to cros_ec_led_probe_one().
> * Make loop variable "int" where they belong.
> * Move "Unknown LED" comment to where it belongs.
> * Register trigger during _probe().
> * Use module_platform_driver() and drop all the custom boilerplate.

If you're fixing many things, then I would expect to receive many
patches.  One patch for functional change please.  If you can reasonably
group fixes of similar elk, then please do.  However one patch that does
a bunch of different things is a no-go from me, sorry.

> [0] https://lore.kernel.org/lkml/20240614093445.GF3029315@google.com/T/#m8750abdef6a968ace765645189170814196c9ce9
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/leds/leds-cros_ec.c | 50 +++++++++++++--------------------------------
>  1 file changed, 14 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c
> index 7bb21a587713..275522b81ea5 100644
> --- a/drivers/leds/leds-cros_ec.c
> +++ b/drivers/leds/leds-cros_ec.c
> @@ -14,8 +14,6 @@
>  #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,
> @@ -152,7 +150,7 @@ static int cros_ec_led_count_subleds(struct device *dev,
>  
>  		if (common_range != range) {
>  			/* The multicolor LED API expects a uniform max_brightness */
> -			dev_warn(dev, "Inconsistent LED brightness values\n");
> +			dev_err(dev, "Inconsistent LED brightness values\n");
>  			return -EINVAL;
>  		}
>  	}
> @@ -176,22 +174,21 @@ static const char *cros_ec_led_get_color_name(struct led_classdev_mc *led_mc_cde
>  	return led_get_color_name(color);
>  }
>  
> -static int cros_ec_led_probe_led(struct device *dev, struct cros_ec_device *cros_ec,
> +static int cros_ec_led_probe_one(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;
> +	int i, ret, num_subleds;
> +	size_t 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;
> +		return 0; /* Unknown LED, skip */
>  	if (ret == -EOPNOTSUPP)
>  		return -ENODEV;
>  	if (ret < 0)
> @@ -247,11 +244,14 @@ 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;
> +	int i, ret = 0;
> +
> +	ret = devm_led_trigger_register(dev, &cros_ec_led_trigger);
> +	if (ret)
> +		return ret;
>  
>  	for (i = 0; i < EC_LED_ID_COUNT; i++) {
> -		ret = cros_ec_led_probe_led(dev, cros_ec, i);
> +		ret = cros_ec_led_probe_one(dev, cros_ec, i);
>  		if (ret)
>  			break;
>  	}
> @@ -260,38 +260,16 @@ static int cros_ec_led_probe(struct platform_device *pdev)
>  }
>  
>  static const struct platform_device_id cros_ec_led_id[] = {
> -	{ DRV_NAME, 0 },
> +	{ "cros-ec-led", 0 },
>  	{}
>  };
>  
>  static struct platform_driver cros_ec_led_driver = {
> -	.driver.name	= DRV_NAME,
> +	.driver.name	= "cros-ec-led",
>  	.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_platform_driver(cros_ec_led_driver);
>  
>  MODULE_DEVICE_TABLE(platform, cros_ec_led_id);
>  MODULE_DESCRIPTION("ChromeOS EC LED Driver");
> 
> ---
> base-commit: b6774dd948171c478c7aa19318b1d7ae9cf6d7a4
> change-id: 20240614-cros_ec-led-review-ec93abc65933
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@weissschuh.net>
> 

-- 
Lee Jones [李琼斯]