| Message ID | 20221214235438.30271-2-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Adds support for PHY LEDs with offload triggers | expand |
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next net-next/master net/master linus/master v6.1 next-20221215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link: https://lore.kernel.org/r/20221214235438.30271-2-ansuelsmth%40gmail.com
patch subject: [PATCH v7 01/11] leds: add support for hardware driven LEDs
config: arc-randconfig-r005-20221214
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
git checkout f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/leds/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/leds/led-triggers.c: In function 'led_trigger_set':
>> drivers/leds/led-triggers.c:205:29: error: 'struct led_classdev' has no member named 'hw_control_status'
205 | led_cdev->hw_control_status(led_cdev))
| ^~
>> drivers/leds/led-triggers.c:206:33: error: 'struct led_classdev' has no member named 'hw_control_stop'
206 | led_cdev->hw_control_stop(led_cdev);
| ^~
vim +205 drivers/leds/led-triggers.c
177
178 /* Caller must ensure led_cdev->trigger_lock held */
179 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
180 {
181 char *event = NULL;
182 char *envp[2];
183 const char *name;
184 int ret;
185
186 if (!led_cdev->trigger && !trig)
187 return 0;
188
189 name = trig ? trig->name : "none";
190 event = kasprintf(GFP_KERNEL, "TRIGGER=%s", name);
191
192 /* Remove any existing trigger */
193 if (led_cdev->trigger) {
194 spin_lock(&led_cdev->trigger->leddev_list_lock);
195 list_del_rcu(&led_cdev->trig_list);
196 spin_unlock(&led_cdev->trigger->leddev_list_lock);
197
198 /* ensure it's no longer visible on the led_cdevs list */
199 synchronize_rcu();
200
201 cancel_work_sync(&led_cdev->set_brightness_work);
202 led_stop_software_blink(led_cdev);
203 /* Disable hardware mode on trigger change if supported */
204 if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
> 205 led_cdev->hw_control_status(led_cdev))
> 206 led_cdev->hw_control_stop(led_cdev);
207 if (led_cdev->trigger->deactivate)
208 led_cdev->trigger->deactivate(led_cdev);
209 device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
210 led_cdev->trigger = NULL;
211 led_cdev->trigger_data = NULL;
212 led_cdev->activated = false;
213 led_set_brightness(led_cdev, LED_OFF);
214 }
215 if (trig) {
216 /* Make sure the trigger support the LED blink mode */
217 if (!led_trigger_is_supported(led_cdev, trig))
218 return -EINVAL;
219
220 spin_lock(&trig->leddev_list_lock);
221 list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
222 spin_unlock(&trig->leddev_list_lock);
223 led_cdev->trigger = trig;
224
225 if (trig->activate)
226 ret = trig->activate(led_cdev);
227 else
228 ret = 0;
229
230 if (ret)
231 goto err_activate;
232
233 ret = device_add_groups(led_cdev->dev, trig->groups);
234 if (ret) {
235 dev_err(led_cdev->dev, "Failed to add trigger attributes\n");
236 goto err_add_groups;
237 }
238 }
239
240 if (event) {
241 envp[0] = event;
242 envp[1] = NULL;
243 if (kobject_uevent_env(&led_cdev->dev->kobj, KOBJ_CHANGE, envp))
244 dev_err(led_cdev->dev,
245 "%s: Error sending uevent\n", __func__);
246 kfree(event);
247 }
248
249 return 0;
250
251 err_add_groups:
252
253 if (trig->deactivate)
254 trig->deactivate(led_cdev);
255 err_activate:
256
257 spin_lock(&led_cdev->trigger->leddev_list_lock);
258 list_del_rcu(&led_cdev->trig_list);
259 spin_unlock(&led_cdev->trigger->leddev_list_lock);
260 synchronize_rcu();
261 led_cdev->trigger = NULL;
262 led_cdev->trigger_data = NULL;
263 led_set_brightness(led_cdev, LED_OFF);
264 kfree(event);
265
266 return ret;
267 }
268 EXPORT_SYMBOL_GPL(led_trigger_set);
269
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next net-next/master net/master linus/master v6.1 next-20221215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link: https://lore.kernel.org/r/20221214235438.30271-2-ansuelsmth%40gmail.com
patch subject: [PATCH v7 01/11] leds: add support for hardware driven LEDs
config: riscv-randconfig-r001-20221214
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 98b13979fb05f3ed288a900deb843e7b27589e58)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
git checkout f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/leds/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/leds/led-triggers.c:205:17: error: no member named 'hw_control_status' in 'struct led_classdev'
led_cdev->hw_control_status(led_cdev))
~~~~~~~~ ^
>> drivers/leds/led-triggers.c:206:14: error: no member named 'hw_control_stop' in 'struct led_classdev'
led_cdev->hw_control_stop(led_cdev);
~~~~~~~~ ^
2 errors generated.
vim +205 drivers/leds/led-triggers.c
177
178 /* Caller must ensure led_cdev->trigger_lock held */
179 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
180 {
181 char *event = NULL;
182 char *envp[2];
183 const char *name;
184 int ret;
185
186 if (!led_cdev->trigger && !trig)
187 return 0;
188
189 name = trig ? trig->name : "none";
190 event = kasprintf(GFP_KERNEL, "TRIGGER=%s", name);
191
192 /* Remove any existing trigger */
193 if (led_cdev->trigger) {
194 spin_lock(&led_cdev->trigger->leddev_list_lock);
195 list_del_rcu(&led_cdev->trig_list);
196 spin_unlock(&led_cdev->trigger->leddev_list_lock);
197
198 /* ensure it's no longer visible on the led_cdevs list */
199 synchronize_rcu();
200
201 cancel_work_sync(&led_cdev->set_brightness_work);
202 led_stop_software_blink(led_cdev);
203 /* Disable hardware mode on trigger change if supported */
204 if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
> 205 led_cdev->hw_control_status(led_cdev))
> 206 led_cdev->hw_control_stop(led_cdev);
207 if (led_cdev->trigger->deactivate)
208 led_cdev->trigger->deactivate(led_cdev);
209 device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
210 led_cdev->trigger = NULL;
211 led_cdev->trigger_data = NULL;
212 led_cdev->activated = false;
213 led_set_brightness(led_cdev, LED_OFF);
214 }
215 if (trig) {
216 /* Make sure the trigger support the LED blink mode */
217 if (!led_trigger_is_supported(led_cdev, trig))
218 return -EINVAL;
219
220 spin_lock(&trig->leddev_list_lock);
221 list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
222 spin_unlock(&trig->leddev_list_lock);
223 led_cdev->trigger = trig;
224
225 if (trig->activate)
226 ret = trig->activate(led_cdev);
227 else
228 ret = 0;
229
230 if (ret)
231 goto err_activate;
232
233 ret = device_add_groups(led_cdev->dev, trig->groups);
234 if (ret) {
235 dev_err(led_cdev->dev, "Failed to add trigger attributes\n");
236 goto err_add_groups;
237 }
238 }
239
240 if (event) {
241 envp[0] = event;
242 envp[1] = NULL;
243 if (kobject_uevent_env(&led_cdev->dev->kobj, KOBJ_CHANGE, envp))
244 dev_err(led_cdev->dev,
245 "%s: Error sending uevent\n", __func__);
246 kfree(event);
247 }
248
249 return 0;
250
251 err_add_groups:
252
253 if (trig->deactivate)
254 trig->deactivate(led_cdev);
255 err_activate:
256
257 spin_lock(&led_cdev->trigger->leddev_list_lock);
258 list_del_rcu(&led_cdev->trig_list);
259 spin_unlock(&led_cdev->trigger->leddev_list_lock);
260 synchronize_rcu();
261 led_cdev->trigger = NULL;
262 led_cdev->trigger_data = NULL;
263 led_set_brightness(led_cdev, LED_OFF);
264 kfree(event);
265
266 return ret;
267 }
268 EXPORT_SYMBOL_GPL(led_trigger_set);
269
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. > @@ -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? > @@ -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.
On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote: > Hi Christian, > > Thanks for the patch. > > I think Andrew's email is offline at the moment. > Notice by gmail spamming me "I CAN'T SEND IT AHHHHH" Holidy times I guess? > 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; > } > ? Much better! > > 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. > I added the default just to mute some compiler warning. But guess if every enum is handled the warning should not be reported. > > @@ -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? > trigger_set first remove any trigger and set the led off. Then apply the new trigger. So the validation is done only when a trigger is actually applied. Think we should understand the best case here. > > @@ -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_ ? Agree they are pretty generic so I can see why... My only concern was making them too long... Maybe reduce them to SW or HW? LEDS_SW_ONLY... LEDS_SW_CONTROLLED? > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Dec 16, 2022 at 05:45:25PM +0100, Christian Marangi wrote: > On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote: > > Hi Christian, > > > > Thanks for the patch. > > > > I think Andrew's email is offline at the moment. > > > > Notice by gmail spamming me "I CAN'T SEND IT AHHHHH" > Holidy times I guess? Sadly, Andrew's email has done this a number of times - and Andrew used to be on IRC so I could prod him about it, but it seems he doesn't hang out on IRC anymore. It's been like it a few days now. > > On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote: > > > @@ -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? > > > > trigger_set first remove any trigger and set the led off. Then apply the > new trigger. So the validation is done only when a trigger is actually > applied. Think we should understand the best case here. I think this is a question that needs to be answered by the LEDs folk, as it's an interface behaviour / quality of implementation issue. > > > @@ -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_ ? > > Agree they are pretty generic so I can see why... My only concern was > making them too long... Maybe reduce them to SW or HW? LEDS_SW_ONLY... > LEDS_SW_CONTROLLED? Seems sensible to me - and as a bonus they get shorter than the above!
On Fri, Dec 16, 2022 at 05:45:25PM +0100, Christian Marangi wrote: > On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote: > > Hi Christian, > > > > Thanks for the patch. > > > > I think Andrew's email is offline at the moment. > > > > Notice by gmail spamming me "I CAN'T SEND IT AHHHHH" > Holidy times I guess? That was part of the problem. Away travelling when foot gun hit foot, and i didn't have access to the tools to fix it while away. Andrew
On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote: > The LED must implement 3 main API: > - hw_control_status(): This asks the LED driver if hardware mode is > enabled or not. > Triggers will check if the offload mode is supported and will be > activated accordingly. If the trigger can't run in software mode, > return -EOPNOTSUPP as the blinking can't be simulated by software. > - hw_control_start(): This will simply enable the hardware mode for > the LED. > With this not declared and hw_control_status() returning true, > it's assumed that the LED is always in hardware mode. > - hw_control_stop(): This will simply disable the hardware mode for > the LED. > With this not declared and hw_control_status() returning true, > it's assumed that the LED is always in hardware mode. > It's advised to the driver to put the LED in the old state but this > is not enforcerd and putting the LED off is also accepted. > Building htmldocs with Sphinx, I got: Documentation/leds/leds-class.rst:190: WARNING: Unexpected indentation. Documentation/leds/leds-class.rst:192: WARNING: Block quote ends without a blank line; unexpected unindent. I have applied the fixup on the API bullet list above: ---- >8 ---- diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst index efd2f68c46a7f9..fc16b503747800 100644 --- a/Documentation/leds/leds-class.rst +++ b/Documentation/leds/leds-class.rst @@ -186,13 +186,15 @@ supported. By default if a LED driver doesn't declare blink_mode, SOFTWARE_CONTR is assumed. The LED must implement 3 main API: + - hw_control_status(): This asks the LED driver if hardware mode is enabled - or not. Triggers will check if the hardware mode is active and will try - to offload their triggers if supported by the driver. + or not. Triggers will check if the hardware mode is active and will try + to offload their triggers if supported by the driver. - hw_control_start(): This will simply enable the hardware mode for the LED. + - hw_control_stop(): This will simply disable the hardware mode for the LED. - It's advised to the driver to put the LED in the old state but this is not - enforcerd and putting the LED off is also accepted. + It's advised to the driver to put the LED in the old state but this is not + enforcerd and putting the LED off is also accepted. With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional and any software only trigger will reject activation as the LED supports only Thanks.
diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst index cd155ead8703..645940b78d81 100644 --- a/Documentation/leds/leds-class.rst +++ b/Documentation/leds/leds-class.rst @@ -169,6 +169,34 @@ Setting the brightness to zero with brightness_set() callback function should completely turn off the LED and cancel the previously programmed hardware blinking function, if any. +Hardware driven LEDs +=================================== + +Some LEDs can be driven by hardware (for example a LED connected to +an ethernet PHY or an ethernet switch can be configured to blink on activity on +the network, which in software is done by the netdev trigger). + +To do such offloading, LED driver must support this and a supported trigger must +be used. + +LED driver should declare the correct blink_mode supported and should set the +blink_mode parameter to one of HARDWARE_CONTROLLED or SOFTWARE_HARDWARE_CONTROLLED. +The trigger will check this option and fail to activate if the blink_mode is not +supported. By default if a LED driver doesn't declare blink_mode, SOFTWARE_CONTROLLED +is assumed. + +The LED must implement 3 main API: +- hw_control_status(): This asks the LED driver if hardware mode is enabled + or not. Triggers will check if the hardware mode is active and will try + to offload their triggers if supported by the driver. +- hw_control_start(): This will simply enable the hardware mode for the LED. +- hw_control_stop(): This will simply disable the hardware mode for the LED. + It's advised to the driver to put the LED in the old state but this is not + enforcerd and putting the LED off is also accepted. + +With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional +and any software only trigger will reject activation as the LED supports only +hardware mode. Known Issues ============ diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 499d0f215a8b..891f4821b2c8 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -49,6 +49,17 @@ config LEDS_BRIGHTNESS_HW_CHANGED See Documentation/ABI/testing/sysfs-class-led for details. +config LEDS_HARDWARE_CONTROL + bool "LED Hardware Control support" + help + This option enabled Hardware control support used by leds that + can be driven in hardware by using supported triggers. + + Hardware blink modes will be exposed by sysfs class in + /sys/class/leds based on the trigger currently active. + + If unsure, say Y. + comment "LED drivers" config LEDS_88PM860X diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 6a8ea94834fa..3516ae3c4c3c 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -164,6 +164,27 @@ static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev) } #endif +#ifdef CONFIG_LEDS_HARDWARE_CONTROL +static int led_classdev_check_blink_mode_functions(struct led_classdev *led_cdev) +{ + int mode = led_cdev->blink_mode; + + if (mode == SOFTWARE_HARDWARE_CONTROLLED && + (!led_cdev->hw_control_status || + !led_cdev->hw_control_start || + !led_cdev->hw_control_stop)) + return -EINVAL; + + if (mode == SOFTWARE_CONTROLLED && + (led_cdev->hw_control_status || + led_cdev->hw_control_start || + led_cdev->hw_control_stop)) + return -EINVAL; + + return 0; +} +#endif + /** * led_classdev_suspend - suspend an led_classdev. * @led_cdev: the led_classdev to suspend. @@ -367,6 +388,12 @@ int led_classdev_register_ext(struct device *parent, if (ret < 0) return ret; +#ifdef CONFIG_LEDS_HARDWARE_CONTROL + ret = led_classdev_check_blink_mode_functions(led_cdev); + if (ret < 0) + return ret; +#endif + mutex_init(&led_cdev->led_access); mutex_lock(&led_cdev->led_access); led_cdev->dev = device_create_with_groups(leds_class, parent, 0, diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c index 072491d3e17b..693c5d0fa980 100644 --- a/drivers/leds/led-triggers.c +++ b/drivers/leds/led-triggers.c @@ -154,6 +154,27 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj, } EXPORT_SYMBOL_GPL(led_trigger_read); +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; +} + /* Caller must ensure led_cdev->trigger_lock held */ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig) { @@ -179,6 +200,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig) cancel_work_sync(&led_cdev->set_brightness_work); led_stop_software_blink(led_cdev); + /* Disable hardware mode on trigger change if supported */ + if (led_cdev->blink_mode != SOFTWARE_CONTROLLED && + led_cdev->hw_control_status(led_cdev)) + led_cdev->hw_control_stop(led_cdev); if (led_cdev->trigger->deactivate) led_cdev->trigger->deactivate(led_cdev); device_remove_groups(led_cdev->dev, led_cdev->trigger->groups); @@ -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; + spin_lock(&trig->leddev_list_lock); list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs); spin_unlock(&trig->leddev_list_lock); diff --git a/include/linux/leds.h b/include/linux/leds.h index ba4861ec73d3..09ff1dc6f48d 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -67,6 +67,12 @@ struct led_hw_trigger_type { int dummy; }; +enum led_blink_modes { + SOFTWARE_CONTROLLED = 0x0, + HARDWARE_CONTROLLED, + SOFTWARE_HARDWARE_CONTROLLED, +}; + struct led_classdev { const char *name; unsigned int brightness; @@ -154,6 +160,32 @@ struct led_classdev { /* LEDs that have private triggers have this set */ struct led_hw_trigger_type *trigger_type; + + /* This report the supported blink_mode. The driver should report the + * correct LED capabilities. + * With this set to HARDWARE_CONTROLLED, LED is always in offload mode + * and triggers can't be simulated by software. + * If the led is HARDWARE_CONTROLLED, status/start/stop function + * are optional. + * By default SOFTWARE_CONTROLLED is set as blink_mode. + */ + enum led_blink_modes blink_mode; +#ifdef CONFIG_LEDS_HARDWARE_CONTROL + /* Ask the LED driver if hardware mode is enabled or not. + * If the option is not declared by the LED driver, SOFTWARE_CONTROLLED + * is assumed. + * Triggers will check if the hardware mode is supported and will be + * activated accordingly. If the trigger can't run in hardware mode, + * return -EOPNOTSUPP as the blinking can't be simulated by software. + */ + bool (*hw_control_status)(struct led_classdev *led_cdev); + /* Set LED in hardware mode */ + int (*hw_control_start)(struct led_classdev *led_cdev); + /* Disable hardware mode for LED. It's advised to the LED driver to put it to + * the old status but that is not mandatory and also putting it off is accepted. + */ + int (*hw_control_stop)(struct led_classdev *led_cdev); +#endif #endif #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED @@ -215,7 +247,6 @@ extern struct led_classdev *of_led_get(struct device_node *np, int index); extern void led_put(struct led_classdev *led_cdev); struct led_classdev *__must_check devm_of_led_get(struct device *dev, int index); - /** * led_blink_set - set blinking with software fallback * @led_cdev: the LED to start blinking @@ -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, +}; + struct led_trigger { /* Trigger Properties */ const char *name; int (*activate)(struct led_classdev *led_cdev); void (*deactivate)(struct led_classdev *led_cdev); + /* Declare if the Trigger supports hardware control to + * offload triggers or supports only software control. + * A trigger can also declare support for hardware control + * if its task is to only configure LED blink modes and expose + * them in sysfs. + */ + enum led_trigger_blink_supported_modes supported_blink_modes; + /* LED-private triggers have this set */ struct led_hw_trigger_type *trigger_type;
Some LEDs can be driven by hardware (for example a LED connected to an ethernet PHY or an ethernet switch can be configured to blink on activity on the network, which in software is done by the netdev trigger). To do such offloading, LED driver must support this and a supported trigger must be used. LED driver should declare the correct blink_mode supported and should set the blink_mode parameter to one of HARDWARE_CONTROLLED or SOFTWARE_HARDWARE_CONTROLLED. The trigger will check this option and fail to activate if the blink_mode is not supported. By default if a LED driver doesn't declare blink_mode, SOFTWARE_CONTROLLED is assumed. The LED must implement 3 main API: - hw_control_status(): This asks the LED driver if hardware mode is enabled or not. Triggers will check if the offload mode is supported and will be activated accordingly. If the trigger can't run in software mode, return -EOPNOTSUPP as the blinking can't be simulated by software. - hw_control_start(): This will simply enable the hardware mode for the LED. With this not declared and hw_control_status() returning true, it's assumed that the LED is always in hardware mode. - hw_control_stop(): This will simply disable the hardware mode for the LED. With this not declared and hw_control_status() returning true, it's assumed that the LED is always in hardware mode. It's advised to the driver to put the LED in the old state but this is not enforcerd and putting the LED off is also accepted. With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional and any software only trigger will reject activation as the LED supports only hardware mode. An additional config CONFIG_LEDS_HARDWARE_CONTROL is added to add support for LEDs that can be controlled by hardware. Cc: Marek Behún <kabel@kernel.org> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- Documentation/leds/leds-class.rst | 28 ++++++++++++++++++ drivers/leds/Kconfig | 11 ++++++++ drivers/leds/led-class.c | 27 ++++++++++++++++++ drivers/leds/led-triggers.c | 29 +++++++++++++++++++ include/linux/leds.h | 47 ++++++++++++++++++++++++++++++- 5 files changed, 141 insertions(+), 1 deletion(-)