RE: [PATCH] wifi: rtw88: Add support for LED blinking

[Ping-Ke Shih <pkshih@realtek.com>]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> Register a struct led_classdev with the kernel's LED subsystem and
> create a throughput-based trigger for it. Then mac80211 makes the LED
> blink.
> 
> Tested with Tenda U12 (RTL8812AU), Tenda U9 (RTL8811CU), TP-Link Archer
> T2U Nano (RTL8811AU), TP-Link Archer T3U Plus (RTL8812BU), Edimax
> EW-7611UCB (RTL8821AU), LM842 (RTL8822CU).
> 
> Also tested with devices which don't have LEDs: the laptop's internal
> RTL8822CE and a no-name RTL8723DU.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtw88/main.c     | 91 ++++++++++++++++++-
>  drivers/net/wireless/realtek/rtw88/main.h     |  9 ++
>  drivers/net/wireless/realtek/rtw88/reg.h      | 12 +++
>  drivers/net/wireless/realtek/rtw88/rtw8812a.c | 23 +++++
>  drivers/net/wireless/realtek/rtw88/rtw8821a.c | 32 +++++++
>  drivers/net/wireless/realtek/rtw88/rtw8821c.c | 25 +++++
>  drivers/net/wireless/realtek/rtw88/rtw8822b.c | 25 +++++
>  drivers/net/wireless/realtek/rtw88/rtw8822c.c | 25 +++++
>  8 files changed, 240 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index 6993f93c8f06..387940839f8b 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -2221,6 +2221,86 @@ void rtw_core_deinit(struct rtw_dev *rtwdev)
>  }
>  EXPORT_SYMBOL(rtw_core_deinit);
> 
> +#ifdef CONFIG_LEDS_CLASS

Not prefer to have #ifdef in code. Please add a led.c and add an entry
  obj-$(CONFIG_LEDS_CLASS) += led.c 
to Makefile. 

Since you enclose whole functions, it looks not a big problem for this case.
But I still want to avoid using of #ifdef to prevent people imitate this wrongly. 

> +
> +static int rtw_led_set_blocking(struct led_classdev *led,
> +                               enum led_brightness brightness)
> +{
> +       struct rtw_dev *rtwdev = container_of(led, struct rtw_dev, led_cdev);
> +
> +       rtwdev->chip->ops->led_set(led, brightness);
> +
> +       return 0;
> +}
> +
> +static void rtw_led_init(struct rtw_dev *rtwdev)
> +{
> +       static const struct ieee80211_tpt_blink rtw_tpt_blink[] = {
> +               { .throughput = 0 * 1024, .blink_time = 334 },
> +               { .throughput = 1 * 1024, .blink_time = 260 },
> +               { .throughput = 5 * 1024, .blink_time = 220 },
> +               { .throughput = 10 * 1024, .blink_time = 190 },
> +               { .throughput = 20 * 1024, .blink_time = 170 },
> +               { .throughput = 50 * 1024, .blink_time = 150 },
> +               { .throughput = 70 * 1024, .blink_time = 130 },
> +               { .throughput = 100 * 1024, .blink_time = 110 },
> +               { .throughput = 200 * 1024, .blink_time = 80 },
> +               { .throughput = 300 * 1024, .blink_time = 50 },
> +       };
> +       struct led_classdev *led = &rtwdev->led_cdev;
> +       int err;
> +
> +       if (!rtwdev->chip->ops->led_set)
> +               return;
> +
> +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB)
> +               led->brightness_set_blocking = rtw_led_set_blocking;
> +       else
> +               led->brightness_set = rtwdev->chip->ops->led_set;
> +
> +       snprintf(rtwdev->led_name, sizeof(rtwdev->led_name),
> +                "rtw88-%s", dev_name(rtwdev->dev));
> +
> +       led->name = rtwdev->led_name;
> +       led->max_brightness = LED_ON;
> +       led->default_trigger =
> +               ieee80211_create_tpt_led_trigger(rtwdev->hw,
> +                                                IEEE80211_TPT_LEDTRIG_FL_RADIO,
> +                                                rtw_tpt_blink,
> +                                                ARRAY_SIZE(rtw_tpt_blink));
> +
> +       err = led_classdev_register(rtwdev->dev, led);
> +       if (err) {
> +               rtw_warn(rtwdev, "Failed to register the LED, error %d\n", err);
> +               return;
> +       }
> +
> +       rtwdev->led_registered = true;
> +}
> +
> +static void rtw_led_deinit(struct rtw_dev *rtwdev)
> +{
> +       struct led_classdev *led = &rtwdev->led_cdev;
> +
> +       if (!rtwdev->led_registered)
> +               return;
> +
> +       rtwdev->chip->ops->led_set(led, LED_OFF);
> +       led_classdev_unregister(led);
> +}
> +
> +#else
> +
> +static void rtw_led_init(struct rtw_dev *rtwdev)
> +{
> +}
> +
> +static void rtw_led_deinit(struct rtw_dev *rtwdev)
> +{
> +}
> +
> +#endif
> +

[...]

> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8812a.c
> b/drivers/net/wireless/realtek/rtw88/rtw8812a.c
> index 21795286a1a0..e16ba8d8a792 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8812a.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8812a.c
> @@ -868,6 +868,26 @@ static void rtw8812a_pwr_track(struct rtw_dev *rtwdev)
>         dm_info->pwr_trk_triggered = false;
>  }
> 
> +#ifdef CONFIG_LEDS_CLASS
> +
> +static void rtw8812a_led_set(struct led_classdev *led,
> +                            enum led_brightness brightness)
> +{
> +       struct rtw_dev *rtwdev = container_of(led, struct rtw_dev, led_cdev);
> +       u8 ledcfg;
> +
> +       ledcfg = rtw_read8(rtwdev, REG_LED_CFG);
> +       ledcfg &= BIT(6) | BIT(4);
> +       ledcfg |= BIT(5);
> +
> +       if (brightness == LED_OFF)
> +               ledcfg |= BIT(3);
> +
> +       rtw_write8(rtwdev, REG_LED_CFG, ledcfg);
> +}
> +
> +#endif
> +
>  static void rtw8812a_fill_txdesc_checksum(struct rtw_dev *rtwdev,
>                                           struct rtw_tx_pkt_info *pkt_info,
>                                           u8 *txdesc)
> @@ -916,6 +936,9 @@ static const struct rtw_chip_ops rtw8812a_ops = {
>         .config_bfee            = NULL,
>         .set_gid_table          = NULL,
>         .cfg_csi_rate           = NULL,
> +#ifdef CONFIG_LEDS_CLASS
> +       .led_set                = rtw8812a_led_set,
> +#endif

Just build the code without checking CONFIG_LEDS_CLASS.
It will waste some space, but acceptable. 

            before  after   delta
rtw8812a.o  15619   15771   152
rtw8821a.o  12922   13186   264
rtw8821c.o  18890   19034   144
rtw8822b.o  24860   25004   144
rtw8822c.o  65963   66155   192

Also I'm thinking if we can move rtw8812a_led_set to led.c as well. However,
it looks very different from chip to chip.