Hi Heiner,
please use a proper subject line.
> 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.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> ---
> 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
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index c95e032..0f084b4 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -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);
> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> index 95d1a66..06c31b9 100644
> --- a/net/bluetooth/Kconfig
> +++ b/net/bluetooth/Kconfig
> @@ -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
> diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
> index 2b15ae8..b3ff12e 100644
> --- a/net/bluetooth/Makefile
> +++ b/net/bluetooth/Makefile
> @@ -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
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 47bcef7..06f603a 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -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);
> diff --git a/net/bluetooth/leds.c b/net/bluetooth/leds.c
> new file mode 100644
> index 0000000..9172c53
> --- /dev/null
> +++ b/net/bluetooth/leds.c
> @@ -0,0 +1,83 @@
> +/*
> + * Copyright 2015, Heiner Kallweit <hkallweit1@xxxxxxxxx>
> + *
> + * 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, \
> + 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.
> +{
> + 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.
> +{
> + 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,
> + 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.
> diff --git a/net/bluetooth/leds.h b/net/bluetooth/leds.h
> new file mode 100644
> index 0000000..068261a
> --- /dev/null
> +++ b/net/bluetooth/leds.h
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright 2015, Heiner Kallweit <hkallweit1@xxxxxxxxx>
> + *
> + * 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
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
