Messages in this thread Patch in this message | | | Date | Sun, 9 Oct 2011 16:57:45 +0300 | From | Dan Carpenter <> | Subject | Re: [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);
| |