[PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink

[PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink

[Bitterblue Smith]

If the chip can have an LED, register a struct led_classdev and enable
hardware-controlled blinking. When the chip is not transmitting or
receiving anything the LED is off. Otherwise the LED will blink
faster or slower according to the throughput.

The LED can be controlled from userspace by writing 0, 1, or 2 to
/sys/class/leds/rtl8xxxu-usbX-Y/brightness:
0 - solid off.
1 - solid on.
2 - hardware-controlled blinking.

In this patch none of the chips advertise having an LED. That will be
added in the next patches.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
 .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  8 +++++
 .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 34 +++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
index 90268479d3ad..c8cee4a24755 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
@@ -1443,6 +1443,8 @@ struct rtl8xxxu_cfo_tracking {
 	u32 packet_count_pre;
 };
 
+#define RTL8XXXU_HW_LED_CONTROL	2
+
 struct rtl8xxxu_priv {
 	struct ieee80211_hw *hw;
 	struct usb_device *udev;
@@ -1564,6 +1566,10 @@ struct rtl8xxxu_priv {
 	struct rtl8xxxu_ra_report ra_report;
 	struct rtl8xxxu_cfo_tracking cfo_tracking;
 	struct rtl8xxxu_ra_info ra_info;
+
+	bool led_registered;
+	char led_name[32];
+	struct led_classdev led_cdev;
 };
 
 struct rtl8xxxu_rx_urb {
@@ -1613,6 +1619,8 @@ struct rtl8xxxu_fileops {
 			     u32 rts_rate);
 	void (*set_crystal_cap) (struct rtl8xxxu_priv *priv, u8 crystal_cap);
 	s8 (*cck_rssi) (struct rtl8xxxu_priv *priv, u8 cck_agc_rpt);
+	int (*led_classdev_brightness_set) (struct led_classdev *led_cdev,
+					    enum led_brightness brightness);
 	int writeN_block_size;
 	int rx_agg_buf_size;
 	char tx_desc_size;
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 35dc777c1fba..b27edd503c23 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -6955,6 +6955,34 @@ static int rtl8xxxu_parse_usb(struct rtl8xxxu_priv *priv,
 	return ret;
 }
 
+static void rtl8xxxu_init_led(struct rtl8xxxu_priv *priv)
+{
+	struct led_classdev *led = &priv->led_cdev;
+
+	led->brightness_set_blocking = priv->fops->led_classdev_brightness_set;
+
+	snprintf(priv->led_name, sizeof(priv->led_name),
+		 "rtl8xxxu-usb%s", dev_name(&priv->udev->dev));
+	led->name = priv->led_name;
+	led->max_brightness = RTL8XXXU_HW_LED_CONTROL;
+
+	if (led_classdev_register(&priv->udev->dev, led))
+		return;
+
+	priv->led_registered = true;
+
+	led->brightness = led->max_brightness;
+	priv->fops->led_classdev_brightness_set(led, led->brightness);
+}
+
+static void rtl8xxxu_deinit_led(struct rtl8xxxu_priv *priv)
+{
+	struct led_classdev *led = &priv->led_cdev;
+
+	priv->fops->led_classdev_brightness_set(led, LED_OFF);
+	led_classdev_unregister(led);
+}
+
 static int rtl8xxxu_probe(struct usb_interface *interface,
 			  const struct usb_device_id *id)
 {
@@ -7135,6 +7163,9 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
 		goto err_set_intfdata;
 	}
 
+	if (priv->fops->led_classdev_brightness_set)
+		rtl8xxxu_init_led(priv);
+
 	return 0;
 
 err_set_intfdata:
@@ -7159,6 +7190,9 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
 	hw = usb_get_intfdata(interface);
 	priv = hw->priv;
 
+	if (priv->led_registered)
+		rtl8xxxu_deinit_led(priv);
+
 	ieee80211_unregister_hw(hw);
 
 	priv->fops->power_off(priv);
-- 
2.38.0

[PATCH 2/4] wifi: rtl8xxxu: Add LED control code for RTL8188EU

[Bitterblue Smith]

By default the LED will blink when there is some activity.

This was tested with a TP-Link TL-WN725N.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
 .../realtek/rtl8xxxu/rtl8xxxu_8188e.c         | 25 +++++++++++++++++++
 .../wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h |  4 +++
 2 files changed, 29 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8188e.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8188e.c
index 08f3b93ad8d0..a99ddb41cd24 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8188e.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8188e.c
@@ -1350,6 +1350,30 @@ static s8 rtl8188e_cck_rssi(struct rtl8xxxu_priv *priv, u8 cck_agc_rpt)
 	return rx_pwr_all;
 }
 
+static int rtl8188eu_led_brightness_set(struct led_classdev *led_cdev,
+					enum led_brightness brightness)
+{
+	struct rtl8xxxu_priv *priv = container_of(led_cdev,
+						  struct rtl8xxxu_priv,
+						  led_cdev);
+	u8 ledcfg = rtl8xxxu_read8(priv, REG_LEDCFG2);
+
+	if (brightness == LED_OFF) {
+		ledcfg &= ~LEDCFG2_HW_LED_CONTROL;
+		ledcfg |= LEDCFG2_SW_LED_CONTROL | LEDCFG2_SW_LED_DISABLE;
+	} else if (brightness == LED_ON) {
+		ledcfg &= ~(LEDCFG2_HW_LED_CONTROL | LEDCFG2_SW_LED_DISABLE);
+		ledcfg |= LEDCFG2_SW_LED_CONTROL;
+	} else if (brightness == RTL8XXXU_HW_LED_CONTROL) {
+		ledcfg &= ~LEDCFG2_SW_LED_DISABLE;
+		ledcfg |= LEDCFG2_HW_LED_CONTROL | LEDCFG2_HW_LED_ENABLE;
+	}
+
+	rtl8xxxu_write8(priv, REG_LEDCFG2, ledcfg);
+
+	return 0;
+}
+
 static void rtl8188e_set_tx_rpt_timing(struct rtl8xxxu_ra_info *ra, u8 timing)
 {
 	u8 idx;
@@ -1851,6 +1875,7 @@ struct rtl8xxxu_fileops rtl8188eu_fops = {
 	.fill_txdesc = rtl8xxxu_fill_txdesc_v3,
 	.set_crystal_cap = rtl8188f_set_crystal_cap,
 	.cck_rssi = rtl8188e_cck_rssi,
+	.led_classdev_brightness_set = rtl8188eu_led_brightness_set,
 	.writeN_block_size = 128,
 	.rx_desc_size = sizeof(struct rtl8xxxu_rxdesc16),
 	.tx_desc_size = sizeof(struct rtl8xxxu_txdesc32),
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h
index 5818b2378bab..d510ce27b1b4 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h
@@ -148,6 +148,10 @@
 #define  LEDCFG0_DPDT_SELECT		BIT(23)
 #define REG_LEDCFG1			0x004d
 #define REG_LEDCFG2			0x004e
+#define  LEDCFG2_HW_LED_CONTROL		BIT(1)
+#define  LEDCFG2_HW_LED_ENABLE		BIT(5)
+#define  LEDCFG2_SW_LED_DISABLE		BIT(3)
+#define  LEDCFG2_SW_LED_CONTROL   	BIT(5)
 #define  LEDCFG2_DPDT_SELECT		BIT(7)
 #define REG_LEDCFG3			0x004f
 #define REG_LEDCFG			REG_LEDCFG2
-- 
2.38.0

[PATCH 3/4] wifi: rtl8xxxu: Add LED control code for RTL8192EU

[Bitterblue Smith]

By default the LED will blink when there is some activity.

This was tested with a cheap "HT-WR813" from Aliexpress.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
 .../realtek/rtl8xxxu/rtl8xxxu_8192e.c         | 24 +++++++++++++++++++
 .../wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
index 4a1c9bcafe31..5cfc00237f42 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
@@ -1764,6 +1764,29 @@ static s8 rtl8192e_cck_rssi(struct rtl8xxxu_priv *priv, u8 cck_agc_rpt)
 	return rx_pwr_all;
 }
 
+static int rtl8192eu_led_brightness_set(struct led_classdev *led_cdev,
+					enum led_brightness brightness)
+{
+	struct rtl8xxxu_priv *priv = container_of(led_cdev,
+						  struct rtl8xxxu_priv,
+						  led_cdev);
+	u8 ledcfg = rtl8xxxu_read8(priv, REG_LEDCFG1);
+
+	if (brightness == LED_OFF) {
+		ledcfg &= ~LEDCFG1_HW_LED_CONTROL;
+		ledcfg |= LEDCFG1_LED_DISABLE;
+	} else if (brightness == LED_ON) {
+		ledcfg &= ~(LEDCFG1_HW_LED_CONTROL | LEDCFG1_LED_DISABLE);
+	} else if (brightness == RTL8XXXU_HW_LED_CONTROL) {
+		ledcfg &= ~LEDCFG1_LED_DISABLE;
+		ledcfg |= LEDCFG1_HW_LED_CONTROL;
+	}
+
+	rtl8xxxu_write8(priv, REG_LEDCFG1, ledcfg);
+
+	return 0;
+}
+
 struct rtl8xxxu_fileops rtl8192eu_fops = {
 	.identify_chip = rtl8192eu_identify_chip,
 	.parse_efuse = rtl8192eu_parse_efuse,
@@ -1788,6 +1811,7 @@ struct rtl8xxxu_fileops rtl8192eu_fops = {
 	.fill_txdesc = rtl8xxxu_fill_txdesc_v2,
 	.set_crystal_cap = rtl8723a_set_crystal_cap,
 	.cck_rssi = rtl8192e_cck_rssi,
+	.led_classdev_brightness_set = rtl8192eu_led_brightness_set,
 	.writeN_block_size = 128,
 	.tx_desc_size = sizeof(struct rtl8xxxu_txdesc40),
 	.rx_desc_size = sizeof(struct rtl8xxxu_rxdesc24),
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h
index d510ce27b1b4..5849fa4e1566 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h
@@ -147,6 +147,8 @@
 #define REG_LEDCFG0			0x004c
 #define  LEDCFG0_DPDT_SELECT		BIT(23)
 #define REG_LEDCFG1			0x004d
+#define  LEDCFG1_HW_LED_CONTROL		BIT(1)
+#define  LEDCFG1_LED_DISABLE		BIT(7)
 #define REG_LEDCFG2			0x004e
 #define  LEDCFG2_HW_LED_CONTROL		BIT(1)
 #define  LEDCFG2_HW_LED_ENABLE		BIT(5)
-- 
2.38.0

[PATCH 4/4] wifi: rtl8xxxu: Add LED control code for RTL8723AU

[Bitterblue Smith]

By default the LED will blink when there is some activity.

This is only compile tested.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
 .../realtek/rtl8xxxu/rtl8xxxu_8723a.c         | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723a.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723a.c
index 5ed523db2d87..5e7b58d395ba 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723a.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723a.c
@@ -457,6 +457,30 @@ s8 rtl8723a_cck_rssi(struct rtl8xxxu_priv *priv, u8 cck_agc_rpt)
 	return rx_pwr_all;
 }
 
+static int rtl8723au_led_brightness_set(struct led_classdev *led_cdev,
+					enum led_brightness brightness)
+{
+	struct rtl8xxxu_priv *priv = container_of(led_cdev,
+						  struct rtl8xxxu_priv,
+						  led_cdev);
+	u8 ledcfg = rtl8xxxu_read8(priv, REG_LEDCFG2);
+
+	if (brightness == LED_OFF) {
+		ledcfg &= ~LEDCFG2_HW_LED_CONTROL;
+		ledcfg |= LEDCFG2_SW_LED_CONTROL | LEDCFG2_SW_LED_DISABLE;
+	} else if (brightness == LED_ON) {
+		ledcfg &= ~(LEDCFG2_HW_LED_CONTROL | LEDCFG2_SW_LED_DISABLE);
+		ledcfg |= LEDCFG2_SW_LED_CONTROL;
+	} else if (brightness == RTL8XXXU_HW_LED_CONTROL) {
+		ledcfg &= ~LEDCFG2_SW_LED_DISABLE;
+		ledcfg |= LEDCFG2_HW_LED_CONTROL | LEDCFG2_HW_LED_ENABLE;
+	}
+
+	rtl8xxxu_write8(priv, REG_LEDCFG2, ledcfg);
+
+	return 0;
+}
+
 struct rtl8xxxu_fileops rtl8723au_fops = {
 	.identify_chip = rtl8723au_identify_chip,
 	.parse_efuse = rtl8723au_parse_efuse,
@@ -482,6 +506,7 @@ struct rtl8xxxu_fileops rtl8723au_fops = {
 	.fill_txdesc = rtl8xxxu_fill_txdesc_v1,
 	.set_crystal_cap = rtl8723a_set_crystal_cap,
 	.cck_rssi = rtl8723a_cck_rssi,
+	.led_classdev_brightness_set = rtl8723au_led_brightness_set,
 	.writeN_block_size = 1024,
 	.rx_agg_buf_size = 16000,
 	.tx_desc_size = sizeof(struct rtl8xxxu_txdesc32),
-- 
2.38.0

RE: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink

[Ping-Ke Shih]

> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Sent: Wednesday, January 18, 2023 6:03 AM
> To: linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
> Subject: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink
> 
> If the chip can have an LED, register a struct led_classdev and enable
> hardware-controlled blinking. When the chip is not transmitting or
> receiving anything the LED is off. Otherwise the LED will blink
> faster or slower according to the throughput.
> 
> The LED can be controlled from userspace by writing 0, 1, or 2 to
> /sys/class/leds/rtl8xxxu-usbX-Y/brightness:
> 0 - solid off.
> 1 - solid on.
> 2 - hardware-controlled blinking.

So, do you like

#define RTL8XXXU_HW_LED_BLINKING	2

> 
> In this patch none of the chips advertise having an LED. That will be
> added in the next patches.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  8 +++++
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 34 +++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index 90268479d3ad..c8cee4a24755 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -1443,6 +1443,8 @@ struct rtl8xxxu_cfo_tracking {
>  	u32 packet_count_pre;
>  };
> 
> +#define RTL8XXXU_HW_LED_CONTROL	2
> +
>  struct rtl8xxxu_priv {
>  	struct ieee80211_hw *hw;
>  	struct usb_device *udev;
> @@ -1564,6 +1566,10 @@ struct rtl8xxxu_priv {
>  	struct rtl8xxxu_ra_report ra_report;
>  	struct rtl8xxxu_cfo_tracking cfo_tracking;
>  	struct rtl8xxxu_ra_info ra_info;
> +
> +	bool led_registered;
> +	char led_name[32];
> +	struct led_classdev led_cdev;
>  };
> 
>  struct rtl8xxxu_rx_urb {
> @@ -1613,6 +1619,8 @@ struct rtl8xxxu_fileops {
>  			     u32 rts_rate);
>  	void (*set_crystal_cap) (struct rtl8xxxu_priv *priv, u8 crystal_cap);
>  	s8 (*cck_rssi) (struct rtl8xxxu_priv *priv, u8 cck_agc_rpt);
> +	int (*led_classdev_brightness_set) (struct led_classdev *led_cdev,
> +					    enum led_brightness brightness);
>  	int writeN_block_size;
>  	int rx_agg_buf_size;
>  	char tx_desc_size;
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 35dc777c1fba..b27edd503c23 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -6955,6 +6955,34 @@ static int rtl8xxxu_parse_usb(struct rtl8xxxu_priv *priv,
>  	return ret;
>  }
> 
> +static void rtl8xxxu_init_led(struct rtl8xxxu_priv *priv)
> +{
> +	struct led_classdev *led = &priv->led_cdev;
> +
> +	led->brightness_set_blocking = priv->fops->led_classdev_brightness_set;
> +
> +	snprintf(priv->led_name, sizeof(priv->led_name),
> +		 "rtl8xxxu-usb%s", dev_name(&priv->udev->dev));
> +	led->name = priv->led_name;
> +	led->max_brightness = RTL8XXXU_HW_LED_CONTROL;
> +
> +	if (led_classdev_register(&priv->udev->dev, led))
> +		return;
> +
> +	priv->led_registered = true;
> +
> +	led->brightness = led->max_brightness;
> +	priv->fops->led_classdev_brightness_set(led, led->brightness);
> +}
> +
> +static void rtl8xxxu_deinit_led(struct rtl8xxxu_priv *priv)
> +{
> +	struct led_classdev *led = &priv->led_cdev;
> +
> +	priv->fops->led_classdev_brightness_set(led, LED_OFF);
> +	led_classdev_unregister(led);
> +}
> +
>  static int rtl8xxxu_probe(struct usb_interface *interface,
>  			  const struct usb_device_id *id)
>  {
> @@ -7135,6 +7163,9 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
>  		goto err_set_intfdata;
>  	}
> 
> +	if (priv->fops->led_classdev_brightness_set)

I prefer to move this checking statement into rtl8xxxu_init_led(). Then,
the flow of rtl8xxxu_probe() looks clean. As well as rtl8xxxu_deinit_led().
Just soft suggestion.

> +		rtl8xxxu_init_led(priv);
> +
>  	return 0;
> 
>  err_set_intfdata:
> @@ -7159,6 +7190,9 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
>  	hw = usb_get_intfdata(interface);
>  	priv = hw->priv;
> 
> +	if (priv->led_registered)
> +		rtl8xxxu_deinit_led(priv);
> +
>  	ieee80211_unregister_hw(hw);
> 
>  	priv->fops->power_off(priv);
> --
> 2.38.0
> 
> ------Please consider the environment before printing this e-mail.

RE: [PATCH 2/4] wifi: rtl8xxxu: Add LED control code for RTL8188EU

[Ping-Ke Shih]

> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Sent: Wednesday, January 18, 2023 6:04 AM
> To: linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
> Subject: [PATCH 2/4] wifi: rtl8xxxu: Add LED control code for RTL8188EU
> 
> By default the LED will blink when there is some activity.
> 
> This was tested with a TP-Link TL-WN725N.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>

Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

RE: [PATCH 3/4] wifi: rtl8xxxu: Add LED control code for RTL8192EU

[Ping-Ke Shih]

> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Sent: Wednesday, January 18, 2023 6:05 AM
> To: linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
> Subject: [PATCH 3/4] wifi: rtl8xxxu: Add LED control code for RTL8192EU
> 
> By default the LED will blink when there is some activity.
> 
> This was tested with a cheap "HT-WR813" from Aliexpress.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>

Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

RE: [PATCH 4/4] wifi: rtl8xxxu: Add LED control code for RTL8723AU

[Ping-Ke Shih]

> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Sent: Wednesday, January 18, 2023 6:06 AM
> To: linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
> Subject: [PATCH 4/4] wifi: rtl8xxxu: Add LED control code for RTL8723AU
> 
> By default the LED will blink when there is some activity.
> 
> This is only compile tested.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>

Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

Re: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink

[Bitterblue Smith]

On 18/01/2023 02:52, Ping-Ke Shih wrote:
> 
> 
>> -----Original Message-----
>> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> Sent: Wednesday, January 18, 2023 6:03 AM
>> To: linux-wireless@vger.kernel.org
>> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
>> Subject: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink
>>
>> If the chip can have an LED, register a struct led_classdev and enable
>> hardware-controlled blinking. When the chip is not transmitting or
>> receiving anything the LED is off. Otherwise the LED will blink
>> faster or slower according to the throughput.
>>
>> The LED can be controlled from userspace by writing 0, 1, or 2 to
>> /sys/class/leds/rtl8xxxu-usbX-Y/brightness:
>> 0 - solid off.
>> 1 - solid on.
>> 2 - hardware-controlled blinking.
> 
> So, do you like
> 
> #define RTL8XXXU_HW_LED_BLINKING	2
> 
I'm not sure what you mean. Can you elaborate?

>>
>> In this patch none of the chips advertise having an LED. That will be
>> added in the next patches.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  8 +++++
>>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 34 +++++++++++++++++++
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> index 90268479d3ad..c8cee4a24755 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> @@ -1443,6 +1443,8 @@ struct rtl8xxxu_cfo_tracking {
>>  	u32 packet_count_pre;
>>  };
>>
>> +#define RTL8XXXU_HW_LED_CONTROL	2
>> +
>>  struct rtl8xxxu_priv {
>>  	struct ieee80211_hw *hw;
>>  	struct usb_device *udev;
>> @@ -1564,6 +1566,10 @@ struct rtl8xxxu_priv {
>>  	struct rtl8xxxu_ra_report ra_report;
>>  	struct rtl8xxxu_cfo_tracking cfo_tracking;
>>  	struct rtl8xxxu_ra_info ra_info;
>> +
>> +	bool led_registered;
>> +	char led_name[32];
>> +	struct led_classdev led_cdev;
>>  };
>>
>>  struct rtl8xxxu_rx_urb {
>> @@ -1613,6 +1619,8 @@ struct rtl8xxxu_fileops {
>>  			     u32 rts_rate);
>>  	void (*set_crystal_cap) (struct rtl8xxxu_priv *priv, u8 crystal_cap);
>>  	s8 (*cck_rssi) (struct rtl8xxxu_priv *priv, u8 cck_agc_rpt);
>> +	int (*led_classdev_brightness_set) (struct led_classdev *led_cdev,
>> +					    enum led_brightness brightness);
>>  	int writeN_block_size;
>>  	int rx_agg_buf_size;
>>  	char tx_desc_size;
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index 35dc777c1fba..b27edd503c23 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -6955,6 +6955,34 @@ static int rtl8xxxu_parse_usb(struct rtl8xxxu_priv *priv,
>>  	return ret;
>>  }
>>
>> +static void rtl8xxxu_init_led(struct rtl8xxxu_priv *priv)
>> +{
>> +	struct led_classdev *led = &priv->led_cdev;
>> +
>> +	led->brightness_set_blocking = priv->fops->led_classdev_brightness_set;
>> +
>> +	snprintf(priv->led_name, sizeof(priv->led_name),
>> +		 "rtl8xxxu-usb%s", dev_name(&priv->udev->dev));
>> +	led->name = priv->led_name;
>> +	led->max_brightness = RTL8XXXU_HW_LED_CONTROL;
>> +
>> +	if (led_classdev_register(&priv->udev->dev, led))
>> +		return;
>> +
>> +	priv->led_registered = true;
>> +
>> +	led->brightness = led->max_brightness;
>> +	priv->fops->led_classdev_brightness_set(led, led->brightness);
>> +}
>> +
>> +static void rtl8xxxu_deinit_led(struct rtl8xxxu_priv *priv)
>> +{
>> +	struct led_classdev *led = &priv->led_cdev;
>> +
>> +	priv->fops->led_classdev_brightness_set(led, LED_OFF);
>> +	led_classdev_unregister(led);
>> +}
>> +
>>  static int rtl8xxxu_probe(struct usb_interface *interface,
>>  			  const struct usb_device_id *id)
>>  {
>> @@ -7135,6 +7163,9 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
>>  		goto err_set_intfdata;
>>  	}
>>
>> +	if (priv->fops->led_classdev_brightness_set)
> 
> I prefer to move this checking statement into rtl8xxxu_init_led(). Then,
> the flow of rtl8xxxu_probe() looks clean. As well as rtl8xxxu_deinit_led().
> Just soft suggestion.
> 
Okay. That works too.

>> +		rtl8xxxu_init_led(priv);
>> +
>>  	return 0;
>>
>>  err_set_intfdata:
>> @@ -7159,6 +7190,9 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
>>  	hw = usb_get_intfdata(interface);
>>  	priv = hw->priv;
>>
>> +	if (priv->led_registered)
>> +		rtl8xxxu_deinit_led(priv);
>> +
>>  	ieee80211_unregister_hw(hw);
>>
>>  	priv->fops->power_off(priv);
>> --
>> 2.38.0
>>
>> ------Please consider the environment before printing this e-mail.

RE: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink

[Ping-Ke Shih]

> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Sent: Wednesday, January 18, 2023 9:53 PM
> To: Ping-Ke Shih <pkshih@realtek.com>; linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>
> Subject: Re: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink
> 
> On 18/01/2023 02:52, Ping-Ke Shih wrote:
> >
> >
> >> -----Original Message-----
> >> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> >> Sent: Wednesday, January 18, 2023 6:03 AM
> >> To: linux-wireless@vger.kernel.org
> >> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
> >> Subject: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink
> >>
> >> If the chip can have an LED, register a struct led_classdev and enable
> >> hardware-controlled blinking. When the chip is not transmitting or
> >> receiving anything the LED is off. Otherwise the LED will blink
> >> faster or slower according to the throughput.
> >>
> >> The LED can be controlled from userspace by writing 0, 1, or 2 to
> >> /sys/class/leds/rtl8xxxu-usbX-Y/brightness:
> >> 0 - solid off.
> >> 1 - solid on.
> >> 2 - hardware-controlled blinking.
> >
> > So, do you like
> >
> > #define RTL8XXXU_HW_LED_BLINKING	2
> >
> I'm not sure what you mean. Can you elaborate?

Originally, I would like to replace RTL8XXXU_HW_LED_CONTROL by
RTL8XXXU_HW_LED_BLINKING, because commit messages say "blinking". 
After reviewing 2/4, 3/4 and 4/4, I aware that this blinking is
hardware behavior. Then, it becomes hard to me to decide which
name is clear. Anyway, original is fine to me.

> 
> >>
> >> In this patch none of the chips advertise having an LED. That will be
> >> added in the next patches.
> >>
> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> >> ---
> >>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  8 +++++
> >>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 34 +++++++++++++++++++
> >>  2 files changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> >> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> >> index 90268479d3ad..c8cee4a24755 100644
> >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> >> @@ -1443,6 +1443,8 @@ struct rtl8xxxu_cfo_tracking {
> >>  	u32 packet_count_pre;
> >>  };
> >>
> >> +#define RTL8XXXU_HW_LED_CONTROL	2
> >> +
> >>  struct rtl8xxxu_priv {
> >>  	struct ieee80211_hw *hw;
> >>  	struct usb_device *udev;
> >> @@ -1564,6 +1566,10 @@ struct rtl8xxxu_priv {
> >>  	struct rtl8xxxu_ra_report ra_report;
> >>  	struct rtl8xxxu_cfo_tracking cfo_tracking;
> >>  	struct rtl8xxxu_ra_info ra_info;
> >> +
> >> +	bool led_registered;
> >> +	char led_name[32];
> >> +	struct led_classdev led_cdev;
> >>  };
> >>
> >>  struct rtl8xxxu_rx_urb {
> >> @@ -1613,6 +1619,8 @@ struct rtl8xxxu_fileops {
> >>  			     u32 rts_rate);
> >>  	void (*set_crystal_cap) (struct rtl8xxxu_priv *priv, u8 crystal_cap);
> >>  	s8 (*cck_rssi) (struct rtl8xxxu_priv *priv, u8 cck_agc_rpt);
> >> +	int (*led_classdev_brightness_set) (struct led_classdev *led_cdev,
> >> +					    enum led_brightness brightness);
> >>  	int writeN_block_size;
> >>  	int rx_agg_buf_size;
> >>  	char tx_desc_size;
> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> index 35dc777c1fba..b27edd503c23 100644
> >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> @@ -6955,6 +6955,34 @@ static int rtl8xxxu_parse_usb(struct rtl8xxxu_priv *priv,
> >>  	return ret;
> >>  }
> >>
> >> +static void rtl8xxxu_init_led(struct rtl8xxxu_priv *priv)
> >> +{
> >> +	struct led_classdev *led = &priv->led_cdev;
> >> +
> >> +	led->brightness_set_blocking = priv->fops->led_classdev_brightness_set;
> >> +
> >> +	snprintf(priv->led_name, sizeof(priv->led_name),
> >> +		 "rtl8xxxu-usb%s", dev_name(&priv->udev->dev));
> >> +	led->name = priv->led_name;
> >> +	led->max_brightness = RTL8XXXU_HW_LED_CONTROL;
> >> +
> >> +	if (led_classdev_register(&priv->udev->dev, led))
> >> +		return;
> >> +
> >> +	priv->led_registered = true;
> >> +
> >> +	led->brightness = led->max_brightness;
> >> +	priv->fops->led_classdev_brightness_set(led, led->brightness);
> >> +}
> >> +
> >> +static void rtl8xxxu_deinit_led(struct rtl8xxxu_priv *priv)
> >> +{
> >> +	struct led_classdev *led = &priv->led_cdev;
> >> +
> >> +	priv->fops->led_classdev_brightness_set(led, LED_OFF);
> >> +	led_classdev_unregister(led);
> >> +}
> >> +
> >>  static int rtl8xxxu_probe(struct usb_interface *interface,
> >>  			  const struct usb_device_id *id)
> >>  {
> >> @@ -7135,6 +7163,9 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
> >>  		goto err_set_intfdata;
> >>  	}
> >>
> >> +	if (priv->fops->led_classdev_brightness_set)
> >
> > I prefer to move this checking statement into rtl8xxxu_init_led(). Then,
> > the flow of rtl8xxxu_probe() looks clean. As well as rtl8xxxu_deinit_led().
> > Just soft suggestion.
> >
> Okay. That works too.
> 
> >> +		rtl8xxxu_init_led(priv);
> >> +
> >>  	return 0;
> >>
> >>  err_set_intfdata:
> >> @@ -7159,6 +7190,9 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
> >>  	hw = usb_get_intfdata(interface);
> >>  	priv = hw->priv;
> >>
> >> +	if (priv->led_registered)
> >> +		rtl8xxxu_deinit_led(priv);
> >> +
> >>  	ieee80211_unregister_hw(hw);
> >>
> >>  	priv->fops->power_off(priv);
> >> --
> >> 2.38.0
> >>
> >> ------Please consider the environment before printing this e-mail.

Re: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink

[Bitterblue Smith]

On 19/01/2023 02:25, Ping-Ke Shih wrote:
> 
> 
>> -----Original Message-----
>> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> Sent: Wednesday, January 18, 2023 9:53 PM
>> To: Ping-Ke Shih <pkshih@realtek.com>; linux-wireless@vger.kernel.org
>> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>
>> Subject: Re: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink
>>
>> On 18/01/2023 02:52, Ping-Ke Shih wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>>>> Sent: Wednesday, January 18, 2023 6:03 AM
>>>> To: linux-wireless@vger.kernel.org
>>>> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
>>>> Subject: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink
>>>>
>>>> If the chip can have an LED, register a struct led_classdev and enable
>>>> hardware-controlled blinking. When the chip is not transmitting or
>>>> receiving anything the LED is off. Otherwise the LED will blink
>>>> faster or slower according to the throughput.
>>>>
>>>> The LED can be controlled from userspace by writing 0, 1, or 2 to
>>>> /sys/class/leds/rtl8xxxu-usbX-Y/brightness:
>>>> 0 - solid off.
>>>> 1 - solid on.
>>>> 2 - hardware-controlled blinking.
>>>
>>> So, do you like
>>>
>>> #define RTL8XXXU_HW_LED_BLINKING	2
>>>
>> I'm not sure what you mean. Can you elaborate?
> 
> Originally, I would like to replace RTL8XXXU_HW_LED_CONTROL by
> RTL8XXXU_HW_LED_BLINKING, because commit messages say "blinking". 
> After reviewing 2/4, 3/4 and 4/4, I aware that this blinking is
> hardware behavior. Then, it becomes hard to me to decide which
> name is clear. Anyway, original is fine to me.
> 
Ah, okay. Now I get it. I will keep the original name.