2016-07-31 04:24:44

by Guodong Xu

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

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 <[email protected]>
---
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


2016-08-17 07:40:46

by Marcel Holtmann

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

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

2016-08-17 02:47:21

by Guodong Xu

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

On 16 August 2016 at 20:33, Marcel Holtmann <[email protected]> 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 tr=
iggers. Combined with hci0-power trigger these are already 3 triggers. And =
if you have 2 Bluetooth controllers in your system, then you have 6 trigger=
s.
> >>
> >
> > 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 an=
d 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 visua=
lize.
>
> and as a result we keep adding senseless triggers to the kernel and bloat=
ing it up for no reason. Especially since it feels like 99% of the LED trig=
gers 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 Blue=
tooth 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, b=
ut I have not found it yet. So please someone enlighten me on how this is s=
uppose to be used with real devices.
> >>
> >> Recently I have added a simple bluetooth-power trigger that combines a=
ll 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. Howe=
ver I can see the case that someone might want to assign one specific Bluet=
ooth 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 els=
e 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 vi=
sualize the WiFi status. Which systems has 4 LEDs to spare to visualize thi=
s.
>
> > 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 c=
reate one "bluetooth" LED trigger that combines power and TX/RX for all con=
trollers. And then allow for individual "bluetooth-hci0" LED triggers so th=
at 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 singl=
e 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
>

2016-08-16 12:33:13

by Marcel Holtmann

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

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

2016-08-16 09:30:21

by Guodong Xu

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

Hi, Marcel

On 16 August 2016 at 14:03, Marcel Holtmann <[email protected]> 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 trigg=
ers. 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 u=
p.
>
> 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 Bluetoo=
th 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 supp=
ose 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, the=
n you can control your LED. Which makes a lot more sense to me since you mo=
st likely have a single Bluetooth LED on your system. And you want it to sh=
ow 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 Bluetoot=
h controller to a LED status.
>
> So instead of adding many independent triggers to each controller, why no=
t create one global bluetooth trigger and one individual bluetooth-hci0 tri=
gger for each controller. And the combine power, tx, rx and whatever else w=
e 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
>

2016-08-16 06:03:33

by Marcel Holtmann

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

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

2016-08-16 02:37:23

by Guodong Xu

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

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 <[email protected]> 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 <[email protected]>
> ---
> 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
>