| Message ID | 1523496031-145755-1-git-send-email-shawn.lin@rock-chips.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/2] Documentation: mmc: add optional cd-delay-ms property | expand |
On 12 April 2018 at 03:20, Shawn Lin <shawn.lin@rock-chips.com> wrote: > Allow to use tunable delay before detecting card after card is inserted > either from firmware, or argument passing to mmc_gpiod_request_cd(). The > default value is 200ms as before, if cd-delay-ms isn't present but using > mmc_gpiod_request_cd() to request slot-gpio interrupt. Exactly why doesn't the 200 ms delay works? It seems like the proper method is instead to use a gpio debouncing time. mmc-slot-gpio already deals with that, but we are defaulting to use zero as debounce period, when mmc_gpiod_request_cd() is called from mmc_of_parse(). Kind regards Uffe > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/mmc/core/host.c | 7 +++++-- > drivers/mmc/core/slot-gpio.c | 11 +++++++++-- > drivers/mmc/host/davinci_mmc.c | 2 +- > drivers/mmc/host/mmci.c | 2 +- > drivers/mmc/host/sdhci-acpi.c | 2 +- > drivers/mmc/host/sdhci-pci-core.c | 3 ++- > include/linux/mmc/slot-gpio.h | 3 ++- > 7 files changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index 64b03d6..cb8babc 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -179,7 +179,7 @@ static void mmc_retune_timer(struct timer_list *t) > int mmc_of_parse(struct mmc_host *host) > { > struct device *dev = host->parent; > - u32 bus_width, drv_type; > + u32 bus_width, drv_type, cd_delay_ms; > int ret; > bool cd_cap_invert, cd_gpio_invert = false; > bool ro_cap_invert, ro_gpio_invert = false; > @@ -230,11 +230,14 @@ int mmc_of_parse(struct mmc_host *host) > } else { > cd_cap_invert = device_property_read_bool(dev, "cd-inverted"); > > + if (device_property_read_u32(dev, "cd-delay-ms", &cd_delay_ms)) > + cd_delay_ms = 200; > + > if (device_property_read_bool(dev, "broken-cd")) > host->caps |= MMC_CAP_NEEDS_POLL; > > ret = mmc_gpiod_request_cd(host, "cd", 0, true, > - 0, &cd_gpio_invert); > + 0, &cd_gpio_invert, cd_delay_ms); > if (!ret) > dev_info(host->parent, "Got CD GPIO\n"); > else if (ret != -ENOENT && ret != -ENOSYS) > diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c > index 31f7dbb..baa6e8d 100644 > --- a/drivers/mmc/core/slot-gpio.c > +++ b/drivers/mmc/core/slot-gpio.c > @@ -28,15 +28,17 @@ struct mmc_gpio { > irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id); > char *ro_label; > char cd_label[0]; > + u32 cd_delay_ms; > }; > > static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id) > { > /* Schedule a card detection after a debounce timeout */ > struct mmc_host *host = dev_id; > + struct mmc_gpio *ctx = host->slot.handler_priv; > > host->trigger_card_event = true; > - mmc_detect_change(host, msecs_to_jiffies(200)); > + mmc_detect_change(host, msecs_to_jiffies(ctx->cd_delay_ms)); > > return IRQ_HANDLED; > } > @@ -49,6 +51,7 @@ int mmc_gpio_alloc(struct mmc_host *host) > > if (ctx) { > ctx->ro_label = ctx->cd_label + len; > + ctx->cd_delay_ms = 200; > snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent)); > snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent)); > host->slot.handler_priv = ctx; > @@ -239,6 +242,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio, > * @debounce: debounce time in microseconds > * @gpio_invert: will return whether the GPIO line is inverted or not, set > * to NULL to ignore > + * @cd_delay_ms: delay time in ms before detecting card after card insert > + * event > * > * Use this function in place of mmc_gpio_request_cd() to use the GPIO > * descriptor API. Note that it must be called prior to mmc_add_host() > @@ -248,7 +253,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio, > */ > int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id, > unsigned int idx, bool override_active_level, > - unsigned int debounce, bool *gpio_invert) > + unsigned int debounce, bool *gpio_invert, > + u32 cd_delay_ms) > { > struct mmc_gpio *ctx = host->slot.handler_priv; > struct gpio_desc *desc; > @@ -269,6 +275,7 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id, > > ctx->override_cd_active_level = override_active_level; > ctx->cd_gpio = desc; > + ctx->cd_delay_ms = cd_delay_ms; > > return 0; > } > diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c > index 8e36317..fcff397 100644 > --- a/drivers/mmc/host/davinci_mmc.c > +++ b/drivers/mmc/host/davinci_mmc.c > @@ -1187,7 +1187,7 @@ static int mmc_davinci_parse_pdata(struct mmc_host *mmc) > mmc->caps |= pdata->caps; > > /* Register a cd gpio, if there is not one, enable polling */ > - ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL); > + ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL, 200); > if (ret == -EPROBE_DEFER) > return ret; > else if (ret) > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 70b0df8..0771c4f 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -1827,7 +1827,7 @@ static int mmci_probe(struct amba_device *dev, > * silently of these do not exist and proceed to try platform data > */ > if (!np) { > - ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL); > + ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL, 200); > if (ret < 0) { > if (ret == -EPROBE_DEFER) > goto clk_disable; > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index 32321bd..4157a29 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -718,7 +718,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > if (sdhci_acpi_flag(c, SDHCI_ACPI_SD_CD)) { > bool v = sdhci_acpi_flag(c, SDHCI_ACPI_SD_CD_OVERRIDE_LEVEL); > > - err = mmc_gpiod_request_cd(host->mmc, NULL, 0, v, 0, NULL); > + err = mmc_gpiod_request_cd(host->mmc, NULL, 0, v, 0, NULL, 200); > if (err) { > if (err == -EPROBE_DEFER) > goto err_free; > diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c > index 787434e..7842a67 100644 > --- a/drivers/mmc/host/sdhci-pci-core.c > +++ b/drivers/mmc/host/sdhci-pci-core.c > @@ -1715,7 +1715,8 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( > > if (slot->cd_idx >= 0) { > ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, > - slot->cd_override_level, 0, NULL); > + slot->cd_override_level, 0, NULL, > + 200); > if (ret == -EPROBE_DEFER) > goto remove; > > diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h > index 06607c5..b13f3a9 100644 > --- a/include/linux/mmc/slot-gpio.h > +++ b/include/linux/mmc/slot-gpio.h > @@ -25,7 +25,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio, > > int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id, > unsigned int idx, bool override_active_level, > - unsigned int debounce, bool *gpio_invert); > + unsigned int debounce, bool *gpio_invert, > + u32 cd_delay); > int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id, > unsigned int idx, bool override_active_level, > unsigned int debounce, bool *gpio_invert); > -- > 1.9.1 > > > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, On 2018/4/12 14:32, Ulf Hansson wrote: > On 12 April 2018 at 03:20, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> Allow to use tunable delay before detecting card after card is inserted >> either from firmware, or argument passing to mmc_gpiod_request_cd(). The >> default value is 200ms as before, if cd-delay-ms isn't present but using >> mmc_gpiod_request_cd() to request slot-gpio interrupt. > > Exactly why doesn't the 200 ms delay works? > > It seems like the proper method is instead to use a gpio debouncing > time. mmc-slot-gpio already deals with that, but we are defaulting to > use zero as debounce period, when mmc_gpiod_request_cd() is called > from mmc_of_parse(). I noticed we was defauting to use zero there, but that's no key point for me to use another delay. That being said not all platforms support gpio debounce, then gpiod_set_debounce() just returns -ENOTSUPP. So mmc_of_parse uses zero debounce period for mmc_gpiod_request_cd() is fine,otherwise mmc_gpiod_request_cd() should be improved to deal with failure of gpiod_set_debounce(). How about: mmc_of_parse() --> mmc_gpiod_request_cd(200ms debounce peroid) { ... if (debounce) gpiod_set_debounce(desc, debounce); if (gpio_invert) *gpio_invert = !gpiod_is_active_low(desc); ctx->override_cd_active_level = override_active_level; ctx->cd_gpio = desc; /* Software debounce */ ctx->cd_delay_ms = max_t(unsigned int, debounce, 200); } > > Kind regards > Uffe > >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/mmc/core/host.c | 7 +++++-- >> drivers/mmc/core/slot-gpio.c | 11 +++++++++-- >> drivers/mmc/host/davinci_mmc.c | 2 +- >> drivers/mmc/host/mmci.c | 2 +- >> drivers/mmc/host/sdhci-acpi.c | 2 +- >> drivers/mmc/host/sdhci-pci-core.c | 3 ++- >> include/linux/mmc/slot-gpio.h | 3 ++- >> 7 files changed, 21 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c >> index 64b03d6..cb8babc 100644 >> --- a/drivers/mmc/core/host.c >> +++ b/drivers/mmc/core/host.c >> @@ -179,7 +179,7 @@ static void mmc_retune_timer(struct timer_list *t) >> int mmc_of_parse(struct mmc_host *host) >> { >> struct device *dev = host->parent; >> - u32 bus_width, drv_type; >> + u32 bus_width, drv_type, cd_delay_ms; >> int ret; >> bool cd_cap_invert, cd_gpio_invert = false; >> bool ro_cap_invert, ro_gpio_invert = false; >> @@ -230,11 +230,14 @@ int mmc_of_parse(struct mmc_host *host) >> } else { >> cd_cap_invert = device_property_read_bool(dev, "cd-inverted"); >> >> + if (device_property_read_u32(dev, "cd-delay-ms", &cd_delay_ms)) >> + cd_delay_ms = 200; >> + >> if (device_property_read_bool(dev, "broken-cd")) >> host->caps |= MMC_CAP_NEEDS_POLL; >> >> ret = mmc_gpiod_request_cd(host, "cd", 0, true, >> - 0, &cd_gpio_invert); >> + 0, &cd_gpio_invert, cd_delay_ms); >> if (!ret) >> dev_info(host->parent, "Got CD GPIO\n"); >> else if (ret != -ENOENT && ret != -ENOSYS) >> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c >> index 31f7dbb..baa6e8d 100644 >> --- a/drivers/mmc/core/slot-gpio.c >> +++ b/drivers/mmc/core/slot-gpio.c >> @@ -28,15 +28,17 @@ struct mmc_gpio { >> irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id); >> char *ro_label; >> char cd_label[0]; >> + u32 cd_delay_ms; >> }; >> >> static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id) >> { >> /* Schedule a card detection after a debounce timeout */ >> struct mmc_host *host = dev_id; >> + struct mmc_gpio *ctx = host->slot.handler_priv; >> >> host->trigger_card_event = true; >> - mmc_detect_change(host, msecs_to_jiffies(200)); >> + mmc_detect_change(host, msecs_to_jiffies(ctx->cd_delay_ms)); >> >> return IRQ_HANDLED; >> } >> @@ -49,6 +51,7 @@ int mmc_gpio_alloc(struct mmc_host *host) >> >> if (ctx) { >> ctx->ro_label = ctx->cd_label + len; >> + ctx->cd_delay_ms = 200; >> snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent)); >> snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent)); >> host->slot.handler_priv = ctx; >> @@ -239,6 +242,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio, >> * @debounce: debounce time in microseconds >> * @gpio_invert: will return whether the GPIO line is inverted or not, set >> * to NULL to ignore >> + * @cd_delay_ms: delay time in ms before detecting card after card insert >> + * event >> * >> * Use this function in place of mmc_gpio_request_cd() to use the GPIO >> * descriptor API. Note that it must be called prior to mmc_add_host() >> @@ -248,7 +253,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio, >> */ >> int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id, >> unsigned int idx, bool override_active_level, >> - unsigned int debounce, bool *gpio_invert) >> + unsigned int debounce, bool *gpio_invert, >> + u32 cd_delay_ms) >> { >> struct mmc_gpio *ctx = host->slot.handler_priv; >> struct gpio_desc *desc; >> @@ -269,6 +275,7 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id, >> >> ctx->override_cd_active_level = override_active_level; >> ctx->cd_gpio = desc; >> + ctx->cd_delay_ms = cd_delay_ms; >> >> return 0; >> } >> diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c >> index 8e36317..fcff397 100644 >> --- a/drivers/mmc/host/davinci_mmc.c >> +++ b/drivers/mmc/host/davinci_mmc.c >> @@ -1187,7 +1187,7 @@ static int mmc_davinci_parse_pdata(struct mmc_host *mmc) >> mmc->caps |= pdata->caps; >> >> /* Register a cd gpio, if there is not one, enable polling */ >> - ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL); >> + ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL, 200); >> if (ret == -EPROBE_DEFER) >> return ret; >> else if (ret) >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index 70b0df8..0771c4f 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -1827,7 +1827,7 @@ static int mmci_probe(struct amba_device *dev, >> * silently of these do not exist and proceed to try platform data >> */ >> if (!np) { >> - ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL); >> + ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL, 200); >> if (ret < 0) { >> if (ret == -EPROBE_DEFER) >> goto clk_disable; >> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c >> index 32321bd..4157a29 100644 >> --- a/drivers/mmc/host/sdhci-acpi.c >> +++ b/drivers/mmc/host/sdhci-acpi.c >> @@ -718,7 +718,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev) >> if (sdhci_acpi_flag(c, SDHCI_ACPI_SD_CD)) { >> bool v = sdhci_acpi_flag(c, SDHCI_ACPI_SD_CD_OVERRIDE_LEVEL); >> >> - err = mmc_gpiod_request_cd(host->mmc, NULL, 0, v, 0, NULL); >> + err = mmc_gpiod_request_cd(host->mmc, NULL, 0, v, 0, NULL, 200); >> if (err) { >> if (err == -EPROBE_DEFER) >> goto err_free; >> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c >> index 787434e..7842a67 100644 >> --- a/drivers/mmc/host/sdhci-pci-core.c >> +++ b/drivers/mmc/host/sdhci-pci-core.c >> @@ -1715,7 +1715,8 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( >> >> if (slot->cd_idx >= 0) { >> ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, >> - slot->cd_override_level, 0, NULL); >> + slot->cd_override_level, 0, NULL, >> + 200); >> if (ret == -EPROBE_DEFER) >> goto remove; >> >> diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h >> index 06607c5..b13f3a9 100644 >> --- a/include/linux/mmc/slot-gpio.h >> +++ b/include/linux/mmc/slot-gpio.h >> @@ -25,7 +25,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio, >> >> int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id, >> unsigned int idx, bool override_active_level, >> - unsigned int debounce, bool *gpio_invert); >> + unsigned int debounce, bool *gpio_invert, >> + u32 cd_delay); >> int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id, >> unsigned int idx, bool override_active_level, >> unsigned int debounce, bool *gpio_invert); >> -- >> 1.9.1 >> >> >> -- >> 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 > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 April 2018 at 09:34, Shawn Lin <shawn.lin@rock-chips.com> wrote: > Hi Ulf, > > On 2018/4/12 14:32, Ulf Hansson wrote: >> >> On 12 April 2018 at 03:20, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>> >>> Allow to use tunable delay before detecting card after card is inserted >>> either from firmware, or argument passing to mmc_gpiod_request_cd(). The >>> default value is 200ms as before, if cd-delay-ms isn't present but using >>> mmc_gpiod_request_cd() to request slot-gpio interrupt. >> >> >> Exactly why doesn't the 200 ms delay works? >> >> It seems like the proper method is instead to use a gpio debouncing >> time. mmc-slot-gpio already deals with that, but we are defaulting to >> use zero as debounce period, when mmc_gpiod_request_cd() is called >> from mmc_of_parse(). > > > I noticed we was defauting to use zero there, but that's no key point > for me to use another delay. That being said not all platforms support > gpio debounce, then gpiod_set_debounce() just returns -ENOTSUPP. So Right, because the HW lacks support or because the gpio driver hasn't implemented support for it? > mmc_of_parse uses zero debounce period for mmc_gpiod_request_cd() is > fine,otherwise mmc_gpiod_request_cd() should be improved to deal with > failure of gpiod_set_debounce(). Right, that may be an option. > > How about: > > mmc_of_parse() > --> mmc_gpiod_request_cd(200ms debounce peroid) { > > ... > > if (debounce) > gpiod_set_debounce(desc, debounce); > > if (gpio_invert) > *gpio_invert = !gpiod_is_active_low(desc); > > ctx->override_cd_active_level = override_active_level; > ctx->cd_gpio = desc; > /* Software debounce */ > ctx->cd_delay_ms = max_t(unsigned int, debounce, 200); > > } Huh, not really sure I get the above. Perhaps if you send a proper patch instead, it becomes easier. However if I understand correctly, you are suggesting to use a 200 ms default debounce value - and in case that is succeeded to be set for the GPIO, then we don't need the delay when scheduling the detect work. No? On top of that, you want the debounce value to come from DT? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018/4/12 15:58, Ulf Hansson wrote: > On 12 April 2018 at 09:34, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> Hi Ulf, >> >> On 2018/4/12 14:32, Ulf Hansson wrote: >>> >>> On 12 April 2018 at 03:20, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>>> >>>> Allow to use tunable delay before detecting card after card is inserted >>>> either from firmware, or argument passing to mmc_gpiod_request_cd(). The >>>> default value is 200ms as before, if cd-delay-ms isn't present but using >>>> mmc_gpiod_request_cd() to request slot-gpio interrupt. >>> >>> >>> Exactly why doesn't the 200 ms delay works? >>> >>> It seems like the proper method is instead to use a gpio debouncing >>> time. mmc-slot-gpio already deals with that, but we are defaulting to >>> use zero as debounce period, when mmc_gpiod_request_cd() is called >>> from mmc_of_parse(). >> >> >> I noticed we was defauting to use zero there, but that's no key point >> for me to use another delay. That being said not all platforms support >> gpio debounce, then gpiod_set_debounce() just returns -ENOTSUPP. So > > Right, because the HW lacks support or because the gpio driver hasn't > implemented support for it? yep, on my platforms, the HW lacks support for that. > >> mmc_of_parse uses zero debounce period for mmc_gpiod_request_cd() is >> fine,otherwise mmc_gpiod_request_cd() should be improved to deal with >> failure of gpiod_set_debounce(). > > Right, that may be an option. > >> >> How about: >> >> mmc_of_parse() >> --> mmc_gpiod_request_cd(200ms debounce peroid) { >> >> ... >> >> if (debounce) >> gpiod_set_debounce(desc, debounce); >> >> if (gpio_invert) >> *gpio_invert = !gpiod_is_active_low(desc); >> >> ctx->override_cd_active_level = override_active_level; >> ctx->cd_gpio = desc; >> /* Software debounce */ >> ctx->cd_delay_ms = max_t(unsigned int, debounce, 200); >> >> } > > Huh, not really sure I get the above. Perhaps if you send a proper > patch instead, it becomes easier. > > However if I understand correctly, you are suggesting to use a 200 ms > default debounce value - and in case that is succeeded to be set for > the GPIO, then we don't need the delay when scheduling the detect > work. No? > > On top of that, you want the debounce value to come from DT? In case the debounce is passed on from the caller, it means the caller drivers want it. So we firstly try it by using gpio debounce, but anyway we could fall back to use software debounce method, namely mmc_detect_change(host, msecs_to_jiffies(ctx->cd_delay_ms)); ctx->cd_delay_ms = max_t(unsigned int, debounce, 200) could make sure we at least have a basic 200ms delay which keeps consistent with current code. But a longer delay is better if the caller ask it via debounce argument. I will post v2 for that if the above makes sense to you. :) > > 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 > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 April 2018 at 10:14, Shawn Lin <shawn.lin@rock-chips.com> wrote: > On 2018/4/12 15:58, Ulf Hansson wrote: >> >> On 12 April 2018 at 09:34, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>> >>> Hi Ulf, >>> >>> On 2018/4/12 14:32, Ulf Hansson wrote: >>>> >>>> >>>> On 12 April 2018 at 03:20, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>>>> >>>>> >>>>> Allow to use tunable delay before detecting card after card is inserted >>>>> either from firmware, or argument passing to mmc_gpiod_request_cd(). >>>>> The >>>>> default value is 200ms as before, if cd-delay-ms isn't present but >>>>> using >>>>> mmc_gpiod_request_cd() to request slot-gpio interrupt. >>>> >>>> >>>> >>>> Exactly why doesn't the 200 ms delay works? >>>> >>>> It seems like the proper method is instead to use a gpio debouncing >>>> time. mmc-slot-gpio already deals with that, but we are defaulting to >>>> use zero as debounce period, when mmc_gpiod_request_cd() is called >>>> from mmc_of_parse(). >>> >>> >>> >>> I noticed we was defauting to use zero there, but that's no key point >>> for me to use another delay. That being said not all platforms support >>> gpio debounce, then gpiod_set_debounce() just returns -ENOTSUPP. So >> >> >> Right, because the HW lacks support or because the gpio driver hasn't >> implemented support for it? > > > yep, on my platforms, the HW lacks support for that. OK. > >> >>> mmc_of_parse uses zero debounce period for mmc_gpiod_request_cd() is >>> fine,otherwise mmc_gpiod_request_cd() should be improved to deal with >>> failure of gpiod_set_debounce(). >> >> >> Right, that may be an option. >> >>> >>> How about: >>> >>> mmc_of_parse() >>> --> mmc_gpiod_request_cd(200ms debounce peroid) { >>> >>> ... >>> >>> if (debounce) >>> gpiod_set_debounce(desc, debounce); >>> >>> if (gpio_invert) >>> *gpio_invert = !gpiod_is_active_low(desc); >>> >>> ctx->override_cd_active_level = override_active_level; >>> ctx->cd_gpio = desc; >>> /* Software debounce */ >>> ctx->cd_delay_ms = max_t(unsigned int, debounce, 200); >>> >>> } >> >> >> Huh, not really sure I get the above. Perhaps if you send a proper >> patch instead, it becomes easier. >> >> However if I understand correctly, you are suggesting to use a 200 ms >> default debounce value - and in case that is succeeded to be set for >> the GPIO, then we don't need the delay when scheduling the detect >> work. No? >> >> On top of that, you want the debounce value to come from DT? > > > In case the debounce is passed on from the caller, it means the caller > drivers want it. So we firstly try it by using gpio debounce, but anyway we > could fall back to use software debounce method, namely > mmc_detect_change(host, msecs_to_jiffies(ctx->cd_delay_ms)); Make sense. > > ctx->cd_delay_ms = max_t(unsigned int, debounce, 200) could make sure > we at least have a basic 200ms delay which keeps consistent with current > code. But a longer delay is better if the caller ask it via debounce > argument. I am not sure we want a longer timeout, unless explicitly requested. But perhaps that is what your are suggesting. > > I will post v2 for that if the above makes sense to you. :) Go ahead and I will review! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 12, 2018 at 10:14 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > On 2018/4/12 15:58, Ulf Hansson wrote: >> On 12 April 2018 at 09:34, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> However if I understand correctly, you are suggesting to use a 200 ms >> default debounce value - and in case that is succeeded to be set for >> the GPIO, then we don't need the delay when scheduling the detect >> work. No? >> >> On top of that, you want the debounce value to come from DT? > > In case the debounce is passed on from the caller, it means the caller > drivers want it. So we firstly try it by using gpio debounce, but anyway we > could fall back to use software debounce method, namely > mmc_detect_change(host, msecs_to_jiffies(ctx->cd_delay_ms)); > > ctx->cd_delay_ms = max_t(unsigned int, debounce, 200) could make sure > we at least have a basic 200ms delay which keeps consistent with current > code. But a longer delay is better if the caller ask it via debounce > argument. The debounce time is dependent on use case so it makes sense for this to come from the configuration for a certain system. 200ms makes a lot of sense though. But there is a real problem with the bigger picture here. Please consult: drivers/input/keyboard/gpio_keys.c As you can see, GPIO keys implements a timer-based software delay. The good thing is that this is thoroughly tested code so if you do this, look at how the input subsystem is doing it. A key isn't very different from a MMC/SD card slot. But the hippo in the living room is of course that this will lead to code duplication. Reinventing the wheel. What I would like to happen, is for software debouncing to be moved into gpiolib and used as a fallback in the cases where the hardware does not support debounce. This way gpiod_set_debounce() will never fail with -ENOTSUPP and we can remove the code from in the input subsystem and reuse it freely for MMC/SD card detection. Probably also in drivers/extcon etc. To me this makes most sense. I understand it might be a bit much to ask and I think I tried to factor this over from input to GPIO at one point but failed. Would you like to give it a try, or do you want me to take a stab at it? (It might take some time...) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 2018/4/12 19:47, Linus Walleij wrote: > On Thu, Apr 12, 2018 at 10:14 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> On 2018/4/12 15:58, Ulf Hansson wrote: >>> On 12 April 2018 at 09:34, Shawn Lin <shawn.lin@rock-chips.com> wrote: > >>> However if I understand correctly, you are suggesting to use a 200 ms >>> default debounce value - and in case that is succeeded to be set for >>> the GPIO, then we don't need the delay when scheduling the detect >>> work. No? >>> >>> On top of that, you want the debounce value to come from DT? >> >> In case the debounce is passed on from the caller, it means the caller >> drivers want it. So we firstly try it by using gpio debounce, but anyway we >> could fall back to use software debounce method, namely >> mmc_detect_change(host, msecs_to_jiffies(ctx->cd_delay_ms)); >> >> ctx->cd_delay_ms = max_t(unsigned int, debounce, 200) could make sure >> we at least have a basic 200ms delay which keeps consistent with current >> code. But a longer delay is better if the caller ask it via debounce >> argument. > > The debounce time is dependent on use case so it makes sense > for this to come from the configuration for a certain system. > 200ms makes a lot of sense though. > > But there is a real problem with the bigger picture here. > > Please consult: > drivers/input/keyboard/gpio_keys.c > > As you can see, GPIO keys implements a timer-based > software delay. The good thing is that this is thoroughly tested > code so if you do this, look at how the input subsystem is > doing it. A key isn't very different from a MMC/SD card slot. > > But the hippo in the living room is of course that this will > lead to code duplication. Reinventing the wheel. > > What I would like to happen, is for software debouncing > to be moved into gpiolib and used as a fallback in the cases > where the hardware does not support debounce. > > This way gpiod_set_debounce() will never fail with -ENOTSUPP > and we can remove the code from in the input subsystem > and reuse it freely for MMC/SD card detection. Probably also > in drivers/extcon etc. > > To me this makes most sense. > > I understand it might be a bit much to ask and I think I tried My current plan is still to slightly improve this in MMC/SD slot-gpio code but not touch timer-based software debouce as you may understand that I need backport it for my employer in short-term, expecially they argue upstream first. :) But I'm interested in timer-based software debouce support in GPIO, and will have a look it later. Btw, have you post patch(es) for thatin the past? It should be helpful for me to investigate it if you could share a link. > to factor this over from input to GPIO at one point but > failed. Would you like to give it a try, or do you want me to > take a stab at it? (It might take some time...) > > 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 > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt index 467cd7b..215b9a2 100644 --- a/Documentation/devicetree/bindings/mmc/mmc.txt +++ b/Documentation/devicetree/bindings/mmc/mmc.txt @@ -19,6 +19,8 @@ Optional properties: - wp-gpios: Specify GPIOs for write protection, see gpio binding - cd-inverted: when present, polarity on the CD line is inverted. See the note below for the case, when a GPIO is used for the CD line +- cd-delay-ms: Set delay time before detecting card after card insert interrupt. + It's only valid when cd-gpios is present. - wp-inverted: when present, polarity on the WP line is inverted. See the note below for the case, when a GPIO is used for the WP line - disable-wp: When set no physical WP line is present. This property should
slot-gpio uses a fixed delay, 200ms, before detecting card after the card is inserted. 200ms doesn't work for some platforms, so some host drivers added their own properties for parsing that from DT, for instance, dw_mmc and pxamci. That being said, it should also be tunable when using slog-gpio. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- Documentation/devicetree/bindings/mmc/mmc.txt | 2 ++ 1 file changed, 2 insertions(+)