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

[Alexandre Courbot <gnurou@xxxxxxxxx>]

On Tue, Aug 12, 2014 at 10:25 AM, Linus Walleij
<linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html