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
Check the status returned by rtw_write8. Update bLedOn only if we could
update the REG_LEDCFG2 register.
Signed-off-by: Martin Kaiser <[email protected]>
---
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
Remove the padapter parameter from the SwLedOn function. padapter can be
derived from the pLed parameter.
Signed-off-by: Martin Kaiser <[email protected]>
---
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
Update bLedOn only if we could update the REG_LEDCFG2 register.
Signed-off-by: Martin Kaiser <[email protected]>
---
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
Remove the padapter parameter from the SwLedOff function. padapter can
be derived from the pLed parameter.
Signed-off-by: Martin Kaiser <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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: [email protected]
Fixes: 8cd574e6af54 ("staging: r8188eu: introduce new hal dir for RTL8188eu driver")
Suggested-by: Michael Straube <[email protected]>
Signed-off-by: Martin Kaiser <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
The C2H_MEM_SZ and FREE_CMDOBJ_SZ defines are not used by the r8188eu
driver. Remove them.
Signed-off-by: Martin Kaiser <[email protected]>
---
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
Hi Martin,
Martin Kaiser <[email protected]> says:
> Update bLedOn only if we could update the REG_LEDCFG2 register.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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: [email protected]
> Fixes: 8cd574e6af54 ("staging: r8188eu: introduce new hal dir for RTL8188eu driver")
> Suggested-by: Michael Straube <[email protected]>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> 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 <[email protected]> # InterTech DMG-02,
TP-Link TL-WN725N
Hi Michael,
Thus wrote Michael Straube ([email protected]):
> 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: [email protected]
> > Fixes: 8cd574e6af54 ("staging: r8188eu: introduce new hal dir for RTL8188eu driver")
> > Suggested-by: Michael Straube <[email protected]>
> > Signed-off-by: Martin Kaiser <[email protected]>
> > ---
> > 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 <[email protected]> # 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
Hi Philipp,
Thus wrote Philipp Hortmann ([email protected]):
> 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 <[email protected]>
> > ---
> > 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
Hi Pavel,
Thus wrote Pavel Skripkin ([email protected]):
> Hi Martin,
> Martin Kaiser <[email protected]> says:
> > Update bLedOn only if we could update the REG_LEDCFG2 register.
> > Signed-off-by: Martin Kaiser <[email protected]>
> > ---
> > 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
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 <[email protected]> # Edimax N150