[4/4] mmc: mmci: augment driver to handle gpio descriptors

Currently the MMCI driver will only handle GPIO descriptors
implicitly through the device tree probe glue in mmc_of_init(),
but devices instatiated other ways such as through board files
and passing descriptors using the GPIO descriptor table will
not be able to exploit descriptors.

Augment the driver to look for a GPIO descriptor if device
tree is not used for the device, and if that doesn't work,
fall back to platform data GPIO assignment using the old
API. The end goal is to get rid of the platform data integer
GPIO assingments from the kernel.

This enable the MMCI-embedding platforms to be converted to
GPIO descritor tables.

Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 53 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 10 deletions(-)

On Tue, Aug 12, 2014 at 10:25 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> -       /* If DT, cd/wp gpios must be supplied through it. */
> -       if (!np && gpio_is_valid(plat->gpio_cd)) {
> -               ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
> -               if (ret)
> -                       goto clk_disable;
> -       }
> -       if (!np && gpio_is_valid(plat->gpio_wp)) {
> -               ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
> -               if (ret)
> -                       goto clk_disable;
> +       /*
> +        * If:
> +        * - not using DT but using a descriptor table, or
> +        * - using a table of descriptors ALONGSIDE DT, or
> +        * look up these descriptors named "cd" and "wp" right here, fail
> +        * silently of these do not exist and proceed to try platform data
> +        */
> +       if (!np) {
> +               ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);

I am not familiar with this driver, but since mmc_gpiod_request_cd()
uses gpiod_get(), can't you call it outside of the (!np) condition?
You should not have to do this kind of check when using the gpiod API.

> +               if (!ret)
> +                       dev_info(mmc_dev(mmc), "got CD GPIO (table)\n");
> +               else {

I think you want to add brackets around the dev_info() line to make
checkpatch.pl happy.

> +                       if (ret == -EPROBE_DEFER)
> +                               goto clk_disable;
> +                       else if (gpio_is_valid(plat->gpio_cd)) {
> +                               ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
> +                               if (ret)
> +                                       goto clk_disable;
> +                               else
> +                                       dev_info(mmc_dev(mmc), "got CD GPIO (pdata)\n");
> +                       } else {
> +                               dev_dbg(mmc_dev(mmc),
> +                                       "no CD GPIO in DT, table or pdata\n");
> +                       }
> +               }
> +
> +               ret = mmc_gpiod_request_ro(mmc, "wp", 0, false, 0);
> +               if (!ret)
> +                       dev_info(mmc_dev(mmc), "got WP GPIO (table)\n");
> +               else {

Same remark as above.

> +                       if (ret == -EPROBE_DEFER)
> +                               goto clk_disable;
> +                       else if (gpio_is_valid(plat->gpio_wp)) {
> +                               ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
> +                               if (ret)
> +                                       goto clk_disable;
> +                               else
> +                                       dev_info(mmc_dev(mmc), "got WP GPIO (pdata)\n");
> +                       } else {
> +                               dev_dbg(mmc_dev(mmc),
> +                                       "no WP GPIO in DT, table or pdata\n");
> +                       }
> +               }

I wonder (again, without knowing better) if this gpiod/gpio fallback
logic should not be put directly into mmc_gpio_request_ro/cd instead
of having two functions? Couldn't other drivers also benefit from it
that way?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Aug 14, 2014 at 9:56 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Aug 12, 2014 at 10:25 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> -       /* If DT, cd/wp gpios must be supplied through it. */
>> -       if (!np && gpio_is_valid(plat->gpio_cd)) {
>> -               ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
>> -               if (ret)
>> -                       goto clk_disable;
>> -       }
>> -       if (!np && gpio_is_valid(plat->gpio_wp)) {
>> -               ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
>> -               if (ret)
>> -                       goto clk_disable;
>> +       /*
>> +        * If:
>> +        * - not using DT but using a descriptor table, or
>> +        * - using a table of descriptors ALONGSIDE DT, or
>> +        * look up these descriptors named "cd" and "wp" right here, fail
>> +        * silently of these do not exist and proceed to try platform data
>> +        */
>> +       if (!np) {
>> +               ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);
>
> I am not familiar with this driver, but since mmc_gpiod_request_cd()
> uses gpiod_get(), can't you call it outside of the (!np) condition?
> You should not have to do this kind of check when using the gpiod API.
(...)
> I wonder (again, without knowing better) if this gpiod/gpio fallback
> logic should not be put directly into mmc_gpio_request_ro/cd instead
> of having two functions? Couldn't other drivers also benefit from it
> that way?

So this is a side-effect of the fact that in the MMC subsystem, the
gpio descriptor is only used in the device tree parsing code.
mmc_of_parse() in drivers/mmc/core/host.c

So if you're *not* using device tree (as some of the MMCI platforms)
they need to make these calls elsewhere.

I agree that there may be a refactoring lurking here, that would
move this out of the DT parse code and into the MMC core.
It should then be called *before* parsing the DT as DT adds
additional options to the GPIO (explicit inversion).

It can be done on top of this patch set though, once these things
are in place.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html