+Linus
On 7 December 2015 at 23:54, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> Commit ce037275861e ("mmc: pwrseq_simple: use GPIO descriptors array API")
> changed the handling MMC power sequence so GPIOs no longer are optional.
>
> This broke SDIO WLAN at least for omap5 that can't yet use the reset GPIOs
> with pwrseq_simple as a wait is needed after enabling the SDIO device.
Can you elaborate on this. Did it break omap5 or not? :-)
Also, I am interested to know more about the waiting period you need.
I assume that's because of the HW's characteristic?
Why can't we have that being described in DT and then make
pwrseq_simple *wait* if needed?
>
> Let's fix the problem by allocating the GPIO values array during init
> depending on the optional GPIOs found.
Certainly it shall be optional! I wonder how I could let that patch
slip through, my bad!
Thanks for fixing this!
>
> Note that depending on the board specific configuration, some of the GPIOs
> can be permanently set up with pulls, so we want to keep pwrseq_simple
> GPIOs optional.
>
> Cc: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
> drivers/mmc/core/pwrseq_simple.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -24,6 +24,7 @@ struct mmc_pwrseq_simple {
> bool clk_enabled;
> struct clk *ext_clk;
> struct gpio_descs *reset_gpios;
> + int *values;
> };
>
> static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
> @@ -31,13 +32,15 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
> {
> int i;
> struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
> - int values[reset_gpios->ndescs];
I would prefer if we don't need to keep this memory on the heap.
Can't we instead keep a local copy of the reset_gpios->ndesc and when
pwrseq->reset_gpios doesn't exist use a default value?
> +
> + if (!reset_gpios || !reset_gpios->ndescs)
> + return;
>
> for (i = 0; i < reset_gpios->ndescs; i++)
> - values[i] = value;
> + pwrseq->values[i] = value;
>
> gpiod_set_array_value_cansleep(reset_gpios->ndescs, reset_gpios->desc,
> - values);
> + pwrseq->values);
> }
>
> static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
> @@ -84,6 +87,7 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
> if (!IS_ERR(pwrseq->ext_clk))
> clk_put(pwrseq->ext_clk);
>
> + kfree(pwrseq->values);
> kfree(pwrseq);
> }
>
> @@ -111,12 +115,20 @@ struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
> goto free;
> }
>
> - pwrseq->reset_gpios = gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
> + pwrseq->reset_gpios = gpiod_get_array_optional(dev, "reset",
> + GPIOD_OUT_HIGH);
> if (IS_ERR(pwrseq->reset_gpios)) {
The original code also considered -ENOSYS, as that's the return code
you get when CONFIG_GPIOLIB is unset, I think we need that as well.
Although, perhaps it's more correct that the gpiolib API returns NULL
instead of ERR_PTR(-ENOSYS)?
I have added Linus, to see if he has any comments on this.
> ret = PTR_ERR(pwrseq->reset_gpios);
> goto clk_put;
> }
>
> + if (pwrseq->reset_gpios && pwrseq->reset_gpios->ndescs) {
> + pwrseq->values = kzalloc(pwrseq->reset_gpios->ndescs *
> + sizeof(int), GFP_KERNEL);
> + if (!pwrseq->values)
> + goto clk_put;
This will leak a gpio cookie as it needs a gpiod_put_array().
> + }
> +
> pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
>
> return &pwrseq->pwrseq;
> --
> 2.6.2
>
One final comment, gpiod_put_array() doesn't deal with NULL pointers.
You need to check that in mmc_pwrseq_simple_free().
Kind regards
Uffe
--
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
