linux-kernel - [PATCH] leds: cros_ec: Implement review comments from Lee

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240614-cros_ec-led-review-v1-1-946f2549fac2@weissschuh.net>
Date: Fri, 14 Jun 2024 12:04:29 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Thomas Weißschuh <thomas@...ssschuh.net>, 
 Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>, 
 Benson Leung <bleung@...omium.org>, Guenter Roeck <groeck@...omium.org>
Cc: linux-leds@...r.kernel.org, chrome-platform@...ts.linux.dev, 
 linux-kernel@...r.kernel.org, 
 Thomas Weißschuh <linux@...ssschuh.net>
Subject: [PATCH] leds: cros_ec: Implement review comments from Lee

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.

[0] https://lore.kernel.org/lkml/20240614093445.GF3029315@google.com/T/#m8750abdef6a968ace765645189170814196c9ce9

Signed-off-by: Thomas Weißschuh <linux@...ssschuh.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@...ssschuh.net>