Messages in this thread | | | Date | Sun, 31 Dec 2006 12:46:47 +0100 | From | Pierre Ossman <> | Subject | Re: [PATCH 2.6.19] mmc: Add support for SDHC cards |
| |
Philip Langdale wrote: > Hi all, > > >
*snip*
> Signed-off-by: Philipl Langdale <philipl@overt.org> >
When you have a commit message larger than the patch, you know there is something wrong. ;)
Please skip the part about MMC at least.
> @@ -588,12 +591,15 @@ > > if (mmc_card_sd(card)) { > csd_struct = UNSTUFF_BITS(resp, 126, 2); > - if (csd_struct != 0) { > + if (csd_struct != 0 && csd_struct != 1) { > printk("%s: unrecognised CSD structure version %d\n", > mmc_hostname(card->host), csd_struct); > mmc_card_set_bad(card); > return; > } > + if (csd_struct == 1) { > + mmc_card_set_blockaddr(card); > + } > > m = UNSTUFF_BITS(resp, 115, 4); > e = UNSTUFF_BITS(resp, 112, 3); >
Actually, the way the spec describes version 2.0 of the CSD leaves my stomach a bit upset. I think we will find cards that put bogus values in the fields, expecting most hosts to use the well defined values. So I'd prefer a switch statement here, where csd_struct == 1 results in hard coded values for many fields.
> @@ -824,6 +835,25 @@ > { > struct mmc_command cmd; > int i, err = 0; > + static const u8 test_pattern = 0xAA; > + > + /* > + * To support SD 2.0 cards, we must always invoke SD_SEND_IF_COND > + * before SD_APP_OP_COND. This command will harmlessly fail for > + * SD 1.0 and MMC cards (fortunately including MMC 4 cards). > + */ > + cmd.opcode = SD_SEND_IF_COND; > + cmd.arg = ((host->ocr_avail & 0xFF8000) != 0) << 8 | test_pattern; > + cmd.flags = MMC_RSP_R7 | MMC_CMD_BCR; > + > + err = mmc_wait_for_cmd(host, &cmd, 0); > + if (err == MMC_ERR_NONE) { > + if ((cmd.resp[0] & 0xFF) != test_pattern) { > + return MMC_ERR_FAILED; > + } else if (ocr != 0) { > + ocr |= 1 << 30; > + } > + } > > cmd.opcode = SD_APP_OP_COND; > cmd.arg = ocr; >
Put this in mmc_setup() with the rest of the initialisation. Hiding in here just confuses things.
Also, please add a comment about why you manipulate the ocr.
> @@ -43,6 +43,7 @@ > #define MMC_RSP_R2 (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC) > #define MMC_RSP_R3 (MMC_RSP_PRESENT) > #define MMC_RSP_R6 (MMC_RSP_PRESENT|MMC_RSP_CRC) > +#define MMC_RSP_R7 (MMC_RSP_PRESENT|MMC_RSP_CRC) > > #define mmc_resp_type(cmd) ((cmd)->flags & (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC|MMC_RSP_BUSY|MMC_RSP_OPCODE)) > >
Wrong. R7 is defined as MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE.
(So is R6 btw, wonder why that is...)
Rgds
-- -- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |