[V4,4/4] mmc: pwrseq_simple: Add support for a reset GPIO pin

Message ID 1421658784-11980-5-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson Jan. 19, 2015, 9:13 a.m.
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(+)

Comments

Ulf Hansson Jan. 23, 2015, 5:01 p.m. | #1
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
Ulf Hansson Feb. 11, 2015, 4:34 a.m. | #2
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
Linus Walleij March 5, 2015, 9:05 a.m. | #3
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

Patch

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