Hi, Thanks for the review. All good points (and I forgot to do a last run through checkpatch, my bad). All have been addressed in the next version (not yet posted). -Olof On Wed, Dec 22, 2010 at 8:22 AM, Wolfram Sang <w.sang@xxxxxxxxxxxxxx> wrote: > Hi Olof, > > looks good to me. Just some minor comments, but in general > > Reviewed-by: Wolfram Sang <w.sang@xxxxxxxxxxxxxx> > > On Wed, Dec 22, 2010 at 02:01:11AM -0600, Olof Johansson wrote: >> SDHCI driver for Tegra. Pretty straight forward, a few pieces of >> functionality left to fill in but nothing that stops it from going >> upstream. Board enablement submitted separately. > > I'd think this can go below the "---" line? > > ... >> This driver plugs in as a new variant of sdhci-pltfm, using the platform >> data structure passed in to specify the GPIOs to use for card detect, >> write protect and card power enablement. >> >> Original driver (of which only the header file is left): >> Signed-off-by: Yvonne Yip <y@xxxxxxxx> >> >> The rest, which has been rewritten by now: >> Signed-off-by: Olof Johansson <olof@xxxxxxxxx> >> Cc: Wolfram Sang <w.sang@xxxxxxxxxxxxxx> >> Cc: Mike Rapoport <mike@xxxxxxxxxxxxxx> >> +static unsigned int tegra_sdhci_get_ro(struct sdhci_host *sdhci) >> +{ >> + struct platform_device *pdev = to_platform_device(mmc_dev(sdhci->mmc)); >> + struct tegra_sdhci_platform_data *plat; >> + >> + plat = pdev->dev.platform_data; >> + >> + if (gpio_is_valid(plat->wp_gpio)) >> + return gpio_get_value(plat->wp_gpio); >> + >> + return -1; > > So, read_only is the default case? I would have gone for returning 0. > Well, safety vs. convenience... > > ... >> + if (rc) { >> + dev_err(mmc_dev(host->mmc), > > Trailing white space > >> + "failed to allocate power gpio\n"); >> + goto out; >> + } >> + tegra_gpio_enable(plat->power_gpio); >> + gpio_direction_output(plat->power_gpio, 1); >> + } >> + >> + if (gpio_is_valid(plat->cd_gpio)) { >> + rc = gpio_request(plat->cd_gpio, "sdhci_cd"); >> + if (rc) { >> + dev_err(mmc_dev(host->mmc), > > ditto > >> + "failed to allocate cd gpio\n"); >> + goto out_power; >> + } >> + tegra_gpio_enable(plat->cd_gpio); >> + >> + rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq, >> + IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING, >> + mmc_hostname(host->mmc), host); >> + >> + if (rc) { >> + dev_err(mmc_dev(host->mmc), "request irq error\n"); >> + goto out_cd; >> + } >> + >> + } >> + >> + if (gpio_is_valid(plat->wp_gpio)) { >> + rc = gpio_request(plat->wp_gpio, "sdhci_wp"); >> + if (rc) { >> + dev_err(mmc_dev(host->mmc), > > ditto > > Kind regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.10 (GNU/Linux) > > iEYEARECAAYFAk0SJccACgkQD27XaX1/VRtnbgCdH8+R7hjsq4YjJIjvQgtkwOrF > GHwAoJBRrdEv6+5d2sxZ5J/AqWsTXLb4 > =RLqI > -----END PGP SIGNATURE----- > > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html