* [PATCH] mmc: slot-gpio: Add support for power gpios @ 2012-09-09 3:01 Chris Ball 2012-09-09 22:05 ` Guennadi Liakhovetski 0 siblings, 1 reply; 5+ messages in thread From: Chris Ball @ 2012-09-09 3:01 UTC (permalink / raw) To: linux-mmc; +Cc: Guennadi Liakhovetski 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mmc: slot-gpio: Add support for power gpios 2012-09-09 3:01 [PATCH] mmc: slot-gpio: Add support for power gpios Chris Ball @ 2012-09-09 22:05 ` Guennadi Liakhovetski 2012-09-10 2:55 ` Chris Ball 2012-09-10 3:01 ` [PATCH v2] " Chris Ball 0 siblings, 2 replies; 5+ messages in thread From: Guennadi Liakhovetski @ 2012-09-09 22:05 UTC (permalink / raw) To: Chris Ball; +Cc: linux-mmc 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/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mmc: slot-gpio: Add support for power gpios 2012-09-09 22:05 ` Guennadi Liakhovetski @ 2012-09-10 2:55 ` Chris Ball 2012-09-10 3:01 ` [PATCH v2] " Chris Ball 1 sibling, 0 replies; 5+ messages in thread From: Chris Ball @ 2012-09-10 2:55 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-mmc 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] mmc: slot-gpio: Add support for power gpios 2012-09-09 22:05 ` Guennadi Liakhovetski 2012-09-10 2:55 ` Chris Ball @ 2012-09-10 3:01 ` Chris Ball 2012-09-10 21:48 ` Guennadi Liakhovetski 1 sibling, 1 reply; 5+ messages in thread From: Chris Ball @ 2012-09-10 3:01 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-mmc 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mmc: slot-gpio: Add support for power gpios 2012-09-10 3:01 ` [PATCH v2] " Chris Ball @ 2012-09-10 21:48 ` Guennadi Liakhovetski 0 siblings, 0 replies; 5+ messages in thread From: Guennadi Liakhovetski @ 2012-09-10 21:48 UTC (permalink / raw) To: Chris Ball; +Cc: linux-mmc 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/ ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-10 21:48 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-09 3:01 [PATCH] mmc: slot-gpio: Add support for power gpios Chris Ball 2012-09-09 22:05 ` Guennadi Liakhovetski 2012-09-10 2:55 ` Chris Ball 2012-09-10 3:01 ` [PATCH v2] " Chris Ball 2012-09-10 21:48 ` Guennadi Liakhovetski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox