Alan, thank you for these comments. I will fix them and only resubmit the patch3 to meego-kernel for review.
> -----Original Message----- > From: Alan Cox [mailto:[email protected]] > Sent: Tuesday, November 16, 2010 9:25 PM > To: Dong, Chuanxiao > Cc: [email protected]; [email protected] > Subject: Re: [PATCH v2 3/3]mmc: implemented the real reset eMMC card part for > MFLD sdhci host controller > > 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
