On Mon, Mar 17, 2014 at 4:38 AM, Ian Campbell ijc@hellion.org.uk wrote:
On Fri, 2014-03-14 at 17:36 +0200, Pantelis Antoniou wrote:
[...]
Thanks for your review. It seems there are still quite a few issues dating back to the original allwinner dumps here.
@linux-sunxi: if anyone wants to volunteer to help cleanup this particular driver I'd be very happy -- there's a lot of it!
+static void dumpmmcreg(struct sunxi_mmc *reg) +{
- debug("dump mmc registers:\n");
- debug("gctrl 0x%08x\n", reg->gctrl);
- debug("clkcr 0x%08x\n", reg->clkcr);
- debug("timeout 0x%08x\n", reg->timeout);
- debug("width 0x%08x\n", reg->width);
- debug("blksz 0x%08x\n", reg->blksz);
[...] lots more debug(foo)
+}
^^^ #ifdef DEBUG here?
I can if you prefer but debug() itself effectively includes the same ifdef so the end result is already the same.
[...]
+/* support 4 mmc hosts */ +struct mmc mmc_dev[4]; +struct sunxi_mmc_host mmc_host[4];
^ hosts & mmc structs can be allocated even for SPL now
Can be or must be?
- struct sunxi_mmc_host *mmchost = &mmc_host[sdc_no];
- static struct sunxi_gpio *gpio_c =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_C];- static struct sunxi_gpio *gpio_f =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_F];+#if CONFIG_MMC1_PG
- static struct sunxi_gpio *gpio_g =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_G];+#endif
- static struct sunxi_gpio *gpio_h =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_H];- static struct sunxi_gpio *gpio_i =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_I];- struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
^ Castings are ugly; rework with a temporary variable.
Ack.
The static's here are odd too and date back to the original alwinner code dumps. I'll get rid of them too.
You can drop the gpio ones in favor of using the sunxi gpio driver.
[...]
- case 3:
/* PI4-CMD, PI5-CLK, PI6~9-D0~D3 : 2 */writel(0x2222 << 16, &gpio_i->cfg[0]);writel(0x22, &gpio_i->cfg[1]);writel(0x555 << 8, &gpio_i->pull[0]);writel(0x555 << 8, &gpio_i->drv[0]);break;- default:
return -1;- }
Lots of magic constants. I have no idea what's going on here. Use a few defines.
Right. These came from the original allwinner dumps so I was worried that they might be undocumented magic, but actually since the are gpio frobbing I reckon I can figure them out.
Should be something like this:
for (pin = SUNXI_GPI(4); pin <= SUNXI_GPI(9); pin++) { sunxi_gpio_set_cfgpin(pin, SUNXI_GPI_SDC3); sunxi_gpio_set_drv(pin, 1); sunxi_gpio_set_pull(pin, SUNXI_PULL_UP); }
Note that SUNXI_GPI_* and SUNXI_PULL_* have not been defined.
I will send a patch for both the macros and MMC pinmux setting.
[..]
Thanks for working on this!
Cheers ChenYu