[PATCH 00/10] staging: r8188eu: led layer fix and cleanups

[PATCH 00/10] staging: r8188eu: led layer fix and cleanups

[Martin Kaiser]

Here's another series to clean up the led layer and fix a problem with
leds during suspend/resume on some devices.

Martin Kaiser (10):
  staging: r8188eu: fix led register settings
  staging: r8188eu: handle rtw_write8 errors in SwLedOn
  staging: r8188eu: fix status updates in SwLedOff
  staging: r8188eu: SwLedOn needs no padapter parameter
  staging: r8188eu: SwLedOff needs no padapter parameter
  staging: r8188eu: remove two unused defines
  staging: r8188eu: don't include rtw_led.h from rtw_cmd.h
  staging: r8188eu: remove padapter from struct led_priv
  staging: r8188eu: set two more state variables
  staging: r8188eu: summarize tx/rx and scan blinking

 drivers/staging/r8188eu/core/rtw_led.c    | 62 +++++++----------------
 drivers/staging/r8188eu/include/rtw_cmd.h |  5 --
 drivers/staging/r8188eu/include/rtw_led.h |  2 -
 3 files changed, 18 insertions(+), 51 deletions(-)

-- 
2.30.2

[PATCH 01/10] staging: r8188eu: fix led register settings

[Martin Kaiser]

Using an InterTech DMG-02 dongle, the led remains on when the system goes
into standby mode. After wakeup, it's no longer possible to control the
led.

It turned out that the register settings to enable or disable the led were
not correct. They worked for some dongles like the Edimax V2 but not for
others like the InterTech DMG-02.

This patch fixes the register settings. Bit 3 in the led_cfg2 register
controls the led status, bit 5 must always be set to be able to control
the led, bit 6 has no influence on the led. Setting the mac_pinmux_cfg
register is not necessary.

These settings were tested with Edimax V2 and InterTech DMG-02.

Cc: stable@vger.kernel.org
Fixes: 8cd574e6af54 ("staging: r8188eu: introduce new hal dir for RTL8188eu driver")
Suggested-by: Michael Straube <straube.linux@gmail.com>
Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_led.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
index 2527c252c3e9..5b214488571b 100644
--- a/drivers/staging/r8188eu/core/rtw_led.c
+++ b/drivers/staging/r8188eu/core/rtw_led.c
@@ -31,40 +31,19 @@ static void ResetLedStatus(struct led_priv *pLed)
 
 static void SwLedOn(struct adapter *padapter, struct led_priv *pLed)
 {
-	u8	LedCfg;
-	int res;
-
 	if (padapter->bDriverStopped)
 		return;
 
-	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);
-	if (res)
-		return;
-
-	rtw_write8(padapter, REG_LEDCFG2, (LedCfg & 0xf0) | BIT(5) | BIT(6)); /*  SW control led0 on. */
+	rtw_write8(padapter, REG_LEDCFG2, BIT(5)); /*  SW control led0 on. */
 	pLed->bLedOn = true;
 }
 
 static void SwLedOff(struct adapter *padapter, struct led_priv *pLed)
 {
-	u8	LedCfg;
-	int res;
-
 	if (padapter->bDriverStopped)
 		goto exit;
 
-	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);/* 0x4E */
-	if (res)
-		goto exit;
-
-	LedCfg &= 0x90; /*  Set to software control. */
-	rtw_write8(padapter, REG_LEDCFG2, (LedCfg | BIT(3)));
-	res = rtw_read8(padapter, REG_MAC_PINMUX_CFG, &LedCfg);
-	if (res)
-		goto exit;
-
-	LedCfg &= 0xFE;
-	rtw_write8(padapter, REG_MAC_PINMUX_CFG, LedCfg);
+	rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3));
 exit:
 	pLed->bLedOn = false;
 }
-- 
2.30.2

[PATCH 02/10] staging: r8188eu: handle rtw_write8 errors in SwLedOn

[Martin Kaiser]

Check the status returned by rtw_write8. Update bLedOn only if we could
update the REG_LEDCFG2 register.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_led.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
index 5b214488571b..4f1cad890cae 100644
--- a/drivers/staging/r8188eu/core/rtw_led.c
+++ b/drivers/staging/r8188eu/core/rtw_led.c
@@ -34,7 +34,9 @@ static void SwLedOn(struct adapter *padapter, struct led_priv *pLed)
 	if (padapter->bDriverStopped)
 		return;
 
-	rtw_write8(padapter, REG_LEDCFG2, BIT(5)); /*  SW control led0 on. */
+	if (rtw_write8(padapter, REG_LEDCFG2, BIT(5)) != _SUCCESS)
+		return;
+
 	pLed->bLedOn = true;
 }
 
-- 
2.30.2

[PATCH 03/10] staging: r8188eu: fix status updates in SwLedOff

[Martin Kaiser]

Update bLedOn only if we could update the REG_LEDCFG2 register.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_led.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
index 4f1cad890cae..38433296d327 100644
--- a/drivers/staging/r8188eu/core/rtw_led.c
+++ b/drivers/staging/r8188eu/core/rtw_led.c
@@ -43,10 +43,11 @@ static void SwLedOn(struct adapter *padapter, struct led_priv *pLed)
 static void SwLedOff(struct adapter *padapter, struct led_priv *pLed)
 {
 	if (padapter->bDriverStopped)
-		goto exit;
+		return;
+
+	if (rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3)) != _SUCCESS)
+		return;
 
-	rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3));
-exit:
 	pLed->bLedOn = false;
 }
 
-- 
2.30.2

[PATCH 04/10] staging: r8188eu: SwLedOn needs no padapter parameter

[Martin Kaiser]

Remove the padapter parameter from the SwLedOn function. padapter can be
derived from the pLed parameter.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_led.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
index 38433296d327..aa8f41edfade 100644
--- a/drivers/staging/r8188eu/core/rtw_led.c
+++ b/drivers/staging/r8188eu/core/rtw_led.c
@@ -29,8 +29,10 @@ static void ResetLedStatus(struct led_priv *pLed)
 	pLed->bLedScanBlinkInProgress = false;
 }
 
-static void SwLedOn(struct adapter *padapter, struct led_priv *pLed)
+static void SwLedOn(struct led_priv *pLed)
 {
+	struct adapter *padapter = container_of(pLed, struct adapter, ledpriv);
+
 	if (padapter->bDriverStopped)
 		return;
 
@@ -67,7 +69,7 @@ static void blink_work(struct work_struct *work)
 	if (pLed->bLedOn)
 		SwLedOff(padapter, pLed);
 	else
-		SwLedOn(padapter, pLed);
+		SwLedOn(pLed);
 
 	switch (pLed->CurrLedState) {
 	case LED_BLINK_SLOWLY:
-- 
2.30.2

[PATCH 05/10] staging: r8188eu: SwLedOff needs no padapter parameter

[Martin Kaiser]

Remove the padapter parameter from the SwLedOff function. padapter can
be derived from the pLed parameter.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_led.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
index aa8f41edfade..56f043d8ff38 100644
--- a/drivers/staging/r8188eu/core/rtw_led.c
+++ b/drivers/staging/r8188eu/core/rtw_led.c
@@ -42,8 +42,10 @@ static void SwLedOn(struct led_priv *pLed)
 	pLed->bLedOn = true;
 }
 
-static void SwLedOff(struct adapter *padapter, struct led_priv *pLed)
+static void SwLedOff(struct led_priv *pLed)
 {
+	struct adapter *padapter = container_of(pLed, struct adapter, ledpriv);
+
 	if (padapter->bDriverStopped)
 		return;
 
@@ -61,13 +63,13 @@ static void blink_work(struct work_struct *work)
 	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
 	if (padapter->pwrctrlpriv.rf_pwrstate != rf_on) {
-		SwLedOff(padapter, pLed);
+		SwLedOff(pLed);
 		ResetLedStatus(pLed);
 		return;
 	}
 
 	if (pLed->bLedOn)
-		SwLedOff(padapter, pLed);
+		SwLedOff(pLed);
 	else
 		SwLedOn(pLed);
 
@@ -141,7 +143,7 @@ void rtl8188eu_DeInitSwLeds(struct adapter *padapter)
 
 	cancel_delayed_work_sync(&ledpriv->blink_work);
 	ResetLedStatus(ledpriv);
-	SwLedOff(padapter, ledpriv);
+	SwLedOff(ledpriv);
 }
 
 void rtw_led_control(struct adapter *padapter, enum LED_CTL_MODE LedAction)
@@ -258,7 +260,7 @@ void rtw_led_control(struct adapter *padapter, enum LED_CTL_MODE LedAction)
 		pLed->bLedWPSBlinkInProgress = false;
 		pLed->bLedScanBlinkInProgress = false;
 		cancel_delayed_work(&pLed->blink_work);
-		SwLedOff(padapter, pLed);
+		SwLedOff(pLed);
 		break;
 	default:
 		break;
-- 
2.30.2

[PATCH 06/10] staging: r8188eu: remove two unused defines

[Martin Kaiser]

The C2H_MEM_SZ and FREE_CMDOBJ_SZ defines are not used by the r8188eu
driver. Remove them.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/include/rtw_cmd.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/r8188eu/include/rtw_cmd.h b/drivers/staging/r8188eu/include/rtw_cmd.h
index 9a76aa85de94..20a65beed166 100644
--- a/drivers/staging/r8188eu/include/rtw_cmd.h
+++ b/drivers/staging/r8188eu/include/rtw_cmd.h
@@ -8,13 +8,9 @@
 #include "rtw_rf.h"
 #include "rtw_led.h"
 
-#define C2H_MEM_SZ (16*1024)
-
 #include "osdep_service.h"
 #include "ieee80211.h" /*  <ieee80211/ieee80211.h> */
 
-#define FREE_CMDOBJ_SZ	128
-
 #define MAX_CMDSZ	1024
 #define MAX_RSPSZ	512
 #define MAX_EVTSZ	1024
-- 
2.30.2

[PATCH 07/10] staging: r8188eu: don't include rtw_led.h from rtw_cmd.h

[Martin Kaiser]

The rtw_cmd.h does not need any definitions from the led layer, there's
no reason to include rtw_led.h.

When I tried to remove this component

struct led_priv {
   struct adapter *padapter;
   ...

I saw compiler errors because of this chain of include files:
drv_types.h -> rtw_cmd.h -> rtw_led.h

rtw_led.h uses struct adapter before it sees the definiton near the end
of drv_types.h. (It seems that a simple struct adapter * prevents this
problem.)

The best option for fixing this issue is to not include rtw_led.h in
rtw_cmd.h.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/include/rtw_cmd.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/r8188eu/include/rtw_cmd.h b/drivers/staging/r8188eu/include/rtw_cmd.h
index 20a65beed166..2f4595f13e86 100644
--- a/drivers/staging/r8188eu/include/rtw_cmd.h
+++ b/drivers/staging/r8188eu/include/rtw_cmd.h
@@ -6,7 +6,6 @@
 
 #include "wlan_bssdef.h"
 #include "rtw_rf.h"
-#include "rtw_led.h"
 
 #include "osdep_service.h"
 #include "ieee80211.h" /*  <ieee80211/ieee80211.h> */
-- 
2.30.2

[PATCH 08/10] staging: r8188eu: remove padapter from struct led_priv

[Martin Kaiser]

The only struct led_priv that's used in the r8188eu driver in embedded in
the driver's global struct adapter. We can use container_of to access the
"outer" structure, there's no need to store a pointer to it.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_led.c    | 3 +--
 drivers/staging/r8188eu/include/rtw_led.h | 2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
index 56f043d8ff38..2dbd7b5ffdd0 100644
--- a/drivers/staging/r8188eu/core/rtw_led.c
+++ b/drivers/staging/r8188eu/core/rtw_led.c
@@ -59,7 +59,7 @@ static void blink_work(struct work_struct *work)
 {
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct led_priv *pLed = container_of(dwork, struct led_priv, blink_work);
-	struct adapter *padapter = pLed->padapter;
+	struct adapter *padapter = container_of(pLed, struct adapter, ledpriv);
 	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
 	if (padapter->pwrctrlpriv.rf_pwrstate != rf_on) {
@@ -132,7 +132,6 @@ void rtl8188eu_InitSwLeds(struct adapter *padapter)
 {
 	struct led_priv *pledpriv = &padapter->ledpriv;
 
-	pledpriv->padapter = padapter;
 	ResetLedStatus(pledpriv);
 	INIT_DELAYED_WORK(&pledpriv->blink_work, blink_work);
 }
diff --git a/drivers/staging/r8188eu/include/rtw_led.h b/drivers/staging/r8188eu/include/rtw_led.h
index f57dcf6c8b24..ea5f5edd9013 100644
--- a/drivers/staging/r8188eu/include/rtw_led.h
+++ b/drivers/staging/r8188eu/include/rtw_led.h
@@ -33,8 +33,6 @@ enum LED_STATE_871x {
 };
 
 struct led_priv {
-	struct adapter *padapter;
-
 	bool bRegUseLed;
 
 	enum LED_STATE_871x	CurrLedState; /*  Current LED state. */
-- 
2.30.2

[PATCH 09/10] staging: r8188eu: set two more state variables

[Martin Kaiser]

Set two more state variables in the blink worker when scan blinking and
tx/rx blinking are finished.

bLedBlinkInProgress is true during tx/rx blinking, bLedScanBlinkInProgress
is true during scan blinking. If we doing neither of the two, we may
safely set both variables to false.

This change makes the scan and tx/rx cases almost identical, we are now
ready to summarize the two cases.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_led.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
index 2dbd7b5ffdd0..f8bd183fba1e 100644
--- a/drivers/staging/r8188eu/core/rtw_led.c
+++ b/drivers/staging/r8188eu/core/rtw_led.c
@@ -90,6 +90,7 @@ static void blink_work(struct work_struct *work)
 				pLed->CurrLedState = LED_BLINK_SLOWLY;
 				schedule_delayed_work(&pLed->blink_work, LED_BLINK_NO_LINK_INTVL);
 			}
+			pLed->bLedBlinkInProgress = false;
 			pLed->bLedScanBlinkInProgress = false;
 		} else {
 			schedule_delayed_work(&pLed->blink_work, LED_BLINK_SCAN_INTVL);
@@ -106,6 +107,7 @@ static void blink_work(struct work_struct *work)
 				schedule_delayed_work(&pLed->blink_work, LED_BLINK_NO_LINK_INTVL);
 			}
 			pLed->bLedBlinkInProgress = false;
+			pLed->bLedScanBlinkInProgress = false;
 		} else {
 			schedule_delayed_work(&pLed->blink_work, LED_BLINK_FASTER_INTVL);
 		}
-- 
2.30.2

[PATCH 10/10] staging: r8188eu: summarize tx/rx and scan blinking

[Martin Kaiser]

Summarize the code for tx/rx blinking and for scan blinking in blink_work.
The only difference is the delay for scheduling the next worker.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_led.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
index f8bd183fba1e..ce8de2eb7845 100644
--- a/drivers/staging/r8188eu/core/rtw_led.c
+++ b/drivers/staging/r8188eu/core/rtw_led.c
@@ -81,21 +81,6 @@ static void blink_work(struct work_struct *work)
 		schedule_delayed_work(&pLed->blink_work, LED_BLINK_LINK_INTVL);
 		break;
 	case LED_BLINK_SCAN:
-		pLed->BlinkTimes--;
-		if (pLed->BlinkTimes == 0) {
-			if (check_fwstate(pmlmepriv, _FW_LINKED)) {
-				pLed->CurrLedState = LED_BLINK_NORMAL;
-				schedule_delayed_work(&pLed->blink_work, LED_BLINK_LINK_INTVL);
-			} else {
-				pLed->CurrLedState = LED_BLINK_SLOWLY;
-				schedule_delayed_work(&pLed->blink_work, LED_BLINK_NO_LINK_INTVL);
-			}
-			pLed->bLedBlinkInProgress = false;
-			pLed->bLedScanBlinkInProgress = false;
-		} else {
-			schedule_delayed_work(&pLed->blink_work, LED_BLINK_SCAN_INTVL);
-		}
-		break;
 	case LED_BLINK_TXRX:
 		pLed->BlinkTimes--;
 		if (pLed->BlinkTimes == 0) {
@@ -109,7 +94,9 @@ static void blink_work(struct work_struct *work)
 			pLed->bLedBlinkInProgress = false;
 			pLed->bLedScanBlinkInProgress = false;
 		} else {
-			schedule_delayed_work(&pLed->blink_work, LED_BLINK_FASTER_INTVL);
+			schedule_delayed_work(&pLed->blink_work,
+					      pLed->CurrLedState == LED_BLINK_SCAN ?
+					      LED_BLINK_SCAN_INTVL : LED_BLINK_FASTER_INTVL);
 		}
 		break;
 	case LED_BLINK_WPS:
-- 
2.30.2

Re: [PATCH 03/10] staging: r8188eu: fix status updates in SwLedOff

[Pavel Skripkin]

Hi Martin,

Martin Kaiser <martin@kaiser.cx> says:
> Update bLedOn only if we could update the REG_LEDCFG2 register.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>   drivers/staging/r8188eu/core/rtw_led.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
> index 4f1cad890cae..38433296d327 100644
> --- a/drivers/staging/r8188eu/core/rtw_led.c
> +++ b/drivers/staging/r8188eu/core/rtw_led.c
> @@ -43,10 +43,11 @@ static void SwLedOn(struct adapter *padapter, struct led_priv *pLed)
>   static void SwLedOff(struct adapter *padapter, struct led_priv *pLed)
>   {
>   	if (padapter->bDriverStopped)
> -		goto exit;
> +		return;
> +
> +	if (rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3)) != _SUCCESS)
> +		return;
>   
> -	rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3));
> -exit:
>   	pLed->bLedOn = false;
>   }
>  

If we don't always update the state then, I think, it's better to inform 
the callers about it

I guess, this won't happen often, but you are changing semantic of the 
function



With regards,
Pavel Skripkin

Re: [PATCH 10/10] staging: r8188eu: summarize tx/rx and scan blinking

[Philipp Hortmann]

On 10/15/22 17:11, Martin Kaiser wrote:
> Summarize the code for tx/rx blinking and for scan blinking in blink_work.
> The only difference is the delay for scheduling the next worker.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>   drivers/staging/r8188eu/core/rtw_led.c | 19 +++----------------
>   1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
> index f8bd183fba1e..ce8de2eb7845 100644
> --- a/drivers/staging/r8188eu/core/rtw_led.c
> +++ b/drivers/staging/r8188eu/core/rtw_led.c
> @@ -81,21 +81,6 @@ static void blink_work(struct work_struct *work)
>   		schedule_delayed_work(&pLed->blink_work, LED_BLINK_LINK_INTVL);
>   		break;
>   	case LED_BLINK_SCAN:
> -		pLed->BlinkTimes--;
> -		if (pLed->BlinkTimes == 0) {
> -			if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> -				pLed->CurrLedState = LED_BLINK_NORMAL;
> -				schedule_delayed_work(&pLed->blink_work, LED_BLINK_LINK_INTVL);
> -			} else {
> -				pLed->CurrLedState = LED_BLINK_SLOWLY;
> -				schedule_delayed_work(&pLed->blink_work, LED_BLINK_NO_LINK_INTVL);
> -			}
> -			pLed->bLedBlinkInProgress = false;
> -			pLed->bLedScanBlinkInProgress = false;
> -		} else {
> -			schedule_delayed_work(&pLed->blink_work, LED_BLINK_SCAN_INTVL);
> -		}
> -		break;
>   	case LED_BLINK_TXRX:
>   		pLed->BlinkTimes--;
>   		if (pLed->BlinkTimes == 0) {
> @@ -109,7 +94,9 @@ static void blink_work(struct work_struct *work)
>   			pLed->bLedBlinkInProgress = false;
>   			pLed->bLedScanBlinkInProgress = false;
>   		} else {
> -			schedule_delayed_work(&pLed->blink_work, LED_BLINK_FASTER_INTVL);
> +			schedule_delayed_work(&pLed->blink_work,
> +					      pLed->CurrLedState == LED_BLINK_SCAN ?
> +					      LED_BLINK_SCAN_INTVL : LED_BLINK_FASTER_INTVL);
>   		}
>   		break;
>   	case LED_BLINK_WPS:


I cannot apply this patch.

Applying: staging: r8188eu: summarize tx/rx and scan blinking
error: patch failed: drivers/staging/r8188eu/core/rtw_led.c:81
error: drivers/staging/r8188eu/core/rtw_led.c: patch does not apply
Patch failed at 0001 staging: r8188eu: summarize tx/rx and scan blinking

Re: [PATCH 01/10] staging: r8188eu: fix led register settings

[Michael Straube]

On 10/15/22 17:11, Martin Kaiser wrote:
> Using an InterTech DMG-02 dongle, the led remains on when the system goes
> into standby mode. After wakeup, it's no longer possible to control the
> led.
> 
> It turned out that the register settings to enable or disable the led were
> not correct. They worked for some dongles like the Edimax V2 but not for
> others like the InterTech DMG-02.
> 
> This patch fixes the register settings. Bit 3 in the led_cfg2 register
> controls the led status, bit 5 must always be set to be able to control
> the led, bit 6 has no influence on the led. Setting the mac_pinmux_cfg
> register is not necessary.
> 
> These settings were tested with Edimax V2 and InterTech DMG-02.
> 
> Cc: stable@vger.kernel.org
> Fixes: 8cd574e6af54 ("staging: r8188eu: introduce new hal dir for RTL8188eu driver")
> Suggested-by: Michael Straube <straube.linux@gmail.com>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>   drivers/staging/r8188eu/core/rtw_led.c | 25 ++-----------------------
>   1 file changed, 2 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
> index 2527c252c3e9..5b214488571b 100644
> --- a/drivers/staging/r8188eu/core/rtw_led.c
> +++ b/drivers/staging/r8188eu/core/rtw_led.c
> @@ -31,40 +31,19 @@ static void ResetLedStatus(struct led_priv *pLed)
>   
>   static void SwLedOn(struct adapter *padapter, struct led_priv *pLed)
>   {
> -	u8	LedCfg;
> -	int res;
> -
>   	if (padapter->bDriverStopped)
>   		return;
>   
> -	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);
> -	if (res)
> -		return;
> -
> -	rtw_write8(padapter, REG_LEDCFG2, (LedCfg & 0xf0) | BIT(5) | BIT(6)); /*  SW control led0 on. */
> +	rtw_write8(padapter, REG_LEDCFG2, BIT(5)); /*  SW control led0 on. */
>   	pLed->bLedOn = true;
>   }
>   
>   static void SwLedOff(struct adapter *padapter, struct led_priv *pLed)
>   {
> -	u8	LedCfg;
> -	int res;
> -
>   	if (padapter->bDriverStopped)
>   		goto exit;
>   
> -	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);/* 0x4E */
> -	if (res)
> -		goto exit;
> -
> -	LedCfg &= 0x90; /*  Set to software control. */
> -	rtw_write8(padapter, REG_LEDCFG2, (LedCfg | BIT(3)));
> -	res = rtw_read8(padapter, REG_MAC_PINMUX_CFG, &LedCfg);
> -	if (res)
> -		goto exit;
> -
> -	LedCfg &= 0xFE;
> -	rtw_write8(padapter, REG_MAC_PINMUX_CFG, LedCfg);
> +	rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3));
>   exit:
>   	pLed->bLedOn = false;
>   }

I tested this also with a TP-Link TL-WN725N now and it works fine.

Tested-by: Michael Straube <straube.linux@gmail.com> # InterTech DMG-02, 
TP-Link TL-WN725N

Re: [PATCH 10/10] staging: r8188eu: summarize tx/rx and scan blinking

[Martin Kaiser]

Hi Philipp,

Thus wrote Philipp Hortmann (philipp.g.hortmann@gmail.com):

> On 10/15/22 17:11, Martin Kaiser wrote:
> > Summarize the code for tx/rx blinking and for scan blinking in blink_work.
> > The only difference is the delay for scheduling the next worker.

> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> > ---
> >   drivers/staging/r8188eu/core/rtw_led.c | 19 +++----------------
> >   1 file changed, 3 insertions(+), 16 deletions(-)

> > diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
> > index f8bd183fba1e..ce8de2eb7845 100644
> > --- a/drivers/staging/r8188eu/core/rtw_led.c
> > +++ b/drivers/staging/r8188eu/core/rtw_led.c
> > @@ -81,21 +81,6 @@ static void blink_work(struct work_struct *work)
> >   		schedule_delayed_work(&pLed->blink_work, LED_BLINK_LINK_INTVL);
> >   		break;
> >   	case LED_BLINK_SCAN:
> > -		pLed->BlinkTimes--;
> > -		if (pLed->BlinkTimes == 0) {
> > -			if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> > -				pLed->CurrLedState = LED_BLINK_NORMAL;
> > -				schedule_delayed_work(&pLed->blink_work, LED_BLINK_LINK_INTVL);
> > -			} else {
> > -				pLed->CurrLedState = LED_BLINK_SLOWLY;
> > -				schedule_delayed_work(&pLed->blink_work, LED_BLINK_NO_LINK_INTVL);
> > -			}
> > -			pLed->bLedBlinkInProgress = false;
> > -			pLed->bLedScanBlinkInProgress = false;
> > -		} else {
> > -			schedule_delayed_work(&pLed->blink_work, LED_BLINK_SCAN_INTVL);
> > -		}
> > -		break;
> >   	case LED_BLINK_TXRX:
> >   		pLed->BlinkTimes--;
> >   		if (pLed->BlinkTimes == 0) {
> > @@ -109,7 +94,9 @@ static void blink_work(struct work_struct *work)
> >   			pLed->bLedBlinkInProgress = false;
> >   			pLed->bLedScanBlinkInProgress = false;
> >   		} else {
> > -			schedule_delayed_work(&pLed->blink_work, LED_BLINK_FASTER_INTVL);
> > +			schedule_delayed_work(&pLed->blink_work,
> > +					      pLed->CurrLedState == LED_BLINK_SCAN ?
> > +					      LED_BLINK_SCAN_INTVL : LED_BLINK_FASTER_INTVL);
> >   		}
> >   		break;
> >   	case LED_BLINK_WPS:


> I cannot apply this patch.

> Applying: staging: r8188eu: summarize tx/rx and scan blinking
> error: patch failed: drivers/staging/r8188eu/core/rtw_led.c:81
> error: drivers/staging/r8188eu/core/rtw_led.c: patch does not apply
> Patch failed at 0001 staging: r8188eu: summarize tx/rx and scan blinking

did you apply the patch "staging: r8188eu: remove bLedLinkBlinkInProgress"
(that I sent on Oct 1st) before this series?

I didn't mention this dependency in the cover letter, sorry for that.

Thanks,
Martin

Re: [PATCH 03/10] staging: r8188eu: fix status updates in SwLedOff

[Martin Kaiser]

Hi Pavel,

Thus wrote Pavel Skripkin (paskripkin@gmail.com):

> Hi Martin,

> Martin Kaiser <martin@kaiser.cx> says:
> > Update bLedOn only if we could update the REG_LEDCFG2 register.

> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> > ---
> >   drivers/staging/r8188eu/core/rtw_led.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)

> > diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
> > index 4f1cad890cae..38433296d327 100644
> > --- a/drivers/staging/r8188eu/core/rtw_led.c
> > +++ b/drivers/staging/r8188eu/core/rtw_led.c
> > @@ -43,10 +43,11 @@ static void SwLedOn(struct adapter *padapter, struct led_priv *pLed)
> >   static void SwLedOff(struct adapter *padapter, struct led_priv *pLed)
> >   {
> >   	if (padapter->bDriverStopped)
> > -		goto exit;
> > +		return;
> > +
> > +	if (rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3)) != _SUCCESS)
> > +		return;
> > -	rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3));
> > -exit:
> >   	pLed->bLedOn = false;
> >   }

> If we don't always update the state then, I think, it's better to inform the
> callers about it

> I guess, this won't happen often, but you are changing semantic of the
> function

Changing the state without changing the led feels like a bug to me. It's
done only for SwLedOff, nor for SwLedOn.

We could add a return value and inform the caller that we could not
change the led register.

How would callers of SwLedOn or SwLedLOff handle such errors? blink_work
looks at bLedOn and calls either SwLedOn or SwLedOff. If bLedOn is not
updated and the led is not changed, the next run of the worker will
retry. This does already happen with the current code, a return value of
SwLedOn/Off would not help here.

Best regards,
Martin

Re: [PATCH 01/10] staging: r8188eu: fix led register settings

[Martin Kaiser]

Hi Michael,

Thus wrote Michael Straube (straube.linux@gmail.com):

> On 10/15/22 17:11, Martin Kaiser wrote:
> > Using an InterTech DMG-02 dongle, the led remains on when the system goes
> > into standby mode. After wakeup, it's no longer possible to control the
> > led.

> > It turned out that the register settings to enable or disable the led were
> > not correct. They worked for some dongles like the Edimax V2 but not for
> > others like the InterTech DMG-02.

> > This patch fixes the register settings. Bit 3 in the led_cfg2 register
> > controls the led status, bit 5 must always be set to be able to control
> > the led, bit 6 has no influence on the led. Setting the mac_pinmux_cfg
> > register is not necessary.

> > These settings were tested with Edimax V2 and InterTech DMG-02.

> > Cc: stable@vger.kernel.org
> > Fixes: 8cd574e6af54 ("staging: r8188eu: introduce new hal dir for RTL8188eu driver")
> > Suggested-by: Michael Straube <straube.linux@gmail.com>
> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> > ---
> >   drivers/staging/r8188eu/core/rtw_led.c | 25 ++-----------------------
> >   1 file changed, 2 insertions(+), 23 deletions(-)

> > diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
> > index 2527c252c3e9..5b214488571b 100644
> > --- a/drivers/staging/r8188eu/core/rtw_led.c
> > +++ b/drivers/staging/r8188eu/core/rtw_led.c
> > @@ -31,40 +31,19 @@ static void ResetLedStatus(struct led_priv *pLed)
> >   static void SwLedOn(struct adapter *padapter, struct led_priv *pLed)
> >   {
> > -	u8	LedCfg;
> > -	int res;
> > -
> >   	if (padapter->bDriverStopped)
> >   		return;
> > -	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);
> > -	if (res)
> > -		return;
> > -
> > -	rtw_write8(padapter, REG_LEDCFG2, (LedCfg & 0xf0) | BIT(5) | BIT(6)); /*  SW control led0 on. */
> > +	rtw_write8(padapter, REG_LEDCFG2, BIT(5)); /*  SW control led0 on. */
> >   	pLed->bLedOn = true;
> >   }
> >   static void SwLedOff(struct adapter *padapter, struct led_priv *pLed)
> >   {
> > -	u8	LedCfg;
> > -	int res;
> > -
> >   	if (padapter->bDriverStopped)
> >   		goto exit;
> > -	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);/* 0x4E */
> > -	if (res)
> > -		goto exit;
> > -
> > -	LedCfg &= 0x90; /*  Set to software control. */
> > -	rtw_write8(padapter, REG_LEDCFG2, (LedCfg | BIT(3)));
> > -	res = rtw_read8(padapter, REG_MAC_PINMUX_CFG, &LedCfg);
> > -	if (res)
> > -		goto exit;
> > -
> > -	LedCfg &= 0xFE;
> > -	rtw_write8(padapter, REG_MAC_PINMUX_CFG, LedCfg);
> > +	rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3));
> >   exit:
> >   	pLed->bLedOn = false;
> >   }

> I tested this also with a TP-Link TL-WN725N now and it works fine.

> Tested-by: Michael Straube <straube.linux@gmail.com> # InterTech DMG-02,
> TP-Link TL-WN725N

thanks for testing with yet another device! Good to hear that the
settings work there as well.

Best regards,
Martin

Re: [PATCH 00/10] staging: r8188eu: led layer fix and cleanups

[Philipp Hortmann]

On 10/15/22 17:11, Martin Kaiser wrote:
> Here's another series to clean up the led layer and fix a problem with
> leds during suspend/resume on some devices.
> 
> Martin Kaiser (10):
>    staging: r8188eu: fix led register settings
>    staging: r8188eu: handle rtw_write8 errors in SwLedOn
>    staging: r8188eu: fix status updates in SwLedOff
>    staging: r8188eu: SwLedOn needs no padapter parameter
>    staging: r8188eu: SwLedOff needs no padapter parameter
>    staging: r8188eu: remove two unused defines
>    staging: r8188eu: don't include rtw_led.h from rtw_cmd.h
>    staging: r8188eu: remove padapter from struct led_priv
>    staging: r8188eu: set two more state variables
>    staging: r8188eu: summarize tx/rx and scan blinking
> 
>   drivers/staging/r8188eu/core/rtw_led.c    | 62 +++++++----------------
>   drivers/staging/r8188eu/include/rtw_cmd.h |  5 --
>   drivers/staging/r8188eu/include/rtw_led.h |  2 -
>   3 files changed, 18 insertions(+), 51 deletions(-)
> 

Apply Patch:
"staging: r8188eu: remove bLedLinkBlinkInProgress"
before this series.

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150