[PATCH v2] Bluetooth: Add LED triggers for HCI frames tx and rx

[PATCH v2] Bluetooth: Add LED triggers for HCI frames tx and rx

[Guodong Xu]

Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
packets available in tx or rx, the LEDs will blink.

For each hci registration, two triggers are added into LED subsystem:
[hdev->name]-tx and [hdev-name]-rx.
Refer to Documentation/leds/leds-class.txt for usage.

Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
WL1835 WiFi/BT combo chip.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         |  6 ++++++
 net/bluetooth/leds.c             | 17 +++++++++++++++++
 net/bluetooth/leds.h             |  2 ++
 4 files changed, 26 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index dc71473..37b8dd9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -398,6 +398,7 @@ struct hci_dev {
 	bdaddr_t		rpa;
 
 	struct led_trigger	*power_led;
+	struct led_trigger	*tx_led, *rx_led;
 
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 45a9fc6..956dce1 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3833,6 +3833,7 @@ static void hci_sched_acl(struct hci_dev *hdev)
 	if (!hci_conn_num(hdev, AMP_LINK) && hdev->dev_type == HCI_AMP)
 		return;
 
+	hci_leds_blink_oneshot(hdev->tx_led);
 	switch (hdev->flow_ctl_mode) {
 	case HCI_FLOW_CTL_MODE_PACKET_BASED:
 		hci_sched_acl_pkt(hdev);
@@ -3856,6 +3857,7 @@ static void hci_sched_sco(struct hci_dev *hdev)
 	if (!hci_conn_num(hdev, SCO_LINK))
 		return;
 
+	hci_leds_blink_oneshot(hdev->tx_led);
 	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
 			BT_DBG("skb %p len %d", skb, skb->len);
@@ -3879,6 +3881,7 @@ static void hci_sched_esco(struct hci_dev *hdev)
 	if (!hci_conn_num(hdev, ESCO_LINK))
 		return;
 
+	hci_leds_blink_oneshot(hdev->tx_led);
 	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK,
 						     &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
@@ -3911,6 +3914,7 @@ static void hci_sched_le(struct hci_dev *hdev)
 			hci_link_tx_to(hdev, LE_LINK);
 	}
 
+	hci_leds_blink_oneshot(hdev->tx_led);
 	cnt = hdev->le_pkts ? hdev->le_cnt : hdev->acl_cnt;
 	tmp = cnt;
 	while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, &quote))) {
@@ -3990,6 +3994,7 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (conn) {
 		hci_conn_enter_active_mode(conn, BT_POWER_FORCE_ACTIVE_OFF);
+		hci_leds_blink_oneshot(hdev->rx_led);
 
 		/* Send to upper protocol */
 		l2cap_recv_acldata(conn, skb, flags);
@@ -4022,6 +4027,7 @@ static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_dev_unlock(hdev);
 
 	if (conn) {
+		hci_leds_blink_oneshot(hdev->rx_led);
 		/* Send to upper protocol */
 		sco_recv_scodata(conn, skb);
 		return;
diff --git a/net/bluetooth/leds.c b/net/bluetooth/leds.c
index 8319c84..ae10c5d 100644
--- a/net/bluetooth/leds.c
+++ b/net/bluetooth/leds.c
@@ -19,6 +19,8 @@ struct hci_basic_led_trigger {
 #define to_hci_basic_led_trigger(arg) container_of(arg, \
 			struct hci_basic_led_trigger, led_trigger)
 
+#define BLUETOOTH_BLINK_DELAY	50 /* ms */
+
 void hci_leds_update_powered(struct hci_dev *hdev, bool enabled)
 {
 	if (hdev->power_led)
@@ -37,6 +39,17 @@ static void power_activate(struct led_classdev *led_cdev)
 	led_trigger_event(led_cdev->trigger, powered ? LED_FULL : LED_OFF);
 }
 
+void hci_leds_blink_oneshot(struct led_trigger *trig)
+{
+	unsigned long led_delay = BLUETOOTH_BLINK_DELAY;
+
+	if (!trig)
+		return;
+
+	BT_DBG("led_trig %p", trig);
+	led_trigger_blink_oneshot(trig, &led_delay, &led_delay, 0);
+}
+
 static struct led_trigger *led_allocate_basic(struct hci_dev *hdev,
 			void (*activate)(struct led_classdev *led_cdev),
 			const char *name)
@@ -71,4 +84,8 @@ void hci_leds_init(struct hci_dev *hdev)
 {
 	/* initialize power_led */
 	hdev->power_led = led_allocate_basic(hdev, power_activate, "power");
+	/* initialize tx_led */
+	hdev->tx_led = led_allocate_basic(hdev, NULL, "tx");
+	/* initialize rx_led */
+	hdev->rx_led = led_allocate_basic(hdev, NULL, "rx");
 }
diff --git a/net/bluetooth/leds.h b/net/bluetooth/leds.h
index a9c4d6e..9b1cccd 100644
--- a/net/bluetooth/leds.h
+++ b/net/bluetooth/leds.h
@@ -9,8 +9,10 @@
 #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_blink_oneshot(struct led_trigger *trig);
 #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_blink_oneshot(struct led_trigger *trig) {}
 #endif
-- 
1.9.1

Re: [PATCH v2] Bluetooth: Add LED triggers for HCI frames tx and rx

[Guodong Xu]

Ping. :)  Would you give me some review opinions on this?

In this revision, I chose to limit LED blinking to tx/rx traffic
packets only. For other types of over-the-air packets, like scanning,
it's not covered.

Thank you.

-Guodong


On 31 July 2016 at 12:24, Guodong Xu <guodong.xu@linaro.org> wrote:
> Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
> packets available in tx or rx, the LEDs will blink.
>
> For each hci registration, two triggers are added into LED subsystem:
> [hdev->name]-tx and [hdev-name]-rx.
> Refer to Documentation/leds/leds-class.txt for usage.
>
> Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
> WL1835 WiFi/BT combo chip.
>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
>  include/net/bluetooth/hci_core.h |  1 +
>  net/bluetooth/hci_core.c         |  6 ++++++
>  net/bluetooth/leds.c             | 17 +++++++++++++++++
>  net/bluetooth/leds.h             |  2 ++
>  4 files changed, 26 insertions(+)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index dc71473..37b8dd9 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -398,6 +398,7 @@ struct hci_dev {
>         bdaddr_t                rpa;
>
>         struct led_trigger      *power_led;
> +       struct led_trigger      *tx_led, *rx_led;
>
>         int (*open)(struct hci_dev *hdev);
>         int (*close)(struct hci_dev *hdev);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 45a9fc6..956dce1 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3833,6 +3833,7 @@ static void hci_sched_acl(struct hci_dev *hdev)
>         if (!hci_conn_num(hdev, AMP_LINK) && hdev->dev_type == HCI_AMP)
>                 return;
>
> +       hci_leds_blink_oneshot(hdev->tx_led);
>         switch (hdev->flow_ctl_mode) {
>         case HCI_FLOW_CTL_MODE_PACKET_BASED:
>                 hci_sched_acl_pkt(hdev);
> @@ -3856,6 +3857,7 @@ static void hci_sched_sco(struct hci_dev *hdev)
>         if (!hci_conn_num(hdev, SCO_LINK))
>                 return;
>
> +       hci_leds_blink_oneshot(hdev->tx_led);
>         while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
>                 while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
>                         BT_DBG("skb %p len %d", skb, skb->len);
> @@ -3879,6 +3881,7 @@ static void hci_sched_esco(struct hci_dev *hdev)
>         if (!hci_conn_num(hdev, ESCO_LINK))
>                 return;
>
> +       hci_leds_blink_oneshot(hdev->tx_led);
>         while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK,
>                                                      &quote))) {
>                 while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> @@ -3911,6 +3914,7 @@ static void hci_sched_le(struct hci_dev *hdev)
>                         hci_link_tx_to(hdev, LE_LINK);
>         }
>
> +       hci_leds_blink_oneshot(hdev->tx_led);
>         cnt = hdev->le_pkts ? hdev->le_cnt : hdev->acl_cnt;
>         tmp = cnt;
>         while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, &quote))) {
> @@ -3990,6 +3994,7 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb)
>
>         if (conn) {
>                 hci_conn_enter_active_mode(conn, BT_POWER_FORCE_ACTIVE_OFF);
> +               hci_leds_blink_oneshot(hdev->rx_led);
>
>                 /* Send to upper protocol */
>                 l2cap_recv_acldata(conn, skb, flags);
> @@ -4022,6 +4027,7 @@ static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
>         hci_dev_unlock(hdev);
>
>         if (conn) {
> +               hci_leds_blink_oneshot(hdev->rx_led);
>                 /* Send to upper protocol */
>                 sco_recv_scodata(conn, skb);
>                 return;
> diff --git a/net/bluetooth/leds.c b/net/bluetooth/leds.c
> index 8319c84..ae10c5d 100644
> --- a/net/bluetooth/leds.c
> +++ b/net/bluetooth/leds.c
> @@ -19,6 +19,8 @@ struct hci_basic_led_trigger {
>  #define to_hci_basic_led_trigger(arg) container_of(arg, \
>                         struct hci_basic_led_trigger, led_trigger)
>
> +#define BLUETOOTH_BLINK_DELAY  50 /* ms */
> +
>  void hci_leds_update_powered(struct hci_dev *hdev, bool enabled)
>  {
>         if (hdev->power_led)
> @@ -37,6 +39,17 @@ static void power_activate(struct led_classdev *led_cdev)
>         led_trigger_event(led_cdev->trigger, powered ? LED_FULL : LED_OFF);
>  }
>
> +void hci_leds_blink_oneshot(struct led_trigger *trig)
> +{
> +       unsigned long led_delay = BLUETOOTH_BLINK_DELAY;
> +
> +       if (!trig)
> +               return;
> +
> +       BT_DBG("led_trig %p", trig);
> +       led_trigger_blink_oneshot(trig, &led_delay, &led_delay, 0);
> +}
> +
>  static struct led_trigger *led_allocate_basic(struct hci_dev *hdev,
>                         void (*activate)(struct led_classdev *led_cdev),
>                         const char *name)
> @@ -71,4 +84,8 @@ void hci_leds_init(struct hci_dev *hdev)
>  {
>         /* initialize power_led */
>         hdev->power_led = led_allocate_basic(hdev, power_activate, "power");
> +       /* initialize tx_led */
> +       hdev->tx_led = led_allocate_basic(hdev, NULL, "tx");
> +       /* initialize rx_led */
> +       hdev->rx_led = led_allocate_basic(hdev, NULL, "rx");
>  }
> diff --git a/net/bluetooth/leds.h b/net/bluetooth/leds.h
> index a9c4d6e..9b1cccd 100644
> --- a/net/bluetooth/leds.h
> +++ b/net/bluetooth/leds.h
> @@ -9,8 +9,10 @@
>  #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_blink_oneshot(struct led_trigger *trig);
>  #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_blink_oneshot(struct led_trigger *trig) {}
>  #endif
> --
> 1.9.1
>

Re: [PATCH v2] Bluetooth: Add LED triggers for HCI frames tx and rx

[Marcel Holtmann]

Hi Guodong,

> Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
> packets available in tx or rx, the LEDs will blink.
> 
> For each hci registration, two triggers are added into LED subsystem:
> [hdev->name]-tx and [hdev-name]-rx.
> Refer to Documentation/leds/leds-class.txt for usage.
> 
> Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
> WL1835 WiFi/BT combo chip.

so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers. If we then maybe add another trigger, then this number just goes up and up.

As far as I can tell you can only assign a single trigger to a LED. So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.

Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.

So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?

Regards

Marcel

Re: [PATCH v2] Bluetooth: Add LED triggers for HCI frames tx and rx

[Guodong Xu]

Hi, Marcel

On 16 August 2016 at 14:03, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Guodong,
>
>> Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
>> packets available in tx or rx, the LEDs will blink.
>>
>> For each hci registration, two triggers are added into LED subsystem:
>> [hdev->name]-tx and [hdev-name]-rx.
>> Refer to Documentation/leds/leds-class.txt for usage.
>>
>> Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
>> WL1835 WiFi/BT combo chip.
>
> so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers.
>

True, 6 triggers. But, taking example for other subsytems, eg. cpu
cores. On my board, I have "heartbeat cpu0 cpu1 cpu2 cpu3 cpu4 cpu5
cpu6 cpu7". It doesn't have to mean you need all of them connected to
some LED(s). Actually, in most of the case, I only need heartbeat.



> If we then maybe add another trigger, then this number just goes up and up.
>
> As far as I can tell you can only assign a single trigger to a LED.
>

That's true. And people got a choice of which feature he wants to visualize.

> So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.
>
> Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.
>
> So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?
>

When I starting this work, I referred to WiFi system. See
CONFIG_MAC80211_LEDS. WiFi system implements these types of triggers "
phy0rx phy0tx phy0assoc phy0radio" for each 'controller'.

Besides, there are also RFKILL which stands for WiFi/BT power status.
RFKILL adds triggers for each module too. Eg. in the below example, I
have one WiFi (phy0), one BT (hci0). Trigger rfkill1 equals to
hci0-power.

Ref: here are all LED triggers I found in my 96boards/HiKey:

# cat trigger
none kbd-scrollock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock
kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock
kbd-ctrlllock kbd-ctrlrlock mmc0 mmc1 heartbeat cpu0 cpu1 cpu2 cpu3
cpu4 cpu5 cpu6 cpu7 mmc2 rfkill0 phy0rx phy0tx phy0assoc phy0radio
hci0-power hci0-tx [hci0-rx] rfkill1

-Guodong

> Regards
>
> Marcel
>

Re: [PATCH v2] Bluetooth: Add LED triggers for HCI frames tx and rx

[Marcel Holtmann]

Hi Guodong,

>>> Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
>>> packets available in tx or rx, the LEDs will blink.
>>> 
>>> For each hci registration, two triggers are added into LED subsystem:
>>> [hdev->name]-tx and [hdev-name]-rx.
>>> Refer to Documentation/leds/leds-class.txt for usage.
>>> 
>>> Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
>>> WL1835 WiFi/BT combo chip.
>> 
>> so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers.
>> 
> 
> True, 6 triggers. But, taking example for other subsytems, eg. cpu
> cores. On my board, I have "heartbeat cpu0 cpu1 cpu2 cpu3 cpu4 cpu5
> cpu6 cpu7". It doesn't have to mean you need all of them connected to
> some LED(s). Actually, in most of the case, I only need heartbeat.
> 
> 
> 
>> If we then maybe add another trigger, then this number just goes up and up.
>> 
>> As far as I can tell you can only assign a single trigger to a LED.
>> 
> 
> That's true. And people got a choice of which feature he wants to visualize.

and as a result we keep adding senseless triggers to the kernel and bloating it up for no reason. Especially since it feels like 99% of the LED triggers are not used at all. This makes no sense to me.

>> So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.
>> 
>> Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.
>> 
>> So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?
>> 
> 
> When I starting this work, I referred to WiFi system. See
> CONFIG_MAC80211_LEDS. WiFi system implements these types of triggers "
> phy0rx phy0tx phy0assoc phy0radio" for each 'controller'.

And I actually wonder who ever used these triggers. You need 4 LEDs to visualize the WiFi status. Which systems has 4 LEDs to spare to visualize this.

> Besides, there are also RFKILL which stands for WiFi/BT power status.
> RFKILL adds triggers for each module too. Eg. in the below example, I
> have one WiFi (phy0), one BT (hci0). Trigger rfkill1 equals to
> hci0-power.
> 
> Ref: here are all LED triggers I found in my 96boards/HiKey:
> 
> # cat trigger
> none kbd-scrollock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock
> kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock
> kbd-ctrlllock kbd-ctrlrlock mmc0 mmc1 heartbeat cpu0 cpu1 cpu2 cpu3
> cpu4 cpu5 cpu6 cpu7 mmc2 rfkill0 phy0rx phy0tx phy0assoc phy0radio
> hci0-power hci0-tx [hci0-rx] rfkill1

And how many LEDs do you have in the your system? I think you are making my point here.

So I think what we need to do is to not add to this madness and instead create one "bluetooth" LED trigger that combines power and TX/RX for all controllers. And then allow for individual "bluetooth-hci0" LED triggers so that you can bind a single Bluetooth controller to a single LED.

For me, if I can not combine hci0-power, hci0-tx and hci0-rx into a single LED, it becomes utterly useless on pretty much every system that is out there.

Regards

Marcel

Re: [PATCH v2] Bluetooth: Add LED triggers for HCI frames tx and rx

[Guodong Xu]

On 16 August 2016 at 20:33, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Guodong,
>
> >>> Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
> >>> packets available in tx or rx, the LEDs will blink.
> >>>
> >>> For each hci registration, two triggers are added into LED subsystem:
> >>> [hdev->name]-tx and [hdev-name]-rx.
> >>> Refer to Documentation/leds/leds-class.txt for usage.
> >>>
> >>> Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
> >>> WL1835 WiFi/BT combo chip.
> >>
> >> so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers.
> >>
> >
> > True, 6 triggers. But, taking example for other subsytems, eg. cpu
> > cores. On my board, I have "heartbeat cpu0 cpu1 cpu2 cpu3 cpu4 cpu5
> > cpu6 cpu7". It doesn't have to mean you need all of them connected to
> > some LED(s). Actually, in most of the case, I only need heartbeat.
> >
> >
> >
> >> If we then maybe add another trigger, then this number just goes up and up.
> >>
> >> As far as I can tell you can only assign a single trigger to a LED.
> >>
> >
> > That's true. And people got a choice of which feature he wants to visualize.
>
> and as a result we keep adding senseless triggers to the kernel and bloating it up for no reason. Especially since it feels like 99% of the LED triggers are not used at all. This makes no sense to me.
>
> >> So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.
> >>
> >> Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.
> >>
> >> So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?
> >>
> >
> > When I starting this work, I referred to WiFi system. See
> > CONFIG_MAC80211_LEDS. WiFi system implements these types of triggers "
> > phy0rx phy0tx phy0assoc phy0radio" for each 'controller'.
>
> And I actually wonder who ever used these triggers. You need 4 LEDs to visualize the WiFi status. Which systems has 4 LEDs to spare to visualize this.
>
> > Besides, there are also RFKILL which stands for WiFi/BT power status.
> > RFKILL adds triggers for each module too. Eg. in the below example, I
> > have one WiFi (phy0), one BT (hci0). Trigger rfkill1 equals to
> > hci0-power.
> >
> > Ref: here are all LED triggers I found in my 96boards/HiKey:
> >
> > # cat trigger
> > none kbd-scrollock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock
> > kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock
> > kbd-ctrlllock kbd-ctrlrlock mmc0 mmc1 heartbeat cpu0 cpu1 cpu2 cpu3
> > cpu4 cpu5 cpu6 cpu7 mmc2 rfkill0 phy0rx phy0tx phy0assoc phy0radio
> > hci0-power hci0-tx [hci0-rx] rfkill1
>
> And how many LEDs do you have in the your system? I think you are making my point here.
>
> So I think what we need to do is to not add to this madness and instead create one "bluetooth" LED trigger that combines power and TX/RX for all controllers. And then allow for individual "bluetooth-hci0" LED triggers so that you can bind a single Bluetooth controller to a single LED.
>
> For me, if I can not combine hci0-power, hci0-tx and hci0-rx into a single LED,

By combining them into a single LED, do you mean such a use case?
 - when hci0 is powered on, this LED starts on.
 - then, when there is tx/rx traffic, this LED should blink (reversely
of course).
 - when hci0 is powered off, this LED turns off.

-Guodong

> it becomes utterly useless on pretty much every system that is out there.
>
> Regards
>
> Marcel
>

Re: [PATCH v2] Bluetooth: Add LED triggers for HCI frames tx and rx

[Marcel Holtmann]

Hi Guodong,

>>>>> Two LED triggers are added into hci_dev: tx_led and rx_led. Upon ACL/SCO
>>>>> packets available in tx or rx, the LEDs will blink.
>>>>> 
>>>>> For each hci registration, two triggers are added into LED subsystem:
>>>>> [hdev->name]-tx and [hdev-name]-rx.
>>>>> Refer to Documentation/leds/leds-class.txt for usage.
>>>>> 
>>>>> Verified on HiKey 96boards, which uses HiSilicon hi6220 SoC and TI
>>>>> WL1835 WiFi/BT combo chip.
>>>> 
>>>> so I have no idea what to do with adding adding hci0-rx and hci0-tx triggers. Combined with hci0-power trigger these are already 3 triggers. And if you have 2 Bluetooth controllers in your system, then you have 6 triggers.
>>>> 
>>> 
>>> True, 6 triggers. But, taking example for other subsytems, eg. cpu
>>> cores. On my board, I have "heartbeat cpu0 cpu1 cpu2 cpu3 cpu4 cpu5
>>> cpu6 cpu7". It doesn't have to mean you need all of them connected to
>>> some LED(s). Actually, in most of the case, I only need heartbeat.
>>> 
>>> 
>>> 
>>>> If we then maybe add another trigger, then this number just goes up and up.
>>>> 
>>>> As far as I can tell you can only assign a single trigger to a LED.
>>>> 
>>> 
>>> That's true. And people got a choice of which feature he wants to visualize.
>> 
>> and as a result we keep adding senseless triggers to the kernel and bloating it up for no reason. Especially since it feels like 99% of the LED triggers are not used at all. This makes no sense to me.
>> 
>>>> So this means to even use these triggers, you need now 3 LEDs per Bluetooth controller. How is that useful for anybody in a real system? Maybe I am missing something here and somehow there is magic to combine triggers, but I have not found it yet. So please someone enlighten me on how this is suppose to be used with real devices.
>>>> 
>>>> Recently I have added a simple bluetooth-power trigger that combines all Bluetooth controllers into a single trigger. If any of them is enabled, then you can control your LED. Which makes a lot more sense to me since you most likely have a single Bluetooth LED on your system. And you want it to show the correct state no matter what Bluetooth controller is in use. However I can see the case that someone might want to assign one specific Bluetooth controller to a LED status.
>>>> 
>>>> So instead of adding many independent triggers to each controller, why not create one global bluetooth trigger and one individual bluetooth-hci0 trigger for each controller. And the combine power, tx, rx and whatever else we need to trigger the LED for?
>>>> 
>>> 
>>> When I starting this work, I referred to WiFi system. See
>>> CONFIG_MAC80211_LEDS. WiFi system implements these types of triggers "
>>> phy0rx phy0tx phy0assoc phy0radio" for each 'controller'.
>> 
>> And I actually wonder who ever used these triggers. You need 4 LEDs to visualize the WiFi status. Which systems has 4 LEDs to spare to visualize this.
>> 
>>> Besides, there are also RFKILL which stands for WiFi/BT power status.
>>> RFKILL adds triggers for each module too. Eg. in the below example, I
>>> have one WiFi (phy0), one BT (hci0). Trigger rfkill1 equals to
>>> hci0-power.
>>> 
>>> Ref: here are all LED triggers I found in my 96boards/HiKey:
>>> 
>>> # cat trigger
>>> none kbd-scrollock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock
>>> kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock
>>> kbd-ctrlllock kbd-ctrlrlock mmc0 mmc1 heartbeat cpu0 cpu1 cpu2 cpu3
>>> cpu4 cpu5 cpu6 cpu7 mmc2 rfkill0 phy0rx phy0tx phy0assoc phy0radio
>>> hci0-power hci0-tx [hci0-rx] rfkill1
>> 
>> And how many LEDs do you have in the your system? I think you are making my point here.
>> 
>> So I think what we need to do is to not add to this madness and instead create one "bluetooth" LED trigger that combines power and TX/RX for all controllers. And then allow for individual "bluetooth-hci0" LED triggers so that you can bind a single Bluetooth controller to a single LED.
>> 
>> For me, if I can not combine hci0-power, hci0-tx and hci0-rx into a single LED,
> 
> By combining them into a single LED, do you mean such a use case?
> - when hci0 is powered on, this LED starts on.
> - then, when there is tx/rx traffic, this LED should blink (reversely
> of course).
> - when hci0 is powered off, this LED turns off.

yes, that is what I am thinking of.

Regards

Marcel