* [PATCH] staging:rts_pstor:Fix SDIO issue
@ 2011-10-09 2:03 wei_wang
2011-10-09 6:06 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: wei_wang @ 2011-10-09 2:03 UTC (permalink / raw)
To: gregkh, devel, linux-kernel; +Cc: wwang
From: wwang <wei_wang@realsil.com.cn>
Fix a bug that SDIO and SD normal card would appear simultaneously if a SDIO card inserted.
Signed-off-by: wwang <wei_wang@realsil.com.cn>
---
drivers/staging/rts_pstor/sd.c | 17 ++++++++++++++---
1 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/rts_pstor/sd.c b/drivers/staging/rts_pstor/sd.c
index fb62eaf..9ce63f7 100644
--- a/drivers/staging/rts_pstor/sd.c
+++ b/drivers/staging/rts_pstor/sd.c
@@ -3094,6 +3094,7 @@ int reset_sd_card(struct rtsx_chip *chip)
{
struct sd_info *sd_card = &(chip->sd_card);
int retval;
+ int reset_pass = 0;
sd_init_reg_addr(chip);
@@ -3134,7 +3135,10 @@ 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)) {
@@ -3143,7 +3147,11 @@ int reset_sd_card(struct rtsx_chip *chip)
TRACE_RET(chip, STATUS_FAIL);
}
}
+ } else {
+ reset_pass = 1;
}
+ } else {
+ reset_pass = 1;
}
} else {
retval = reset_sd(chip);
@@ -3161,13 +3169,16 @@ int reset_sd_card(struct rtsx_chip *chip)
if (!chip->sd_io) {
retval = reset_mmc(chip);
+ if (retval == STATUS_SUCCESS)
+ reset_pass = 1;
}
+ } else {
+ reset_pass = 1;
}
}
- if (retval != STATUS_SUCCESS) {
+ if (!reset_pass)
TRACE_RET(chip, STATUS_FAIL);
- }
retval = sd_set_clock_divider(chip, SD_CLK_DIVIDE_0);
if (retval != STATUS_SUCCESS) {
--
1.7.7
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging:rts_pstor:Fix SDIO issue
2011-10-09 2:03 [PATCH] staging:rts_pstor:Fix SDIO issue wei_wang
@ 2011-10-09 6:06 ` Dan Carpenter
2011-10-09 6:48 ` wwang
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2011-10-09 6:06 UTC (permalink / raw)
To: wei_wang; +Cc: gregkh, devel, linux-kernel
Adding the check for if (sd_check_err_code(chip, SD_NO_CARD)) is
good, but introducing the new "reset_pass" variable is wrong.
I don't think you are updating the new variable consistently on
all paths. For example, if it's a 0x5209 chip and the code is like
this:
retval = sd_change_bank_voltage(chip, SD_IO_3V3);
if (retval != STATUS_SUCCESS) {
TRACE_RET(chip, STATUS_FAIL);
}
In the old system that was considered a successful reset, but under
your new system we don't update "reset_pass" here so we pass this
test and then we immediately return STATUS_FAIL because reset_pass
isn't set. If that's what you intended then just write it like that.
retval = sd_change_bank_voltage(chip, SD_IO_3V3);
TRACE_RET(chip, STATUS_FAIL);
So could you write it again but just set retval = STATUS_FAIL; for
the new failure pathes. Or better yet, just return STATUS_FAIL
directly. So this code would be:
> + if (retval == STATUS_SUCCESS)
> + reset_pass = 1;
if (retval != STATUS_SUCCESS)
TRACE_RET(chip, retval);
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging:rts_pstor:Fix SDIO issue
2011-10-09 6:06 ` Dan Carpenter
@ 2011-10-09 6:48 ` wwang
2011-10-09 13:57 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: wwang @ 2011-10-09 6:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: gregkh, devel, linux-kernel
Dear Carpenter:
Yes, the new code is exactly what I intended.
As to the following code, the old system responded fully wrong. It
should return STATUS_FAIL here instead.
3144 if (CHECK_PID(chip, 0x5209)) {
3145 retval = sd_change_bank_voltage(chip, SD_IO_3V3);
3146 if (retval != STATUS_SUCCESS) {
3147 TRACE_RET(chip, STATUS_FAIL);
3148 }
3149 }
Indeed, returning STATUS_FAIL directly would look like more consice and
neat. But in the following code (line 3164) it would not work because we
may need to call reset_mmc function. In order to keep the code
consistent, I introduce the "reset_pass" variable.
Another method to fix this bug is to add "retval = STATUS_FAIL" after
line 3167. But I don't think this style is clear enough, comparing with
adding a new variable.
3157 retval = reset_sd(chip);
3158 if (retval != STATUS_SUCCESS) {
3159 if (sd_check_err_code(chip, SD_NO_CARD)) {
3160 TRACE_RET(chip, STATUS_FAIL);
3161 }
3162
3163 if (CHECK_PID(chip, 0x5209)) {
3164 retval = sd_change_bank_voltage(chip, SD_IO_3V3);
3165 if (retval != STATUS_SUCCESS) {
3166 TRACE_RET(chip, STATUS_FAIL);
3167 }
3168 }
3169
3170 if (!chip->sd_io) {
3171 retval = reset_mmc(chip);
3172 if (retval == STATUS_SUCCESS)
3173 reset_pass = 1;
3174 }
3175 } else {
3176 reset_pass = 1;
3177 }
Best regards,
wwang
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging:rts_pstor:Fix SDIO issue
2011-10-09 6:48 ` wwang
@ 2011-10-09 13:57 ` Dan Carpenter
2011-10-10 1:51 ` wwang
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2011-10-09 13:57 UTC (permalink / raw)
To: wwang; +Cc: devel, gregkh, linux-kernel
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);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging:rts_pstor:Fix SDIO issue
2011-10-09 13:57 ` Dan Carpenter
@ 2011-10-10 1:51 ` wwang
0 siblings, 0 replies; 9+ messages in thread
From: wwang @ 2011-10-10 1:51 UTC (permalink / raw)
To: Dan Carpenter; +Cc: devel, gregkh, linux-kernel
Dear Carpenter:
I will resend the patch
Best Regards,
wwang
于 2011年10月09日 21:57, Dan Carpenter 写道:
> 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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] staging:rts_pstor:Fix SDIO issue
@ 2011-10-10 1:51 wei_wang
2011-10-10 6:12 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: wei_wang @ 2011-10-10 1:51 UTC (permalink / raw)
To: gregkh, devel, linux-kernel; +Cc: wwang
From: wwang <wei_wang@realsil.com.cn>
Fix a bug that SDIO and SD normal card would appear simultaneously if a SDIO card inserted.
Signed-off-by: wwang <wei_wang@realsil.com.cn>
---
drivers/staging/rts_pstor/sd.c | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/rts_pstor/sd.c b/drivers/staging/rts_pstor/sd.c
index fb62eaf..ebf529a 100644
--- a/drivers/staging/rts_pstor/sd.c
+++ b/drivers/staging/rts_pstor/sd.c
@@ -3134,14 +3134,15 @@ 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);
- }
+ sd_change_bank_voltage(chip, SD_IO_3V3);
+ TRACE_RET(chip, STATUS_FAIL);
}
}
}
@@ -3157,17 +3158,16 @@ int reset_sd_card(struct rtsx_chip *chip)
if (retval != STATUS_SUCCESS) {
TRACE_RET(chip, STATUS_FAIL);
}
+ retval = STATUS_FAIL;
}
- if (!chip->sd_io) {
+ if (!chip->sd_io)
retval = reset_mmc(chip);
- }
}
}
- if (retval != STATUS_SUCCESS) {
+ if (retval != STATUS_SUCCESS)
TRACE_RET(chip, STATUS_FAIL);
- }
retval = sd_set_clock_divider(chip, SD_CLK_DIVIDE_0);
if (retval != STATUS_SUCCESS) {
--
1.7.7
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging:rts_pstor:Fix SDIO issue
2011-10-10 1:51 wei_wang
@ 2011-10-10 6:12 ` Dan Carpenter
0 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2011-10-10 6:12 UTC (permalink / raw)
To: wei_wang; +Cc: gregkh, devel, linux-kernel
On Mon, Oct 10, 2011 at 09:51:44AM +0800, wei_wang@realsil.com.cn wrote:
> @@ -3157,17 +3158,16 @@ int reset_sd_card(struct rtsx_chip *chip)
> if (retval != STATUS_SUCCESS) {
> TRACE_RET(chip, STATUS_FAIL);
> }
> + retval = STATUS_FAIL;
> }
>
> - if (!chip->sd_io) {
> + if (!chip->sd_io)
> retval = reset_mmc(chip);
> - }
+ if (retval != STATUS_SUCCESS)
+ TRACE_RET(chip, STATUS_FAIL);
> }
> }
>
> - if (retval != STATUS_SUCCESS) {
> + if (retval != STATUS_SUCCESS)
> TRACE_RET(chip, STATUS_FAIL);
> - }
Then you can remove this check. It's better to have the checks for
failure as soon after the function call as possible.
>
> retval = sd_set_clock_divider(chip, SD_CLK_DIVIDE_0);
> if (retval != STATUS_SUCCESS) {
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] staging:rts_pstor:Fix SDIO issue
@ 2011-10-10 6:47 wei_wang
2011-10-10 7:55 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: wei_wang @ 2011-10-10 6:47 UTC (permalink / raw)
To: gregkh, devel, linux-kernel; +Cc: wwang
From: wwang <wei_wang@realsil.com.cn>
Fix a bug that SDIO and SD normal card would appear simultaneously if a SDIO card inserted.
Signed-off-by: wwang <wei_wang@realsil.com.cn>
---
drivers/staging/rts_pstor/sd.c | 31 +++++++++++++++----------------
1 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/rts_pstor/sd.c b/drivers/staging/rts_pstor/sd.c
index fb62eaf..aab6909 100644
--- a/drivers/staging/rts_pstor/sd.c
+++ b/drivers/staging/rts_pstor/sd.c
@@ -3134,41 +3134,40 @@ 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) {
+ if (chip->sd_io) {
+ TRACE_RET(chip, STATUS_FAIL);
+ } else {
retval = reset_mmc(chip);
+ if (retval != STATUS_SUCCESS)
+ TRACE_RET(chip, STATUS_FAIL);
}
}
}
- if (retval != STATUS_SUCCESS) {
- TRACE_RET(chip, STATUS_FAIL);
- }
-
retval = sd_set_clock_divider(chip, SD_CLK_DIVIDE_0);
if (retval != STATUS_SUCCESS) {
TRACE_RET(chip, STATUS_FAIL);
--
1.7.7
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-10-10 7:58 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-09 2:03 [PATCH] staging:rts_pstor:Fix SDIO issue wei_wang
2011-10-09 6:06 ` Dan Carpenter
2011-10-09 6:48 ` wwang
2011-10-09 13:57 ` Dan Carpenter
2011-10-10 1:51 ` wwang
2011-10-10 1:51 wei_wang
2011-10-10 6:12 ` Dan Carpenter
2011-10-10 6:47 wei_wang
2011-10-10 7:55 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®