Re: [PATCH 3/3] mmc: add sdhci-tegra driver for Tegra SoCs

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2538 bytes --]

Hi Olof,

looks good to me. Just some minor comments, but in general

Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>

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@palm.com>
> 
> The rest, which has been rewritten by now:
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Cc: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Mike Rapoport <mike@compulab.co.il>
> +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/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]