[PATCH 1/2] mmc: slot-gpio: use devm_* managed functions to ease users

[PATCH 1/2] mmc: slot-gpio: use devm_* managed functions to ease users

[Shawn Guo]

Use devm_* managed functions, so that slot-gpio users do not have to
call mmc_gpio_free_ro/cd to free up resources requested in
mmc_gpio_request_ro/cd.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/mmc/core/slot-gpio.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 16a1c0b..8b55748 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -106,7 +106,8 @@ int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio)
 
 	ctx = host->slot.handler_priv;
 
-	ret = gpio_request_one(gpio, GPIOF_DIR_IN, ctx->ro_label);
+	ret = devm_gpio_request_one(&host->class_dev, gpio, GPIOF_DIR_IN,
+				    ctx->ro_label);
 	if (ret < 0)
 		return ret;
 
@@ -128,7 +129,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio)
 
 	ctx = host->slot.handler_priv;
 
-	ret = gpio_request_one(gpio, GPIOF_DIR_IN, ctx->cd_label);
+	ret = devm_gpio_request_one(&host->class_dev, gpio, GPIOF_DIR_IN,
+				    ctx->cd_label);
 	if (ret < 0)
 		/*
 		 * don't bother freeing memory. It might still get used by other
@@ -146,7 +148,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio)
 		irq = -EINVAL;
 
 	if (irq >= 0) {
-		ret = request_threaded_irq(irq, NULL, mmc_gpio_cd_irqt,
+		ret = devm_request_threaded_irq(&host->class_dev, irq,
+			NULL, mmc_gpio_cd_irqt,
 			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 			ctx->cd_label, host);
 		if (ret < 0)
@@ -175,7 +178,7 @@ void mmc_gpio_free_ro(struct mmc_host *host)
 	gpio = ctx->ro_gpio;
 	ctx->ro_gpio = -EINVAL;
 
-	gpio_free(gpio);
+	devm_gpio_free(&host->class_dev, gpio);
 }
 EXPORT_SYMBOL(mmc_gpio_free_ro);
 
@@ -188,13 +191,13 @@ void mmc_gpio_free_cd(struct mmc_host *host)
 		return;
 
 	if (host->slot.cd_irq >= 0) {
-		free_irq(host->slot.cd_irq, host);
+		devm_free_irq(&host->class_dev, host->slot.cd_irq, host);
 		host->slot.cd_irq = -EINVAL;
 	}
 
 	gpio = ctx->cd_gpio;
 	ctx->cd_gpio = -EINVAL;
 
-	gpio_free(gpio);
+	devm_gpio_free(&host->class_dev, gpio);
 }
 EXPORT_SYMBOL(mmc_gpio_free_cd);
-- 
1.7.9.5

[PATCH 2/2] mmc: sdhci-esdhc-imx: use slot-gpio helpers for CD and WP

[Shawn Guo]

Use slot-gpio helpers to save some code in the driver.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |   56 +++++++++++-------------------------
 1 file changed, 16 insertions(+), 40 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index e07df81..dd7fcc1 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -21,6 +21,7 @@
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>
 #include <linux/mmc/sdio.h>
+#include <linux/mmc/slot-gpio.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
@@ -147,17 +148,16 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
 	struct esdhc_platform_data *boarddata = &imx_data->boarddata;
 
-	/* fake CARD_PRESENT flag */
 	u32 val = readl(host->ioaddr + reg);
 
-	if (unlikely((reg == SDHCI_PRESENT_STATE)
-			&& gpio_is_valid(boarddata->cd_gpio))) {
-		if (gpio_get_value(boarddata->cd_gpio))
-			/* no card, if a valid gpio says so... */
+	if (unlikely(reg == SDHCI_PRESENT_STATE)) {
+		/*
+		 * After SDHCI core gets improved to never query
+		 * SDHCI_CARD_PRESENT state in GPIO case, we can
+		 * remove this check.
+		 */
+		if (boarddata->cd_type == ESDHC_CD_GPIO)
 			val &= ~SDHCI_CARD_PRESENT;
-		else
-			/* ... in all other cases assume card is present */
-			val |= SDHCI_CARD_PRESENT;
 	}
 
 	if (unlikely(reg == SDHCI_CAPABILITIES)) {
@@ -362,8 +362,7 @@ static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
 
 	switch (boarddata->wp_type) {
 	case ESDHC_WP_GPIO:
-		if (gpio_is_valid(boarddata->wp_gpio))
-			return gpio_get_value(boarddata->wp_gpio);
+		return mmc_gpio_get_ro(host->mmc);
 	case ESDHC_WP_CONTROLLER:
 		return !(readl(host->ioaddr + SDHCI_PRESENT_STATE) &
 			       SDHCI_WRITE_PROTECT);
@@ -394,14 +393,6 @@ static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
 	.ops = &sdhci_esdhc_ops,
 };
 
-static irqreturn_t cd_irq(int irq, void *data)
-{
-	struct sdhci_host *sdhost = (struct sdhci_host *)data;
-
-	tasklet_schedule(&sdhost->card_tasklet);
-	return IRQ_HANDLED;
-};
-
 #ifdef CONFIG_OF
 static int
 sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
@@ -527,37 +518,22 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 
 	/* write_protect */
 	if (boarddata->wp_type == ESDHC_WP_GPIO) {
-		err = devm_gpio_request_one(&pdev->dev, boarddata->wp_gpio,
-					    GPIOF_IN, "ESDHC_WP");
+		err = mmc_gpio_request_ro(host->mmc, boarddata->wp_gpio);
 		if (err) {
-			dev_warn(mmc_dev(host->mmc),
-				 "no write-protect pin available!\n");
-			boarddata->wp_gpio = -EINVAL;
+			dev_err(mmc_dev(host->mmc),
+				"failed to request write-protect gpio!\n");
+			goto disable_clk;
 		}
-	} else {
-		boarddata->wp_gpio = -EINVAL;
+		host->mmc->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
 	}
 
 	/* card_detect */
-	if (boarddata->cd_type != ESDHC_CD_GPIO)
-		boarddata->cd_gpio = -EINVAL;
-
 	switch (boarddata->cd_type) {
 	case ESDHC_CD_GPIO:
-		err = devm_gpio_request_one(&pdev->dev, boarddata->cd_gpio,
-					    GPIOF_IN, "ESDHC_CD");
+		err = mmc_gpio_request_cd(host->mmc, boarddata->cd_gpio);
 		if (err) {
 			dev_err(mmc_dev(host->mmc),
-				"no card-detect pin available!\n");
-			goto disable_clk;
-		}
-
-		err = devm_request_irq(&pdev->dev,
-				 gpio_to_irq(boarddata->cd_gpio), cd_irq,
-				 IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
-				 mmc_hostname(host->mmc), host);
-		if (err) {
-			dev_err(mmc_dev(host->mmc), "request irq error\n");
+				"failed to request card-detect gpio!\n");
 			goto disable_clk;
 		}
 		/* fall through */
-- 
1.7.9.5

[PATCH 1/2] mmc: slot-gpio: use devm_* managed functions to ease users

[Guennadi Liakhovetski]

Hi Shawn

On Fri, 7 Dec 2012, Shawn Guo wrote:

> Use devm_* managed functions, so that slot-gpio users do not have to
> call mmc_gpio_free_ro/cd to free up resources requested in
> mmc_gpio_request_ro/cd.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

Thanks for the patch, but I'm not sure this is a good idea. Firstly, using 
devm_* allocation functions means, that normally you don't have to free 
these resources explicitly any more. So, actually you would have to remove 
free_irq() and gpio_free() calls completely from the API instead of 
replacing them with devm_* analogs. Secondly, I do use devm_kzalloc() in 
the API because I really want that allocated memory to just be allocated 
once and stay there until the device is freed. Whereas with with card 
detection and write-protect pins - usually you're right, you would only 
allocate those once during the lifetime of your host device. But - are we 
sure there cannot be exceptions? What if someone decides to switch between 
CD-IRQ and CD-polling at runtime? Or do something equally weird with their 
GPIOs... At least I personally don't think I have sufficient knowledge of 
all possible configurations to make such a decision. Think about pinctrl 
etc. What if at runtime the CD pin has to be released by mmc to be used 
by some other device? Anyway, I'm fine either way, but I'm just not sure 
how reasonable this change is.

Thanks
Guennadi

> ---
>  drivers/mmc/core/slot-gpio.c |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index 16a1c0b..8b55748 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -106,7 +106,8 @@ int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio)
>  
>  	ctx = host->slot.handler_priv;
>  
> -	ret = gpio_request_one(gpio, GPIOF_DIR_IN, ctx->ro_label);
> +	ret = devm_gpio_request_one(&host->class_dev, gpio, GPIOF_DIR_IN,
> +				    ctx->ro_label);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -128,7 +129,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio)
>  
>  	ctx = host->slot.handler_priv;
>  
> -	ret = gpio_request_one(gpio, GPIOF_DIR_IN, ctx->cd_label);
> +	ret = devm_gpio_request_one(&host->class_dev, gpio, GPIOF_DIR_IN,
> +				    ctx->cd_label);
>  	if (ret < 0)
>  		/*
>  		 * don't bother freeing memory. It might still get used by other
> @@ -146,7 +148,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio)
>  		irq = -EINVAL;
>  
>  	if (irq >= 0) {
> -		ret = request_threaded_irq(irq, NULL, mmc_gpio_cd_irqt,
> +		ret = devm_request_threaded_irq(&host->class_dev, irq,
> +			NULL, mmc_gpio_cd_irqt,
>  			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  			ctx->cd_label, host);
>  		if (ret < 0)
> @@ -175,7 +178,7 @@ void mmc_gpio_free_ro(struct mmc_host *host)
>  	gpio = ctx->ro_gpio;
>  	ctx->ro_gpio = -EINVAL;
>  
> -	gpio_free(gpio);
> +	devm_gpio_free(&host->class_dev, gpio);
>  }
>  EXPORT_SYMBOL(mmc_gpio_free_ro);
>  
> @@ -188,13 +191,13 @@ void mmc_gpio_free_cd(struct mmc_host *host)
>  		return;
>  
>  	if (host->slot.cd_irq >= 0) {
> -		free_irq(host->slot.cd_irq, host);
> +		devm_free_irq(&host->class_dev, host->slot.cd_irq, host);
>  		host->slot.cd_irq = -EINVAL;
>  	}
>  
>  	gpio = ctx->cd_gpio;
>  	ctx->cd_gpio = -EINVAL;
>  
> -	gpio_free(gpio);
> +	devm_gpio_free(&host->class_dev, gpio);
>  }
>  EXPORT_SYMBOL(mmc_gpio_free_cd);
> -- 
> 1.7.9.5
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH 1/2] mmc: slot-gpio: use devm_* managed functions to ease users

[Shawn Guo]

On Fri, Dec 07, 2012 at 02:49:43PM +0100, Guennadi Liakhovetski wrote:
> Hi Shawn
> 
> On Fri, 7 Dec 2012, Shawn Guo wrote:
> 
> > Use devm_* managed functions, so that slot-gpio users do not have to
> > call mmc_gpio_free_ro/cd to free up resources requested in
> > mmc_gpio_request_ro/cd.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> 
> Thanks for the patch, but I'm not sure this is a good idea. Firstly, using 
> devm_* allocation functions means, that normally you don't have to free 
> these resources explicitly any more. So, actually you would have to remove 
> free_irq() and gpio_free() calls completely from the API instead of 
> replacing them with devm_* analogs.

With the changes, most of the slot-gpio users will only need to call
mmc_gpio_request_* functions at probe time, nothing else.  That said,
they will not call mmc_gpio_free_* functions at all.  I patched
mmc_gpio_free_* functions replacing free_irq() and gpio_free() with
devm_* versions to 1) ease the migration of exiting users calling
mmc_gpio_free_*; 2) provide a mean for users to manually free resources
for whatever reasons.

> Secondly, I do use devm_kzalloc() in 
> the API because I really want that allocated memory to just be allocated 
> once and stay there until the device is freed. Whereas with with card 
> detection and write-protect pins - usually you're right, you would only 
> allocate those once during the lifetime of your host device. But - are we 
> sure there cannot be exceptions? What if someone decides to switch between 
> CD-IRQ and CD-polling at runtime? Or do something equally weird with their 
> GPIOs... At least I personally don't think I have sufficient knowledge of 
> all possible configurations to make such a decision. Think about pinctrl 
> etc. What if at runtime the CD pin has to be released by mmc to be used 
> by some other device?

mmc_gpio_request_* and mmc_gpio_free_* pair are still there anyway.
Users can manually call them in any way they want.

> Anyway, I'm fine either way, but I'm just not sure 
> how reasonable this change is.

My intension is to save slot-gpio users from checking GPIO CD/WP cases
and calling mmc_gpio_request_* functions, so that we can have a cleaner
.probe error path and .remove function.

Shawn

[PATCH 1/2] mmc: slot-gpio: use devm_* managed functions to ease users

[Chris Ball]

Hi Shawn, Guennadi,

On Fri, Dec 07 2012, Shawn Guo wrote:
>> > Use devm_* managed functions, so that slot-gpio users do not have to
>> > call mmc_gpio_free_ro/cd to free up resources requested in
>> > mmc_gpio_request_ro/cd.
>> > 
>> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> 
>> Thanks for the patch, but I'm not sure this is a good idea. Firstly, using 
>> devm_* allocation functions means, that normally you don't have to free 
>> these resources explicitly any more. So, actually you would have to remove 
>> free_irq() and gpio_free() calls completely from the API instead of 
>> replacing them with devm_* analogs.
>
> With the changes, most of the slot-gpio users will only need to call
> mmc_gpio_request_* functions at probe time, nothing else.  That said,
> they will not call mmc_gpio_free_* functions at all.  I patched
> mmc_gpio_free_* functions replacing free_irq() and gpio_free() with
> devm_* versions to 1) ease the migration of exiting users calling
> mmc_gpio_free_*; 2) provide a mean for users to manually free resources
> for whatever reasons.

I'd like to find a way to merge this -- maybe we can compromise by
adding a comment or some documentation that explains that you need to be
careful (and use _request_* and _free_*) to avoid allocating more than
once if you're doing something odd like runtime switching between CD
methods?

Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 1/2] mmc: slot-gpio: use devm_* managed functions to ease users

[Guennadi Liakhovetski]

On Mon, 10 Dec 2012, Chris Ball wrote:

> Hi Shawn, Guennadi,
> 
> On Fri, Dec 07 2012, Shawn Guo wrote:
> >> > Use devm_* managed functions, so that slot-gpio users do not have to
> >> > call mmc_gpio_free_ro/cd to free up resources requested in
> >> > mmc_gpio_request_ro/cd.
> >> > 
> >> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> >> 
> >> Thanks for the patch, but I'm not sure this is a good idea. Firstly, using 
> >> devm_* allocation functions means, that normally you don't have to free 
> >> these resources explicitly any more. So, actually you would have to remove 
> >> free_irq() and gpio_free() calls completely from the API instead of 
> >> replacing them with devm_* analogs.
> >
> > With the changes, most of the slot-gpio users will only need to call
> > mmc_gpio_request_* functions at probe time, nothing else.  That said,
> > they will not call mmc_gpio_free_* functions at all.  I patched
> > mmc_gpio_free_* functions replacing free_irq() and gpio_free() with
> > devm_* versions to 1) ease the migration of exiting users calling
> > mmc_gpio_free_*; 2) provide a mean for users to manually free resources
> > for whatever reasons.
> 
> I'd like to find a way to merge this -- maybe we can compromise by
> adding a comment or some documentation that explains that you need to be
> careful (and use _request_* and _free_*) to avoid allocating more than
> once if you're doing something odd like runtime switching between CD
> methods?

Ok, so, you agree, that the normal case is, when these GPIO functions are 
requested only once during probing and are released during driver 
unbinding, and that it's worth optimisine for this case. In principle I 
agree, and so far all 3 users of this API do that. So, maybe it would be 
good to extend this patch series with a patch (or 3 patches, if per-driver 
is preferred), removing mmc_gpio_free_*() calls from those drivers. I 
think, however, there is one more thing to consider here: the slot-gpio 
API contains a GPIO CD IRQ handler, which after these changes will only be 
freed after the calls to mmc_remove_host() and mmc_free_host(). This 
should be safe, because mmc_remove_host() sets the .rescan_disable flag. 
After the recent patch the GPIO CD ISR also calls host->ops->card_event(), 
so, hosts muct make sure, that this function is safe to call even after 
the driver's .remove() method has completed. With that in mind, I think, 
we're fine with this patch.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH 1/2] mmc: slot-gpio: use devm_* managed functions to ease users

[Shawn Guo]

On Tue, Dec 11, 2012 at 08:32:14AM +0100, Guennadi Liakhovetski wrote:
> Ok, so, you agree, that the normal case is, when these GPIO functions are 
> requested only once during probing and are released during driver 
> unbinding, and that it's worth optimisine for this case. In principle I 
> agree, and so far all 3 users of this API do that. So, maybe it would be 
> good to extend this patch series with a patch (or 3 patches, if per-driver 
> is preferred), removing mmc_gpio_free_*() calls from those drivers.

Ok, will do (with single patch approach first).

> I 
> think, however, there is one more thing to consider here: the slot-gpio 
> API contains a GPIO CD IRQ handler, which after these changes will only be 
> freed after the calls to mmc_remove_host() and mmc_free_host(). This 
> should be safe, because mmc_remove_host() sets the .rescan_disable flag. 
> After the recent patch the GPIO CD ISR also calls host->ops->card_event(), 
> so, hosts muct make sure, that this function is safe to call even after 
> the driver's .remove() method has completed. With that in mind, I think, 
> we're fine with this patch.
> 
Thanks for the point.  I think we should be fine as client drivers'
.remove() hook will generally call mmc_remove_host() and mmc_free_host()
in there.

Shawn