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

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 02/01/2025 03:48, Ping-Ke Shih wrote:
> 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. 
> 
> 

Yes, and RTL8814A will add one more unique led_set function.