On Tue, 16 Nov 2010 17:32:39 +0800
"Chuanxiao.Dong" <[email protected]> wrote:
> From e981cd75059c7059ea553dba97e6a39d56aac58f Mon Sep 17 00:00:00 2001
> +extern int emmc_rst_gpio_setup(int);
> +extern void emmc_rst_gpio_release(int);
This hardcodes assumptions about gpio pins being used. I'd also change
the naming to mfld_emmc_.. or similar to avoid clashes with other
possible future core mmc code
> +int emmc_rst_gpio_setup(int device)
Pass the struct pci_dev or host that way if you need to do things to
the PCI device itself or print messages you can. That would be more
typical I suspect of non Medfield cases.
> + if (device == PCI_DEVID_MFL_EMMC0)
> + gpio = get_gpio_by_name("emmc0_rst");
> + else if (device == PCI_DEVID_MFL_EMMC1)
> + gpio = get_gpio_by_name("emmc1_rst");
> + else
> + return -ENODEV;
Make this a function as we have two copies and we don't want them ever
to get out of sync
> +
> + /* if the gpio number is -1, return
> + * -ENODEV indicates no gpio is avalible
> + * for hardware reset
> + * */
> + if (gpio == -1)
> + return -ENODEV;
> +
> + /* request reset pin for eMMC */
> + ret = gpio_request(gpio, "eMMC_rst_pin");
> + if (ret < 0) {
> + pr_err("gpio %d request failed\n", gpio);
> + return -1;
return ret
> + }
> + /* set to be output and to be low state */
> + ret = gpio_direction_output(gpio, 0);
> + if (ret < 0) {
> + pr_err("gpio %d direction output failed\n", gpio);
> + goto gpio_output_failed;
> + }
> + return gpio;
> +
> +gpio_output_failed:
> + gpio_free(gpio);
> + return -ENODEV;
return ret
> diff --git a/drivers/mmc/host/sdhci-pci.c
> b/drivers/mmc/host/sdhci-pci.c index 7bdbd9c..5fd2347 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -18,6 +18,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/slab.h>
> #include <linux/device.h>
> +#include <linux/gpio.h>
>
> #include <linux/mmc/host.h>
>
> @@ -25,6 +26,10 @@
> #include <asm/io.h>
> #include <asm/intel_scu_ipc.h>
>
> +#if defined(CONFIG_X86_MRST)
> +#include <asm/mrst.h>
> +#endif
This doesn't belong in core code if at all possible
> +#if CONFIG_GPIOLIB
> + __gpio_set_value(host->rst_pin, 1);
> + udelay(300);
> + __gpio_set_value(host->rst_pin, 0);
> + return 0;
> +#else
> + return -ENODEV;
> +#endif
This callback also belongs in the mrst code I think not in sdhci-pci
> + /* disable reset eMMC card pin first
> + * If platform is MFLD, will enable
> + * reset pin later. If not, just reserve
> + * for other platform to enable it.
> + * */
> + host->rst_pin = -ENODEV;
Again this should be generalised
> host->hw_name = "PCI";
> host->ops = &sdhci_pci_ops;
> host->quirks = chip->quirks;
> @@ -817,6 +852,10 @@ static struct sdhci_pci_slot * __devinit
> sdhci_pci_probe_slot( if (ret)
> goto remove;
>
> +#if defined(CONFIG_X86_MRST)
> + host->rst_pin = emmc_rst_gpio_setup(pdev->device);
> +#endif
and some sort of general callback off the quirks/device table we
already have.
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index dc712bb..ec646dc 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -96,6 +96,8 @@ struct sdhci_host {
> int irq; /* Device IRQ */
> void __iomem *ioaddr; /* Mapped address */
>
> + int rst_pin;
> +
Better is probably
void *host_private;
that would let other devices/platforms hang anything they need off it.
> const struct sdhci_ops *ops; /* Low level hw
> interface */
> struct regulator *vmmc; /* Power regulator */
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
