[PATCH] SDHCI-S3C updates

[PATCH] SDHCI-S3C updates

[Marek Szyprowski]

Hello,

This patch series includes various updates to sdhci-s3c driver. The
patches has been rebased onto latest -mm kernel tree from
git://zen-kernel.org/kernel/mmotm.git (2010-07-27-14-56 snapshot).

Some patches has been already accepted by Andrew Morton, but they became
incompatible after threaded_irq changes in sdhci driver, so they have
been dropped. I've adapted them to new driver base, see the change log
in each patch.

All required platform changes has been already merged by Kukjin Kim and
are available in linux-next kernel tree - see commit:
848a3aeee01c8c805f8dad4ae2963473975410d1 from 
git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git
for-next branch.

A complete list of patches:

[PATCHv2 1/4] sdhci-s3c: add support for the non standard minimal clock value
[PATCH 2/4] sdhci-s3c: enable SDHCI_QUIRK_NO_HISPD_BIT quirk
[PATCHv5 3/4] sdhci-s3c: add support for new card detection methods (driver part)
[PATCH 4/4] sdhci-s3c: add regulator support

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

[PATCHv2 1/4] sdhci-s3c: add support for the non standard minimal clock value

[Marek Szyprowski]

S3C SDHCI host controller can change the source for generating mmc clock.
By default host bus clock is used, what causes some problems on machines
with 133MHz bus, because the SDHCI divider cannot be as high get proper
clock value for identification mode. This is not a problem for the
controller, because it can generate lower frequencies from other clock
sources. This patch adds a new quirk to SDHCI driver to calculate the
minimal supported clock frequency.

This fixes the flood of the following warnings on Samsung S5PV210 SoCs:
mmc0: Minimum clock frequency too high for identification mode

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Changes since V1:
- rebased onto latest -mm kernel tree

---
 drivers/mmc/host/sdhci-of-esdhc.c |    1 +
 drivers/mmc/host/sdhci-s3c.c      |   29 +++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c          |    2 +-
 drivers/mmc/host/sdhci.h          |    2 ++
 4 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index e9f99fe..763b364 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -124,6 +124,7 @@ struct sdhci_of_data sdhci_esdhc = {
 		  SDHCI_QUIRK_BROKEN_CARD_DETECTION |
 		  SDHCI_QUIRK_NO_BUSY_IRQ |
 		  SDHCI_QUIRK_NONSTANDARD_CLOCK |
+		  SDHCI_QUIRK_NONSTANDARD_MINCLOCK |
 		  SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |
 		  SDHCI_QUIRK_PIO_NEEDS_DELAY |
 		  SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET |
diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index d7058ee..5218f2b 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -203,9 +203,36 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock)
 	}
 }
 
+/**
+ * sdhci_s3c_get_min_clock - callback to get minimal supported clock value
+ * @host: The SDHCI host being queried
+ *
+ * To init mmc host properly a minimal clock value is needed. For high system
+ * bus clock's values the standard formula gives values out of allowed range.
+ * The clock still can be set to lower values, if clock source other then
+ * system bus is selected.
+*/
+static unsigned int sdhci_s3c_get_min_clock(struct sdhci_host *host)
+{
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	unsigned int delta, min = UINT_MAX;
+	int src;
+
+	for (src = 0; src < MAX_BUS_CLK; src++) {
+		delta = sdhci_s3c_consider_clock(ourhost, src, 0);
+		if (delta == UINT_MAX)
+			continue;
+		/* delta is a negative value in this case */
+		if (-delta < min)
+			min = -delta;
+	}
+	return min;
+}
+
 static struct sdhci_ops sdhci_s3c_ops = {
 	.get_max_clock		= sdhci_s3c_get_max_clk,
 	.set_clock		= sdhci_s3c_set_clock,
+	.get_min_clock		= sdhci_s3c_get_min_clock,
 };
 
 static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
@@ -309,6 +336,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	host->quirks = 0;
 	host->irq = irq;
 
+	host->quirks |= SDHCI_QUIRK_NONSTANDARD_MINCLOCK;
+
 	/* Setup quirks for the controller */
 	host->quirks |= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC;
 
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 07b2695..682b285 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1776,7 +1776,7 @@ int sdhci_add_host(struct sdhci_host *host)
 	 * Set host parameters.
 	 */
 	mmc->ops = &sdhci_ops;
-	if (host->quirks & SDHCI_QUIRK_NONSTANDARD_CLOCK &&
+	if (host->quirks & SDHCI_QUIRK_NONSTANDARD_MINCLOCK &&
 			host->ops->get_min_clock)
 		mmc->f_min = host->ops->get_min_clock(host);
 	else
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 2be34b3..087cfd9 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -247,6 +247,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_NO_HISPD_BIT			(1<<27)
 /* Controller is missing device caps. Use caps provided by host */
 #define SDHCI_QUIRK_MISSING_CAPS			(1<<28)
+/* Controller has nonstandard clock management */
+#define SDHCI_QUIRK_NONSTANDARD_MINCLOCK		(1<<29)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.7.1.569.g6f426

[PATCHv2 1/4] sdhci-s3c: add support for the non standard minimal clock value

[Ben Dooks]

On 28/07/10 15:19, Marek Szyprowski wrote:
> S3C SDHCI host controller can change the source for generating mmc clock.
> By default host bus clock is used, what causes some problems on machines
> with 133MHz bus, because the SDHCI divider cannot be as high get proper
> clock value for identification mode. This is not a problem for the
> controller, because it can generate lower frequencies from other clock
> sources. This patch adds a new quirk to SDHCI driver to calculate the
> minimal supported clock frequency.
> 
> This fixes the flood of the following warnings on Samsung S5PV210 SoCs:
> mmc0: Minimum clock frequency too high for identification mode
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 
> Changes since V1:
> - rebased onto latest -mm kernel tree
> 
> ---
>  drivers/mmc/host/sdhci-of-esdhc.c |    1 +
>  drivers/mmc/host/sdhci-s3c.c      |   29 +++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c          |    2 +-
>  drivers/mmc/host/sdhci.h          |    2 ++
>  4 files changed, 33 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> index e9f99fe..763b364 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -124,6 +124,7 @@ struct sdhci_of_data sdhci_esdhc = {
>  		  SDHCI_QUIRK_BROKEN_CARD_DETECTION |
>  		  SDHCI_QUIRK_NO_BUSY_IRQ |
>  		  SDHCI_QUIRK_NONSTANDARD_CLOCK |
> +		  SDHCI_QUIRK_NONSTANDARD_MINCLOCK |
>  		  SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |
>  		  SDHCI_QUIRK_PIO_NEEDS_DELAY |
>  		  SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET |
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index d7058ee..5218f2b 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -203,9 +203,36 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock)
>  	}
>  }
>  
> +/**
> + * sdhci_s3c_get_min_clock - callback to get minimal supported clock value
> + * @host: The SDHCI host being queried
> + *
> + * To init mmc host properly a minimal clock value is needed. For high system
> + * bus clock's values the standard formula gives values out of allowed range.
> + * The clock still can be set to lower values, if clock source other then
> + * system bus is selected.
> +*/
> +static unsigned int sdhci_s3c_get_min_clock(struct sdhci_host *host)
> +{
> +	struct sdhci_s3c *ourhost = to_s3c(host);
> +	unsigned int delta, min = UINT_MAX;
> +	int src;
> +
> +	for (src = 0; src < MAX_BUS_CLK; src++) {
> +		delta = sdhci_s3c_consider_clock(ourhost, src, 0);
> +		if (delta == UINT_MAX)
> +			continue;
> +		/* delta is a negative value in this case */
> +		if (-delta < min)
> +			min = -delta;
> +	}
> +	return min;
> +}
> +
>  static struct sdhci_ops sdhci_s3c_ops = {
>  	.get_max_clock		= sdhci_s3c_get_max_clk,
>  	.set_clock		= sdhci_s3c_set_clock,
> +	.get_min_clock		= sdhci_s3c_get_min_clock,
>  };
>  
>  static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
> @@ -309,6 +336,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  	host->quirks = 0;
>  	host->irq = irq;
>  
> +	host->quirks |= SDHCI_QUIRK_NONSTANDARD_MINCLOCK;
> +
>  	/* Setup quirks for the controller */
>  	host->quirks |= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC;
>  
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 07b2695..682b285 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1776,7 +1776,7 @@ int sdhci_add_host(struct sdhci_host *host)
>  	 * Set host parameters.
>  	 */
>  	mmc->ops = &sdhci_ops;
> -	if (host->quirks & SDHCI_QUIRK_NONSTANDARD_CLOCK &&
> +	if (host->quirks & SDHCI_QUIRK_NONSTANDARD_MINCLOCK &&
>  			host->ops->get_min_clock)
>  		mmc->f_min = host->ops->get_min_clock(host);

We're begining to run out of quirk space, given these fields will
be initialised to NULL if not used, then why not just check for
the op being present?

[PATCHv2 1/4] sdhci-s3c: add support for the non standard minimal clock value

[Marek Szyprowski]

Hello,

On Wednesday, July 28, 2010 6:48 PM Ben Dooks wrote:

> On 28/07/10 15:19, Marek Szyprowski wrote:
> > S3C SDHCI host controller can change the source for generating mmc clock.
> > By default host bus clock is used, what causes some problems on machines
> > with 133MHz bus, because the SDHCI divider cannot be as high get proper
> > clock value for identification mode. This is not a problem for the
> > controller, because it can generate lower frequencies from other clock
> > sources. This patch adds a new quirk to SDHCI driver to calculate the
> > minimal supported clock frequency.
> >
> > This fixes the flood of the following warnings on Samsung S5PV210 SoCs:
> > mmc0: Minimum clock frequency too high for identification mode
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >
> > Changes since V1:
> > - rebased onto latest -mm kernel tree
> >
> > ---
> >  drivers/mmc/host/sdhci-of-esdhc.c |    1 +
> >  drivers/mmc/host/sdhci-s3c.c      |   29 +++++++++++++++++++++++++++++
> >  drivers/mmc/host/sdhci.c          |    2 +-
> >  drivers/mmc/host/sdhci.h          |    2 ++
> >  4 files changed, 33 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-
> of-esdhc.c
> > index e9f99fe..763b364 100644
> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > @@ -124,6 +124,7 @@ struct sdhci_of_data sdhci_esdhc = {
> >  		  SDHCI_QUIRK_BROKEN_CARD_DETECTION |
> >  		  SDHCI_QUIRK_NO_BUSY_IRQ |
> >  		  SDHCI_QUIRK_NONSTANDARD_CLOCK |
> > +		  SDHCI_QUIRK_NONSTANDARD_MINCLOCK |
> >  		  SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |
> >  		  SDHCI_QUIRK_PIO_NEEDS_DELAY |
> >  		  SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET |
> > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> > index d7058ee..5218f2b 100644
> > --- a/drivers/mmc/host/sdhci-s3c.c
> > +++ b/drivers/mmc/host/sdhci-s3c.c
> > @@ -203,9 +203,36 @@ static void sdhci_s3c_set_clock(struct sdhci_host
> *host, unsigned int clock)
> >  	}
> >  }
> >
> > +/**
> > + * sdhci_s3c_get_min_clock - callback to get minimal supported clock
> value
> > + * @host: The SDHCI host being queried
> > + *
> > + * To init mmc host properly a minimal clock value is needed. For high
> system
> > + * bus clock's values the standard formula gives values out of allowed
> range.
> > + * The clock still can be set to lower values, if clock source other
> then
> > + * system bus is selected.
> > +*/
> > +static unsigned int sdhci_s3c_get_min_clock(struct sdhci_host *host)
> > +{
> > +	struct sdhci_s3c *ourhost = to_s3c(host);
> > +	unsigned int delta, min = UINT_MAX;
> > +	int src;
> > +
> > +	for (src = 0; src < MAX_BUS_CLK; src++) {
> > +		delta = sdhci_s3c_consider_clock(ourhost, src, 0);
> > +		if (delta == UINT_MAX)
> > +			continue;
> > +		/* delta is a negative value in this case */
> > +		if (-delta < min)
> > +			min = -delta;
> > +	}
> > +	return min;
> > +}
> > +
> >  static struct sdhci_ops sdhci_s3c_ops = {
> >  	.get_max_clock		= sdhci_s3c_get_max_clk,
> >  	.set_clock		= sdhci_s3c_set_clock,
> > +	.get_min_clock		= sdhci_s3c_get_min_clock,
> >  };
> >
> >  static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
> > @@ -309,6 +336,8 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >  	host->quirks = 0;
> >  	host->irq = irq;
> >
> > +	host->quirks |= SDHCI_QUIRK_NONSTANDARD_MINCLOCK;
> > +
> >  	/* Setup quirks for the controller */
> >  	host->quirks |= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC;
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 07b2695..682b285 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1776,7 +1776,7 @@ int sdhci_add_host(struct sdhci_host *host)
> >  	 * Set host parameters.
> >  	 */
> >  	mmc->ops = &sdhci_ops;
> > -	if (host->quirks & SDHCI_QUIRK_NONSTANDARD_CLOCK &&
> > +	if (host->quirks & SDHCI_QUIRK_NONSTANDARD_MINCLOCK &&
> >  			host->ops->get_min_clock)
> >  		mmc->f_min = host->ops->get_min_clock(host);
> 
> We're begining to run out of quirk space, given these fields will
> be initialised to NULL if not used, then why not just check for
> the op being present?

Ok, I can remove the quirk and just check if the op is present. However
having a separate quirk makes the code easier to understand (quirk shows 
that the particular implementation differs from the standard).

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 2/4] sdhci-s3c: enable SDHCI_QUIRK_NO_HISPD_BIT quirk

[Marek Szyprowski]

This patch enables SDHCI_QUIRK_NO_HISPD_BIT on Samsung SDHCI driver.
This solves detection problems with some external SD cards. This change
has been tested on S5PC100 and S5PC110. It has no inpact on driver
speed.

Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/mmc/host/sdhci-s3c.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 5218f2b..9938ded 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -337,6 +337,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	host->irq = irq;
 
 	host->quirks |= SDHCI_QUIRK_NONSTANDARD_MINCLOCK;
+	host->quirks |= SDHCI_QUIRK_NO_HISPD_BIT;
 
 	/* Setup quirks for the controller */
 	host->quirks |= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC;
-- 
1.7.1.569.g6f426

[PATCHv5 3/4] sdhci-s3c: add support for new card detection methods (driver part)

[Marek Szyprowski]

On some Samsung SoCs not all SDHCI controllers have card detect (CD)
line. For some embedded designs it is not even needed, because ususally
the device (like SDIO flash memory or wifi controller) is permanently
wired to the controller. There are also systems which have a card detect
line connected to some of the external interrupt lines or the presence
of the card depends on some other actions (like enabling a power
regulator).

This patch adds support for all these cases. The following card
detection methods are possible:

1. internal sdhci host card detect line
2. external event
3. external gpio interrupt
4. no card detect line, controller will poll for the card
5. no card detect line, card is permanently wired to the controller
(once detected host won't poll it any more)

By default, all existing code would use method #1, what is compatible
with the previous version of the driver.

In case of external event, two callbacks must be provided in platdata:
ext_cd_init and ext_cd_cleanup. Both of them get a callback to a
function that notifies the s3c-sdhci host contoller as their argument.
That callback function should be called from the even dispatcher to let
host notice the card insertion/removal.

In case of external gpio interrupt, a gpio pin number must be provided
in platdata (ext_cd_gpio parameter), as well as the information about
the polarity of that gpio pin (ext_cd_gpio_invert). By default
(ext_cd_gpio_invert == 0) gpio value 0 means 'card has been removed',
but this can be changed to 'card has been removed' when
ext_cd_gpio_invert == 1.

This patch adds all required changes to sdhci-s3c driver.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Changes since V4:
- adapted to threaded irq approach in sdhci driver
- rebased onto latest -mm kernel tree
- fixed problem when gpio-base card detection was not called on driver
init

Changes since V3:
- renamed patch to avoid confusion with the patch for the s3c-sdhci
platform changes - added "(driver part)" in subject

Changes since V2:
According to Andrew Morton's suggestion local_irq_save() call in the
sdhci_s3c_notify_change function has been replaced by spin_lock_irqsave
(host driver already has a spinlock that is used for protecting internal
state of the driver).

Changes since V1:
- added support for gpio external interrupt card based detect method
  directly to sdhci-s3c driver

---
 drivers/mmc/host/sdhci-s3c.c |   78 ++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     |   47 ++++++++++++++-----------
 drivers/mmc/host/sdhci.h     |    1 +
 3 files changed, 106 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 9938ded..606e695 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/gpio.h>
 
 #include <linux/mmc/host.h>
 
@@ -44,6 +45,8 @@ struct sdhci_s3c {
 	struct resource		*ioarea;
 	struct s3c_sdhci_platdata *pdata;
 	unsigned int		cur_clk;
+	int			ext_cd_irq;
+	int			ext_cd_gpio;
 
 	struct clk		*clk_io;
 	struct clk		*clk_bus[MAX_BUS_CLK];
@@ -235,6 +238,36 @@ static struct sdhci_ops sdhci_s3c_ops = {
 	.get_min_clock		= sdhci_s3c_get_min_clock,
 };
 
+static void sdhci_s3c_notify_change(struct platform_device *dev, int state)
+{
+	struct sdhci_host *host = platform_get_drvdata(dev);
+	if (host) {
+		mutex_lock(&host->lock);
+		if (state) {
+			dev_dbg(&dev->dev, "card inserted.\n");
+			host->flags &= ~SDHCI_DEVICE_DEAD;
+			host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+			sdhci_card_detect(host);
+		} else {
+			dev_dbg(&dev->dev, "card removed.\n");
+			host->flags |= SDHCI_DEVICE_DEAD;
+			host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+			sdhci_card_detect(host);
+		}
+		mutex_unlock(&host->lock);
+	}
+}
+
+static irqreturn_t sdhci_s3c_gpio_card_detect_thread(int irq, void *dev_id)
+{
+	struct sdhci_s3c *sc = dev_id;
+	int status = gpio_get_value(sc->ext_cd_gpio);
+	if (sc->pdata->ext_cd_gpio_invert)
+		status = !status;
+	sdhci_s3c_notify_change(sc->pdev, status);
+	return IRQ_HANDLED;
+}
+
 static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 {
 	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
@@ -272,6 +305,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	sc->host = host;
 	sc->pdev = pdev;
 	sc->pdata = pdata;
+	sc->ext_cd_gpio = -1;
 
 	platform_set_drvdata(pdev, host);
 
@@ -355,6 +389,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	 * SDHCI block, or a missing configuration that needs to be set. */
 	host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ;
 
+	if (pdata->cd_type == S3C_SDHCI_CD_NONE ||
+	    pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
+		host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+
+	if (pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
+		host->mmc->caps = MMC_CAP_NONREMOVABLE;
+
 	host->quirks |= (SDHCI_QUIRK_32BIT_DMA_ADDR |
 			 SDHCI_QUIRK_32BIT_DMA_SIZE);
 
@@ -367,6 +408,33 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 		goto err_add_host;
 	}
 
+	/* pdata->ext_cd_init might call sdhci_s3c_notify_change immediately,
+	   so it can be called only after sdhci_add_host() */
+	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_init)
+		pdata->ext_cd_init(&sdhci_s3c_notify_change);
+
+	if (pdata->cd_type == S3C_SDHCI_CD_GPIO &&
+	    gpio_is_valid(pdata->ext_cd_gpio)) {
+		int status;
+
+		gpio_request(pdata->ext_cd_gpio, "SDHCI EXT CD");
+		sc->ext_cd_gpio = pdata->ext_cd_gpio;
+
+		sc->ext_cd_irq = gpio_to_irq(pdata->ext_cd_gpio);
+		if (sc->ext_cd_irq &&
+		    request_threaded_irq(sc->ext_cd_irq, NULL, sdhci_s3c_gpio_card_detect_thread,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				dev_name(&pdev->dev), sc)) {
+			dev_err(&pdev->dev, "cannot request irq for card detect\n");
+			sc->ext_cd_irq = 0;
+		}
+
+		status = gpio_get_value(sc->ext_cd_gpio);
+		if (sc->pdata->ext_cd_gpio_invert)
+			status = !status;
+		sdhci_s3c_notify_change(sc->pdev, status);
+	}
+
 	return 0;
 
  err_add_host:
@@ -391,10 +459,20 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 
 static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
 {
+	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
 	struct sdhci_host *host =  platform_get_drvdata(pdev);
 	struct sdhci_s3c *sc = sdhci_priv(host);
 	int ptr;
 
+	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_cleanup)
+		pdata->ext_cd_cleanup(&sdhci_s3c_notify_change);
+
+	if (sc->ext_cd_irq)
+		free_irq(sc->ext_cd_irq, sc);
+
+	if (sc->ext_cd_gpio != -1)
+		gpio_free(sc->ext_cd_gpio);
+
 	sdhci_remove_host(host, 1);
 
 	for (ptr = 0; ptr < 3; ptr++) {
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 682b285..aa82344 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1244,26 +1244,6 @@ static const struct mmc_host_ops sdhci_ops = {
  *                                                                           *
 \*****************************************************************************/
 
-static void sdhci_card_detect(struct sdhci_host *host)
-{
-	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
-		if (host->mrq) {
-			printk(KERN_ERR "%s: Card removed during transfer!\n",
-				mmc_hostname(host->mmc));
-			printk(KERN_ERR "%s: Resetting controller.\n",
-				mmc_hostname(host->mmc));
-
-			sdhci_reset(host, SDHCI_RESET_CMD);
-			sdhci_reset(host, SDHCI_RESET_DATA);
-
-			host->mrq->cmd->error = -ENOMEDIUM;
-			schedule_work(&host->finish_work);
-		}
-	}
-
-	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
-}
-
 static void sdhci_finish_work(struct work_struct *wk)
 {
 	struct sdhci_host *host;
@@ -1627,6 +1607,33 @@ EXPORT_SYMBOL_GPL(sdhci_resume_host);
 
 /*****************************************************************************\
  *                                                                           *
+ * Implementation dependent callbacks                                        *
+ *                                                                           *
+\*****************************************************************************/
+
+void sdhci_card_detect(struct sdhci_host *host)
+{
+	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
+		if (host->mrq) {
+			printk(KERN_ERR "%s: Card removed during transfer!\n",
+				mmc_hostname(host->mmc));
+			printk(KERN_ERR "%s: Resetting controller.\n",
+				mmc_hostname(host->mmc));
+
+			sdhci_reset(host, SDHCI_RESET_CMD);
+			sdhci_reset(host, SDHCI_RESET_DATA);
+
+			host->mrq->cmd->error = -ENOMEDIUM;
+			schedule_work(&host->finish_work);
+		}
+	}
+
+	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
+}
+EXPORT_SYMBOL_GPL(sdhci_card_detect);
+
+/*****************************************************************************\
+ *                                                                           *
  * Device allocation/registration                                            *
  *                                                                           *
 \*****************************************************************************/
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 087cfd9..561235b 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -417,6 +417,7 @@ static inline void *sdhci_priv(struct sdhci_host *host)
 	return (void *)host->private;
 }
 
+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);
 
-- 
1.7.1.569.g6f426

[PATCHv5 3/4] sdhci-s3c: add support for new card detection methods (driver part)

[Maurus Cuelenaere]

 Op 28-07-10 16:19, Marek Szyprowski schreef:
> On some Samsung SoCs not all SDHCI controllers have card detect (CD)
> line. For some embedded designs it is not even needed, because ususally
> the device (like SDIO flash memory or wifi controller) is permanently
> wired to the controller. There are also systems which have a card detect
> line connected to some of the external interrupt lines or the presence
> of the card depends on some other actions (like enabling a power
> regulator).
>
> This patch adds support for all these cases. The following card
> detection methods are possible:
>
> 1. internal sdhci host card detect line
> 2. external event
> 3. external gpio interrupt
> 4. no card detect line, controller will poll for the card
> 5. no card detect line, card is permanently wired to the controller
> (once detected host won't poll it any more)
>
> By default, all existing code would use method #1, what is compatible
> with the previous version of the driver.
>
> In case of external event, two callbacks must be provided in platdata:
> ext_cd_init and ext_cd_cleanup. Both of them get a callback to a
> function that notifies the s3c-sdhci host contoller as their argument.
> That callback function should be called from the even dispatcher to let
> host notice the card insertion/removal.
>
> In case of external gpio interrupt, a gpio pin number must be provided
> in platdata (ext_cd_gpio parameter), as well as the information about
> the polarity of that gpio pin (ext_cd_gpio_invert). By default
> (ext_cd_gpio_invert == 0) gpio value 0 means 'card has been removed',
> but this can be changed to 'card has been removed' when
> ext_cd_gpio_invert == 1.
>
> This patch adds all required changes to sdhci-s3c driver.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>
> Changes since V4:
> - adapted to threaded irq approach in sdhci driver
> - rebased onto latest -mm kernel tree
> - fixed problem when gpio-base card detection was not called on driver
> init
>
> Changes since V3:
> - renamed patch to avoid confusion with the patch for the s3c-sdhci
> platform changes - added "(driver part)" in subject
>
> Changes since V2:
> According to Andrew Morton's suggestion local_irq_save() call in the
> sdhci_s3c_notify_change function has been replaced by spin_lock_irqsave
> (host driver already has a spinlock that is used for protecting internal
> state of the driver).
>
> Changes since V1:
> - added support for gpio external interrupt card based detect method
>   directly to sdhci-s3c driver
>
> ---
>  drivers/mmc/host/sdhci-s3c.c |   78 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c     |   47 ++++++++++++++-----------
>  drivers/mmc/host/sdhci.h     |    1 +
>  3 files changed, 106 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 9938ded..606e695 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -18,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/gpio.h>
>  
>  #include <linux/mmc/host.h>
>  
> @@ -44,6 +45,8 @@ struct sdhci_s3c {
>  	struct resource		*ioarea;
>  	struct s3c_sdhci_platdata *pdata;
>  	unsigned int		cur_clk;
> +	int			ext_cd_irq;
> +	int			ext_cd_gpio;
>  
>  	struct clk		*clk_io;
>  	struct clk		*clk_bus[MAX_BUS_CLK];
> @@ -235,6 +238,36 @@ static struct sdhci_ops sdhci_s3c_ops = {
>  	.get_min_clock		= sdhci_s3c_get_min_clock,
>  };
>  
> +static void sdhci_s3c_notify_change(struct platform_device *dev, int state)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(dev);
> +	if (host) {
> +		mutex_lock(&host->lock);
> +		if (state) {
> +			dev_dbg(&dev->dev, "card inserted.\n");
> +			host->flags &= ~SDHCI_DEVICE_DEAD;
> +			host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +			sdhci_card_detect(host);
> +		} else {
> +			dev_dbg(&dev->dev, "card removed.\n");
> +			host->flags |= SDHCI_DEVICE_DEAD;
> +			host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +			sdhci_card_detect(host);
> +		}

You could move the sdhci_card_detect outside the if.

> +		mutex_unlock(&host->lock);
> +	}
> +}
> +
> +static irqreturn_t sdhci_s3c_gpio_card_detect_thread(int irq, void *dev_id)
> +{
> +	struct sdhci_s3c *sc = dev_id;
> +	int status = gpio_get_value(sc->ext_cd_gpio);
> +	if (sc->pdata->ext_cd_gpio_invert)
> +		status = !status;
> +	sdhci_s3c_notify_change(sc->pdev, status);
> +	return IRQ_HANDLED;
> +}
> +
>  static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  {
>  	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
> @@ -272,6 +305,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  	sc->host = host;
>  	sc->pdev = pdev;
>  	sc->pdata = pdata;
> +	sc->ext_cd_gpio = -1;
>  
>  	platform_set_drvdata(pdev, host);
>  
> @@ -355,6 +389,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  	 * SDHCI block, or a missing configuration that needs to be set. */
>  	host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ;
>  
> +	if (pdata->cd_type == S3C_SDHCI_CD_NONE ||
> +	    pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
> +		host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +
> +	if (pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
> +		host->mmc->caps = MMC_CAP_NONREMOVABLE;
> +
>  	host->quirks |= (SDHCI_QUIRK_32BIT_DMA_ADDR |
>  			 SDHCI_QUIRK_32BIT_DMA_SIZE);
>  
> @@ -367,6 +408,33 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  		goto err_add_host;
>  	}
>  
> +	/* pdata->ext_cd_init might call sdhci_s3c_notify_change immediately,
> +	   so it can be called only after sdhci_add_host() */
> +	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_init)
> +		pdata->ext_cd_init(&sdhci_s3c_notify_change);
> +
> +	if (pdata->cd_type == S3C_SDHCI_CD_GPIO &&
> +	    gpio_is_valid(pdata->ext_cd_gpio)) {
> +		int status;
> +
> +		gpio_request(pdata->ext_cd_gpio, "SDHCI EXT CD");

Shouldn't you check the return value of gpio_request()?

> +		sc->ext_cd_gpio = pdata->ext_cd_gpio;
> +
> +		sc->ext_cd_irq = gpio_to_irq(pdata->ext_cd_gpio);
> +		if (sc->ext_cd_irq &&
> +		    request_threaded_irq(sc->ext_cd_irq, NULL, sdhci_s3c_gpio_card_detect_thread,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				dev_name(&pdev->dev), sc)) {
> +			dev_err(&pdev->dev, "cannot request irq for card detect\n");
> +			sc->ext_cd_irq = 0;
> +		}
> +
> +		status = gpio_get_value(sc->ext_cd_gpio);
> +		if (sc->pdata->ext_cd_gpio_invert)
> +			status = !status;
> +		sdhci_s3c_notify_change(sc->pdev, status);
> +	}
> +
>  	return 0;
>  
>   err_add_host:
> @@ -391,10 +459,20 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  
>  static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>  {
> +	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
>  	struct sdhci_host *host =  platform_get_drvdata(pdev);
>  	struct sdhci_s3c *sc = sdhci_priv(host);
>  	int ptr;
>  
> +	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_cleanup)
> +		pdata->ext_cd_cleanup(&sdhci_s3c_notify_change);
> +
> +	if (sc->ext_cd_irq)
> +		free_irq(sc->ext_cd_irq, sc);
> +
> +	if (sc->ext_cd_gpio != -1)

Perhaps if (gpio_is_valid(...)) ?

> +		gpio_free(sc->ext_cd_gpio);
> +
>  	sdhci_remove_host(host, 1);
>  
>  	for (ptr = 0; ptr < 3; ptr++) {

-- 
Maurus Cuelenaere

[PATCHv5 3/4] sdhci-s3c: add support for new card detection methods (driver part)

[Marek Szyprowski]

Hello,

On Wednesday, July 28, 2010 4:40 PM Maurus Cuelenaere wrote:

>  Op 28-07-10 16:19, Marek Szyprowski schreef:
> > On some Samsung SoCs not all SDHCI controllers have card detect (CD)
> > line. For some embedded designs it is not even needed, because ususally
> > the device (like SDIO flash memory or wifi controller) is permanently
> > wired to the controller. There are also systems which have a card detect
> > line connected to some of the external interrupt lines or the presence
> > of the card depends on some other actions (like enabling a power
> > regulator).
> >
> > This patch adds support for all these cases. The following card
> > detection methods are possible:
> >
> > 1. internal sdhci host card detect line
> > 2. external event
> > 3. external gpio interrupt
> > 4. no card detect line, controller will poll for the card
> > 5. no card detect line, card is permanently wired to the controller
> > (once detected host won't poll it any more)
> >
> > By default, all existing code would use method #1, what is compatible
> > with the previous version of the driver.
> >
> > In case of external event, two callbacks must be provided in platdata:
> > ext_cd_init and ext_cd_cleanup. Both of them get a callback to a
> > function that notifies the s3c-sdhci host contoller as their argument.
> > That callback function should be called from the even dispatcher to let
> > host notice the card insertion/removal.
> >
> > In case of external gpio interrupt, a gpio pin number must be provided
> > in platdata (ext_cd_gpio parameter), as well as the information about
> > the polarity of that gpio pin (ext_cd_gpio_invert). By default
> > (ext_cd_gpio_invert == 0) gpio value 0 means 'card has been removed',
> > but this can be changed to 'card has been removed' when
> > ext_cd_gpio_invert == 1.
> >
> > This patch adds all required changes to sdhci-s3c driver.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >
> > Changes since V4:
> > - adapted to threaded irq approach in sdhci driver
> > - rebased onto latest -mm kernel tree
> > - fixed problem when gpio-base card detection was not called on driver
> > init
> >
> > Changes since V3:
> > - renamed patch to avoid confusion with the patch for the s3c-sdhci
> > platform changes - added "(driver part)" in subject
> >
> > Changes since V2:
> > According to Andrew Morton's suggestion local_irq_save() call in the
> > sdhci_s3c_notify_change function has been replaced by spin_lock_irqsave
> > (host driver already has a spinlock that is used for protecting internal
> > state of the driver).
> >
> > Changes since V1:
> > - added support for gpio external interrupt card based detect method
> >   directly to sdhci-s3c driver
> >
> > ---
> >  drivers/mmc/host/sdhci-s3c.c |   78
> ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/mmc/host/sdhci.c     |   47 ++++++++++++++-----------
> >  drivers/mmc/host/sdhci.h     |    1 +
> >  3 files changed, 106 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> > index 9938ded..606e695 100644
> > --- a/drivers/mmc/host/sdhci-s3c.c
> > +++ b/drivers/mmc/host/sdhci-s3c.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> > +#include <linux/gpio.h>
> >
> >  #include <linux/mmc/host.h>
> >
> > @@ -44,6 +45,8 @@ struct sdhci_s3c {
> >  	struct resource		*ioarea;
> >  	struct s3c_sdhci_platdata *pdata;
> >  	unsigned int		cur_clk;
> > +	int			ext_cd_irq;
> > +	int			ext_cd_gpio;
> >
> >  	struct clk		*clk_io;
> >  	struct clk		*clk_bus[MAX_BUS_CLK];
> > @@ -235,6 +238,36 @@ static struct sdhci_ops sdhci_s3c_ops = {
> >  	.get_min_clock		= sdhci_s3c_get_min_clock,
> >  };
> >
> > +static void sdhci_s3c_notify_change(struct platform_device *dev, int
> state)
> > +{
> > +	struct sdhci_host *host = platform_get_drvdata(dev);
> > +	if (host) {
> > +		mutex_lock(&host->lock);
> > +		if (state) {
> > +			dev_dbg(&dev->dev, "card inserted.\n");
> > +			host->flags &= ~SDHCI_DEVICE_DEAD;
> > +			host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > +			sdhci_card_detect(host);
> > +		} else {
> > +			dev_dbg(&dev->dev, "card removed.\n");
> > +			host->flags |= SDHCI_DEVICE_DEAD;
> > +			host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > +			sdhci_card_detect(host);
> > +		}
> 
> You could move the sdhci_card_detect outside the if.

Rigth, I will fix this.

> > +		mutex_unlock(&host->lock);
> > +	}
> > +}
> > +
> > +static irqreturn_t sdhci_s3c_gpio_card_detect_thread(int irq, void
> *dev_id)
> > +{
> > +	struct sdhci_s3c *sc = dev_id;
> > +	int status = gpio_get_value(sc->ext_cd_gpio);
> > +	if (sc->pdata->ext_cd_gpio_invert)
> > +		status = !status;
> > +	sdhci_s3c_notify_change(sc->pdev, status);
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
> >  {
> >  	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
> > @@ -272,6 +305,7 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >  	sc->host = host;
> >  	sc->pdev = pdev;
> >  	sc->pdata = pdata;
> > +	sc->ext_cd_gpio = -1;
> >
> >  	platform_set_drvdata(pdev, host);
> >
> > @@ -355,6 +389,13 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >  	 * SDHCI block, or a missing configuration that needs to be set. */
> >  	host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ;
> >
> > +	if (pdata->cd_type == S3C_SDHCI_CD_NONE ||
> > +	    pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
> > +		host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > +
> > +	if (pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
> > +		host->mmc->caps = MMC_CAP_NONREMOVABLE;
> > +
> >  	host->quirks |= (SDHCI_QUIRK_32BIT_DMA_ADDR |
> >  			 SDHCI_QUIRK_32BIT_DMA_SIZE);
> >
> > @@ -367,6 +408,33 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >  		goto err_add_host;
> >  	}
> >
> > +	/* pdata->ext_cd_init might call sdhci_s3c_notify_change immediately,
> > +	   so it can be called only after sdhci_add_host() */
> > +	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_init)
> > +		pdata->ext_cd_init(&sdhci_s3c_notify_change);
> > +
> > +	if (pdata->cd_type == S3C_SDHCI_CD_GPIO &&
> > +	    gpio_is_valid(pdata->ext_cd_gpio)) {
> > +		int status;
> > +
> > +		gpio_request(pdata->ext_cd_gpio, "SDHCI EXT CD");
> 
> Shouldn't you check the return value of gpio_request()?

Yes, I it should. I will fix this.

> 
> > +		sc->ext_cd_gpio = pdata->ext_cd_gpio;
> > +
> > +		sc->ext_cd_irq = gpio_to_irq(pdata->ext_cd_gpio);
> > +		if (sc->ext_cd_irq &&
> > +		    request_threaded_irq(sc->ext_cd_irq, NULL,
> sdhci_s3c_gpio_card_detect_thread,
> > +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > +				dev_name(&pdev->dev), sc)) {
> > +			dev_err(&pdev->dev, "cannot request irq for card
> detect\n");
> > +			sc->ext_cd_irq = 0;
> > +		}
> > +
> > +		status = gpio_get_value(sc->ext_cd_gpio);
> > +		if (sc->pdata->ext_cd_gpio_invert)
> > +			status = !status;
> > +		sdhci_s3c_notify_change(sc->pdev, status);
> > +	}
> > +
> >  	return 0;
> >
> >   err_add_host:
> > @@ -391,10 +459,20 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >
> >  static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
> >  {
> > +	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
> >  	struct sdhci_host *host =  platform_get_drvdata(pdev);
> >  	struct sdhci_s3c *sc = sdhci_priv(host);
> >  	int ptr;
> >
> > +	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_cleanup)
> > +		pdata->ext_cd_cleanup(&sdhci_s3c_notify_change);
> > +
> > +	if (sc->ext_cd_irq)
> > +		free_irq(sc->ext_cd_irq, sc);
> > +
> > +	if (sc->ext_cd_gpio != -1)
> 
> Perhaps if (gpio_is_valid(...)) ?

-1 is intentionally assigned in s3c_sdhci_probe() when no gpio pin is used
for card detection. I can change this to gpio_is_valid() if this increases
the code readability.

> 
> > +		gpio_free(sc->ext_cd_gpio);
> > +
> >  	sdhci_remove_host(host, 1);
> >
> >  	for (ptr = 0; ptr < 3; ptr++) {
> 
> --
> Maurus Cuelenaere

Thanks for your comments!

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

[PATCHv5 3/4] sdhci-s3c: add support for new card detection methods (driver part)

[Ben Dooks]

On 28/07/10 15:19, Marek Szyprowski wrote:
> On some Samsung SoCs not all SDHCI controllers have card detect (CD)
> line. For some embedded designs it is not even needed, because ususally
> the device (like SDIO flash memory or wifi controller) is permanently
> wired to the controller. There are also systems which have a card detect
> line connected to some of the external interrupt lines or the presence
> of the card depends on some other actions (like enabling a power
> regulator).
> 
> This patch adds support for all these cases. The following card
> detection methods are possible:
> 
> 1. internal sdhci host card detect line
> 2. external event
> 3. external gpio interrupt
> 4. no card detect line, controller will poll for the card
> 5. no card detect line, card is permanently wired to the controller
> (once detected host won't poll it any more)
> 
> By default, all existing code would use method #1, what is compatible
> with the previous version of the driver.
> 
> In case of external event, two callbacks must be provided in platdata:
> ext_cd_init and ext_cd_cleanup. Both of them get a callback to a
> function that notifies the s3c-sdhci host contoller as their argument.
> That callback function should be called from the even dispatcher to let
> host notice the card insertion/removal.
> 
> In case of external gpio interrupt, a gpio pin number must be provided
> in platdata (ext_cd_gpio parameter), as well as the information about
> the polarity of that gpio pin (ext_cd_gpio_invert). By default
> (ext_cd_gpio_invert == 0) gpio value 0 means 'card has been removed',
> but this can be changed to 'card has been removed' when
> ext_cd_gpio_invert == 1.
> 
> This patch adds all required changes to sdhci-s3c driver.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 
> Changes since V4:
> - adapted to threaded irq approach in sdhci driver
> - rebased onto latest -mm kernel tree
> - fixed problem when gpio-base card detection was not called on driver
> init
> 
> Changes since V3:
> - renamed patch to avoid confusion with the patch for the s3c-sdhci
> platform changes - added "(driver part)" in subject
> 
> Changes since V2:
> According to Andrew Morton's suggestion local_irq_save() call in the
> sdhci_s3c_notify_change function has been replaced by spin_lock_irqsave
> (host driver already has a spinlock that is used for protecting internal
> state of the driver).
> 
> Changes since V1:
> - added support for gpio external interrupt card based detect method
>   directly to sdhci-s3c driver
> 
> ---
>  drivers/mmc/host/sdhci-s3c.c |   78 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c     |   47 ++++++++++++++-----------
>  drivers/mmc/host/sdhci.h     |    1 +
>  3 files changed, 106 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 9938ded..606e695 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -18,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/gpio.h>
>  
>  #include <linux/mmc/host.h>
>  
> @@ -44,6 +45,8 @@ struct sdhci_s3c {
>  	struct resource		*ioarea;
>  	struct s3c_sdhci_platdata *pdata;
>  	unsigned int		cur_clk;
> +	int			ext_cd_irq;
> +	int			ext_cd_gpio;
>  
>  	struct clk		*clk_io;
>  	struct clk		*clk_bus[MAX_BUS_CLK];
> @@ -235,6 +238,36 @@ static struct sdhci_ops sdhci_s3c_ops = {
>  	.get_min_clock		= sdhci_s3c_get_min_clock,
>  };
>  
> +static void sdhci_s3c_notify_change(struct platform_device *dev, int state)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(dev);
> +	if (host) {
> +		mutex_lock(&host->lock);
> +		if (state) {
> +			dev_dbg(&dev->dev, "card inserted.\n");
> +			host->flags &= ~SDHCI_DEVICE_DEAD;
> +			host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +			sdhci_card_detect(host);
> +		} else {
> +			dev_dbg(&dev->dev, "card removed.\n");
> +			host->flags |= SDHCI_DEVICE_DEAD;
> +			host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +			sdhci_card_detect(host);
> +		}
> +		mutex_unlock(&host->lock);
> +	}
> +}
> +
> +static irqreturn_t sdhci_s3c_gpio_card_detect_thread(int irq, void *dev_id)
> +{
> +	struct sdhci_s3c *sc = dev_id;
> +	int status = gpio_get_value(sc->ext_cd_gpio);
> +	if (sc->pdata->ext_cd_gpio_invert)
> +		status = !status;

maybe move the invert into sdhci_s3c_notify_change()

> +	sdhci_s3c_notify_change(sc->pdev, status);
> +	return IRQ_HANDLED;
> +}
> +
>  static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  {
>  	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
> @@ -272,6 +305,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  	sc->host = host;
>  	sc->pdev = pdev;
>  	sc->pdata = pdata;
> +	sc->ext_cd_gpio = -1;
>  
>  	platform_set_drvdata(pdev, host);
>  
> @@ -355,6 +389,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  	 * SDHCI block, or a missing configuration that needs to be set. */
>  	host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ;
>  
> +	if (pdata->cd_type == S3C_SDHCI_CD_NONE ||
> +	    pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
> +		host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +
> +	if (pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
> +		host->mmc->caps = MMC_CAP_NONREMOVABLE;
> +
>  	host->quirks |= (SDHCI_QUIRK_32BIT_DMA_ADDR |
>  			 SDHCI_QUIRK_32BIT_DMA_SIZE);
>  
> @@ -367,6 +408,33 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  		goto err_add_host;
>  	}
>  
> +	/* pdata->ext_cd_init might call sdhci_s3c_notify_change immediately,
> +	   so it can be called only after sdhci_add_host() */
> +	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_init)
> +		pdata->ext_cd_init(&sdhci_s3c_notify_change);

do we really need a callback for this?

> +
> +	if (pdata->cd_type == S3C_SDHCI_CD_GPIO &&
> +	    gpio_is_valid(pdata->ext_cd_gpio)) {
> +		int status;
> +
> +		gpio_request(pdata->ext_cd_gpio, "SDHCI EXT CD");
> +		sc->ext_cd_gpio = pdata->ext_cd_gpio;
> +
> +		sc->ext_cd_irq = gpio_to_irq(pdata->ext_cd_gpio);
> +		if (sc->ext_cd_irq &&
> +		    request_threaded_irq(sc->ext_cd_irq, NULL, sdhci_s3c_gpio_card_detect_thread,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				dev_name(&pdev->dev), sc)) {
> +			dev_err(&pdev->dev, "cannot request irq for card detect\n");
> +			sc->ext_cd_irq = 0;
> +		}
> +
> +		status = gpio_get_value(sc->ext_cd_gpio);
> +		if (sc->pdata->ext_cd_gpio_invert)
> +			status = !status;
> +		sdhci_s3c_notify_change(sc->pdev, status);
> +	}
> +
>  	return 0;
>  
>   err_add_host:
> @@ -391,10 +459,20 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  
>  static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>  {
> +	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
>  	struct sdhci_host *host =  platform_get_drvdata(pdev);
>  	struct sdhci_s3c *sc = sdhci_priv(host);
>  	int ptr;
>  
> +	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_cleanup)
> +		pdata->ext_cd_cleanup(&sdhci_s3c_notify_change);
> +
> +	if (sc->ext_cd_irq)
> +		free_irq(sc->ext_cd_irq, sc);
> +
> +	if (sc->ext_cd_gpio != -1)
> +		gpio_free(sc->ext_cd_gpio);
> +
>  	sdhci_remove_host(host, 1);
>  
>  	for (ptr = 0; ptr < 3; ptr++) {
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 682b285..aa82344 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1244,26 +1244,6 @@ static const struct mmc_host_ops sdhci_ops = {
>   *                                                                           *
>  \*****************************************************************************/
>  
> -static void sdhci_card_detect(struct sdhci_host *host)
> -{
> -	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
> -		if (host->mrq) {
> -			printk(KERN_ERR "%s: Card removed during transfer!\n",
> -				mmc_hostname(host->mmc));
> -			printk(KERN_ERR "%s: Resetting controller.\n",
> -				mmc_hostname(host->mmc));
> -
> -			sdhci_reset(host, SDHCI_RESET_CMD);
> -			sdhci_reset(host, SDHCI_RESET_DATA);
> -
> -			host->mrq->cmd->error = -ENOMEDIUM;
> -			schedule_work(&host->finish_work);
> -		}
> -	}
> -
> -	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
> -}
> -
>  static void sdhci_finish_work(struct work_struct *wk)
>  {
>  	struct sdhci_host *host;
> @@ -1627,6 +1607,33 @@ EXPORT_SYMBOL_GPL(sdhci_resume_host);
>  
>  /*****************************************************************************\
>   *                                                                           *
> + * Implementation dependent callbacks                                        *
> + *                                                                           *
> +\*****************************************************************************/
> +
> +void sdhci_card_detect(struct sdhci_host *host)
> +{
> +	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
> +		if (host->mrq) {
> +			printk(KERN_ERR "%s: Card removed during transfer!\n",
> +				mmc_hostname(host->mmc));
> +			printk(KERN_ERR "%s: Resetting controller.\n",
> +				mmc_hostname(host->mmc));
> +
> +			sdhci_reset(host, SDHCI_RESET_CMD);
> +			sdhci_reset(host, SDHCI_RESET_DATA);
> +
> +			host->mrq->cmd->error = -ENOMEDIUM;
> +			schedule_work(&host->finish_work);
> +		}
> +	}
> +
> +	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
> +}
> +EXPORT_SYMBOL_GPL(sdhci_card_detect);

somehow this function seems to have been changed without a lot of
obvious changes?

[PATCHv5 3/4] sdhci-s3c: add support for new card detection methods (driver part)

[Marek Szyprowski]

Hello,

On Wednesday, July 28, 2010 7:03 PM Ben Dooks wrote:

> On 28/07/10 15:19, Marek Szyprowski wrote:
> > On some Samsung SoCs not all SDHCI controllers have card detect (CD)
> > line. For some embedded designs it is not even needed, because ususally
> > the device (like SDIO flash memory or wifi controller) is permanently
> > wired to the controller. There are also systems which have a card detect
> > line connected to some of the external interrupt lines or the presence
> > of the card depends on some other actions (like enabling a power
> > regulator).
> >
> > This patch adds support for all these cases. The following card
> > detection methods are possible:
> >
> > 1. internal sdhci host card detect line
> > 2. external event
> > 3. external gpio interrupt
> > 4. no card detect line, controller will poll for the card
> > 5. no card detect line, card is permanently wired to the controller
> > (once detected host won't poll it any more)
> >
> > By default, all existing code would use method #1, what is compatible
> > with the previous version of the driver.
> >
> > In case of external event, two callbacks must be provided in platdata:
> > ext_cd_init and ext_cd_cleanup. Both of them get a callback to a
> > function that notifies the s3c-sdhci host contoller as their argument.
> > That callback function should be called from the even dispatcher to let
> > host notice the card insertion/removal.
> >
> > In case of external gpio interrupt, a gpio pin number must be provided
> > in platdata (ext_cd_gpio parameter), as well as the information about
> > the polarity of that gpio pin (ext_cd_gpio_invert). By default
> > (ext_cd_gpio_invert == 0) gpio value 0 means 'card has been removed',
> > but this can be changed to 'card has been removed' when
> > ext_cd_gpio_invert == 1.
> >
> > This patch adds all required changes to sdhci-s3c driver.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >
> > Changes since V4:
> > - adapted to threaded irq approach in sdhci driver
> > - rebased onto latest -mm kernel tree
> > - fixed problem when gpio-base card detection was not called on driver
> > init
> >
> > Changes since V3:
> > - renamed patch to avoid confusion with the patch for the s3c-sdhci
> > platform changes - added "(driver part)" in subject
> >
> > Changes since V2:
> > According to Andrew Morton's suggestion local_irq_save() call in the
> > sdhci_s3c_notify_change function has been replaced by spin_lock_irqsave
> > (host driver already has a spinlock that is used for protecting internal
> > state of the driver).
> >
> > Changes since V1:
> > - added support for gpio external interrupt card based detect method
> >   directly to sdhci-s3c driver
> >
> > ---
> >  drivers/mmc/host/sdhci-s3c.c |   78
> ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/mmc/host/sdhci.c     |   47 ++++++++++++++-----------
> >  drivers/mmc/host/sdhci.h     |    1 +
> >  3 files changed, 106 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> > index 9938ded..606e695 100644
> > --- a/drivers/mmc/host/sdhci-s3c.c
> > +++ b/drivers/mmc/host/sdhci-s3c.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> > +#include <linux/gpio.h>
> >
> >  #include <linux/mmc/host.h>
> >
> > @@ -44,6 +45,8 @@ struct sdhci_s3c {
> >  	struct resource		*ioarea;
> >  	struct s3c_sdhci_platdata *pdata;
> >  	unsigned int		cur_clk;
> > +	int			ext_cd_irq;
> > +	int			ext_cd_gpio;
> >
> >  	struct clk		*clk_io;
> >  	struct clk		*clk_bus[MAX_BUS_CLK];
> > @@ -235,6 +238,36 @@ static struct sdhci_ops sdhci_s3c_ops = {
> >  	.get_min_clock		= sdhci_s3c_get_min_clock,
> >  };
> >
> > +static void sdhci_s3c_notify_change(struct platform_device *dev, int
> state)
> > +{
> > +	struct sdhci_host *host = platform_get_drvdata(dev);
> > +	if (host) {
> > +		mutex_lock(&host->lock);
> > +		if (state) {
> > +			dev_dbg(&dev->dev, "card inserted.\n");
> > +			host->flags &= ~SDHCI_DEVICE_DEAD;
> > +			host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > +			sdhci_card_detect(host);
> > +		} else {
> > +			dev_dbg(&dev->dev, "card removed.\n");
> > +			host->flags |= SDHCI_DEVICE_DEAD;
> > +			host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > +			sdhci_card_detect(host);
> > +		}
> > +		mutex_unlock(&host->lock);
> > +	}
> > +}
> > +
> > +static irqreturn_t sdhci_s3c_gpio_card_detect_thread(int irq, void
> *dev_id)
> > +{
> > +	struct sdhci_s3c *sc = dev_id;
> > +	int status = gpio_get_value(sc->ext_cd_gpio);
> > +	if (sc->pdata->ext_cd_gpio_invert)
> > +		status = !status;
> 
> maybe move the invert into sdhci_s3c_notify_change()

IMHO this is not a good idea. pdata->ext_cd_gpio_invert is used only for
GPIO-type card detection. External callback-based card detection can perform
similar inversion by itself, no need to duplicate it in sdhci-s3c driver.

> 
> > +	sdhci_s3c_notify_change(sc->pdev, status);
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
> >  {
> >  	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
> > @@ -272,6 +305,7 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >  	sc->host = host;
> >  	sc->pdev = pdev;
> >  	sc->pdata = pdata;
> > +	sc->ext_cd_gpio = -1;
> >
> >  	platform_set_drvdata(pdev, host);
> >
> > @@ -355,6 +389,13 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >  	 * SDHCI block, or a missing configuration that needs to be set. */
> >  	host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ;
> >
> > +	if (pdata->cd_type == S3C_SDHCI_CD_NONE ||
> > +	    pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
> > +		host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > +
> > +	if (pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
> > +		host->mmc->caps = MMC_CAP_NONREMOVABLE;
> > +
> >  	host->quirks |= (SDHCI_QUIRK_32BIT_DMA_ADDR |
> >  			 SDHCI_QUIRK_32BIT_DMA_SIZE);
> >
> > @@ -367,6 +408,33 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >  		goto err_add_host;
> >  	}
> >
> > +	/* pdata->ext_cd_init might call sdhci_s3c_notify_change immediately,
> > +	   so it can be called only after sdhci_add_host() */
> > +	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_init)
> > +		pdata->ext_cd_init(&sdhci_s3c_notify_change);
> 
> do we really need a callback for this?

Yes, there are cases that will use it. An example is a wifi sdio card,
permanently wired to sdhci controller, enabled/disabled by some rfkill glue.
This glue enables power regulators and notifies sdhci driver that the card
has been 'inserted' or 'removed'. This saves some power as otherwise sdhci
driver would need to pool for the card presence.

> > +
> > +	if (pdata->cd_type == S3C_SDHCI_CD_GPIO &&
> > +	    gpio_is_valid(pdata->ext_cd_gpio)) {
> > +		int status;
> > +
> > +		gpio_request(pdata->ext_cd_gpio, "SDHCI EXT CD");
> > +		sc->ext_cd_gpio = pdata->ext_cd_gpio;
> > +
> > +		sc->ext_cd_irq = gpio_to_irq(pdata->ext_cd_gpio);
> > +		if (sc->ext_cd_irq &&
> > +		    request_threaded_irq(sc->ext_cd_irq, NULL,
> sdhci_s3c_gpio_card_detect_thread,
> > +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > +				dev_name(&pdev->dev), sc)) {
> > +			dev_err(&pdev->dev, "cannot request irq for card
> detect\n");
> > +			sc->ext_cd_irq = 0;
> > +		}
> > +
> > +		status = gpio_get_value(sc->ext_cd_gpio);
> > +		if (sc->pdata->ext_cd_gpio_invert)
> > +			status = !status;
> > +		sdhci_s3c_notify_change(sc->pdev, status);
> > +	}
> > +
> >  	return 0;
> >
> >   err_add_host:
> > @@ -391,10 +459,20 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >
> >  static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
> >  {
> > +	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
> >  	struct sdhci_host *host =  platform_get_drvdata(pdev);
> >  	struct sdhci_s3c *sc = sdhci_priv(host);
> >  	int ptr;
> >
> > +	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_cleanup)
> > +		pdata->ext_cd_cleanup(&sdhci_s3c_notify_change);
> > +
> > +	if (sc->ext_cd_irq)
> > +		free_irq(sc->ext_cd_irq, sc);
> > +
> > +	if (sc->ext_cd_gpio != -1)
> > +		gpio_free(sc->ext_cd_gpio);
> > +
> >  	sdhci_remove_host(host, 1);
> >
> >  	for (ptr = 0; ptr < 3; ptr++) {
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 682b285..aa82344 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1244,26 +1244,6 @@ static const struct mmc_host_ops sdhci_ops = {
> >   *
> *
> >
> \**************************************************************************
> ***/
> >
> > -static void sdhci_card_detect(struct sdhci_host *host)
> > -{
> > -	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
> > -		if (host->mrq) {
> > -			printk(KERN_ERR "%s: Card removed during transfer!\n",
> > -				mmc_hostname(host->mmc));
> > -			printk(KERN_ERR "%s: Resetting controller.\n",
> > -				mmc_hostname(host->mmc));
> > -
> > -			sdhci_reset(host, SDHCI_RESET_CMD);
> > -			sdhci_reset(host, SDHCI_RESET_DATA);
> > -
> > -			host->mrq->cmd->error = -ENOMEDIUM;
> > -			schedule_work(&host->finish_work);
> > -		}
> > -	}
> > -
> > -	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
> > -}
> > -
> >  static void sdhci_finish_work(struct work_struct *wk)
> >  {
> >  	struct sdhci_host *host;
> > @@ -1627,6 +1607,33 @@ EXPORT_SYMBOL_GPL(sdhci_resume_host);
> >
> >
> /**************************************************************************
> ***\
> >   *
> *
> > + * Implementation dependent callbacks
> *
> > + *
> *
> >
> +\*************************************************************************
> ****/
> > +
> > +void sdhci_card_detect(struct sdhci_host *host)
> > +{
> > +	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
> > +		if (host->mrq) {
> > +			printk(KERN_ERR "%s: Card removed during transfer!\n",
> > +				mmc_hostname(host->mmc));
> > +			printk(KERN_ERR "%s: Resetting controller.\n",
> > +				mmc_hostname(host->mmc));
> > +
> > +			sdhci_reset(host, SDHCI_RESET_CMD);
> > +			sdhci_reset(host, SDHCI_RESET_DATA);
> > +
> > +			host->mrq->cmd->error = -ENOMEDIUM;
> > +			schedule_work(&host->finish_work);
> > +		}
> > +	}
> > +
> > +	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_card_detect);
> 
> somehow this function seems to have been changed without a lot of
> obvious changes?

I've just moved this function out of 'workers section' to the new 
'implementation dependent callbacks' section to make it easier to read the code.
The function itself has been modified when sdhci driver has been converted to
use threaded irqs rather than tasklets.

I you think that it would be better to leave the function in the place where it
was, just let me know.

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 4/4] sdhci-s3c: add regulator support

[Marek Szyprowski]

This patch adds support for regulator API to sdhci-s3c driver. Regulators
can be used to disable power in suspended state to reduce dissipated
energy.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mmc/host/sdhci-s3c.c |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 606e695..b9d46a5 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -19,6 +19,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/mmc/host.h>
 
@@ -50,6 +51,7 @@ struct sdhci_s3c {
 
 	struct clk		*clk_io;
 	struct clk		*clk_bus[MAX_BUS_CLK];
+	struct regulator	*vmmc;
 };
 
 static inline struct sdhci_s3c *to_s3c(struct sdhci_host *host)
@@ -309,6 +311,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, host);
 
+	sc->vmmc = regulator_get(dev, "vmmc");
+	if (IS_ERR(sc->vmmc)) {
+		dev_warn(dev, "no vmmc regulator found\n");
+		sc->vmmc = NULL;
+	} else
+		regulator_enable(sc->vmmc);
+
 	sc->clk_io = clk_get(dev, "hsmmc");
 	if (IS_ERR(sc->clk_io)) {
 		dev_err(dev, "failed to get io clock\n");
@@ -482,6 +491,11 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
 	clk_disable(sc->clk_io);
 	clk_put(sc->clk_io);
 
+	if (sc->vmmc) {
+		regulator_disable(sc->vmmc);
+		regulator_put(sc->vmmc);
+	}
+
 	iounmap(host->ioaddr);
 	release_resource(sc->ioarea);
 	kfree(sc->ioarea);
@@ -496,15 +510,29 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
 
 static int sdhci_s3c_suspend(struct platform_device *dev, pm_message_t pm)
 {
+	int ret = 0;
 	struct sdhci_host *host = platform_get_drvdata(dev);
+	struct sdhci_s3c *sc = sdhci_priv(host);
 
 	sdhci_suspend_host(host, pm);
-	return 0;
+
+	if (sc->vmmc)
+		ret = regulator_disable(sc->vmmc);
+
+	return ret;
 }
 
 static int sdhci_s3c_resume(struct platform_device *dev)
 {
 	struct sdhci_host *host = platform_get_drvdata(dev);
+	struct sdhci_s3c *sc = sdhci_priv(host);
+
+	if (sc->vmmc) {
+		int ret = regulator_disable(sc->vmmc);
+		if (ret)
+			return ret;
+		mdelay(2);
+	}
 
 	sdhci_resume_host(host);
 	return 0;
-- 
1.7.1.569.g6f426

[PATCH 4/4] sdhci-s3c: add regulator support

[Maurus Cuelenaere]

 Op 28-07-10 16:19, Marek Szyprowski schreef:
> This patch adds support for regulator API to sdhci-s3c driver. Regulators
> can be used to disable power in suspended state to reduce dissipated
> energy.

I'm not sure about this, when I would try to do this I'd look at implementing
this in the sdhci driver itself instead of a subdriver of it.

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/mmc/host/sdhci-s3c.c |   30 +++++++++++++++++++++++++++++-
>  1 files changed, 29 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 606e695..b9d46a5 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -19,6 +19,7 @@
>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/gpio.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include <linux/mmc/host.h>
>  
> @@ -50,6 +51,7 @@ struct sdhci_s3c {
>  
>  	struct clk		*clk_io;
>  	struct clk		*clk_bus[MAX_BUS_CLK];
> +	struct regulator	*vmmc;
>  };
>  
>  static inline struct sdhci_s3c *to_s3c(struct sdhci_host *host)
> @@ -309,6 +311,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, host);
>  
> +	sc->vmmc = regulator_get(dev, "vmmc");
> +	if (IS_ERR(sc->vmmc)) {
> +		dev_warn(dev, "no vmmc regulator found\n");
> +		sc->vmmc = NULL;
> +	} else
> +		regulator_enable(sc->vmmc);
> +
>  	sc->clk_io = clk_get(dev, "hsmmc");
>  	if (IS_ERR(sc->clk_io)) {
>  		dev_err(dev, "failed to get io clock\n");
> @@ -482,6 +491,11 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>  	clk_disable(sc->clk_io);
>  	clk_put(sc->clk_io);
>  
> +	if (sc->vmmc) {
> +		regulator_disable(sc->vmmc);
> +		regulator_put(sc->vmmc);
> +	}
> +
>  	iounmap(host->ioaddr);
>  	release_resource(sc->ioarea);
>  	kfree(sc->ioarea);
> @@ -496,15 +510,29 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>  
>  static int sdhci_s3c_suspend(struct platform_device *dev, pm_message_t pm)
>  {
> +	int ret = 0;
>  	struct sdhci_host *host = platform_get_drvdata(dev);
> +	struct sdhci_s3c *sc = sdhci_priv(host);
>  
>  	sdhci_suspend_host(host, pm);
> -	return 0;
> +
> +	if (sc->vmmc)
> +		ret = regulator_disable(sc->vmmc);
> +
> +	return ret;
>  }
>  
>  static int sdhci_s3c_resume(struct platform_device *dev)
>  {
>  	struct sdhci_host *host = platform_get_drvdata(dev);
> +	struct sdhci_s3c *sc = sdhci_priv(host);
> +
> +	if (sc->vmmc) {
> +		int ret = regulator_disable(sc->vmmc);
> +		if (ret)
> +			return ret;
> +		mdelay(2);

Shouldn't these delays be handled in the regulator framework itself?

> +	}
>  
>  	sdhci_resume_host(host);
>  	return 0;


-- 
Maurus Cuelenaere

[PATCH 4/4] sdhci-s3c: add regulator support

[Mark Brown]

On 28 Jul 2010, at 07:48, Maurus Cuelenaere <mcuelenaere@gmail.com> wrote:
>> 
>> +	struct sdhci_s3c *sc = sdhci_priv(host);
>> +
>> +	if (sc->vmmc) {
>> +		int ret = regulator_disable(sc->vmmc);
>> +		if (ret)
>> +			return ret;
>> +		mdelay(2);
> 
> Shouldn't these delays be handled in the regulator framework itself?

A 2ms delay on power down seems suspicious for a regulator. I'm not sure why this is required but if it is I suspect it's due to a large cap on the regulator output and light load rather than something that's always true for whatever regulator is providing the supply.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100728/cb7a4997/attachment-0001.html>

[PATCH 4/4] sdhci-s3c: add regulator support

[Maurus Cuelenaere]

 Op 28-07-10 17:41, Mark Brown schreef:
> On 28 Jul 2010, at 07:48, Maurus Cuelenaere <mcuelenaere@gmail.com> wrote:
>>> +	struct sdhci_s3c *sc = sdhci_priv(host);
>>> +
>>> +	if (sc->vmmc) {
>>> +		int ret = regulator_disable(sc->vmmc);
>>> +		if (ret)
>>> +			return ret;
>>> +		mdelay(2);
>> Shouldn't these delays be handled in the regulator framework itself?
> A 2ms delay on power down seems suspicious for a regulator. I'm not sure why this is required but if it is I suspect it's due to a large cap on the regulator output and light load rather than something that's always true for whatever regulator is providing the supply.

I wasn't suggesting to do the delay in the framework *itself*, rather in the
regulator driver and/or the board platform code which needs this delay.

-- 
Maurus Cuelenaere

[PATCH 4/4] sdhci-s3c: add regulator support

[Mark Brown]

On Wed, Jul 28, 2010 at 07:06:22PM +0200, Maurus Cuelenaere wrote:
>  Op 28-07-10 17:41, Mark Brown schreef:

> >>> +	if (sc->vmmc) {
> >>> +		int ret = regulator_disable(sc->vmmc);
> >>> +		if (ret)
> >>> +			return ret;
> >>> +		mdelay(2);

> >> Shouldn't these delays be handled in the regulator framework itself?

> > A 2ms delay on power down seems suspicious for a regulator. I'm not
> > sure why this is required but if it is I suspect it's due to a large
> > cap on the regulator output and light load rather than something
> > that's always true for whatever regulator is providing the supply.

> I wasn't suggesting to do the delay in the framework *itself*, rather in the
> regulator driver and/or the board platform code which needs this delay.

It's unlikely to be a property of the regulator in general, it sounds
like it's a property of the MMC application somehow (most likely due to
very light loading).  Possibly it should be platform configurable, but I
suspect in the MMC driver.

It'd be nice to know exactly what the delay is for...

[PATCH 4/4] sdhci-s3c: add regulator support

[Marek Szyprowski]

Hello,

On Wednesday, July 28, 2010 4:48 PM Maurus Cuelenaere wrote:


> -----Original Message-----
> From: [mailto:mcuelenaere at gmail.com]
> Sent: 
> To: Marek Szyprowski
> Cc: linux-mmc at vger.kernel.org; linux-samsung-soc at vger.kernel.org; linux-
> arm-kernel at lists.infradead.org; kyungmin.park at samsung.com;
> kgene.kim at samsung.com; ben-linux at fluff.org; akpm at linux-foundation.org;
> broonie at opensource.wolfsonmicro.com
> Subject: Re: [PATCH 4/4] sdhci-s3c: add regulator support
> 
>  Op 28-07-10 16:19, Marek Szyprowski schreef:
> > This patch adds support for regulator API to sdhci-s3c driver. Regulators
> > can be used to disable power in suspended state to reduce dissipated
> > energy.
> 
> I'm not sure about this, when I would try to do this I'd look at
> implementing
> this in the sdhci driver itself instead of a subdriver of it.

Well, if you think that sdhci driver is better place for this I can move all
my changes to the main driver.

> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  drivers/mmc/host/sdhci-s3c.c |   30 +++++++++++++++++++++++++++++-
> >  1 files changed, 29 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> > index 606e695..b9d46a5 100644
> > --- a/drivers/mmc/host/sdhci-s3c.c
> > +++ b/drivers/mmc/host/sdhci-s3c.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> >  #include <linux/gpio.h>
> > +#include <linux/regulator/consumer.h>
> >
> >  #include <linux/mmc/host.h>
> >
> > @@ -50,6 +51,7 @@ struct sdhci_s3c {
> >
> >  	struct clk		*clk_io;
> >  	struct clk		*clk_bus[MAX_BUS_CLK];
> > +	struct regulator	*vmmc;
> >  };
> >
> >  static inline struct sdhci_s3c *to_s3c(struct sdhci_host *host)
> > @@ -309,6 +311,13 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> >
> >  	platform_set_drvdata(pdev, host);
> >
> > +	sc->vmmc = regulator_get(dev, "vmmc");
> > +	if (IS_ERR(sc->vmmc)) {
> > +		dev_warn(dev, "no vmmc regulator found\n");
> > +		sc->vmmc = NULL;
> > +	} else
> > +		regulator_enable(sc->vmmc);
> > +
> >  	sc->clk_io = clk_get(dev, "hsmmc");
> >  	if (IS_ERR(sc->clk_io)) {
> >  		dev_err(dev, "failed to get io clock\n");
> > @@ -482,6 +491,11 @@ static int __devexit sdhci_s3c_remove(struct
> platform_device *pdev)
> >  	clk_disable(sc->clk_io);
> >  	clk_put(sc->clk_io);
> >
> > +	if (sc->vmmc) {
> > +		regulator_disable(sc->vmmc);
> > +		regulator_put(sc->vmmc);
> > +	}
> > +
> >  	iounmap(host->ioaddr);
> >  	release_resource(sc->ioarea);
> >  	kfree(sc->ioarea);
> > @@ -496,15 +510,29 @@ static int __devexit sdhci_s3c_remove(struct
> platform_device *pdev)
> >
> >  static int sdhci_s3c_suspend(struct platform_device *dev, pm_message_t
> pm)
> >  {
> > +	int ret = 0;
> >  	struct sdhci_host *host = platform_get_drvdata(dev);
> > +	struct sdhci_s3c *sc = sdhci_priv(host);
> >
> >  	sdhci_suspend_host(host, pm);
> > -	return 0;
> > +
> > +	if (sc->vmmc)
> > +		ret = regulator_disable(sc->vmmc);
> > +
> > +	return ret;
> >  }
> >
> >  static int sdhci_s3c_resume(struct platform_device *dev)
> >  {
> >  	struct sdhci_host *host = platform_get_drvdata(dev);
> > +	struct sdhci_s3c *sc = sdhci_priv(host);
> > +
> > +	if (sc->vmmc) {
> > +		int ret = regulator_disable(sc->vmmc);

Hmm, I was in a real hurry if I missed this. Should be regulator_enable()
of course. Looks that I confused patches and posted an older version...

> > +		if (ret)
> > +			return ret;
> > +		mdelay(2);
> 
> Shouldn't these delays be handled in the regulator framework itself?

This is just a left-over from an older version, should be definitely 
removed. Sorry for the confusion.
 
> > +	}
> >
> >  	sdhci_resume_host(host);
> >  	return 0;


Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 4/4] sdhci-s3c: add regulator support

[Mark Brown]

On Wed, Jul 28, 2010 at 04:19:57PM +0200, Marek Szyprowski wrote:

>  static int sdhci_s3c_suspend(struct platform_device *dev, pm_message_t pm)
>  {
> +	int ret = 0;
>  	struct sdhci_host *host = platform_get_drvdata(dev);
> +	struct sdhci_s3c *sc = sdhci_priv(host);
>  
>  	sdhci_suspend_host(host, pm);
> -	return 0;
> +
> +	if (sc->vmmc)
> +		ret = regulator_disable(sc->vmmc);
> +

So, on suspend you disable the regulator...

> +	struct sdhci_s3c *sc = sdhci_priv(host);
> +
> +	if (sc->vmmc) {
> +		int ret = regulator_disable(sc->vmmc);
> +		if (ret)
> +			return ret;

...and on resume you disable it again.  I'm surprised this works
properly after resume, at the very least the core is going to get
confused about the reference counts I'd expect.

I suspect this is for something we need to do better in the regulator
API - restoring the state of the regulators after a resume if the
hardware changes them.  A lot of regulators just do this in hardware so
we didn't need to worry about it previously.

> +		mdelay(2);

This could do with some sort of comment.