Re: [RFC PATCH v6 02/11] leds: add function to configure hardware controlled LED
flat view
From: Andrew Lunn <andrew@lunn.ch>
Date: 2022-05-04 23:48:01
Also in:
linux-devicetree, linux-doc, linux-leds, lkml
+In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the +requested blink mode (flags) or not.
-EOPNOTSUPP might be clearer.
quoted hunk ↗ jump to hunk
+In ZERO hw_control_configure() should return 0 with success operation or error. + +The unsigned long flag is specific to the trigger and change across them. It's in the LED +driver interest know how to elaborate this flag and to declare support for a +particular trigger. For this exact reason explicit support for the specific +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode +with a not supported trigger. +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will +fail as the driver doesn't support that specific offload trigger or doesn't know +how to handle the provided flags. + Known Issues ============diff --git a/include/linux/leds.h b/include/linux/leds.h index 09ff1dc6f48d..b5aad67fecfb 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h@@ -73,6 +73,16 @@ enum led_blink_modes { SOFTWARE_HARDWARE_CONTROLLED, }; +#ifdef CONFIG_LEDS_HARDWARE_CONTROL +enum blink_mode_cmd { + BLINK_MODE_ENABLE, /* Enable the hardware blink mode */ + BLINK_MODE_DISABLE, /* Disable the hardware blink mode */ + BLINK_MODE_READ, /* Read the status of the hardware blink mode */ + BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */ + BLINK_MODE_ZERO, /* Disable any hardware blink active */ +}; +#endif
Skip the #ifdef. The enum itself takes no space if never used, and it makes the driver simpler if they always exist.
quoted hunk ↗ jump to hunk
+ struct led_classdev { const char *name; unsigned int brightness;@@ -185,6 +195,17 @@ struct led_classdev { * the old status but that is not mandatory and also putting it off is accepted. */ int (*hw_control_stop)(struct led_classdev *led_cdev); + /* This will be used to configure the various blink modes LED support in hardware + * mode. + * The LED driver require to support the active trigger and will elaborate the + * unsigned long flag and do the operation based on the provided cmd. + * Current operation are enable,disable,supported and status. + * A trigger will use this to enable or disable the asked blink mode, check the + * status of the blink mode or ask if the blink mode can run in hardware mode. + */ + int (*hw_control_configure)(struct led_classdev *led_cdev, + unsigned long flag, + enum blink_mode_cmd cmd); #endif #endif@@ -454,6 +475,24 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev) return led_cdev->trigger_data; } +#ifdef CONFIG_LEDS_HARDWARE_CONTROL +static inline bool led_trigger_blink_mode_is_supported(struct led_classdev *led_cdev, + unsigned long flag) +{ + int ret; + + /* Sanity check: make sure led support hw mode */ + if (led_cdev->blink_mode == SOFTWARE_CONTROLLED) + return false; + + ret = led_cdev->hw_control_configure(led_cdev, flag, BLINK_MODE_SUPPORTED); + if (ret > 0) + return true; + + return false; +} +#endif
Please add a version which returns false when
CONFIG_LEDS_HARDWARE_CONTROL is disabled.
Does this actually need to be an inline function?
Andrew