Greg, Thanks for this detailled review. Please see comments inline, and new patch attached and inlined. > > drivers/mmc/host/sdhci-pci.c | 42 > ++++++++++++++++++++++++++++++++++++++++++ > > drivers/mmc/host/sdhci.c | 30 ++++++++++++++++++++++-------- > > drivers/mmc/host/sdhci.h | 1 + > > 3 files changed, 65 insertions(+), 8 deletions(-) > > Is this patch accepted upstream yet? If not, why not? If so, what is > the git commit id of it? AFWN, latest we have 3 months to make this upstream accepted upstream. The effort will begin next week.
>
> >
> > diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> > index 7bdbd9c..f251d06 100644
> > --- a/drivers/mmc/host/sdhci-pci.c
> > +++ b/drivers/mmc/host/sdhci-pci.c
> > @@ -24,6 +24,7 @@
> > #include <asm/scatterlist.h>
> > #include <asm/io.h>
> > #include <asm/intel_scu_ipc.h>
> > +#include <linux/regulator/consumer.h>
>
> That needs to go before the asm/ includes.
ok
> +/*************************************************************************
> ****\
> > + *
> *
> > + * SDHCI regulator workaround
> *
> > + * One some platforms, the regulators associated to the mmc are
> available *
> > + * late in the boot.
> > *
> > + * sdhci_pci_request_regulators() is called by platform code to retry
> getting*
> > + * the regulators associated to pci sdhcis *
>
> Those trailing * don't seem to line up for me.
>
> +\*************************************************************************
> ****/
>
> That's not the kernel comment style.
I know, this is the style of this particular file.
Please see our new attempt following.
> > +static int try_request_regulator(struct device *dev, void *data)
> > +{
> > + struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> > + struct sdhci_pci_chip *chip;
> > + struct sdhci_pci_slot *slot;
> > + struct sdhci_host *host;
>
> Mix of tabs and no tabs for variables?
Not sure what your are telling, see attach patch.
>
> > + int i;
> > + chip = pci_get_drvdata(pdev);
>
> You need an extra line after the i declaration.
ok
>
> > + if (!chip)
> > + return 0;
>
> Shouldn't this be an error?
No this is not an error. There is a race between sdhci probe and platform code.
>
> > +
> > + for (i = 0; i < chip->num_slots; i++) {
> > + slot = chip->slots[i];
> > + host = slot->host;
> > + if (!host)
> > + continue;
> > + sdhci_try_get_regulator(host);
> > + }
> > + return 0;
> > +}
> > +static struct pci_driver sdhci_driver;
>
> Extra lines here please
>
> > +int sdhci_pci_request_regulators(void)
> > +{
> > + return driver_for_each_device(&sdhci_driver.driver, NULL, NULL,
> try_request_regulator);
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_pci_request_regulators);
>
> You obviously ignored the checkpatch.pl warnings for a valid reason,
> what was it?
Cosmetic. If you really care, we added a line break to split the function call.
>
> >
> > +void sdhci_try_get_regulator(struct sdhci_host *host)
>
> Where's the documentation on this new, global function? It needs to be
> here, in kerneldoc format.
Good point.
>
> > +{
> > + struct regulator *vmmc;
> > + unsigned long flags;
> > + if (!host->vmmc) {
> > + vmmc = regulator_get(mmc_dev(host->mmc), "vmmc");
> > + if (!IS_ERR(vmmc)) {
> > + spin_lock_irqsave(&host->lock, flags);
> > + if (!host->vmmc) {
> > + host->vmmc = vmmc;
> > + spin_unlock_irqrestore(&host->lock, flags);
> > + regulator_enable(host->vmmc);
> > + mmc_detect_change(host->mmc, 0);
> > + } else { /* race! we got the regulator twice */
>
> Put the comment on the next line.
Ok.
>
> > + spin_unlock_irqrestore(&host->lock, flags);
> > + regulator_put(vmmc);
> > + }
> > + }
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_try_get_regulator);
> > int sdhci_add_host(struct sdhci_host *host)
>
> Extra line before the new function please.
Ok.
>From 1c3a4346975cc7d8195437573b28ea0f6ef0c7c2 Mon Sep 17 00:00:00 2001
From: Pierre Tardy <[email protected]>
Date: Fri, 12 Nov 2010 17:37:48 +0100
Subject: [PATCH 1/6] sdhci: allow platform to trigger regulator detection
One some platforms, the regulators associated to the mmc are available
late in the boot.
There might be race condition between the time gpio expander controlling the
mmc regulator is probed and sdhci is probed.
sdhci_pci_request_regulators() is called by platform code at a time where it is
known that the regulator
is available to retry getting the regulators associated to pci sdhcis
Signed-off-by: Pierre Tardy <[email protected]>
Signed-off-by: Sebastien Busson <[email protected]>
---
drivers/mmc/host/sdhci-pci.c | 51 ++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/host/sdhci.c | 39 +++++++++++++++++++++++++------
drivers/mmc/host/sdhci.h | 1 +
3 files changed, 83 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 7bdbd9c..30b1905 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -20,6 +20,7 @@
#include <linux/device.h>
#include <linux/mmc/host.h>
+#include <linux/regulator/consumer.h>
#include <asm/scatterlist.h>
#include <asm/io.h>
@@ -637,6 +638,54 @@ static struct sdhci_ops sdhci_pci_ops = {
.enable_dma = sdhci_pci_enable_dma,
};
+
+/*****************************************************************************\
+ * *
+ * SDHCI regulator workaround *
+ * *
+\*****************************************************************************/
+
+static int try_request_regulator(struct device *dev, void *data)
+{
+ struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+ struct sdhci_pci_chip *chip;
+ struct sdhci_pci_slot *slot;
+ struct sdhci_host *host;
+ int i;
+
+ chip = pci_get_drvdata(pdev);
+ if (!chip)
+ return 0;
+
+ for (i = 0; i < chip->num_slots; i++) {
+ slot = chip->slots[i];
+ host = slot->host;
+ if (!host)
+ continue;
+ sdhci_try_get_regulator(host);
+ }
+ return 0;
+}
+
+static struct pci_driver sdhci_driver;
+
+/**
+ * sdhci_pci_request_regulators - retry requesting regulators of
+ * all sdhci-pci devices
+ *
+ * One some platforms, the regulators associated to the mmc are available
+ * late in the boot.
+ * sdhci_pci_request_regulators() is called by platform code to retry
+ * getting the regulators associated to pci sdhcis
+ */
+
+int sdhci_pci_request_regulators(void)
+{
+ return driver_for_each_device(&sdhci_driver.driver,
+ NULL, NULL, try_request_regulator);
+}
+EXPORT_SYMBOL_GPL(sdhci_pci_request_regulators);
+
/*****************************************************************************\
* *
* Suspend/resume *
@@ -932,7 +981,9 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
chip->slots[i] = slot;
}
+ /* prevent race condition with platform code */
+ sdhci_pci_request_regulators();
return 0;
free:
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f6a2b8a..7b9a3b3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1778,6 +1778,36 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
EXPORT_SYMBOL_GPL(sdhci_alloc_host);
+/**
+ * sdhci_pci_request_regulators - try requesting regulator of
+ * a sdhci device
+ *
+ * We take care of race conditions here between sdhci_add_host() (probe)
+ * and platform code that may kick a retry at anytime during boot.
+ */
+void sdhci_try_get_regulator(struct sdhci_host *host)
+{
+ struct regulator *vmmc;
+ unsigned long flags;
+ if (!host->vmmc) {
+ vmmc = regulator_get(mmc_dev(host->mmc), "vmmc");
+ if (!IS_ERR(vmmc)) {
+ spin_lock_irqsave(&host->lock, flags);
+ if (!host->vmmc) {
+ host->vmmc = vmmc;
+ spin_unlock_irqrestore(&host->lock, flags);
+ regulator_enable(host->vmmc);
+ mmc_detect_change(host->mmc, 0);
+ } else {
+ /* race! we got the regulator twice */
+ spin_unlock_irqrestore(&host->lock, flags);
+ regulator_put(vmmc);
+ }
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(sdhci_try_get_regulator);
+
int sdhci_add_host(struct sdhci_host *host)
{
struct mmc_host *mmc;
@@ -2049,14 +2079,7 @@ int sdhci_add_host(struct sdhci_host *host)
mmc_hostname(mmc), host);
if (ret)
goto untasklet;
-
- host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
- if (IS_ERR(host->vmmc)) {
- printk(KERN_INFO "%s: no vmmc regulator found\n",
mmc_hostname(mmc));
- host->vmmc = NULL;
- } else {
- regulator_enable(host->vmmc);
- }
+ sdhci_try_get_regulator(host);
sdhci_init(host, 0);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 74b72f7..b3661ba 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -314,6 +314,7 @@ static inline void *sdhci_priv(struct sdhci_host *host)
extern void sdhci_card_detect(struct sdhci_host *host);
extern int sdhci_add_host(struct sdhci_host *host);
extern void sdhci_remove_host(struct sdhci_host *host, int dead);
+extern void sdhci_try_get_regulator(struct sdhci_host *host);
#ifdef CONFIG_PM
extern int sdhci_suspend_host(struct sdhci_host *host, pm_message_t state);
--
1.6.2.2
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
0001-sdhci-allow-platform-to-trigger-regulator-detection.patch
Description: 0001-sdhci-allow-platform-to-trigger-regulator-detection.patch
_______________________________________________ MeeGo-kernel mailing list [email protected] http://lists.meego.com/listinfo/meego-kernel
