| Message ID | 1409137253-25189-3-git-send-email-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 27 August 2014 13:00, Linus Walleij <linus.walleij@linaro.org> wrote: > This switches the central MMC OF parser to use gpio descriptors > instead of grabbing GPIOs explicitly from the device tree. > This strips out an unecessary use of the integer-based GPIO > API that we want to get rid of, cuts down on code as the > gpio descriptor code will handle active low flags. > > Acked-by: Alexandre Courbot <acourbot@nvidia.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Thanks! Applied for next. Kind regards Uffe > --- > ChangeLog v1->v2: > - Restore error reporting as done in previous stand-alone > patch. > --- > drivers/mmc/core/host.c | 68 +++++++++++++++++-------------------------------- > 1 file changed, 23 insertions(+), 45 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index 95cceae96944..6f7ed9c50346 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -310,9 +310,7 @@ int mmc_of_parse(struct mmc_host *host) > { > struct device_node *np; > u32 bus_width; > - bool explicit_inv_wp, gpio_inv_wp = false; > - enum of_gpio_flags flags; > - int len, ret, gpio; > + int len, ret; > > if (!host->parent || !host->parent->of_node) > return 0; > @@ -360,60 +358,40 @@ int mmc_of_parse(struct mmc_host *host) > if (of_find_property(np, "non-removable", &len)) { > host->caps |= MMC_CAP_NONREMOVABLE; > } else { > - bool explicit_inv_cd, gpio_inv_cd = false; > - > - explicit_inv_cd = of_property_read_bool(np, "cd-inverted"); > + if (of_property_read_bool(np, "cd-inverted")) > + host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > > if (of_find_property(np, "broken-cd", &len)) > host->caps |= MMC_CAP_NEEDS_POLL; > > - gpio = of_get_named_gpio_flags(np, "cd-gpios", 0, &flags); > - if (gpio == -EPROBE_DEFER) > - return gpio; > - if (gpio_is_valid(gpio)) { > - if (!(flags & OF_GPIO_ACTIVE_LOW)) > - gpio_inv_cd = true; > - > - ret = mmc_gpio_request_cd(host, gpio, 0); > - if (ret < 0) { > - dev_err(host->parent, > - "Failed to request CD GPIO #%d: %d!\n", > - gpio, ret); > + ret = mmc_gpiod_request_cd(host, "cd", 0, false, 0); > + if (ret) { > + if (ret == -EPROBE_DEFER) > return ret; > - } else { > - dev_info(host->parent, "Got CD GPIO #%d.\n", > - gpio); > + if (ret != -ENOENT) { > + dev_err(host->parent, > + "Failed to request CD GPIO: %d\n", > + ret); > } > - } > - > - if (explicit_inv_cd ^ gpio_inv_cd) > - host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > + } else > + dev_info(host->parent, "Got CD GPIO\n"); > } > > /* Parse Write Protection */ > - explicit_inv_wp = of_property_read_bool(np, "wp-inverted"); > - > - gpio = of_get_named_gpio_flags(np, "wp-gpios", 0, &flags); > - if (gpio == -EPROBE_DEFER) { > - ret = -EPROBE_DEFER; > - goto out; > - } > - if (gpio_is_valid(gpio)) { > - if (!(flags & OF_GPIO_ACTIVE_LOW)) > - gpio_inv_wp = true; > + if (of_property_read_bool(np, "wp-inverted")) > + host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; > > - ret = mmc_gpio_request_ro(host, gpio); > - if (ret < 0) { > - dev_err(host->parent, > - "Failed to request WP GPIO: %d!\n", ret); > + ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0); > + if (ret) { > + if (ret == -EPROBE_DEFER) > goto out; > - } else { > - dev_info(host->parent, "Got WP GPIO #%d.\n", > - gpio); > + if (ret != -ENOENT) { > + dev_err(host->parent, > + "Failed to request WP GPIO: %d\n", > + ret); > } > - } > - if (explicit_inv_wp ^ gpio_inv_wp) > - host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; > + } else > + dev_info(host->parent, "Got WP GPIO\n"); > > if (of_find_property(np, "cap-sd-highspeed", &len)) > host->caps |= MMC_CAP_SD_HIGHSPEED; > -- > 1.9.3 > -- 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 Wed, Aug 27, 2014 at 1:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > This switches the central MMC OF parser to use gpio descriptors > instead of grabbing GPIOs explicitly from the device tree. > This strips out an unecessary use of the integer-based GPIO > API that we want to get rid of, cuts down on code as the > gpio descriptor code will handle active low flags. > > Acked-by: Alexandre Courbot <acourbot@nvidia.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - Restore error reporting as done in previous stand-alone > patch. Hi Ulf, the v2 version of this patch set should be working fine on v3.17-rc4, as I merged a patch setting up the correct prototypes for systems without gpiolib. Fenguang's build tests are working on it atleast, I promise... Tell me if you want me to resend the patch set or if you can take the same patches again, but on top of -rc4. There is no change in them. 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
On 9 September 2014 09:05, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Aug 27, 2014 at 1:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > >> This switches the central MMC OF parser to use gpio descriptors >> instead of grabbing GPIOs explicitly from the device tree. >> This strips out an unecessary use of the integer-based GPIO >> API that we want to get rid of, cuts down on code as the >> gpio descriptor code will handle active low flags. >> >> Acked-by: Alexandre Courbot <acourbot@nvidia.com> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> --- >> ChangeLog v1->v2: >> - Restore error reporting as done in previous stand-alone >> patch. > > Hi Ulf, > > the v2 version of this patch set should be working fine on > v3.17-rc4, as I merged a patch setting up the correct > prototypes for systems without gpiolib. > > Fenguang's build tests are working on it atleast, I promise... > > Tell me if you want me to resend the patch set or if you can > take the same patches again, but on top of -rc4. There is > no change in them. > Hi Linus, I have re-applied them again, let's hope for better luck this time. :-) Kind regards Uffe -- 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
Hello Linus, On Wed, Aug 27, 2014 at 1:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > This switches the central MMC OF parser to use gpio descriptors > instead of grabbing GPIOs explicitly from the device tree. > This strips out an unecessary use of the integer-based GPIO > API that we want to get rid of, cuts down on code as the > gpio descriptor code will handle active low flags. > > Acked-by: Alexandre Courbot <acourbot@nvidia.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > Card detection is failing in some boards because this patch changes some semantics. I tried to come up with a fix but I didn't find an obvious way to do it (more on that below). > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index 95cceae96944..6f7ed9c50346 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -310,9 +310,7 @@ int mmc_of_parse(struct mmc_host *host) > { > struct device_node *np; > u32 bus_width; > - bool explicit_inv_wp, gpio_inv_wp = false; > - enum of_gpio_flags flags; > - int len, ret, gpio; > + int len, ret; > > if (!host->parent || !host->parent->of_node) > return 0; > @@ -360,60 +358,40 @@ int mmc_of_parse(struct mmc_host *host) > if (of_find_property(np, "non-removable", &len)) { > host->caps |= MMC_CAP_NONREMOVABLE; > } else { > - bool explicit_inv_cd, gpio_inv_cd = false; > - > - explicit_inv_cd = of_property_read_bool(np, "cd-inverted"); > + if (of_property_read_bool(np, "cd-inverted")) > + host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > The card-detect signal active high flag is set if the "cd-inverted" property is found but in the original code MMC_CAP2_CD_ACTIVE_HIGH was set only if "cd-inverted" was true and the card detect GPIO flag was OF_GPIO_ACTIVE_HIGH. > if (of_find_property(np, "broken-cd", &len)) > host->caps |= MMC_CAP_NEEDS_POLL; > > - gpio = of_get_named_gpio_flags(np, "cd-gpios", 0, &flags); > - if (gpio == -EPROBE_DEFER) > - return gpio; > - if (gpio_is_valid(gpio)) { > - if (!(flags & OF_GPIO_ACTIVE_LOW)) > - gpio_inv_cd = true; > - > - ret = mmc_gpio_request_cd(host, gpio, 0); > - if (ret < 0) { > - dev_err(host->parent, > - "Failed to request CD GPIO #%d: %d!\n", > - gpio, ret); > + ret = mmc_gpiod_request_cd(host, "cd", 0, false, 0); Now false is passed as the override_active_level bool parameter but mmc_gpio_request_cd() sets ctx->override_cd_active_level = true. Which means that after this patch, mmc_gpio_get_cd() does not take into account if host->caps2 has MMC_CAP2_CD_ACTIVE_HIGH set. > + if (ret) { > + if (ret == -EPROBE_DEFER) > return ret; > - } else { > - dev_info(host->parent, "Got CD GPIO #%d.\n", > - gpio); > + if (ret != -ENOENT) { > + dev_err(host->parent, > + "Failed to request CD GPIO: %d\n", > + ret); > } > - } > - > - if (explicit_inv_cd ^ gpio_inv_cd) > - host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > + } else > + dev_info(host->parent, "Got CD GPIO\n"); > } > > /* Parse Write Protection */ > - explicit_inv_wp = of_property_read_bool(np, "wp-inverted"); > - > - gpio = of_get_named_gpio_flags(np, "wp-gpios", 0, &flags); > - if (gpio == -EPROBE_DEFER) { > - ret = -EPROBE_DEFER; > - goto out; > - } > - if (gpio_is_valid(gpio)) { > - if (!(flags & OF_GPIO_ACTIVE_LOW)) > - gpio_inv_wp = true; > + if (of_property_read_bool(np, "wp-inverted")) > + host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; > > - ret = mmc_gpio_request_ro(host, gpio); > - if (ret < 0) { > - dev_err(host->parent, > - "Failed to request WP GPIO: %d!\n", ret); > + ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0); > + if (ret) { > + if (ret == -EPROBE_DEFER) > goto out; > - } else { > - dev_info(host->parent, "Got WP GPIO #%d.\n", > - gpio); > + if (ret != -ENOENT) { > + dev_err(host->parent, > + "Failed to request WP GPIO: %d\n", > + ret); > } > - } > - if (explicit_inv_wp ^ gpio_inv_wp) > - host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; Same issue with write-protect signal active high. After this patch MMC_CAP2_RO_ACTIVE_HIGH is always set while before it was conditionally set depending of the write-protect GPIO flag value. > + } else > + dev_info(host->parent, "Got WP GPIO\n"); > > if (of_find_property(np, "cap-sd-highspeed", &len)) > host->caps |= MMC_CAP_SD_HIGHSPEED; > -- Passing true to mmc_gpiod_request_cd() and not setting MMC_CAP2_RO_ACTIVE_HIGH makes card detection work again on this board. The former is a one-liner but the later is more complicated since AFAIU the idea of the GPIO descriptor code is to handle the flags but in this case this detail is needed. Should we set MMC_CAP2_{CD,RO}_ACTIVE_HIGH in mmc_gpiod_request_{cd,ro} or add functions to get the flags for these cd and wp GPIO in order to set the host->caps2 flags according to the old logic? Thanks a lot and best regards, Javier -- 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 Tue, Sep 30, 2014 at 1:30 PM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > On Wed, Aug 27, 2014 at 1:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> This switches the central MMC OF parser to use gpio descriptors >> instead of grabbing GPIOs explicitly from the device tree. >> This strips out an unecessary use of the integer-based GPIO >> API that we want to get rid of, cuts down on code as the >> gpio descriptor code will handle active low flags. >> >> Acked-by: Alexandre Courbot <acourbot@nvidia.com> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > Card detection is failing in some boards because this patch changes > some semantics. I tried to come up with a fix but I didn't find an > obvious way to do it (more on that below). I think some of this may be weird semantics: the CAP flags in the MMC subsystem, MMC_CAP2_CD_ACTIVE_HIGH and MMC_CAP2_RO_ACTIVE_HIGH are somewhat confusing since the GPIO subsystem nowadays has its own concept of a line active high or low. The MMC caps make a lot of sense if the host has dedicated lines to read CD and WP, and does not use a GPIO for this. When a GPIO descriptor is in use, the thing gets a bit convoluted: what if both the GPIO *and* the CAP flag is set to inversion? So the old code did this with XOR: if (explicit_inv_cd ^ gpio_inv_cd) host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; Meaning double inversion -> no inversion. I guess I was sort of hoping that nobody did this crazy thing. Turns out I was wrong, so have to restore the old semantics... I'm sending a patch for you to test. 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
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index 95cceae96944..6f7ed9c50346 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -310,9 +310,7 @@ int mmc_of_parse(struct mmc_host *host) { struct device_node *np; u32 bus_width; - bool explicit_inv_wp, gpio_inv_wp = false; - enum of_gpio_flags flags; - int len, ret, gpio; + int len, ret; if (!host->parent || !host->parent->of_node) return 0; @@ -360,60 +358,40 @@ int mmc_of_parse(struct mmc_host *host) if (of_find_property(np, "non-removable", &len)) { host->caps |= MMC_CAP_NONREMOVABLE; } else { - bool explicit_inv_cd, gpio_inv_cd = false; - - explicit_inv_cd = of_property_read_bool(np, "cd-inverted"); + if (of_property_read_bool(np, "cd-inverted")) + host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; if (of_find_property(np, "broken-cd", &len)) host->caps |= MMC_CAP_NEEDS_POLL; - gpio = of_get_named_gpio_flags(np, "cd-gpios", 0, &flags); - if (gpio == -EPROBE_DEFER) - return gpio; - if (gpio_is_valid(gpio)) { - if (!(flags & OF_GPIO_ACTIVE_LOW)) - gpio_inv_cd = true; - - ret = mmc_gpio_request_cd(host, gpio, 0); - if (ret < 0) { - dev_err(host->parent, - "Failed to request CD GPIO #%d: %d!\n", - gpio, ret); + ret = mmc_gpiod_request_cd(host, "cd", 0, false, 0); + if (ret) { + if (ret == -EPROBE_DEFER) return ret; - } else { - dev_info(host->parent, "Got CD GPIO #%d.\n", - gpio); + if (ret != -ENOENT) { + dev_err(host->parent, + "Failed to request CD GPIO: %d\n", + ret); } - } - - if (explicit_inv_cd ^ gpio_inv_cd) - host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; + } else + dev_info(host->parent, "Got CD GPIO\n"); } /* Parse Write Protection */ - explicit_inv_wp = of_property_read_bool(np, "wp-inverted"); - - gpio = of_get_named_gpio_flags(np, "wp-gpios", 0, &flags); - if (gpio == -EPROBE_DEFER) { - ret = -EPROBE_DEFER; - goto out; - } - if (gpio_is_valid(gpio)) { - if (!(flags & OF_GPIO_ACTIVE_LOW)) - gpio_inv_wp = true; + if (of_property_read_bool(np, "wp-inverted")) + host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; - ret = mmc_gpio_request_ro(host, gpio); - if (ret < 0) { - dev_err(host->parent, - "Failed to request WP GPIO: %d!\n", ret); + ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0); + if (ret) { + if (ret == -EPROBE_DEFER) goto out; - } else { - dev_info(host->parent, "Got WP GPIO #%d.\n", - gpio); + if (ret != -ENOENT) { + dev_err(host->parent, + "Failed to request WP GPIO: %d\n", + ret); } - } - if (explicit_inv_wp ^ gpio_inv_wp) - host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; + } else + dev_info(host->parent, "Got WP GPIO\n"); if (of_find_property(np, "cap-sd-highspeed", &len)) host->caps |= MMC_CAP_SD_HIGHSPEED;