[PATCH 1/2] Bluetooth: Add LED triggers for HCI frames tx and rx

Subsystems: bluetooth subsystem, the rest

STALE3673d

8 messages, 2 authors, 2016-07-15 · open the first message on its own page

[PATCH 1/2] Bluetooth: Add LED triggers for HCI frames tx and rx

From: Guodong Xu <hidden>
Date: 2016-06-23 04:59:24

Two LED triggers are defined: tx_led and rx_led. Upon frames
available in HCI core layer, for tx or for rx, the combined LED
can blink.

Verified on HiKey, 96boards. It uses hi6220 SoC and TI WL1835 combo
chip.

Signed-off-by: Guodong Xu <redacted>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         |  3 +++
 net/bluetooth/leds.c             | 15 +++++++++++++++
 net/bluetooth/leds.h             |  2 ++
 4 files changed, 21 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..c6e1210 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3248,6 +3248,7 @@ int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	skb_queue_tail(&hdev->rx_q, skb);
 	queue_work(hdev->workqueue, &hdev->rx_work);
 
+	hci_leds_blink_oneshot(hdev->rx_led);
 	return 0;
 }
 EXPORT_SYMBOL(hci_recv_frame);
@@ -3325,6 +3326,8 @@ static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 		BT_ERR("%s sending frame failed (%d)", hdev->name, err);
 		kfree_skb(skb);
 	}
+
+	hci_leds_blink_oneshot(hdev->tx_led);
 }
 
 /* Send HCI command */
diff --git a/net/bluetooth/leds.c b/net/bluetooth/leds.c
index 8319c84..c4825d5 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,15 @@ 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;
+	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 +82,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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] arm64: dts: hikey: set bluetooth led trigger

From: Guodong Xu <hidden>
Date: 2016-06-23 04:59:31

Set bluetooth led trigger to hci0-rx, and so LED blinks on hci
frame receiving.

Signed-off-by: Guodong Xu <redacted>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index e92a30c..5032792 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -132,7 +132,7 @@
 		bt_active_led {
 			label = "bt_active";
 			gpios = <&gpio4 7 0>; /* <&gpio_bt_active_led>; */
-			linux,default-trigger = "hci0rx";
+			linux,default-trigger = "hci0-rx";
 			default-state = "off";
 		};
 	};
-- 
1.9.1

Re: [PATCH 1/2] Bluetooth: Add LED triggers for HCI frames tx and rx

From: Guodong Xu <hidden>
Date: 2016-07-12 05:31:08

Dear maintainers,

Would you have a review on this?

-Guodong

On 23 June 2016 at 12:58, Guodong Xu [off-list ref] wrote:
quoted hunk
Two LED triggers are defined: tx_led and rx_led. Upon frames
available in HCI core layer, for tx or for rx, the combined LED
can blink.

Verified on HiKey, 96boards. It uses hi6220 SoC and TI WL1835 combo
chip.

Signed-off-by: Guodong Xu <redacted>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         |  3 +++
 net/bluetooth/leds.c             | 15 +++++++++++++++
 net/bluetooth/leds.h             |  2 ++
 4 files changed, 21 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..c6e1210 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3248,6 +3248,7 @@ int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
        skb_queue_tail(&hdev->rx_q, skb);
        queue_work(hdev->workqueue, &hdev->rx_work);

+       hci_leds_blink_oneshot(hdev->rx_led);
        return 0;
 }
 EXPORT_SYMBOL(hci_recv_frame);
@@ -3325,6 +3326,8 @@ static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
                BT_ERR("%s sending frame failed (%d)", hdev->name, err);
                kfree_skb(skb);
        }
+
+       hci_leds_blink_oneshot(hdev->tx_led);
 }

 /* Send HCI command */
diff --git a/net/bluetooth/leds.c b/net/bluetooth/leds.c
index 8319c84..c4825d5 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,15 @@ 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;
+       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 +82,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 1/2] Bluetooth: Add LED triggers for HCI frames tx and rx

From: Marcel Holtmann <marcel@holtmann.org>
Date: 2016-07-13 08:08:20

Hi Guodong,
quoted hunk
Two LED triggers are defined: tx_led and rx_led. Upon frames
available in HCI core layer, for tx or for rx, the combined LED
can blink.

Verified on HiKey, 96boards. It uses hi6220 SoC and TI WL1835 combo
chip.

Signed-off-by: Guodong Xu <redacted>
---
include/net/bluetooth/hci_core.h |  1 +
net/bluetooth/hci_core.c         |  3 +++
net/bluetooth/leds.c             | 15 +++++++++++++++
net/bluetooth/leds.h             |  2 ++
4 files changed, 21 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..c6e1210 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3248,6 +3248,7 @@ int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
	skb_queue_tail(&hdev->rx_q, skb);
	queue_work(hdev->workqueue, &hdev->rx_work);

+	hci_leds_blink_oneshot(hdev->rx_led);
	return 0;
}
EXPORT_SYMBOL(hci_recv_frame);
@@ -3325,6 +3326,8 @@ static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
		BT_ERR("%s sending frame failed (%d)", hdev->name, err);
		kfree_skb(skb);
	}
+
+	hci_leds_blink_oneshot(hdev->tx_led);
}
so I am not convinced that this is the right way of doing TX/RX activity leds. This would have them purely based on HCI traffic and this would include control frames. I think that we want activity leds for actual radio activity. Meaning that when we have an active connection and ACL packets are exchanged or when we are scanning.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Bluetooth: Add LED triggers for HCI frames tx and rx

From: Guodong Xu <hidden>
Date: 2016-07-14 07:08:33

On 13 July 2016 at 16:07, Marcel Holtmann [off-list ref] wrote:
Hi Guodong,
quoted
Two LED triggers are defined: tx_led and rx_led. Upon frames
available in HCI core layer, for tx or for rx, the combined LED
can blink.

Verified on HiKey, 96boards. It uses hi6220 SoC and TI WL1835 combo
chip.

Signed-off-by: Guodong Xu <redacted>
---
include/net/bluetooth/hci_core.h |  1 +
net/bluetooth/hci_core.c         |  3 +++
net/bluetooth/leds.c             | 15 +++++++++++++++
net/bluetooth/leds.h             |  2 ++
4 files changed, 21 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..c6e1210 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3248,6 +3248,7 @@ int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
      skb_queue_tail(&hdev->rx_q, skb);
      queue_work(hdev->workqueue, &hdev->rx_work);

+     hci_leds_blink_oneshot(hdev->rx_led);
      return 0;
}
EXPORT_SYMBOL(hci_recv_frame);
@@ -3325,6 +3326,8 @@ static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
              BT_ERR("%s sending frame failed (%d)", hdev->name, err);
              kfree_skb(skb);
      }
+
+     hci_leds_blink_oneshot(hdev->tx_led);
}
so I am not convinced that this is the right way of doing TX/RX activity leds. This would have them purely based on HCI traffic and this would include control frames. I think that we want activity leds for actual radio activity. Meaning that when we have an active connection and ACL packets are exchanged or when we are scanning.
Thanks. I got your point. I will revise the code and submit again.
Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Add LED triggers for HCI frames tx and rx

From: Guodong Xu <hidden>
Date: 2016-07-14 09:31:07

On 13 July 2016 at 16:07, Marcel Holtmann [off-list ref] wrote:
Hi Guodong,
quoted
Two LED triggers are defined: tx_led and rx_led. Upon frames
available in HCI core layer, for tx or for rx, the combined LED
can blink.

Verified on HiKey, 96boards. It uses hi6220 SoC and TI WL1835 combo
chip.

Signed-off-by: Guodong Xu <redacted>
---
include/net/bluetooth/hci_core.h |  1 +
net/bluetooth/hci_core.c         |  3 +++
net/bluetooth/leds.c             | 15 +++++++++++++++
net/bluetooth/leds.h             |  2 ++
4 files changed, 21 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..c6e1210 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3248,6 +3248,7 @@ int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
      skb_queue_tail(&hdev->rx_q, skb);
      queue_work(hdev->workqueue, &hdev->rx_work);

+     hci_leds_blink_oneshot(hdev->rx_led);
      return 0;
}
EXPORT_SYMBOL(hci_recv_frame);
@@ -3325,6 +3326,8 @@ static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
              BT_ERR("%s sending frame failed (%d)", hdev->name, err);
              kfree_skb(skb);
      }
+
+     hci_leds_blink_oneshot(hdev->tx_led);
}
so I am not convinced that this is the right way of doing TX/RX activity leds. This would have them purely based on HCI traffic and this would include control frames.
Hi, Marcel

+       if (hci_skb_pkt_type(skb) == HCI_ACLDATA_PKT ||
+           hci_skb_pkt_type(skb) == HCI_SCODATA_PKT)
+               hci_leds_blink_oneshot(hdev->rx_led);
+

By adding pkt_type checks, we can limit LED blinks only at ACL/SCO
DATA_PKT. Similar checks can be added for tx_led too.

Does this look good to you? If you agree, I can send v2 to include this change.
I think that we want activity leds for actual radio activity. Meaning that when we have an active connection and ACL packets are exchanged or when we are scanning.
Radio is controlled in Bluetooth module side, not Host. So I'm afraid
actual radio activity cannot be captured in its exact form in Host
side. The purpose of this patch is not for radio, it is for
incoming/outgoing data. For example, it can give user a blinking
indicator on RX frames after a Bluetooth mouse is connected. Another
example, it can blink at bluetooth streamed audio (TX).

Would that make sense?

-Guodong
Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Add LED triggers for HCI frames tx and rx

From: Marcel Holtmann <marcel@holtmann.org>
Date: 2016-07-14 09:42:39

Hi Guodong,
quoted
quoted
Two LED triggers are defined: tx_led and rx_led. Upon frames
available in HCI core layer, for tx or for rx, the combined LED
can blink.

Verified on HiKey, 96boards. It uses hi6220 SoC and TI WL1835 combo
chip.

Signed-off-by: Guodong Xu <redacted>
---
include/net/bluetooth/hci_core.h |  1 +
net/bluetooth/hci_core.c         |  3 +++
net/bluetooth/leds.c             | 15 +++++++++++++++
net/bluetooth/leds.h             |  2 ++
4 files changed, 21 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..c6e1210 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3248,6 +3248,7 @@ int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
     skb_queue_tail(&hdev->rx_q, skb);
     queue_work(hdev->workqueue, &hdev->rx_work);

+     hci_leds_blink_oneshot(hdev->rx_led);
     return 0;
}
EXPORT_SYMBOL(hci_recv_frame);
@@ -3325,6 +3326,8 @@ static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
             BT_ERR("%s sending frame failed (%d)", hdev->name, err);
             kfree_skb(skb);
     }
+
+     hci_leds_blink_oneshot(hdev->tx_led);
}
so I am not convinced that this is the right way of doing TX/RX activity leds. This would have them purely based on HCI traffic and this would include control frames.
Hi, Marcel

+       if (hci_skb_pkt_type(skb) == HCI_ACLDATA_PKT ||
+           hci_skb_pkt_type(skb) == HCI_SCODATA_PKT)
+               hci_leds_blink_oneshot(hdev->rx_led);
+

By adding pkt_type checks, we can limit LED blinks only at ACL/SCO
DATA_PKT. Similar checks can be added for tx_led too.

Does this look good to you? If you agree, I can send v2 to include this change.
we are not going to keep comparing packet types over and over again. The code is already doing that. So functions like hci_sched_acl and hci_acldata_packet are already present in the code.
quoted
I think that we want activity leds for actual radio activity. Meaning that when we have an active connection and ACL packets are exchanged or when we are scanning.
Radio is controlled in Bluetooth module side, not Host. So I'm afraid
actual radio activity cannot be captured in its exact form in Host
side. The purpose of this patch is not for radio, it is for
incoming/outgoing data. For example, it can give user a blinking
indicator on RX frames after a Bluetooth mouse is connected. Another
example, it can blink at bluetooth streamed audio (TX).
So radio activity is caused by RX/TX meaning ACL/SCO packets for sure. There is also other radio activity caused by scanning, inquiry, create connection etc. And the question is if this should be there as well.

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Add LED triggers for HCI frames tx and rx

From: Guodong Xu <hidden>
Date: 2016-07-15 12:08:59

On 14 July 2016 at 17:42, Marcel Holtmann [off-list ref] wrote:
Hi Guodong,
quoted
quoted
quoted
Two LED triggers are defined: tx_led and rx_led. Upon frames
available in HCI core layer, for tx or for rx, the combined LED
can blink.

Verified on HiKey, 96boards. It uses hi6220 SoC and TI WL1835 combo
chip.

Signed-off-by: Guodong Xu <redacted>
---
include/net/bluetooth/hci_core.h |  1 +
net/bluetooth/hci_core.c         |  3 +++
net/bluetooth/leds.c             | 15 +++++++++++++++
net/bluetooth/leds.h             |  2 ++
4 files changed, 21 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..c6e1210 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3248,6 +3248,7 @@ int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb)
     skb_queue_tail(&hdev->rx_q, skb);
     queue_work(hdev->workqueue, &hdev->rx_work);

+     hci_leds_blink_oneshot(hdev->rx_led);
     return 0;
}
EXPORT_SYMBOL(hci_recv_frame);
@@ -3325,6 +3326,8 @@ static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
             BT_ERR("%s sending frame failed (%d)", hdev->name, err);
             kfree_skb(skb);
     }
+
+     hci_leds_blink_oneshot(hdev->tx_led);
}
so I am not convinced that this is the right way of doing TX/RX activity leds. This would have them purely based on HCI traffic and this would include control frames.
Hi, Marcel

+       if (hci_skb_pkt_type(skb) == HCI_ACLDATA_PKT ||
+           hci_skb_pkt_type(skb) == HCI_SCODATA_PKT)
+               hci_leds_blink_oneshot(hdev->rx_led);
+

By adding pkt_type checks, we can limit LED blinks only at ACL/SCO
DATA_PKT. Similar checks can be added for tx_led too.

Does this look good to you? If you agree, I can send v2 to include this change.
we are not going to keep comparing packet types over and over again. The code is already doing that. So functions like hci_sched_acl and hci_acldata_packet are already present in the code.
quoted
quoted
I think that we want activity leds for actual radio activity. Meaning that when we have an active connection and ACL packets are exchanged or when we are scanning.
Radio is controlled in Bluetooth module side, not Host. So I'm afraid
actual radio activity cannot be captured in its exact form in Host
side. The purpose of this patch is not for radio, it is for
incoming/outgoing data. For example, it can give user a blinking
indicator on RX frames after a Bluetooth mouse is connected. Another
example, it can blink at bluetooth streamed audio (TX).
So radio activity is caused by RX/TX meaning ACL/SCO packets for sure. There is also other radio activity caused by scanning, inquiry, create connection etc. And the question is if this should be there as well.
I wouldn't suggest to cover scanning, inquiry, etc. Scanning, for
example, Host sends a scan command to Bluetooth module (in an
asynchronous way). After a given interval, Host receives inquiry
results. Over this period, at host side, we don't know what kind of
radio activity is happening in BT module side.

Let's say this patch provide LED indicators to only data packets
activity (ACL and SCO). Can we?

-Guodong
Regards

Marcel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help