Re: [PATCH v7 01/11] leds: add support for hardware driven LEDs
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
Date: 2022-12-15 16:13:37
Also in:
linux-devicetree, linux-doc, linux-leds, lkml
Hi Christian, Thanks for the patch. I think Andrew's email is offline at the moment. On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
+static bool led_trigger_is_supported(struct led_classdev *led_cdev,
+ struct led_trigger *trigger)
+{
+ switch (led_cdev->blink_mode) {
+ case SOFTWARE_CONTROLLED:
+ if (trigger->supported_blink_modes == HARDWARE_ONLY)
+ return 0;
+ break;
+ case HARDWARE_CONTROLLED:
+ if (trigger->supported_blink_modes == SOFTWARE_ONLY)
+ return 0;
+ break;
+ case SOFTWARE_HARDWARE_CONTROLLED:
+ break;
+ default:
+ return 0;
+ }
+
+ return 1;
Should be returning true/false. I'm not sure I'm a fan of the style of
this though - wouldn't the following be easier to read?
switch (led_cdev->blink_mode) {
case SOFTWARE_CONTROLLED:
return trigger->supported_blink_modes != HARDWARE_ONLY;
case HARDWARE_CONTROLLED:
return trigger->supported_blink_modes != SOFTWARE_ONLY;
case SOFTWARE_HARDWARE_CONTROLLED:
return true;
}
?
Also, does it really need a default case - without it, when the
led_blink_modes enum is expanded and the switch statement isn't
updated, we'll get a compiler warning which will prompt this to be
updated - whereas, with a default case, it won't.
quoted hunk ↗ jump to hunk
@@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig) led_set_brightness(led_cdev, LED_OFF); } if (trig) { + /* Make sure the trigger support the LED blink mode */ + if (!led_trigger_is_supported(led_cdev, trig)) + return -EINVAL;
Shouldn't validation happen before we start taking any actions? In other words, before we remove the previous trigger?
quoted hunk ↗ jump to hunk
@@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev) #define TRIG_NAME_MAX 50 +enum led_trigger_blink_supported_modes { + SOFTWARE_ONLY = SOFTWARE_CONTROLLED, + HARDWARE_ONLY = HARDWARE_CONTROLLED, + SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
I suspect all these generic names are asking for eventual namespace clashes. Maybe prefix them with LED_ ? Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!