lkml.org 
[lkml]   [2011]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] staging:rts_pstor:Fix SDIO issue
What I'm saying is, it's confusing to add another variable which is
only subtly different from retval. Here is what it looks like after
we apply the patch.

int reset_pass = 0;

/* skip 45 lines */
retval = sd_change_bank_voltage(chip, SD_IO_3V3);
if (retval != STATUS_SUCCESS) {
TRACE_RET(chip, STATUS_FAIL);
}
/* skip 30 lines */
if (!reset_pass)
TRACE_RET(chip, STATUS_FAIL);
The reviewer would have to read through 80 lines of code to find that
we don't care about the return value from sd_change_bank_voltage().
Don't do it that way.

Here is how I would write what it. I can't actually test this since
I don't have the hardware...

diff --git a/drivers/staging/rts_pstor/sd.c b/drivers/staging/rts_pstor/sd.c
index fb62eaf..46bf0e1 100644
--- a/drivers/staging/rts_pstor/sd.c
+++ b/drivers/staging/rts_pstor/sd.c
@@ -3134,39 +3134,36 @@ int reset_sd_card(struct rtsx_chip *chip)

if (chip->sd_ctl & RESET_MMC_FIRST) {
retval = reset_mmc(chip);
- if ((retval != STATUS_SUCCESS) && !sd_check_err_code(chip, SD_NO_CARD)) {
+ if (retval != STATUS_SUCCESS) {
+ if (sd_check_err_code(chip, SD_NO_CARD))
+ TRACE_RET(chip, STATUS_FAIL);
+
retval = reset_sd(chip);
if (retval != STATUS_SUCCESS) {
- if (CHECK_PID(chip, 0x5209)) {
- retval = sd_change_bank_voltage(chip, SD_IO_3V3);
- if (retval != STATUS_SUCCESS) {
- TRACE_RET(chip, STATUS_FAIL);
- }
- }
+ if (CHECK_PID(chip, 0x5209))
+ sd_change_bank_voltage(chip, SD_IO_3V3);
+ TRACE_RET(chip, STATUS_FAIL);
}
}
} else {
retval = reset_sd(chip);
if (retval != STATUS_SUCCESS) {
- if (sd_check_err_code(chip, SD_NO_CARD)) {
+ if (sd_check_err_code(chip, SD_NO_CARD))
TRACE_RET(chip, STATUS_FAIL);
- }

if (CHECK_PID(chip, 0x5209)) {
retval = sd_change_bank_voltage(chip, SD_IO_3V3);
- if (retval != STATUS_SUCCESS) {
+ if (retval != STATUS_SUCCESS)
TRACE_RET(chip, STATUS_FAIL);
- }
}

- if (!chip->sd_io) {
- retval = reset_mmc(chip);
- }
- }
- }
+ if (chip->sd_io)
+ TRACE_RET(chip, STATUS_FAIL);

- if (retval != STATUS_SUCCESS) {
- TRACE_RET(chip, STATUS_FAIL);
+ retval = reset_mmc(chip);
+ if (retval != STATUS_SUCCESS)
+ TRACE_RET(chip, STATUS_FAIL);
+ }
}

retval = sd_set_clock_divider(chip, SD_CLK_DIVIDE_0);

\
 
 \ /
  Last update: 2011-10-09 16:03    [from the cache]
©2003-2011 Jasper Spaans