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

[Olof Johansson <olof@lixom.net>]

Hi,

On Wed, Dec 15, 2010 at 01:23:58PM +0200, Mike Rapoport wrote:
> Hi Olof,
> 
> Overall looks good, just some nitpicking comments.
> 
[...]

> > +struct tegra_sdhci_platform_data {
> > +	int cd_gpio;
> > +	int wp_gpio;
> > +	int power_gpio;
> 
> The power_gpio seems to be unused...

Yep, will remove.

> > +static unsigned int tegra_sdhci_get_ro(struct sdhci_host *sdhci)
> > +{
> > +	struct tegra_sdhci_host *host = sdhci_priv(sdhci);
> > +
> > +	if (host->wp_gpio != -1)
> 
> if (gpio_is_valid(host->wp_gpio)) whould be better IMO.
[...]
> > +	if (plat->cd_gpio != -1) {
> 
> if (gpio_is_valid(host->wp_gpio)) whould be better IMO.
> 

Fair enough (x2).

> > +		rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
> > +			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > +			mmc_hostname(sdhci->mmc), sdhci);
> > +
> > +		if (rc)
> > +			goto err_remove_host;
> > +	}
> 
> It seems to me that the tegra_sdhci_probe should handle gpio initialization,
> rather then keep gpio_request etc calls in the board files.

Yeah, I had been planning on moving that over but not at this pass. I can do that
though.

> > +	printk(KERN_INFO "sdhci%d: initialized irq %d ioaddr %p\n", pdev->id,
> > +			sdhci->irq, sdhci->ioaddr);
> > +
> 
> dev_info?

Yep, will change.



Thanks!


-Olof