| Message ID | 1421658784-11980-5-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 23 January 2015 at 16:56, Javier Martinez Canillas <javier@dowhile0.org> wrote: > Hello Ulf, > > On Mon, Jan 19, 2015 at 10:13 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev) >> { >> struct mmc_pwrseq_simple *pwrseq; >> + int ret = 0; >> >> pwrseq = kzalloc(sizeof(struct mmc_pwrseq_simple), GFP_KERNEL); >> if (!pwrseq) >> return -ENOMEM; >> >> + pwrseq->reset_gpio = gpiod_get_index(dev, "reset", 0, GPIOD_OUT_HIGH); > > Any reason to not use the devm_gpiod_get_index() managed version instead? This struct device don't have a bound driver to it. Thus this device won't be freed automagically from the ->remove() or failed ->probe() path. > > AFAICT mmc_free_host() will free the device so in that case you won't > need to call gpiod_put() in mmc_pwrseq_simple_free(). > > This will also make easier to extend pwrseq_simple to support multiple > GPIOs like the DT binding implies. > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 February 2015 at 10:12, Alexandre Courbot <gnurou@gmail.com> wrote: > On Mon, Jan 19, 2015 at 6:13 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> The need for reset GPIOs has several times been pointed out from >> erlier posted patchsets. Especially some WLAN chips which are >> attached to an SDIO interface may use a GPIO reset. >> >> The reset GPIO is asserted at initialization and prior we start the >> power up procedure. The GPIO will be de-asserted right after the power >> has been provided to the card, from the ->post_power_on() callback. >> >> Note, the reset GPIO is optional. Thus we don't return an error even if >> we can't find a GPIO for the consumer. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > Excellent, with this I can probe the wifi chip on NVIDIA SHIELD which > requires a GPIO to be high for reset to be de-asserted. > > The series: > > Tested-by: Alexandre Courbot <acourbot@nvidia.com> Thanks for testing! Unfortunate, I can't add you tag since this patchset is already in the PR for 3.20. > >> --- >> >> Changes in v4: >> - Fixed call to kfree(). >> >> --- >> drivers/mmc/core/pwrseq_simple.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c >> index 61c991e..0958c69 100644 >> --- a/drivers/mmc/core/pwrseq_simple.c >> +++ b/drivers/mmc/core/pwrseq_simple.c >> @@ -11,6 +11,7 @@ >> #include <linux/slab.h> >> #include <linux/device.h> >> #include <linux/err.h> >> +#include <linux/gpio/consumer.h> >> >> #include <linux/mmc/host.h> >> >> @@ -18,31 +19,68 @@ >> >> struct mmc_pwrseq_simple { >> struct mmc_pwrseq pwrseq; >> + struct gpio_desc *reset_gpio; >> }; >> >> +static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host) >> +{ >> + struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq, >> + struct mmc_pwrseq_simple, pwrseq); >> + >> + if (!IS_ERR(pwrseq->reset_gpio)) >> + gpiod_set_value_cansleep(pwrseq->reset_gpio, 1); >> +} >> + >> +static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host) >> +{ >> + struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq, >> + struct mmc_pwrseq_simple, pwrseq); >> + >> + if (!IS_ERR(pwrseq->reset_gpio)) >> + gpiod_set_value_cansleep(pwrseq->reset_gpio, 0); >> +} >> + >> static void mmc_pwrseq_simple_free(struct mmc_host *host) >> { >> struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq, >> struct mmc_pwrseq_simple, pwrseq); >> >> + if (!IS_ERR(pwrseq->reset_gpio)) >> + gpiod_put(pwrseq->reset_gpio); >> + >> kfree(pwrseq); >> host->pwrseq = NULL; >> } >> >> static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = { >> + .pre_power_on = mmc_pwrseq_simple_pre_power_on, >> + .post_power_on = mmc_pwrseq_simple_post_power_on, >> + .power_off = mmc_pwrseq_simple_pre_power_on, >> .free = mmc_pwrseq_simple_free, >> }; >> >> int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev) >> { >> struct mmc_pwrseq_simple *pwrseq; >> + int ret = 0; >> >> pwrseq = kzalloc(sizeof(struct mmc_pwrseq_simple), GFP_KERNEL); >> if (!pwrseq) >> return -ENOMEM; >> >> + pwrseq->reset_gpio = gpiod_get_index(dev, "reset", 0, GPIOD_OUT_HIGH); > > gpiod_get() will translate to exactly the same, with less characters. > > Actually I see that the version in -next has support for multiple > GPIOs. You will probably want to look at Rojhalat's latest work on > GPIO arrays: > > http://permalink.gmane.org/gmane.linux.kernel.gpio/6126 Cool! > > This code would be a great candidate to use this GPIO array API, but > since it is not in -next yet (should happen soon though) you might > want to consider doing it later. > > Btw, I wasted a considerable amount of time on one of the defunct > previous attempts at power sequences, so I'm interested in reviewing > future versions of this patchset if you don't mind adding me to the CC > list. :) :-) I will keep you posted for future updates. Feel free to post patches improving the mmc-pwrseq code yourself, I will happily review them. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 11, 2015 at 5:34 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 10 February 2015 at 10:12, Alexandre Courbot <gnurou@gmail.com> wrote: >> This code would be a great candidate to use this GPIO array API, but >> since it is not in -next yet (should happen soon though) you might >> want to consider doing it later. This is now in my git and -next, I can take patches to pwrseq if Ulf ACKs them to go through my GPIO tree. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c index 61c991e..0958c69 100644 --- a/drivers/mmc/core/pwrseq_simple.c +++ b/drivers/mmc/core/pwrseq_simple.c @@ -11,6 +11,7 @@ #include <linux/slab.h> #include <linux/device.h> #include <linux/err.h> +#include <linux/gpio/consumer.h> #include <linux/mmc/host.h> @@ -18,31 +19,68 @@ struct mmc_pwrseq_simple { struct mmc_pwrseq pwrseq; + struct gpio_desc *reset_gpio; }; +static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host) +{ + struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq, + struct mmc_pwrseq_simple, pwrseq); + + if (!IS_ERR(pwrseq->reset_gpio)) + gpiod_set_value_cansleep(pwrseq->reset_gpio, 1); +} + +static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host) +{ + struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq, + struct mmc_pwrseq_simple, pwrseq); + + if (!IS_ERR(pwrseq->reset_gpio)) + gpiod_set_value_cansleep(pwrseq->reset_gpio, 0); +} + static void mmc_pwrseq_simple_free(struct mmc_host *host) { struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq, struct mmc_pwrseq_simple, pwrseq); + if (!IS_ERR(pwrseq->reset_gpio)) + gpiod_put(pwrseq->reset_gpio); + kfree(pwrseq); host->pwrseq = NULL; } static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = { + .pre_power_on = mmc_pwrseq_simple_pre_power_on, + .post_power_on = mmc_pwrseq_simple_post_power_on, + .power_off = mmc_pwrseq_simple_pre_power_on, .free = mmc_pwrseq_simple_free, }; int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev) { struct mmc_pwrseq_simple *pwrseq; + int ret = 0; pwrseq = kzalloc(sizeof(struct mmc_pwrseq_simple), GFP_KERNEL); if (!pwrseq) return -ENOMEM; + pwrseq->reset_gpio = gpiod_get_index(dev, "reset", 0, GPIOD_OUT_HIGH); + if (IS_ERR(pwrseq->reset_gpio) && + PTR_ERR(pwrseq->reset_gpio) != -ENOENT && + PTR_ERR(pwrseq->reset_gpio) != -ENOSYS) { + ret = PTR_ERR(pwrseq->reset_gpio); + goto free; + } + pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops; host->pwrseq = &pwrseq->pwrseq; return 0; +free: + kfree(pwrseq); + return ret; }
The need for reset GPIOs has several times been pointed out from erlier posted patchsets. Especially some WLAN chips which are attached to an SDIO interface may use a GPIO reset. The reset GPIO is asserted at initialization and prior we start the power up procedure. The GPIO will be de-asserted right after the power has been provided to the card, from the ->post_power_on() callback. Note, the reset GPIO is optional. Thus we don't return an error even if we can't find a GPIO for the consumer. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- Changes in v4: - Fixed call to kfree(). --- drivers/mmc/core/pwrseq_simple.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)