mmc: slot-gpio: Add support for power gpios

PATCH Superseded Chris Ball 2 participants 5 messages Sep 9, 2012 – Sep 10, 2012 MMC, SD and SDIO

lore

Share & tools

lore

Chris Ball

author

2012-09-09 03:01 UTC #0 lore raw

PATCH 1/1 mmc: slot-gpio: Add support for power gpios

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

Signed-off-by Chris Ball <[email protected]>

---
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(-)

drivers/mmc/core/slot-gpio.c +64 -1

index 4f9b366..6c785dd 100644
@@ -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;

include/linux/mmc/host.h +1 -0

index d5d9bd4..5a00cc1 100644
@@ -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;

include/linux/mmc/slot-gpio.h +3 -0

index 7d88d27..d083dfa 100644
@@ -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

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Guennadi Liakhovetski

2012-09-09 22:05 UTC #1 lore raw

Hi Chris

Quoted 5 lines · Chris Ball

A power gpio is a sideband output gpio that controls card power.
Signed-off-by Chris Ball <[email protected]>
---
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.

Quoted 6 lines

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?

Quoted 20 lines

 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(-)

drivers/mmc/core/slot-gpio.c +2 -0

index 4f9b366..6c785dd 100644
@@ -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];
 };

drivers/mmc/core/slot-gpio.c static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id) · line 40

Quoted 6 lines

drivers/mmc/core/slot-gpio.c +3 -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.

drivers/mmc/core/slot-gpio.c static int mmc_gpio_alloc(struct mmc_host *host) · line 55

Quoted 11 lines

drivers/mmc/core/slot-gpio.c +3 -1

 	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:)

Quoted 41 lines

drivers/mmc/core/slot-gpio.c +20 -0

 				   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;

drivers/mmc/core/slot-gpio.c int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio) · line 144

Quoted 10 lines

drivers/mmc/core/slot-gpio.c +10 -0

+
+	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.

drivers/mmc/core/slot-gpio.c int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio) · line 150

Quoted 6 lines

drivers/mmc/core/slot-gpio.c +6 -0

+
+	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

Quoted 58 lines

drivers/mmc/core/slot-gpio.c +20 -0

+
+	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;

include/linux/mmc/host.h +1 -0

index d5d9bd4..5a00cc1 100644
@@ -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;

include/linux/mmc/slot-gpio.h +3 -0

index 7d88d27..d083dfa 100644
@@ -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

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

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

Chris Ball

author

2012-09-10 02:55 UTC #2 lore raw

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.

Quoted 8 lines · Guennadi Liakhovetski

Quoted 3 lines
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.

Quoted 3 lines

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.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Chris Ball

author

2012-09-10 03:01 UTC #3 lore raw

PATCH v2 1/1 mmc: slot-gpio: Add support for power gpios

In reply to Guennadi Liakhovetski

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

Signed-off-by Chris Ball <[email protected]>

---
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(-)

drivers/mmc/core/slot-gpio.c +69 -1

index 08c6b3d..cf7e826 100644
@@ -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;

include/linux/mmc/host.h +1 -0

index d5d9bd4..5a00cc1 100644
@@ -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;

include/linux/mmc/slot-gpio.h +3 -0

index 7d88d27..d083dfa 100644
@@ -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

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Guennadi Liakhovetski

2012-09-10 21:48 UTC #4 lore raw

Hi Chris

Quoted 1 line · Chris Ball

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

Quoted 166 lines

Signed-off-by Chris Ball <[email protected]>

---
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(-)

drivers/mmc/core/slot-gpio.c +69 -1

index 08c6b3d..cf7e826 100644
@@ -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;

include/linux/mmc/host.h +1 -0

index d5d9bd4..5a00cc1 100644
@@ -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;

include/linux/mmc/slot-gpio.h +3 -0

index 7d88d27..d083dfa 100644
@@ -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

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

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