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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx> <http://printf.net/>
> One Laptop Per Child
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
