| Message ID | 1407864355-21545-3-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: > 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. > > As it is in no way a bug not to supply CD/WP GPIOs the messages > about them not being specified have been depromoted from > dev_err() to dev_dbg(). Acked-by: Alexandre Courbot <acourbot@nvidia.com> -- 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 12 August 2014 19:25, 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. > > As it is in no way a bug not to supply CD/WP GPIOs the messages > about them not being specified have been depromoted from > dev_err() to dev_dbg(). > > Cc: Alexandre Courbot <gnurou@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Thanks! Applied for next. Kind regards Uffe > --- > drivers/mmc/core/host.c | 68 +++++++++++++++---------------------------------- > 1 file changed, 21 insertions(+), 47 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index 95cceae96944..048c6d687cc9 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,36 @@ 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 (explicit_inv_cd ^ gpio_inv_cd) > - host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > + dev_dbg(host->parent, > + "Failed to request CD GPIO: %d\n", > + ret); > + } else > + dev_info(host->parent, "Got CD GPIO\n"); > } > > /* Parse Write Protection */ > - explicit_inv_wp = of_property_read_bool(np, "wp-inverted"); > + if (of_property_read_bool(np, "wp-inverted")) > + host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; > > - 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; > - > - 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 (explicit_inv_wp ^ gpio_inv_wp) > - host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; > + dev_dbg(host->parent, > + "Failed to request WP GPIO: %d\n", > + ret); > + } 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-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Tue, Aug 12, 2014 at 07:25:54PM +0200, Linus Walleij 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. > > As it is in no way a bug not to supply CD/WP GPIOs the messages > about them not being specified have been depromoted from > dev_err() to dev_dbg(). I think the intention of the current code is to issue an error if a valid GPIO was specified but could not be obtained. It should not issue an error just because the CD/WP GPIOs are not specified. This patch changes this behaviour. If the GPIO is specified explicitly but cannot be obtained for some reason, the driver's probe calling mmc_of_parse() will probably be successful. Since debug level messages are usually not enabled, the problem with the GPIO may not even be visible in the logs. For example, if I specify the same GPIO for CD and WP in the DTS: [ 2.093442] kirkwood-pinctrl f1010000.pin-controller: request pin 44 (PIN44) for mvebu-gpio:44 [ 2.093459] mvsdio f1090000.mvsdio: Got CD GPIO [ 2.118734] mv643xx_eth_port mv643xx_eth_port.0 eth0: port 0 with MAC address 00:50:43:c3:55:55 One can only infer from the missing "Got WP GPIO" that something went wrong. Previously, the driver would not even load: ... [ 1.973402] mvsdio f1090000.mvsdio: Failed to request WP GPIO: -16! [ 1.979831] mvsdio: probe of f1090000.mvsdio failed with error -16 One can argue whether the current behaviour is too strict (and we did that in the past, leading to the current code), but issuing only a debug message when allocating an explicitly requested resource fails is too tolerant, IMHO. - Simon -- 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 Mon, Aug 18, 2014 at 2:46 PM, Simon Baatz <gmbnomis@gmail.com> wrote: > On Tue, Aug 12, 2014 at 07:25:54PM +0200, Linus Walleij 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. >> >> As it is in no way a bug not to supply CD/WP GPIOs the messages >> about them not being specified have been depromoted from >> dev_err() to dev_dbg(). > > I think the intention of the current code is to issue an > error if a valid GPIO was specified but could not be obtained. OK I'll post a fixup. 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/core/host.c b/drivers/mmc/core/host.c index 95cceae96944..048c6d687cc9 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,36 @@ 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 (explicit_inv_cd ^ gpio_inv_cd) - host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; + dev_dbg(host->parent, + "Failed to request CD GPIO: %d\n", + ret); + } else + dev_info(host->parent, "Got CD GPIO\n"); } /* Parse Write Protection */ - explicit_inv_wp = of_property_read_bool(np, "wp-inverted"); + if (of_property_read_bool(np, "wp-inverted")) + host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; - 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; - - 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 (explicit_inv_wp ^ gpio_inv_wp) - host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; + dev_dbg(host->parent, + "Failed to request WP GPIO: %d\n", + ret); + } else + dev_info(host->parent, "Got WP GPIO\n"); if (of_find_property(np, "cap-sd-highspeed", &len)) host->caps |= MMC_CAP_SD_HIGHSPEED;
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. As it is in no way a bug not to supply CD/WP GPIOs the messages about them not being specified have been depromoted from dev_err() to dev_dbg(). Cc: Alexandre Courbot <gnurou@gmail.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/mmc/core/host.c | 68 +++++++++++++++---------------------------------- 1 file changed, 21 insertions(+), 47 deletions(-)