| Message ID | 1421658784-11980-5-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? 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. Best regards, Javier -- 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 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 linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Ulf, On Fri, Jan 23, 2015 at 6:01 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> 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. > Right, I missed that. Thanks a lot for the clarification. Best regards, Javier -- 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
Hello Ulf, On Mon, Jan 19, 2015 at 10:13 AM, 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> > --- Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Best regards, Javier -- 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 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> > --- > > 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 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. :) -- 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 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 linux-mmc" 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(+)