staging: r8188eu: core: remove condition with no effect

PATCH In review Saurav Girepunje 5 participants 11 messages Aug 21, 2021 – Aug 28, 2021 Staging

lore

Share & tools

lore

Phillip Potter

Acked-by on v1 1/1

2021-08-21 23:58 UTC #1 lore raw

Quoted 26 lines · Saurav Girepunje

Remove the condition with no effect (if == else) in rtw_led.c file.

Signed-off-by Saurav Girepunje <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_led.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/staging/r8188eu/core/rtw_led.c +1 -5

index 22d4df9c92a5..76cbd5f19f90 100644
@@ -148,10 +148,7 @@ static void SwLedBlink(struct LED_871x *pLed)
                        _set_timer(&(pLed->BlinkTimer), LED_BLINK_SLOWLY_INTERVAL);
                        break;
                case LED_BLINK_WPS:
-                       if (pLed->BlinkingLedState == RTW_LED_ON)
-                               _set_timer(&(pLed->BlinkTimer), LED_BLINK_LONG_INTERVAL);
-                       else
-                               _set_timer(&(pLed->BlinkTimer), LED_BLINK_LONG_INTERVAL);
+                       _set_timer(&(pLed->BlinkTimer), LED_BLINK_LONG_INTERVAL);
                        break;
                default:
                        _set_timer(&(pLed->BlinkTimer), LED_BLINK_SLOWLY_INTERVAL);
--

Quoted 2 lines

2.30.2

Thanks for this, looks good.

Acked-by Phillip Potter <[email protected]>

Regards,
Phil

Fabio M. De Francesco

2021-08-22 11:06 UTC #2 lore raw

Quoted 12 lines · CEST Phillip Potter

On Sat, 21 Aug 2021 at 21:23, Saurav Girepunje
Quoted 8 lines · <[email protected]> wrote:
Remove the condition with no effect (if == else) in rtw_led.c file.
Signed-off-by Saurav Girepunje <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_led.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Quoted 6 lines

Thanks for this, looks good.
Acked-by Phillip Potter <[email protected]>
Regards,
Phil

Dear Philip,

Before acking, please check at least if it applies to the current version of the tree and check if it compiles without adding warnings and / or errors. :-)

Thanks,

Fabio

Greg KH

maintainer

2021-08-22 12:36 UTC #3 lore raw

Quoted 17 lines · Fabio M. De Francesco

Quoted 16 lines · CEST Phillip Potter
On Sat, 21 Aug 2021 at 21:23, Saurav Girepunje
Quoted 8 lines · <[email protected]> wrote:
Remove the condition with no effect (if == else) in rtw_led.c file.
Signed-off-by Saurav Girepunje <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_led.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Thanks for this, looks good.
Acked-by Phillip Potter <[email protected]>

Quoted 7 lines

Quoted 2 lines
Regards,
Phil
Dear Philip,
Before acking, please check at least if it applies to the current version of the tree and check if it compiles without adding warnings and / or errors. :-)

That is not necessary or needed here, I can do that when I apply the patch.

greg k-h

Greg KH

maintainer

2021-08-26 10:35 UTC #4 lore raw

Quoted 21 lines · Greg KH

Quoted 20 lines · Fabio M. De Francesco
Quoted 18 lines · CEST Phillip Potter
On Sat, 21 Aug 2021 at 21:23, Saurav Girepunje
Quoted 8 lines · <[email protected]> wrote:
Remove the condition with no effect (if == else) in rtw_led.c file.
Signed-off-by Saurav Girepunje <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_led.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Thanks for this, looks good.
Acked-by Phillip Potter <[email protected]>
Regards,
Phil

Quoted 7 lines

Quoted 4 lines
Dear Philip,
Before acking, please check at least if it applies to the current version of the tree and check if it compiles without adding warnings and / or errors. :-)
That is not necessary or needed here, I can do that when I apply the patch.

As proof of that, this patch applied just fine to my tree right now...

thanks,

greg k-h

SAURAV GIREPUNJE

author

2021-08-28 12:24 UTC #5 lore raw

Quoted 27 lines · Greg KH

Quoted 26 lines · Greg KH
Quoted 24 lines · Fabio M. De Francesco
Quoted 12 lines · CEST Phillip Potter
On Sat, 21 Aug 2021 at 21:23, Saurav Girepunje
Quoted 8 lines · <[email protected]> wrote:
Remove the condition with no effect (if == else) in rtw_led.c file.
Signed-off-by Saurav Girepunje <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_led.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Quoted 6 lines
Thanks for this, looks good.
Acked-by Phillip Potter <[email protected]>
Regards,
Phil
Dear Philip,
Before acking, please check at least if it applies to the current version of the tree and check if it compiles without adding warnings and / or errors. :-)

Quoted 8 lines

Quoted 2 lines
That is not necessary or needed here, I can do that when I apply the patch.
As proof of that, this patch applied just fine to my tree right now...
thanks,
greg k-h

Thanks greg

Phillip Potter

2021-08-22 14:58 UTC #6 lore raw

In reply to Fabio M. De Francesco

Quoted 21 lines · Fabio M. De Francesco

Quoted 12 lines · CEST Phillip Potter
On Sat, 21 Aug 2021 at 21:23, Saurav Girepunje
Quoted 8 lines · <[email protected]> wrote:
Remove the condition with no effect (if == else) in rtw_led.c file.
Signed-off-by Saurav Girepunje <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_led.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Quoted 6 lines
Thanks for this, looks good.
Acked-by Phillip Potter <[email protected]>
Regards,
Phil

Quoted 12 lines

Dear Philip,
Before acking, please check at least if it applies to the current version of the tree and check if it compiles without adding warnings and / or errors. :-)
Thanks,
Fabio

Dear Fabio,

An Acked-by merely signals acknowledgement of the patch, and that is looks OK to the person offering the tag. Please see the following quote from the kernel.org documentation:
"Acked-by: is not as formal as Signed-off-by:. It is a record that the acker has at least reviewed the patch and has indicated acceptance." It is not, to my knowledge, a commitment from the reviewer that the patch applies to the given tree at that precise moment in time.

I reviewed the patch, and indicated my acceptance - the content of the patch is fine. Whilst I will often make an effort to merge + build test many patches, I will not do this with all of them, I simply don't have the time due to other commitments. You can be assured that if I have offered this tag I have at least read the patch and it looks correct to me.

Particularly with a driver as in flux as this one, there are going to be many merge conflicts. Advice such as this to me is not particularly helpful, as I can promise you I'm trying :-)

Regards,
Phil

Fabio M. De Francesco

2021-08-22 16:24 UTC #7 lore raw

Quoted 9 lines · CEST Phillip Potter

Dear Fabio,
An Acked-by merely signals acknowledgement of the patch, and that is looks OK to the person offering the tag. Please see the following quote from the kernel.org documentation:
"Acked-by: is not as formal as Signed-off-by:. It is a record that the acker has at least reviewed the patch and has indicated acceptance." It is not, to my knowledge, a commitment from the reviewer that the patch applies to the given tree at that precise moment in time.

Dear Philip,

I didn't mean to be harsh with you, I apologize if this is the message I conveyed. Really!

Quoted 6 lines

I reviewed the patch, and indicated my acceptance - the content of the patch is fine. Whilst I will often make an effort to merge + build test many patches, I will not do this with all of them, I simply don't have the time due to other commitments. You can be assured that if I have offered this tag I have at least read the patch and it looks correct to me.

Now it is clearer to me what acking means. I've given only a handful of acks because I thought I should also check if they applied and if they build. It takes time. Now I understand it is not required. Thanks.

Quoted 3 lines

Particularly with a driver as in flux as this one, there are going to be many merge conflicts. Advice such as this to me is not particularly helpful, as I can promise you I'm trying :-)

Please, don't ever think I'm not more than sure that you give a lot of your _unpaid_ time to the kernel and I thank you very much I know what it means, because I too have other commitments :-)

Cheers,

Fabio

Quoted 3 lines

Regards,
Phil

Phillip Potter

2021-08-22 23:58 UTC #8 lore raw

Quoted 6 lines · Fabio M. De Francesco

Dear Philip,
I didn't mean to be harsh with you, I apologize if this is the message I conveyed. Really!

Dear Fabio,

No apology necessary :-)

Quoted 6 lines

Now it is clearer to me what acking means. I've given only a handful of acks because I thought I should also check if they applied and if they build. It takes time. Now I understand it is not required. Thanks.

Yeah I try to ack as many as I can now, but couldn't possibly do as many as I do if I had to build test them all, especially with my own code to write and a family/full time non-kernel dev role. It is certainly desirable for the majority, but if the code looks good/correct and is a simple patch then in my mind an Acked-by is fine to give in any case. That said, I will no doubt make mistakes on that.

Quoted 4 lines

Please, don't ever think I'm not more than sure that you give a lot of your _unpaid_ time to the kernel and I thank you very much I know what it means, because I too have other commitments :-)

Thank you, and I appreciate your involvement so keep up the good work :-)

Regards,
Phil

SAURAV GIREPUNJE

author

2021-08-25 03:54 UTC #9 lore raw

Quoted 28 lines · Phillip Potter

Quoted 6 lines · Fabio M. De Francesco
Dear Philip,
I didn't mean to be harsh with you, I apologize if this is the message I conveyed. Really!
Dear Fabio,
No apology necessary :-)
Quoted 6 lines
Now it is clearer to me what acking means. I've given only a handful of acks because I thought I should also check if they applied and if they build. It takes time. Now I understand it is not required. Thanks.
Yeah I try to ack as many as I can now, but couldn't possibly do as many as I do if I had to build test them all, especially with my own code to write and a family/full time non-kernel dev role. It is certainly desirable for the majority, but if the code looks good/correct and is a simple patch then in my mind an Acked-by is fine to give in any case. That said, I will no doubt make mistakes on that.
Quoted 1 line

Quoted 8 lines

Quoted 3 lines
Please, don't ever think I'm not more than sure that you give a lot of your _unpaid_ time to the kernel and I thank you very much I know what it means, because I too have other commitments :-)
Thank you, and I appreciate your involvement so keep up the good work :-)
Regards,
Phil

Thanks Michael, Phil, Fabio for review.

Michael Straube

2021-08-22 10:59 UTC #10 lore raw

In reply to Saurav Girepunje

Quoted 25 lines · Saurav Girepunje

Remove the condition with no effect (if == else) in rtw_led.c file.

Signed-off-by Saurav Girepunje <[email protected]>

---
  drivers/staging/r8188eu/core/rtw_led.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)

drivers/staging/r8188eu/core/rtw_led.c +1 -5

index 22d4df9c92a5..76cbd5f19f90 100644
@@ -148,10 +148,7 @@ static void SwLedBlink(struct LED_871x *pLed)
  			_set_timer(&(pLed->BlinkTimer), LED_BLINK_SLOWLY_INTERVAL);
  			break;
  		case LED_BLINK_WPS:
-			if (pLed->BlinkingLedState == RTW_LED_ON)
-				_set_timer(&(pLed->BlinkTimer), LED_BLINK_LONG_INTERVAL);
-			else
-				_set_timer(&(pLed->BlinkTimer), LED_BLINK_LONG_INTERVAL);
+			_set_timer(&(pLed->BlinkTimer), LED_BLINK_LONG_INTERVAL);
  			break;
  		default:
  			_set_timer(&(pLed->BlinkTimer), LED_BLINK_SLOWLY_INTERVAL);
--

Quoted 3 lines

2.30.2

Hi Saurav,

this does not apply to staging-testing. Please rebase against staging-testing and send v2.

Thanks,

Michael