Re: [PATCH] staging:rts_pstor:Fix SDIO issue
From: Dan Carpenter
Date: Sun Oct 09 2011 - 10:01:29 EST
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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/