Staging: rts5208: sd: Remove redundant code

PATCH In review Shivani Bhardwaj 3 participants 9 messages Oct 13, 2015 – Oct 14, 2015 Staging

lore

Share & tools

lore

Shivani Bhardwaj

author

2015-10-13 23:00 UTC #0 lore raw

PATCH 1/1 Staging: rts5208: sd: Remove redundant code

Change the order of statements and remove extra code.

Signed-off-by Shivani Bhardwaj <[email protected]>

---
 drivers/staging/rts5208/sd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/staging/rts5208/sd.c +4 -7

index d6c4982..4269be9 100644
@@ -2707,17 +2707,14 @@ RTY_SD_RST:
 		return STATUS_FAIL;
 	}
 
+	CLR_SD_HCXC(sd_card);
+
 	if (hi_cap_flow) {
 		if (rsp[1] & 0x40)
 			SET_SD_HCXC(sd_card);
-		else
-			CLR_SD_HCXC(sd_card);
-
-		support_1v8 = false;
-	} else {
-		CLR_SD_HCXC(sd_card);
-		support_1v8 = false;
 	}
+
+	support_1v8 = false;
 	dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
 
 	if (support_1v8) {

Signature · 1 line

2.1.0

Greg KH

maintainer

2015-10-13 23:14 UTC #1 lore raw

Quoted 23 lines · Shivani Bhardwaj

Change the order of statements and remove extra code.

Signed-off-by Shivani Bhardwaj <[email protected]>

---
 drivers/staging/rts5208/sd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/staging/rts5208/sd.c +2 -3

index d6c4982..4269be9 100644
@@ -2707,17 +2707,14 @@ RTY_SD_RST:
 		return STATUS_FAIL;
 	}
 
+	CLR_SD_HCXC(sd_card);
+
 	if (hi_cap_flow) {
 		if (rsp[1] & 0x40)
 			SET_SD_HCXC(sd_card);
-		else
-			CLR_SD_HCXC(sd_card);
-

drivers/staging/rts5208/sd.c RTY_SD_RST: · line 2720

Quoted 10 lines

drivers/staging/rts5208/sd.c +2 -4

-		support_1v8 = false;
-	} else {
-		CLR_SD_HCXC(sd_card);
-		support_1v8 = false;
 	}
+
+	support_1v8 = false;
 	dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
 
 	if (support_1v8) {

Are you sure you didn't change the logic of what is happening here?

Why are you making this change? What prompted it?

thanks,

greg k-h

Shivani Bhardwaj

author

2015-10-13 23:24 UTC #2 lore raw

Quoted 37 lines · Greg KH

Quoted 23 lines · Shivani Bhardwaj

Change the order of statements and remove extra code.

Signed-off-by Shivani Bhardwaj <[email protected]>

---
 drivers/staging/rts5208/sd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/staging/rts5208/sd.c +2 -3

index d6c4982..4269be9 100644
@@ -2707,17 +2707,14 @@ RTY_SD_RST:
              return STATUS_FAIL;
      }
 
+     CLR_SD_HCXC(sd_card);
+
      if (hi_cap_flow) {
              if (rsp[1] & 0x40)
                      SET_SD_HCXC(sd_card);
-             else
-                     CLR_SD_HCXC(sd_card);
-

drivers/staging/rts5208/sd.c RTY_SD_RST: · line 2720

Quoted 10 lines

drivers/staging/rts5208/sd.c +2 -4

-             support_1v8 = false;
-     } else {
-             CLR_SD_HCXC(sd_card);
-             support_1v8 = false;
      }
+
+     support_1v8 = false;
      dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
 
      if (support_1v8) {

Are you sure you didn't change the logic of what is happening here?

Quoted 5 lines

Why are you making this change? What prompted it?
thanks,
greg k-h

Logic remains fine, I think. support_1v8 statement was common to if and else so it is taken out. Also, it can be noted that programmer wants to set sd_card value only if hi_cap_flow is not null and (rsp[1] & 0x40) is true. For rest of the cases, programmer wants to clear the sd_card.
I think I'm sure about the changes made.

I came across this code while running notnull.cocci against this. However, after I checked the code, cocci script was not very apt, I saw this, took references from
http://lxr.free-electrons.com/source/drivers/staging/rts5208/rtsx_chip.h#L421
about the macros and deleted the extra code by changing order. I've explained the logic above, if you still find any discrepancies, please tell me.

Thank you.

Greg KH

maintainer

2015-10-13 23:39 UTC #3 lore raw

Quoted 42 lines · Shivani Bhardwaj

Quoted 41 lines · Greg KH

Quoted 23 lines · Shivani Bhardwaj

Change the order of statements and remove extra code.

Signed-off-by Shivani Bhardwaj <[email protected]>

---
 drivers/staging/rts5208/sd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/staging/rts5208/sd.c +2 -3

index d6c4982..4269be9 100644
@@ -2707,17 +2707,14 @@ RTY_SD_RST:
              return STATUS_FAIL;
      }
 
+     CLR_SD_HCXC(sd_card);
+
      if (hi_cap_flow) {
              if (rsp[1] & 0x40)
                      SET_SD_HCXC(sd_card);
-             else
-                     CLR_SD_HCXC(sd_card);
-

drivers/staging/rts5208/sd.c RTY_SD_RST: · line 2720

Quoted 10 lines

drivers/staging/rts5208/sd.c +2 -4

-             support_1v8 = false;
-     } else {
-             CLR_SD_HCXC(sd_card);
-             support_1v8 = false;
      }
+
+     support_1v8 = false;
      dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
 
      if (support_1v8) {

Are you sure you didn't change the logic of what is happening here?

Why are you making this change? What prompted it?

thanks,

Quoted 8 lines

Quoted 1 line
greg k-h
Logic remains fine, I think. support_1v8 statement was common to if and else so it is taken out. Also, it can be noted that programmer wants to set sd_card value only if hi_cap_flow is not null and (rsp[1] & 0x40) is true. For rest of the cases, programmer wants to clear the sd_card.
I think I'm sure about the changes made.

But now you always call CLR_SD_HCXC() where before there was one code path that did not make that call. Are you sure it is safe to call CLR_SD_HCXC() before SET_SD_HCXC()?

thanks,

greg k-h

Shivani Bhardwaj

author

2015-10-13 23:45 UTC #4 lore raw

Quoted 52 lines · Greg KH

Quoted 42 lines · Shivani Bhardwaj
Quoted 41 lines · Greg KH
Quoted 23 lines · Shivani Bhardwaj
Change the order of statements and remove extra code.
Signed-off-by Shivani Bhardwaj <[email protected]>
---
 drivers/staging/rts5208/sd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/staging/rts5208/sd.c +2 -3
index d6c4982..4269be9 100644
@@ -2707,17 +2707,14 @@ RTY_SD_RST:
              return STATUS_FAIL;
      }
 
+     CLR_SD_HCXC(sd_card);
+
      if (hi_cap_flow) {
              if (rsp[1] & 0x40)
                      SET_SD_HCXC(sd_card);
-             else
-                     CLR_SD_HCXC(sd_card);
-
drivers/staging/rts5208/sd.c RTY_SD_RST: · line 2720
Quoted 10 lines
drivers/staging/rts5208/sd.c +2 -4
-             support_1v8 = false;
-     } else {
-             CLR_SD_HCXC(sd_card);
-             support_1v8 = false;
      }
+
+     support_1v8 = false;
      dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
 
      if (support_1v8) {
Are you sure you didn't change the logic of what is happening here?
Why are you making this change? What prompted it?
thanks,
Quoted 8 lines
Quoted 1 line
greg k-h
Logic remains fine, I think. support_1v8 statement was common to if and else so it is taken out. Also, it can be noted that programmer wants to set sd_card value only if hi_cap_flow is not null and (rsp[1] & 0x40) is true. For rest of the cases, programmer wants to clear the sd_card.
I think I'm sure about the changes made.

Quoted 7 lines

But now you always call CLR_SD_HCXC() where before there was one code path that did not make that call. Are you sure it is safe to call CLR_SD_HCXC() before SET_SD_HCXC()?
thanks,
greg k-h

I don't think it should be a problem because that is what we ultimately want in our code - to clear the memory card subject to constraints that one of the two if conditions is wrong. Now, if both the if statements, that is,
if (hi_cap_flow) and if (rsp[1] & 0x40)

turn out to be true, they set the cleared sd_card again.

That is what we wanted. If both the if statements are true - set the sd_card else clear it up. :)

Shivani Bhardwaj

author

2015-10-13 23:50 UTC #5 lore raw

Quoted 67 lines · Shivani Bhardwaj

Quoted 52 lines · Greg KH
Quoted 42 lines · Shivani Bhardwaj
Quoted 41 lines · Greg KH
Quoted 23 lines · Shivani Bhardwaj
Change the order of statements and remove extra code.
Signed-off-by Shivani Bhardwaj <[email protected]>
---
 drivers/staging/rts5208/sd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/staging/rts5208/sd.c +2 -3
index d6c4982..4269be9 100644
@@ -2707,17 +2707,14 @@ RTY_SD_RST:
              return STATUS_FAIL;
      }
 
+     CLR_SD_HCXC(sd_card);
+
      if (hi_cap_flow) {
              if (rsp[1] & 0x40)
                      SET_SD_HCXC(sd_card);
-             else
-                     CLR_SD_HCXC(sd_card);
-
drivers/staging/rts5208/sd.c RTY_SD_RST: · line 2720
Quoted 10 lines
drivers/staging/rts5208/sd.c +2 -4
-             support_1v8 = false;
-     } else {
-             CLR_SD_HCXC(sd_card);
-             support_1v8 = false;
      }
+
+     support_1v8 = false;
      dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
 
      if (support_1v8) {
Are you sure you didn't change the logic of what is happening here?
Why are you making this change? What prompted it?
thanks,
Quoted 8 lines
Quoted 1 line
greg k-h
Logic remains fine, I think. support_1v8 statement was common to if and else so it is taken out. Also, it can be noted that programmer wants to set sd_card value only if hi_cap_flow is not null and (rsp[1] & 0x40) is true. For rest of the cases, programmer wants to clear the sd_card.
I think I'm sure about the changes made.
Quoted 7 lines
But now you always call CLR_SD_HCXC() where before there was one code path that did not make that call. Are you sure it is safe to call CLR_SD_HCXC() before SET_SD_HCXC()?
thanks,
greg k-h
I don't think it should be a problem because that is what we ultimately want in our code - to clear the memory card subject to constraints that one of the two if conditions is wrong. Now, if both the if statements, that is,
if (hi_cap_flow) and if (rsp[1] & 0x40)

Quoted 4 lines

turn out to be true, they set the cleared sd_card again.
That is what we wanted. If both the if statements are true - set the sd_card else clear it up. :)

Also, a plus point is that even if the value is to be set (I do not know the inside mechanism of how it will happen) , there would not be any scope of errors (considering some bits are to be written/ logically operated) because sd_card would be empty. :)

Arnd Bergmann

2015-10-14 09:24 UTC #6 lore raw

In reply to Shivani Bhardwaj

Quoted 27 lines · On Wednesday 14 October 2015 05:15:12 Shivani Bhardwaj wrote:

Quoted 26 lines · Greg KH
Quoted 25 lines · Shivani Bhardwaj
Quoted 24 lines · Greg KH
Quoted 9 lines · Shivani Bhardwaj
Change the order of statements and remove extra code.
drivers/staging/rts5208/sd.c +2 -3
+     CLR_SD_HCXC(sd_card);
+
      if (hi_cap_flow) {
              if (rsp[1] & 0x40)
                      SET_SD_HCXC(sd_card);
-             else
-                     CLR_SD_HCXC(sd_card);
-
drivers/staging/rts5208/sd.c RTY_SD_RST: · line 2720
Quoted 10 lines
drivers/staging/rts5208/sd.c +2 -4
-             support_1v8 = false;
-     } else {
-             CLR_SD_HCXC(sd_card);
-             support_1v8 = false;
      }
+
+     support_1v8 = false;
      dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
 
      if (support_1v8) {
Are you sure you didn't change the logic of what is happening here?
Why are you making this change? What prompted it?

...

Quoted 10 lines

I don't think it should be a problem because that is what we ultimately want in our code - to clear the memory card subject to constraints that one of the two if conditions is wrong. Now, if both the if statements, that is,
if (hi_cap_flow) and if (rsp[1] & 0x40)
turn out to be true, they set the cleared sd_card again.
That is what we wanted. If both the if statements are true - set the sd_card else clear it up.

I guess this would be easier to verify if it were not obscured by the SET_SD_HCXC/CLR_SD_HCXC macros ;-)

Your change looks like a correct transformation, and it would be logical to remove the support_1v8 variable next, as it can never be true.

However, I wonder whether that is just a bug in the driver, as UHS-1 cards are required to support 1.8v operation, and there is clearly code to handle it here. Maybe one of them should actually say "support_1v8 = true".

Arnd

Shivani Bhardwaj

author

2015-10-14 10:16 UTC #7 lore raw

Quoted 47 lines · Arnd Bergmann

Quoted 27 lines · On Wednesday 14 October 2015 05:15:12 Shivani Bhardwaj wrote:
Quoted 26 lines · Greg KH
Quoted 25 lines · Shivani Bhardwaj
Quoted 24 lines · Greg KH
Quoted 9 lines · Shivani Bhardwaj
Change the order of statements and remove extra code.
drivers/staging/rts5208/sd.c +2 -3
+     CLR_SD_HCXC(sd_card);
+
      if (hi_cap_flow) {
              if (rsp[1] & 0x40)
                      SET_SD_HCXC(sd_card);
-             else
-                     CLR_SD_HCXC(sd_card);
-
drivers/staging/rts5208/sd.c RTY_SD_RST: · line 2720
Quoted 10 lines
drivers/staging/rts5208/sd.c +2 -4
-             support_1v8 = false;
-     } else {
-             CLR_SD_HCXC(sd_card);
-             support_1v8 = false;
      }
+
+     support_1v8 = false;
      dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
 
      if (support_1v8) {
Are you sure you didn't change the logic of what is happening here?
Why are you making this change? What prompted it?
...
Quoted 10 lines
I don't think it should be a problem because that is what we ultimately want in our code - to clear the memory card subject to constraints that one of the two if conditions is wrong. Now, if both the if statements, that is,
if (hi_cap_flow) and if (rsp[1] & 0x40)
turn out to be true, they set the cleared sd_card again.
That is what we wanted. If both the if statements are true - set the sd_card else clear it up.
I guess this would be easier to verify if it were not obscured by the SET_SD_HCXC/CLR_SD_HCXC macros ;-)
Your change looks like a correct transformation, and it would be logical to remove the support_1v8 variable next, as it can never be true.

Quoted 6 lines

However, I wonder whether that is just a bug in the driver, as UHS-1 cards are required to support 1.8v operation, and there is clearly code to handle it here. Maybe one of them should actually say "support_1v8 = true".
Arnd

I do not exactly understand the role of variables here. But, as far as I can guess (after having checked the secure digital) support_1v8 should be true for hi_cap_flow=false. That is, if capacity flow is less, we can allow voltage to be 1.8V otherwise it should be 3.3V. However, I really do not understand what the array rsp is for. So, I'm not sure what its status should be.
Please correct me if I am wrong.
Thanks

Arnd Bergmann

2015-10-14 13:44 UTC #8 lore raw

Quoted 26 lines · On Wednesday 14 October 2015 15:46:09 Shivani Bhardwaj wrote:

Quoted 25 lines · Arnd Bergmann
Quoted 8 lines · On Wednesday 14 October 2015 05:15:12 Shivani Bhardwaj wrote:
Quoted 1 line · Greg KH
On Wed, Oct 14, 2015 at 04:54:45AM +0530, Shivani Bhardwaj wrote:
I don't think it should be a problem because that is what we ultimately want in our code - to clear the memory card subject to constraints that one of the two if conditions is wrong. Now, if both the if statements, that is,
if (hi_cap_flow) and if (rsp[1] & 0x40)
Quoted 4 lines
turn out to be true, they set the cleared sd_card again.
That is what we wanted. If both the if statements are true - set the sd_card else clear it up.
I guess this would be easier to verify if it were not obscured by the SET_SD_HCXC/CLR_SD_HCXC macros
Your change looks like a correct transformation, and it would be logical to remove the support_1v8 variable next, as it can never be true.
However, I wonder whether that is just a bug in the driver, as UHS-1 cards are required to support 1.8v operation, and there is clearly code to handle it here. Maybe one of them should actually say "support_1v8 = true".

Quoted 5 lines

Quoted 1 line
Arnd
I do not exactly understand the role of variables here. But, as far as I can guess (after having checked the secure digital) support_1v8 should be true for hi_cap_flow=false.

Yes, that seems like a good guess, but it's impossible to know if that is correct without having a look at some datasheet or specification.

Quoted 5 lines

That is, if capacity flow is
less, we can allow voltage to be 1.8V otherwise it should be 3.3V. However, I really do not understand what the array rsp is for. So, I'm not sure what its status should be.
Please correct me if I am wrong.

'rsp' here is the response from this command:

retval = sd_send_cmd_get_rsp(chip, SD_APP_OP_COND, voltage,
SD_RSP_TYPE_R3, rsp, 5);

This is a standard SD card command (ACMD41) that is documented in the SD card "Physical Layer Simplified Specification" version 4.10.

I have not looked any further, but the answer is probably in there.

Arnd