All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] Documentation: mmc: add optional cd-delay-ms property
@ 2018-04-12  1:20 Shawn Lin
  2018-04-12  1:20 ` [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted Shawn Lin
  0 siblings, 1 reply; 9+ messages in thread
From: Shawn Lin @ 2018-04-12  1:20 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: Rob Herring, Adrian Hunter, linux-mmc, devicetree, Shawn Lin

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(+)

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
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted
  2018-04-12  1:20 [PATCH 1/2] Documentation: mmc: add optional cd-delay-ms property Shawn Lin
@ 2018-04-12  1:20 ` Shawn Lin
  2018-04-12  6:32   ` Ulf Hansson
  0 siblings, 1 reply; 9+ messages in thread
From: Shawn Lin @ 2018-04-12  1:20 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: Rob Herring, Adrian Hunter, linux-mmc, devicetree, Shawn Lin

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.

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted
  2018-04-12  1:20 ` [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted Shawn Lin
@ 2018-04-12  6:32   ` Ulf Hansson
  2018-04-12  7:34     ` Shawn Lin
  0 siblings, 1 reply; 9+ messages in thread
From: Ulf Hansson @ 2018-04-12  6:32 UTC (permalink / raw)
  To: Shawn Lin; +Cc: Rob Herring, Adrian Hunter, linux-mmc, devicetree

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted
  2018-04-12  6:32   ` Ulf Hansson
@ 2018-04-12  7:34     ` Shawn Lin
  2018-04-12  7:58       ` Ulf Hansson
  0 siblings, 1 reply; 9+ messages in thread
From: Shawn Lin @ 2018-04-12  7:34 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: shawn.lin, Rob Herring, Adrian Hunter, linux-mmc, devicetree

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
> 
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted
  2018-04-12  7:34     ` Shawn Lin
@ 2018-04-12  7:58       ` Ulf Hansson
  2018-04-12  8:14         ` Shawn Lin
  0 siblings, 1 reply; 9+ messages in thread
From: Ulf Hansson @ 2018-04-12  7:58 UTC (permalink / raw)
  To: Shawn Lin; +Cc: Rob Herring, Adrian Hunter, linux-mmc, devicetree

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted
  2018-04-12  7:58       ` Ulf Hansson
@ 2018-04-12  8:14         ` Shawn Lin
  2018-04-12 11:31           ` Ulf Hansson
  2018-04-12 11:47           ` Linus Walleij
  0 siblings, 2 replies; 9+ messages in thread
From: Shawn Lin @ 2018-04-12  8:14 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: shawn.lin, Rob Herring, Adrian Hunter, linux-mmc, devicetree

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
> 
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted
  2018-04-12  8:14         ` Shawn Lin
@ 2018-04-12 11:31           ` Ulf Hansson
  2018-04-12 11:47           ` Linus Walleij
  1 sibling, 0 replies; 9+ messages in thread
From: Ulf Hansson @ 2018-04-12 11:31 UTC (permalink / raw)
  To: Shawn Lin; +Cc: Rob Herring, Adrian Hunter, linux-mmc, devicetree

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted
  2018-04-12  8:14         ` Shawn Lin
  2018-04-12 11:31           ` Ulf Hansson
@ 2018-04-12 11:47           ` Linus Walleij
  2018-04-13  1:26             ` Shawn Lin
  1 sibling, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2018-04-12 11:47 UTC (permalink / raw)
  To: Shawn Lin, Dmitry Torokhov
  Cc: Ulf Hansson, Rob Herring, Adrian Hunter, linux-mmc,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted
  2018-04-12 11:47           ` Linus Walleij
@ 2018-04-13  1:26             ` Shawn Lin
  0 siblings, 0 replies; 9+ messages in thread
From: Shawn Lin @ 2018-04-13  1:26 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Dmitry Torokhov, shawn.lin, Ulf Hansson, Rob Herring,
	Adrian Hunter, linux-mmc,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

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
> 
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-04-13  1:26 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-12  1:20 [PATCH 1/2] Documentation: mmc: add optional cd-delay-ms property Shawn Lin
2018-04-12  1:20 ` [PATCH 2/2] mmc: core: Add tunable delay before detecting card after card is inserted Shawn Lin
2018-04-12  6:32   ` Ulf Hansson
2018-04-12  7:34     ` Shawn Lin
2018-04-12  7:58       ` Ulf Hansson
2018-04-12  8:14         ` Shawn Lin
2018-04-12 11:31           ` Ulf Hansson
2018-04-12 11:47           ` Linus Walleij
2018-04-13  1:26             ` Shawn Lin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.