bluetooth: core: RfC - add basic LED triggers
Subsystems (2)
PATCH v3 1/1 bluetooth: core: RfC - add basic LED triggers
In the bluetooth subsystem I miss the option to use LEDs to indicate the status. This patch is loosely based on the LED triggers in mac80211 and adds a basic LED trigger (powered up) plus a config option to enable the BT LED triggers.
It works fine on my system however I don't know the bluetooth stack in too much detail and therefore appreciate any comment.
---
v2:
- completely rewrote the code to be as generic as possible and minimize the changes to struct hci_dev
- make the details of the LED triggers opaque to struct hci_dev
- avoid using ifdef CONFIG_BT_LEDS in hci_core.h
- start with the radio trigger only
- factor out the generic basic led trigger code to facilitate adding further basic LED triggers
- fix indentation in Kconfig
- rename led.[ch] to leds.[ch]
v3:
- use term power instead of radio
- rename hci_led_ to hci_leds_
- remove the counter
- use IS_ENABLED for checking the config option
--- include/net/bluetooth/hci_core.h | 3 ++ net/bluetooth/Kconfig | 9 +++++ net/bluetooth/Makefile | 1 + net/bluetooth/hci_core.c | 8 ++++ net/bluetooth/leds.c | 83 ++++++++++++++++++++++++++++++++++++++++ net/bluetooth/leds.h | 18 +++++++++ 6 files changed, 122 insertions(+) create mode 100644 net/bluetooth/leds.c create mode 100644 net/bluetooth/leds.h
include/net/bluetooth/hci_core.h +3 -0
@@ -25,6 +25,7 @@#ifndef __HCI_CORE_H#define __HCI_CORE_H +#include <linux/leds.h> #include <net/bluetooth/hci.h> #include <net/bluetooth/hci_sock.h>
@@ -395,6 +396,8 @@ struct hci_dev {struct delayed_work rpa_expired;bdaddr_t rpa; +struct led_trigger *power_led; +int (*open)(struct hci_dev *hdev);int (*close)(struct hci_dev *hdev);int (*flush)(struct hci_dev *hdev);
net/bluetooth/Kconfig +9 -0
@@ -69,6 +69,15 @@ config BT_6LOWPAN help IPv6 compression over Bluetooth Low Energy. +config BT_LEDS + bool "Enable LED triggers" + depends on BT + depends on LEDS_CLASS + select LEDS_TRIGGERS + help + This option selects a few LED triggers for different + Bluetooth events. + config BT_SELFTEST bool "Bluetooth self testing support" depends on BT && DEBUG_KERNEL
net/bluetooth/Makefile +1 -0
@@ -17,6 +17,7 @@ bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \ bluetooth-$(CONFIG_BT_BREDR) += sco.o bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o +bluetooth-$(CONFIG_BT_LEDS) += leds.o bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o bluetooth-$(CONFIG_BT_SELFTEST) += selftest.o
net/bluetooth/hci_core.c +8 -0
@@ -40,6 +40,7 @@#include "hci_request.h" #include "hci_debugfs.h" #include "smp.h" +#include "leds.h" static void hci_rx_work (struct work_struct *work);static void hci_cmd_work (struct work_struct *work);
@@ -1395,6 +1396,7 @@ static int hci_dev_do_open(struct hci_dev *hdev)hci_dev_set_flag (hdev,HCI_RPA_EXPIRED );set_bit (HCI_UP , &hdev->flags);hci_sock_dev_event (hdev,HCI_DEV_UP ); +hci_leds_update_powered (hdev, true); if (!hci_dev_test_flag(hdev, HCI_SETUP) && !hci_dev_test_flag(hdev, HCI_CONFIG) && !hci_dev_test_flag(hdev, HCI_UNCONFIGURED) &&
@@ -1532,6 +1534,8 @@ int hci_dev_do_close(struct hci_dev *hdev)return 0 ; } +hci_leds_update_powered (hdev, false); +/* Flush RX and TX works */ flush_work (&hdev->tx_work);flush_work (&hdev->rx_work);
@@ -3067,6 +3071,8 @@ int hci_register_dev(struct hci_dev *hdev)if (error <0 ) goto err_wqueue; +hci_leds_init (hdev); + hdev->rfkill =rfkill_alloc (hdev->name, &hdev->dev,RFKILL_TYPE_BLUETOOTH , &hci_rfkill_ops, hdev);
@@ -3128,6 +3134,8 @@ void hci_unregister_dev(struct hci_dev *hdev) id = hdev->id; +hci_leds_exit (hdev); +write_lock (&hci_dev_list_lock);list_del (&hdev->list);write_unlock (&hci_dev_list_lock);
net/bluetooth/leds.c new file +83 -0
83 lines not on this page · raw message
net/bluetooth/leds.h new file +18 -0
@@ -0,0 +1,18 @@ +/* +* Copyright 2015, Heiner Kallweit <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/ + +#if IS_ENABLED (CONFIG_BT_LEDS ) +void hci_leds_update_powered (struct hci_dev *hdev,bool enabled); +void hci_leds_init (struct hci_dev *hdev); +void hci_leds_exit (struct hci_dev *hdev); +#else +static inline void hci_leds_update_powered (struct hci_dev *hdev, +bool enabled) {} +static inline void hci_leds_init (struct hci_dev *hdev) {} +static inline void hci_leds_exit (struct hci_dev *hdev) {} +#endif
Signature · 1 line
2.6.4
Marcel Holtmann
maintainerHi Heiner,
please use a proper subject line.
Quoted 7 lines
In the bluetooth subsystem I miss the option to use LEDs to indicate the status. This patch is loosely based on the LED triggers in mac80211 and adds a basic LED trigger (powered up) plus a config option to enable the BT LED triggers.
It works fine on my system however I don't know the bluetooth stack in too much detail and therefore appreciate any comment.
And I need a useful commit message. This is not suitable.
Quoted 154 lines
Signed-off-by Heiner Kallweit <[email protected]>---
v2:
- completely rewrote the code to be as generic as possible and minimize the changes to structhci_dev
- make the details of the LED triggers opaque to structhci_dev
- avoid using ifdefCONFIG_BT_LEDSinhci_core.h
- start with the radio trigger only
- factor out the generic basic led trigger code to facilitate adding further basic LED triggers
- fix indentation inKconfig
- rename led.[ch] to leds.[ch]
v3:
- use term power instead of radio
- renamehci_led_tohci_leds_
- remove the counter
- useIS_ENABLEDfor checking the config option
---include/net/bluetooth/hci_core.h | 3 ++ net/bluetooth/Kconfig | 9 +++++ net/bluetooth/Makefile | 1 + net/bluetooth/hci_core.c | 8 ++++ net/bluetooth/leds.c | 83 ++++++++++++++++++++++++++++++++++++++++ net/bluetooth/leds.h | 18 +++++++++ 6 files changed, 122 insertions(+)create mode 100644
net/bluetooth/leds.c
create mode 100644net/bluetooth/leds.hinclude/net/bluetooth/hci_core.h +3
-0@@ -25,6 +25,7 @@#ifndef __HCI_CORE_H#define __HCI_CORE_H +#include <linux/leds.h> #include <net/bluetooth/hci.h> #include <net/bluetooth/hci_sock.h> @@ -395,6 +396,8 @@ struct hci_dev {struct delayed_work rpa_expired;bdaddr_t rpa; +struct led_trigger *power_led; +int (*open)(struct hci_dev *hdev);int (*close)(struct hci_dev *hdev);int (*flush)(struct hci_dev *hdev);net/bluetooth/Kconfig +9
-0@@ -69,6 +69,15 @@ config BT_6LOWPAN help IPv6 compression over Bluetooth Low Energy. +config BT_LEDS + bool "Enable LED triggers" + depends on BT + depends on LEDS_CLASS + select LEDS_TRIGGERS + help + This option selects a few LED triggers for different + Bluetooth events. +config
BT_SELFTESTbool "Bluetooth self testing support" depends on BT && DEBUG_KERNELnet/bluetooth/Makefile +0
-0@@ -17,6 +17,7 @@ bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \bluetooth-$(CONFIG_BT_BREDR) += sco.o
bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
+bluetooth-$(CONFIG_BT_LEDS) += leds.o
bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o
bluetooth-$(CONFIG_BT_SELFTEST) += selftest.onet/bluetooth/hci_core.c +8
-0@@ -40,6 +40,7 @@#include "hci_request.h" #include "hci_debugfs.h" #include "smp.h" +#include "leds.h" static void hci_rx_work (struct work_struct *work);static void hci_cmd_work (struct work_struct *work);@@ -1395,6 +1396,7 @@ static int hci_dev_do_open(struct hci_dev *hdev)hci_dev_set_flag (hdev,HCI_RPA_EXPIRED );set_bit (HCI_UP , &hdev->flags);hci_sock_dev_event (hdev,HCI_DEV_UP ); +hci_leds_update_powered (hdev, true); if (!hci_dev_test_flag(hdev, HCI_SETUP) && !hci_dev_test_flag(hdev, HCI_CONFIG) && !hci_dev_test_flag(hdev, HCI_UNCONFIGURED) &&@@ -1532,6 +1534,8 @@ int hci_dev_do_close(struct hci_dev *hdev)return 0 ; } +hci_leds_update_powered (hdev, false); +/* Flush RX and TX works */ flush_work (&hdev->tx_work);flush_work (&hdev->rx_work);@@ -3067,6 +3071,8 @@ int hci_register_dev(struct hci_dev *hdev)if (error <0 ) goto err_wqueue; +hci_leds_init (hdev); + hdev->rfkill =rfkill_alloc (hdev->name, &hdev->dev,RFKILL_TYPE_BLUETOOTH , &hci_rfkill_ops, hdev);@@ -3128,6 +3134,8 @@ void hci_unregister_dev(struct hci_dev *hdev) id = hdev->id; +hci_leds_exit (hdev); +write_lock (&hci_dev_list_lock);list_del (&hdev->list);write_unlock (&hci_dev_list_lock);net/bluetooth/leds.c new file +19
-0@@ -0,0 +1,83 @@ +/* +* Copyright 2015, Heiner Kallweit <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/ + +#include <net/bluetooth/bluetooth.h> +#include <net/bluetooth/hci_core.h> + +#include "leds.h" + +struct hci_basic_led_trigger { +struct led_trigger led_trigger; +struct hci_dev *hdev; +}; + +#define to_hci_basic_led_trigger (arg) container_of(arg, \
net/bluetooth/leds.c line 29
Quoted 10 lines
net/bluetooth/leds.c new file +10
-0+ struct hci_basic_led_trigger, led_trigger) + +void hci_leds_update_powered(struct hci_dev *hdev, bool enabled) +{ +if (hdev->power_led) +led_trigger_event (hdev->power_led, + enabled ?LED_FULL :LED_OFF ); +} + +static void hci_power_led_activate (struct led_classdev *led_cdev)
Since this function is local to this file, maybe shortening to power_activate is enough.
net/bluetooth/leds.c line 42
Quoted 13 lines
net/bluetooth/leds.c new file +13
-0+{ +struct hci_basic_led_trigger *htrig; +bool powered; + + htrig =to_hci_basic_led_trigger (led_cdev->trigger); + powered =test_bit (HCI_UP , &htrig->hdev->flags); + +led_trigger_event (led_cdev->trigger, powered ?LED_FULL :LED_OFF ); +} + +static struct led_trigger *hci_basic_led_allocate (struct hci_dev *hdev, +void (*activate)(struct led_classdev *led_cdev), +const char *name)
Same here. I think using led_alloc_basic is enough.
Quoted 31 lines
net/bluetooth/leds.c new file +31
-0+{ +struct hci_basic_led_trigger *htrig; + + htrig =devm_kzalloc (&hdev->dev,sizeof (*htrig),GFP_KERNEL ); +if (!htrig) +return NULL ; + + htrig->hdev = hdev; + htrig->led_trigger.activate = activate; + htrig->led_trigger.name =devm_kasprintf (&hdev->dev,GFP_KERNEL , +"%s-%s" , hdev->name, + name); +if (!htrig->led_trigger.name) + goto err_alloc; + +if (led_trigger_register (&htrig->led_trigger)) + goto err_register; + +return &htrig->led_trigger; + +err_register: +devm_kfree (&hdev->dev, (void *)htrig->led_trigger.name); +err_alloc: +devm_kfree (&hdev->dev, htrig); +return NULL ; +} + +void hci_leds_init (struct hci_dev *hdev) +{ +/* initialize power_led */ + hdev->power_led = hci_basic_led_allocate(hdev,
net/bluetooth/leds.c line 83
Quoted 10 lines
net/bluetooth/leds.c new file +10
-0+ hci_power_led_activate, + "power"); +} + +void hci_leds_exit (struct hci_dev *hdev) +{ +if (hdev->power_led) +led_trigger_unregister (hdev->power_led); +} +
You have an extra empty line at the end of the file. Please remove that.
Quoted 14 lines
net/bluetooth/leds.h new file +8
-0@@ -0,0 +1,18 @@ +/* +* Copyright 2015, Heiner Kallweit <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/ +
net/bluetooth/leds.h line 18
Quoted 10 lines
net/bluetooth/leds.h new file +10
-0+#if IS_ENABLED (CONFIG_BT_LEDS ) +void hci_leds_update_powered (struct hci_dev *hdev,bool enabled); +void hci_leds_init (struct hci_dev *hdev); +void hci_leds_exit (struct hci_dev *hdev); +#else +static inline void hci_leds_update_powered (struct hci_dev *hdev, +bool enabled) {} +static inline void hci_leds_init (struct hci_dev *hdev) {} +static inline void hci_leds_exit (struct hci_dev *hdev) {} +#endif
Regards
Marcel