| Message ID | 1421405273-19117-5-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 16 January 2015 at 12:34, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: > On 16 January 2015 at 11:47, 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); >> + 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); > > Hi Ulf, > > this kfree looks a bit fishy (there's one more instance of it in > mmc_pwrseq_simple_free). This is the error path, which means I need to clean up data that I have allocated. mmc_pwrseq_simple_free() is called when host drivers ->remove() callback invokes mmc_free_host(). In that case the host->pwrseq has been assigned. 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 16 January 2015 at 13:45, Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Fri, Jan 16, 2015 at 01:37:41PM +0100, Ulf Hansson wrote: >> On 16 January 2015 at 12:34, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: >> > On 16 January 2015 at 11:47, 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); >> >> + 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); >> > >> > Hi Ulf, >> > >> > this kfree looks a bit fishy (there's one more instance of it in >> > mmc_pwrseq_simple_free). >> >> This is the error path, which means I need to clean up data that I >> have allocated. > > I think Tomeu meant that the '&' must be removed. > Of course, thanks! Unless I get some more comments I will fix it when I queue the patch. 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
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c index 7f87bc1..42d9836 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 v3: - Updated commit message to make it clear that we support one reset GPIO. --- drivers/mmc/core/pwrseq_simple.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)