Dear Ajay Bhargav,
In message 1310106168-17166-3-git-send-email-ajay.bhargav@einfochips.com you wrote:
This patch provide support for MMC on GuruPlug-Display in uboot.
diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile index a8fe17a..7f88031 100644 --- a/drivers/mmc/Makefile +++ b/drivers/mmc/Makefile @@ -38,6 +38,7 @@ COBJS-$(CONFIG_OMAP3_MMC) += omap3_mmc.o COBJS-$(CONFIG_OMAP_HSMMC) += omap_hsmmc.o COBJS-$(CONFIG_PXA_MMC) += pxa_mmc.o COBJS-$(CONFIG_S5P_MMC) += s5p_mmc.o +COBJS-$(CONFIG_PXASDH) += pxa_sdh.o
Please keep list sorted.
--- /dev/null +++ b/drivers/mmc/pxa_sdh.c @@ -0,0 +1,677 @@ +/**************************************************************************
- Copyright (c) 2009, 2010 Marvell International Ltd.
Incorrect multi-line comment style. etc. etc.
...
+#define CLKRT_OFF (~0) +#define GET_REG(host, reg) readw(host->regbase + reg) +#define SET_REG(host, val, reg) writew(val, host->regbase + reg) +#define DATA_DIRECTION_READ(data) (data->flags & MMC_DATA_READ) +#define SET_REG_BIT(host, bit_mask, reg) \
SET_REG(host, \GET_REG(host, reg) | (bit_mask), reg)+#define CLEAR_REG_BIT(host, bit_mask, reg) \
SET_REG(host, \GET_REG(host, reg) & ~(bit_mask), reg)+#define SET_REG_BITS(host, bits_pos, bits_mask, val, reg) \
{SET_REG(host, \GET_REG(host, reg) & ~(bits_mask << bits_pos), reg); \SET_REG(host, \GET_REG(host, reg) | (val << bits_pos), reg); }+#define GET_REG_BITS(host, bit_pos, bits_mask, reg) \
((GET_REG(host, reg) >> bit_pos) & bits_mask)
NAK. Please don't invent your own accessors, instead use the existing ones. Also, make sure to use proper C structs instead of base address + offset notation.
+static void pxa_sdh_finish_request(struct pxa_sdh_host *host) +{
+#ifdef CONFIG_MMC_DEBUG
Drop that blank line above (and all similar ones).
...
do {ret = pxa_sdh_process_irq(host, (DMA_INT | XFER_COMP));/* If error or xfer completed (in which case* bytes_xfered is reset to 0) break */
Incorrect multiline-comment ...
+static int pxa_sdh_cmd_done(struct pxa_sdh_host *host) +{
- struct mmc_cmd *cmd = host->cmd;
- u32 resp[8];
- BUG_ON(!cmd);
- /* get cmd response */
- resp[0] = GET_REG(host, SD_RESP_0);
- resp[1] = GET_REG(host, SD_RESP_1);
- resp[2] = GET_REG(host, SD_RESP_2);
- resp[3] = GET_REG(host, SD_RESP_3);
- resp[4] = GET_REG(host, SD_RESP_4);
- resp[5] = GET_REG(host, SD_RESP_5);
- resp[6] = GET_REG(host, SD_RESP_6);
- resp[7] = readb(host->regbase + SD_RESP_7);
NAK - see above, please use proper I/O accessors.
...
char *src = (char *) data->src + host->bytes_xfered;for (i = 0; i < blk_size; i += sizeof(u32))writel(*(u32 *)(src + i),host->regbase + SD_BUF_DPORT_0);
Braces needed for multiline statement.
- if (host->bytes_xfered < host->data_len) {
host_dma_bdry_size = 0x1000 << (((u16) GET_REG(host,SD_BLOCK_SIZE)) >> HOST_DMA_BDRY_OFFSET);if (host_dma_bdry_size < data->blocksize)host->bytes_xfered = host->bytes_xfered +host_dma_bdry_size;
Braces needed for multiline statement.
elsehost->bytes_xfered = host->bytes_xfered +data->blocksize;
Braces needed for multiline statement. Please fix globally.
- switch (host->port_num) {
- case 0:
/* Setup the MMC/SD(1) Host Controller Clock */writel(0x19, 0xd4282854);udelay(10);writel(0x1B, 0xd4282854);break;- case 1:
/* Setup the MMC/SD(2) Host Controller Clock */writel(0x10, 0xd4282858);udelay(10);writel(0x12, 0xd4282858);break;- case 2:
/* Setup the MMC/SD(3) Host Controller Clock */writel(0x19, 0xd42828e0);udelay(10);writel(0x1b, 0xd42828e0);break;- case 3:
/* Setup the MMC/SD(4) Host Controller Clock */writel(0x10, 0xd42828e4);udelay(10);writel(0x12, 0xd42828e4);break;
Please provide symbolic constants / definitions for these (and similar) magic numbers.
...
--- /dev/null +++ b/drivers/mmc/pxa_sdh.h @@ -0,0 +1,241 @@
...
+/* register definitions of PXA SD Host Controller*/ +#define SD_SYS_ADDR_LOW 0x0000 /* DMA System Address Low */ +#define SD_SYS_ADDR_HIGH 0x0002 /* DMA System Address High */ +#define SD_BLOCK_SIZE 0x0004 /* Block Size*/
...
NAK. Please use a C struct instead.
...
+#define HOST_DMA_BDRY_MASK ((u16)0x7) +#define BLOCK_SIZE_OFFSET 0 +#define BLOCK_SIZE_MASK ((u16)0x0fff) +#define BLOCK_SIZE_MAX ((u16)0x0800)
Please drop all these casts (and get rid of the parens).
Best regards,
Wolfgang Denk