| Message ID | 1407864355-21545-4-git-send-email-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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-mmc" 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-mmc" 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/host/mmci.c b/drivers/mmc/host/mmci.c index 7ad463e9741c..24a83af6d153 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -1552,16 +1552,49 @@ static int mmci_probe(struct amba_device *dev, writel(0, host->base + MMCIMASK1); writel(0xfff, host->base + MMCICLEAR); - /* 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); + if (!ret) + dev_info(mmc_dev(mmc), "got CD GPIO (table)\n"); + else { + 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 { + 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"); + } + } } ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
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(-)