diff mbox series

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

Message ID 20221015151115.232095-4-martin@kaiser.cx
State New
Headers
Series staging: r8188eu: led layer fix and cleanups |

Commit Message

Martin Kaiser Oct. 15, 2022, 3:11 p.m. UTC 
  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(-)

Comments

Pavel Skripkin Oct. 15, 2022, 3:42 p.m. UTC | #1 
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
Martin Kaiser Oct. 16, 2022, 4:21 p.m. UTC | #2 
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
diff mbox series

Patch

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;
 }