[PATCH] mmc: slot-gpio: Add support for power gpios

[PATCH] mmc: slot-gpio: Add support for power gpios

[Chris Ball]

A power gpio is a sideband output gpio that controls card power.

Signed-off-by: Chris Ball <cjb@laptop.org>
---
Hi Guennadi, what do you think of this patch?  I'm hoping to use it
first to de-duplicate power-gpio handling between sdhci-pxa and
sdhci-tegra, and then to create a generic DT parser for MMC.

The zero-length array trick might be becoming unmaintainable as we add
more labels to the struct like this.  Do you think we should keep it
as-is, or change to individual allocations?  Thanks!

 drivers/mmc/core/slot-gpio.c  | 65 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mmc/host.h      |  1 +
 include/linux/mmc/slot-gpio.h |  3 ++
 3 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 4f9b366..6c785dd 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -20,6 +20,8 @@
 struct mmc_gpio {
 	int ro_gpio;
 	int cd_gpio;
+	int pwr_gpio;
+	char *pwr_label;
 	char *ro_label;
 	char cd_label[0];
 };
@@ -33,6 +35,9 @@ static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
 
 static int mmc_gpio_alloc(struct mmc_host *host)
 {
+	/* The "+ 4" is to leave room for a space, two-char identifier
+	 * and \0 after each label in the mmc_gpio struct.
+	 */
 	size_t len = strlen(dev_name(host->parent)) + 4;
 	struct mmc_gpio *ctx;
 
@@ -45,14 +50,19 @@ static int mmc_gpio_alloc(struct mmc_host *host)
 		 * before device_add(), i.e., between mmc_alloc_host() and
 		 * mmc_add_host()
 		 */
-		ctx = devm_kzalloc(&host->class_dev, sizeof(*ctx) + 2 * len,
+
+		/* len*3 because we have three strings to allocate on the end. */
+		ctx = devm_kzalloc(&host->class_dev, sizeof(*ctx) + len*3,
 				   GFP_KERNEL);
 		if (ctx) {
 			ctx->ro_label = ctx->cd_label + len;
+			ctx->pwr_label = ctx->cd_label + len*2;
 			snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
 			snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
+			snprintf(ctx->pwr_label, len, "%s pw", dev_name(host->parent));
 			ctx->cd_gpio = -EINVAL;
 			ctx->ro_gpio = -EINVAL;
+			ctx->pwr_gpio = -EINVAL;
 			host->slot.handler_priv = ctx;
 		}
 	}
@@ -74,6 +84,18 @@ int mmc_gpio_get_ro(struct mmc_host *host)
 }
 EXPORT_SYMBOL(mmc_gpio_get_ro);
 
+int mmc_gpio_get_pwr(struct mmc_host *host)
+{
+	struct mmc_gpio *ctx = host->slot.handler_priv;
+
+	if (!ctx || !gpio_is_valid(ctx->pwr_gpio))
+		return -ENOSYS;
+
+	return !gpio_get_value_cansleep(ctx->pwr_gpio) ^
+		!!(host->caps2 & MMC_CAP2_PWR_ACTIVE_HIGH);
+}
+EXPORT_SYMBOL(mmc_gpio_get_pwr);
+
 int mmc_gpio_get_cd(struct mmc_host *host)
 {
 	struct mmc_gpio *ctx = host->slot.handler_priv;
@@ -105,6 +127,32 @@ int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio)
 }
 EXPORT_SYMBOL(mmc_gpio_request_ro);
 
+int mmc_gpio_request_pwr(struct mmc_host *host, unsigned int gpio)
+{
+	struct mmc_gpio *ctx;
+	int ret;
+	unsigned long gpio_flags;
+
+	if (!gpio_is_valid(gpio))
+		return -EINVAL;
+
+	ret = mmc_gpio_alloc(host);
+	if (ret < 0)
+		return ret;
+
+	ctx = host->slot.handler_priv;
+	ctx->pwr_gpio = gpio;
+
+	gpio_flags = GPIOF_DIR_OUT;
+	if (host->caps2 & MMC_CAP2_PWR_ACTIVE_HIGH)
+		gpio_flags |= GPIOF_INIT_HIGH;
+	else
+		gpio_flags |= GPIOF_INIT_LOW;
+
+	return gpio_request_one(gpio, gpio_flags, ctx->pwr_label);
+}
+EXPORT_SYMBOL(mmc_gpio_request_pwr);
+
 int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio)
 {
 	struct mmc_gpio *ctx;
@@ -168,6 +216,21 @@ void mmc_gpio_free_ro(struct mmc_host *host)
 }
 EXPORT_SYMBOL(mmc_gpio_free_ro);
 
+void mmc_gpio_free_pwr(struct mmc_host *host)
+{
+	struct mmc_gpio *ctx = host->slot.handler_priv;
+	int gpio;
+
+	if (!ctx || !gpio_is_valid(ctx->pwr_gpio))
+		return;
+
+	gpio = ctx->pwr_gpio;
+	ctx->pwr_gpio = -EINVAL;
+
+	gpio_free(gpio);
+}
+EXPORT_SYMBOL(mmc_gpio_free_pwr);
+
 void mmc_gpio_free_cd(struct mmc_host *host)
 {
 	struct mmc_gpio *ctx = host->slot.handler_priv;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index d5d9bd4..5a00cc1 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -257,6 +257,7 @@ struct mmc_host {
 #define MMC_CAP2_HC_ERASE_SZ	(1 << 9)	/* High-capacity erase size */
 #define MMC_CAP2_CD_ACTIVE_HIGH	(1 << 10)	/* Card-detect signal active high */
 #define MMC_CAP2_RO_ACTIVE_HIGH	(1 << 11)	/* Write-protect signal active high */
+#define MMC_CAP2_PWR_ACTIVE_HIGH (1 << 12)	/* Card power signal active high */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 	unsigned int        power_notify_type;
diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
index 7d88d27..d083dfa 100644
--- a/include/linux/mmc/slot-gpio.h
+++ b/include/linux/mmc/slot-gpio.h
@@ -21,4 +21,7 @@ int mmc_gpio_get_cd(struct mmc_host *host);
 int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio);
 void mmc_gpio_free_cd(struct mmc_host *host);
 
+int mmc_gpio_get_pwr(struct mmc_host *host);
+int mmc_gpio_request_pwr(struct mmc_host *host, unsigned int gpio);
+void mmc_gpio_free_pwr(struct mmc_host *host);
 #endif
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: slot-gpio: Add support for power gpios

[Guennadi Liakhovetski]

Hi Chris

On Sat, 8 Sep 2012, Chris Ball wrote:

> A power gpio is a sideband output gpio that controls card power.
> 
> Signed-off-by: Chris Ball <cjb@laptop.org>
> ---
> Hi Guennadi, what do you think of this patch?  I'm hoping to use it

Looks good in general to me, thanks! Just a couple of nit-picks below.

> first to de-duplicate power-gpio handling between sdhci-pxa and
> sdhci-tegra, and then to create a generic DT parser for MMC.
> 
> The zero-length array trick might be becoming unmaintainable as we add
> more labels to the struct like this.  Do you think we should keep it
> as-is, or change to individual allocations?  Thanks!

Yeah, it's becoming a bit messy... But I also don't find dynamically 
allocating multiple tiny memory areas with the same life-cycle 
particularly elegant... Are you expecting many more GPIOs to be added 
here? But just imagine, if we switch to individual allocations: we'd need 
those pointers in the struct anyway and would have to check allocation 
failures for each string... Don't think it would look much prettier. What 
we could do, though, we could add macros for various pin function names, 
their lengths, offsets and the total length, to make the calculations look 
less cryptic. Would that be a good alternative?

> 
>  drivers/mmc/core/slot-gpio.c  | 65 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mmc/host.h      |  1 +
>  include/linux/mmc/slot-gpio.h |  3 ++
>  3 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index 4f9b366..6c785dd 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -20,6 +20,8 @@
>  struct mmc_gpio {
>  	int ro_gpio;
>  	int cd_gpio;
> +	int pwr_gpio;
> +	char *pwr_label;
>  	char *ro_label;
>  	char cd_label[0];
>  };
> @@ -33,6 +35,9 @@ static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
>  
>  static int mmc_gpio_alloc(struct mmc_host *host)
>  {
> +	/* The "+ 4" is to leave room for a space, two-char identifier
> +	 * and \0 after each label in the mmc_gpio struct.
> +	 */

Generally, I'm trying not to play coding-style police, but I think, it is 
good to try to preserve a unique coding style across files. In this file I 
so far use the recommended multi-line comment style, i.e.

	/*
	 * line 1
	 * line 2
	 * ...
	 * line N
	 */

Would be good to keep it.

>  	size_t len = strlen(dev_name(host->parent)) + 4;
>  	struct mmc_gpio *ctx;
>  
> @@ -45,14 +50,19 @@ static int mmc_gpio_alloc(struct mmc_host *host)
>  		 * before device_add(), i.e., between mmc_alloc_host() and
>  		 * mmc_add_host()
>  		 */
> -		ctx = devm_kzalloc(&host->class_dev, sizeof(*ctx) + 2 * len,
> +
> +		/* len*3 because we have three strings to allocate on the end. */
> +		ctx = devm_kzalloc(&host->class_dev, sizeof(*ctx) + len*3,

Also here, please, use a space around '*': "len * 3" or "3 * len" - 
whichever you prefer:)

>  				   GFP_KERNEL);
>  		if (ctx) {
>  			ctx->ro_label = ctx->cd_label + len;
> +			ctx->pwr_label = ctx->cd_label + len*2;
>  			snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
>  			snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
> +			snprintf(ctx->pwr_label, len, "%s pw", dev_name(host->parent));
>  			ctx->cd_gpio = -EINVAL;
>  			ctx->ro_gpio = -EINVAL;
> +			ctx->pwr_gpio = -EINVAL;
>  			host->slot.handler_priv = ctx;
>  		}
>  	}
> @@ -74,6 +84,18 @@ int mmc_gpio_get_ro(struct mmc_host *host)
>  }
>  EXPORT_SYMBOL(mmc_gpio_get_ro);
>  
> +int mmc_gpio_get_pwr(struct mmc_host *host)
> +{
> +	struct mmc_gpio *ctx = host->slot.handler_priv;
> +
> +	if (!ctx || !gpio_is_valid(ctx->pwr_gpio))
> +		return -ENOSYS;
> +
> +	return !gpio_get_value_cansleep(ctx->pwr_gpio) ^
> +		!!(host->caps2 & MMC_CAP2_PWR_ACTIVE_HIGH);
> +}
> +EXPORT_SYMBOL(mmc_gpio_get_pwr);
> +
>  int mmc_gpio_get_cd(struct mmc_host *host)
>  {
>  	struct mmc_gpio *ctx = host->slot.handler_priv;
> @@ -105,6 +127,32 @@ int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio)
>  }
>  EXPORT_SYMBOL(mmc_gpio_request_ro);
>  
> +int mmc_gpio_request_pwr(struct mmc_host *host, unsigned int gpio)
> +{
> +	struct mmc_gpio *ctx;
> +	int ret;
> +	unsigned long gpio_flags;
> +
> +	if (!gpio_is_valid(gpio))
> +		return -EINVAL;
> +
> +	ret = mmc_gpio_alloc(host);
> +	if (ret < 0)
> +		return ret;
> +
> +	ctx = host->slot.handler_priv;
> +	ctx->pwr_gpio = gpio;

Like in the previous patch - I would only do this, if gpio_request_one() 
was successful.

> +
> +	gpio_flags = GPIOF_DIR_OUT;
> +	if (host->caps2 & MMC_CAP2_PWR_ACTIVE_HIGH)
> +		gpio_flags |= GPIOF_INIT_HIGH;
> +	else
> +		gpio_flags |= GPIOF_INIT_LOW;

You could use GPIOF_OUT_INIT_LOW and GPIOF_OUT_INIT_HIGH directly, if you 
like.

Thanks
Guennadi

> +
> +	return gpio_request_one(gpio, gpio_flags, ctx->pwr_label);
> +}
> +EXPORT_SYMBOL(mmc_gpio_request_pwr);
> +
>  int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio)
>  {
>  	struct mmc_gpio *ctx;
> @@ -168,6 +216,21 @@ void mmc_gpio_free_ro(struct mmc_host *host)
>  }
>  EXPORT_SYMBOL(mmc_gpio_free_ro);
>  
> +void mmc_gpio_free_pwr(struct mmc_host *host)
> +{
> +	struct mmc_gpio *ctx = host->slot.handler_priv;
> +	int gpio;
> +
> +	if (!ctx || !gpio_is_valid(ctx->pwr_gpio))
> +		return;
> +
> +	gpio = ctx->pwr_gpio;
> +	ctx->pwr_gpio = -EINVAL;
> +
> +	gpio_free(gpio);
> +}
> +EXPORT_SYMBOL(mmc_gpio_free_pwr);
> +
>  void mmc_gpio_free_cd(struct mmc_host *host)
>  {
>  	struct mmc_gpio *ctx = host->slot.handler_priv;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index d5d9bd4..5a00cc1 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -257,6 +257,7 @@ struct mmc_host {
>  #define MMC_CAP2_HC_ERASE_SZ	(1 << 9)	/* High-capacity erase size */
>  #define MMC_CAP2_CD_ACTIVE_HIGH	(1 << 10)	/* Card-detect signal active high */
>  #define MMC_CAP2_RO_ACTIVE_HIGH	(1 << 11)	/* Write-protect signal active high */
> +#define MMC_CAP2_PWR_ACTIVE_HIGH (1 << 12)	/* Card power signal active high */
>  
>  	mmc_pm_flag_t		pm_caps;	/* supported pm features */
>  	unsigned int        power_notify_type;
> diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
> index 7d88d27..d083dfa 100644
> --- a/include/linux/mmc/slot-gpio.h
> +++ b/include/linux/mmc/slot-gpio.h
> @@ -21,4 +21,7 @@ int mmc_gpio_get_cd(struct mmc_host *host);
>  int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio);
>  void mmc_gpio_free_cd(struct mmc_host *host);
>  
> +int mmc_gpio_get_pwr(struct mmc_host *host);
> +int mmc_gpio_request_pwr(struct mmc_host *host, unsigned int gpio);
> +void mmc_gpio_free_pwr(struct mmc_host *host);
>  #endif
> -- 
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
> 

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

Re: [PATCH] mmc: slot-gpio: Add support for power gpios

[Chris Ball]

Hi Guennadi,

Thanks for getting to this review so quickly!  I agree with all of your
comments.  I'm sending out v2 of both patches now.

On Sun, Sep 09 2012, Guennadi Liakhovetski wrote:
>> The zero-length array trick might be becoming unmaintainable as we add
>> more labels to the struct like this.  Do you think we should keep it
>> as-is, or change to individual allocations?  Thanks!
>
> Yeah, it's becoming a bit messy... But I also don't find dynamically 
> allocating multiple tiny memory areas with the same life-cycle 
> particularly elegant... Are you expecting many more GPIOs to be added 
> here?

No, I don't expect needing to add more -- I just found myself wondering
if there was a better way and thought I'd mention it.  Performing
multiple allocations is unattractive too, I agree.

> What we could do, though, we could add macros for various pin function
> names, their lengths, offsets and the total length, to make the
> calculations look less cryptic. Would that be a good alternative?

Adding the comments is probably fine for now; I'd be okay with waiting
to see if we need more GPIO types and revisiting a better solution then.

Thanks!

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

[PATCH v2] mmc: slot-gpio: Add support for power gpios

[Chris Ball]

A power gpio is a sideband output gpio that controls card power.

Signed-off-by: Chris Ball <cjb@laptop.org>
---
Changelog:

v2, addressing Guennadi's review comments:
 - More consistent comment and operator coding style
 - Only store pwr_gpio in ctx if requesting it was successful
 - Use GPIOF_OUT_INIT_{LOW,HIGH} directly

 drivers/mmc/core/slot-gpio.c  | 70 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mmc/host.h      |  1 +
 include/linux/mmc/slot-gpio.h |  3 ++
 3 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 08c6b3d..cf7e826 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -20,6 +20,8 @@
 struct mmc_gpio {
 	int ro_gpio;
 	int cd_gpio;
+	int pwr_gpio;
+	char *pwr_label;
 	char *ro_label;
 	char cd_label[0];
 };
@@ -33,6 +35,10 @@ static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
 
 static int mmc_gpio_alloc(struct mmc_host *host)
 {
+	/*
+	 * The "+ 4" is to leave room for a space, two-char identifier
+	 * and \0 after each label in the mmc_gpio struct.
+	 */
 	size_t len = strlen(dev_name(host->parent)) + 4;
 	struct mmc_gpio *ctx;
 
@@ -45,14 +51,19 @@ static int mmc_gpio_alloc(struct mmc_host *host)
 		 * before device_add(), i.e., between mmc_alloc_host() and
 		 * mmc_add_host()
 		 */
-		ctx = devm_kzalloc(&host->class_dev, sizeof(*ctx) + 2 * len,
+
+		/* len * 3 because we have 3 strings to allocate on the end. */
+		ctx = devm_kzalloc(&host->class_dev, sizeof(*ctx) + len * 3,
 				   GFP_KERNEL);
 		if (ctx) {
 			ctx->ro_label = ctx->cd_label + len;
+			ctx->pwr_label = ctx->cd_label + len * 2;
 			snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
 			snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
+			snprintf(ctx->pwr_label, len, "%s pw", dev_name(host->parent));
 			ctx->cd_gpio = -EINVAL;
 			ctx->ro_gpio = -EINVAL;
+			ctx->pwr_gpio = -EINVAL;
 			host->slot.handler_priv = ctx;
 		}
 	}
@@ -74,6 +85,18 @@ int mmc_gpio_get_ro(struct mmc_host *host)
 }
 EXPORT_SYMBOL(mmc_gpio_get_ro);
 
+int mmc_gpio_get_pwr(struct mmc_host *host)
+{
+	struct mmc_gpio *ctx = host->slot.handler_priv;
+
+	if (!ctx || !gpio_is_valid(ctx->pwr_gpio))
+		return -ENOSYS;
+
+	return !gpio_get_value_cansleep(ctx->pwr_gpio) ^
+		!!(host->caps2 & MMC_CAP2_PWR_ACTIVE_HIGH);
+}
+EXPORT_SYMBOL(mmc_gpio_get_pwr);
+
 int mmc_gpio_get_cd(struct mmc_host *host)
 {
 	struct mmc_gpio *ctx = host->slot.handler_priv;
@@ -110,6 +133,36 @@ int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio)
 }
 EXPORT_SYMBOL(mmc_gpio_request_ro);
 
+int mmc_gpio_request_pwr(struct mmc_host *host, unsigned int gpio)
+{
+	struct mmc_gpio *ctx;
+	int ret;
+	unsigned long gpio_flags;
+
+	if (!gpio_is_valid(gpio))
+		return -EINVAL;
+
+	ret = mmc_gpio_alloc(host);
+	if (ret < 0)
+		return ret;
+
+	ctx = host->slot.handler_priv;
+
+	if (host->caps2 & MMC_CAP2_PWR_ACTIVE_HIGH)
+		gpio_flags = GPIOF_OUT_INIT_HIGH;
+	else
+		gpio_flags = GPIOF_OUT_INIT_LOW;
+
+	ret = gpio_request_one(gpio, gpio_flags, ctx->pwr_label);
+	if (ret < 0)
+		return ret;
+
+	ctx->pwr_gpio = gpio;
+
+	return 0;
+}
+EXPORT_SYMBOL(mmc_gpio_request_pwr);
+
 int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio)
 {
 	struct mmc_gpio *ctx;
@@ -173,6 +226,21 @@ void mmc_gpio_free_ro(struct mmc_host *host)
 }
 EXPORT_SYMBOL(mmc_gpio_free_ro);
 
+void mmc_gpio_free_pwr(struct mmc_host *host)
+{
+	struct mmc_gpio *ctx = host->slot.handler_priv;
+	int gpio;
+
+	if (!ctx || !gpio_is_valid(ctx->pwr_gpio))
+		return;
+
+	gpio = ctx->pwr_gpio;
+	ctx->pwr_gpio = -EINVAL;
+
+	gpio_free(gpio);
+}
+EXPORT_SYMBOL(mmc_gpio_free_pwr);
+
 void mmc_gpio_free_cd(struct mmc_host *host)
 {
 	struct mmc_gpio *ctx = host->slot.handler_priv;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index d5d9bd4..5a00cc1 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -257,6 +257,7 @@ struct mmc_host {
 #define MMC_CAP2_HC_ERASE_SZ	(1 << 9)	/* High-capacity erase size */
 #define MMC_CAP2_CD_ACTIVE_HIGH	(1 << 10)	/* Card-detect signal active high */
 #define MMC_CAP2_RO_ACTIVE_HIGH	(1 << 11)	/* Write-protect signal active high */
+#define MMC_CAP2_PWR_ACTIVE_HIGH (1 << 12)	/* Card power signal active high */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 	unsigned int        power_notify_type;
diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
index 7d88d27..d083dfa 100644
--- a/include/linux/mmc/slot-gpio.h
+++ b/include/linux/mmc/slot-gpio.h
@@ -21,4 +21,7 @@ int mmc_gpio_get_cd(struct mmc_host *host);
 int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio);
 void mmc_gpio_free_cd(struct mmc_host *host);
 
+int mmc_gpio_get_pwr(struct mmc_host *host);
+int mmc_gpio_request_pwr(struct mmc_host *host, unsigned int gpio);
+void mmc_gpio_free_pwr(struct mmc_host *host);
 #endif
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v2] mmc: slot-gpio: Add support for power gpios

[Guennadi Liakhovetski]

Hi Chris

On Sun, 9 Sep 2012, Chris Ball wrote:

> A power gpio is a sideband output gpio that controls card power.

Thanks for the patch. It looks good formally now, thanks for taking into 
account my comments. However, I'm wondering: this GPIO is different from 
the previous two - it's an output. So, I've got a couple of doubts:

1. in mmc_gpio_request_pwr() you request GPIO as output and immediately 
turn power on. Is this really what we want to do?

2. since this is a card power GPIO, you also want to toggle it, so, you'd 
need a mmc_gpio_set_pwr() function too.

3. now, this is the biggest one. Are we sure we need this at all? I think, 
you should just set up a fixed (or a GPIO, if more than one voltage is 
possible, which isn't supported by this patch) regulator and use the 
mmc_regulator_*() API from your MMC host driver's .set_ios() method. Don't 
you think this would be a better option? Or it wouldn't work for you?

Thanks
Guennadi

> 
> Signed-off-by: Chris Ball <cjb@laptop.org>
> ---
> Changelog:
> 
> v2, addressing Guennadi's review comments:
>  - More consistent comment and operator coding style
>  - Only store pwr_gpio in ctx if requesting it was successful
>  - Use GPIOF_OUT_INIT_{LOW,HIGH} directly
> 
>  drivers/mmc/core/slot-gpio.c  | 70 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mmc/host.h      |  1 +
>  include/linux/mmc/slot-gpio.h |  3 ++
>  3 files changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index 08c6b3d..cf7e826 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -20,6 +20,8 @@
>  struct mmc_gpio {
>  	int ro_gpio;
>  	int cd_gpio;
> +	int pwr_gpio;
> +	char *pwr_label;
>  	char *ro_label;
>  	char cd_label[0];
>  };
> @@ -33,6 +35,10 @@ static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
>  
>  static int mmc_gpio_alloc(struct mmc_host *host)
>  {
> +	/*
> +	 * The "+ 4" is to leave room for a space, two-char identifier
> +	 * and \0 after each label in the mmc_gpio struct.
> +	 */
>  	size_t len = strlen(dev_name(host->parent)) + 4;
>  	struct mmc_gpio *ctx;
>  
> @@ -45,14 +51,19 @@ static int mmc_gpio_alloc(struct mmc_host *host)
>  		 * before device_add(), i.e., between mmc_alloc_host() and
>  		 * mmc_add_host()
>  		 */
> -		ctx = devm_kzalloc(&host->class_dev, sizeof(*ctx) + 2 * len,
> +
> +		/* len * 3 because we have 3 strings to allocate on the end. */
> +		ctx = devm_kzalloc(&host->class_dev, sizeof(*ctx) + len * 3,
>  				   GFP_KERNEL);
>  		if (ctx) {
>  			ctx->ro_label = ctx->cd_label + len;
> +			ctx->pwr_label = ctx->cd_label + len * 2;
>  			snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
>  			snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
> +			snprintf(ctx->pwr_label, len, "%s pw", dev_name(host->parent));
>  			ctx->cd_gpio = -EINVAL;
>  			ctx->ro_gpio = -EINVAL;
> +			ctx->pwr_gpio = -EINVAL;
>  			host->slot.handler_priv = ctx;
>  		}
>  	}
> @@ -74,6 +85,18 @@ int mmc_gpio_get_ro(struct mmc_host *host)
>  }
>  EXPORT_SYMBOL(mmc_gpio_get_ro);
>  
> +int mmc_gpio_get_pwr(struct mmc_host *host)
> +{
> +	struct mmc_gpio *ctx = host->slot.handler_priv;
> +
> +	if (!ctx || !gpio_is_valid(ctx->pwr_gpio))
> +		return -ENOSYS;
> +
> +	return !gpio_get_value_cansleep(ctx->pwr_gpio) ^
> +		!!(host->caps2 & MMC_CAP2_PWR_ACTIVE_HIGH);
> +}
> +EXPORT_SYMBOL(mmc_gpio_get_pwr);
> +
>  int mmc_gpio_get_cd(struct mmc_host *host)
>  {
>  	struct mmc_gpio *ctx = host->slot.handler_priv;
> @@ -110,6 +133,36 @@ int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio)
>  }
>  EXPORT_SYMBOL(mmc_gpio_request_ro);
>  
> +int mmc_gpio_request_pwr(struct mmc_host *host, unsigned int gpio)
> +{
> +	struct mmc_gpio *ctx;
> +	int ret;
> +	unsigned long gpio_flags;
> +
> +	if (!gpio_is_valid(gpio))
> +		return -EINVAL;
> +
> +	ret = mmc_gpio_alloc(host);
> +	if (ret < 0)
> +		return ret;
> +
> +	ctx = host->slot.handler_priv;
> +
> +	if (host->caps2 & MMC_CAP2_PWR_ACTIVE_HIGH)
> +		gpio_flags = GPIOF_OUT_INIT_HIGH;
> +	else
> +		gpio_flags = GPIOF_OUT_INIT_LOW;
> +
> +	ret = gpio_request_one(gpio, gpio_flags, ctx->pwr_label);
> +	if (ret < 0)
> +		return ret;
> +
> +	ctx->pwr_gpio = gpio;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mmc_gpio_request_pwr);
> +
>  int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio)
>  {
>  	struct mmc_gpio *ctx;
> @@ -173,6 +226,21 @@ void mmc_gpio_free_ro(struct mmc_host *host)
>  }
>  EXPORT_SYMBOL(mmc_gpio_free_ro);
>  
> +void mmc_gpio_free_pwr(struct mmc_host *host)
> +{
> +	struct mmc_gpio *ctx = host->slot.handler_priv;
> +	int gpio;
> +
> +	if (!ctx || !gpio_is_valid(ctx->pwr_gpio))
> +		return;
> +
> +	gpio = ctx->pwr_gpio;
> +	ctx->pwr_gpio = -EINVAL;
> +
> +	gpio_free(gpio);
> +}
> +EXPORT_SYMBOL(mmc_gpio_free_pwr);
> +
>  void mmc_gpio_free_cd(struct mmc_host *host)
>  {
>  	struct mmc_gpio *ctx = host->slot.handler_priv;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index d5d9bd4..5a00cc1 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -257,6 +257,7 @@ struct mmc_host {
>  #define MMC_CAP2_HC_ERASE_SZ	(1 << 9)	/* High-capacity erase size */
>  #define MMC_CAP2_CD_ACTIVE_HIGH	(1 << 10)	/* Card-detect signal active high */
>  #define MMC_CAP2_RO_ACTIVE_HIGH	(1 << 11)	/* Write-protect signal active high */
> +#define MMC_CAP2_PWR_ACTIVE_HIGH (1 << 12)	/* Card power signal active high */
>  
>  	mmc_pm_flag_t		pm_caps;	/* supported pm features */
>  	unsigned int        power_notify_type;
> diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
> index 7d88d27..d083dfa 100644
> --- a/include/linux/mmc/slot-gpio.h
> +++ b/include/linux/mmc/slot-gpio.h
> @@ -21,4 +21,7 @@ int mmc_gpio_get_cd(struct mmc_host *host);
>  int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio);
>  void mmc_gpio_free_cd(struct mmc_host *host);
>  
> +int mmc_gpio_get_pwr(struct mmc_host *host);
> +int mmc_gpio_request_pwr(struct mmc_host *host, unsigned int gpio);
> +void mmc_gpio_free_pwr(struct mmc_host *host);
>  #endif
> -- 
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
> 

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