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