[RFC PATCH 0/4] pinctrl: at91: various fixes

[RFC PATCH 0/4] pinctrl: at91: various fixes

[Boris BREZILLON]

Hello,

This patch series fixes some errors in the pinctrl drivers:
- typos in dt-binding macros and pinctrl-at91 driver (patch 1)
- fix several at91-pinctrl functions (patch 2 and 3)
- rework the debounce config handling (patches 4)

The last point is the most important one: the at91sam9x5/pio3 controller
does not provide a per pin debounce time config. Instead it provides a
common debounce time for all the pins on a given bank (PIOX).

In this series I proposed 2 solutions to gracefully handle this limitation:
1) Handle debounce time conflicts at config time (PATCH 4/4).
   In other words, all the pins on a given bank using the debounce option must
   use the same debounce time. If a device tries to configure a pin with
   conflicting a debounce time, this will result in an -EINVAL error return
   during the pin configuration.
2) Provide a device tree property to define the default debounce time on a
   given bank (PATCH alt 4/4)


I prefer the first solution, as it provides a more future proof approach:
- the generic pinconf layer provides a per pin debounce time config and if
  we plan to support it we should take this into consideration
- IMHO we should be able to (re)configure the debounce time after bootup and
  the second solution does not provide any way to do this

I might be wrong, so please feel free to share your thoughts about this.

Best Regards,

Boris


Boris BREZILLON (4):
  pinctrl: at91: fix typos
  pinctrl: at91: reset caller's config variable before setting flags
  pinctrl: at91: fix get_debounce/deglitch functions for sam9x5
    controller
  pinctrl: at91: check for debounce time conflicts

 drivers/pinctrl/pinctrl-at91.c     |   60 +++++++++++++++++++++++++++---------
 include/dt-bindings/pinctrl/at91.h |    2 +-
 2 files changed, 46 insertions(+), 16 deletions(-)

-- 
1.7.9.5

[RFC PATCH 1/4] pinctrl: at91: fix typos

[Boris BREZILLON]

Fix AT91_PINCTRL_DEBOUNCE_VAL dt macro typo.
Fix at91_pinctrl_mux_ops callback typos.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 drivers/pinctrl/pinctrl-at91.c     |    6 +++---
 include/dt-bindings/pinctrl/at91.h |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 19afb9a..50b555a 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -144,11 +144,11 @@ struct at91_pinctrl_mux_ops {
 	void (*mux_C_periph)(void __iomem *pio, unsigned mask);
 	void (*mux_D_periph)(void __iomem *pio, unsigned mask);
 	bool (*get_deglitch)(void __iomem *pio, unsigned pin);
-	void (*set_deglitch)(void __iomem *pio, unsigned mask, bool in_on);
+	void (*set_deglitch)(void __iomem *pio, unsigned mask, bool is_on);
 	bool (*get_debounce)(void __iomem *pio, unsigned pin, u32 *div);
-	void (*set_debounce)(void __iomem *pio, unsigned mask, bool in_on, u32 div);
+	void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on, u32 div);
 	bool (*get_pulldown)(void __iomem *pio, unsigned pin);
-	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool in_on);
+	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
 	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
 	/* irq */
diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h
index d7988b4..0fee6ff 100644
--- a/include/dt-bindings/pinctrl/at91.h
+++ b/include/dt-bindings/pinctrl/at91.h
@@ -16,7 +16,7 @@
 #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
 #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
 #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
-#define AT91_PINCTRL_DEBOUNCE_VA(x)	(x << 17)
+#define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
 
 #define AT91_PINCTRL_PULL_UP_DEGLITCH	(AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DEGLITCH)
 
-- 
1.7.9.5

[RFC PATCH 2/4] pinctrl: at91: fix sam9x5 debounce/deglitch functions

[Boris BREZILLON]

Replace at91_mux_get_deglitch with at91_mux_pio3_get_deglitch when using
sam9x5 (pio3) IP.
at91_mux_get_deglitch only test the activation of the "Input Filter" which
may be overloaded by the activation of the "Input Filter Slow Clock" to use
the input filter as a debounce filter instead of a deglitch filter.

Fix at91_mux_pio3_get_debounce to test the activation of the Input Filter
before testing the activation of the debounce filter (Input Filter Slow
Clock depends on Input Filter).

Fix at91_mux_pio3_set_debounce function to avoid disabling the deglitch
filter ("Input Filter") when debounce filter is disabled.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 drivers/pinctrl/pinctrl-at91.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 50b555a..6624bce 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -417,6 +417,14 @@ static void at91_mux_set_deglitch(void __iomem *pio, unsigned mask, bool is_on)
 	__raw_writel(mask, pio + (is_on ? PIO_IFER : PIO_IFDR));
 }
 
+static bool at91_mux_pio3_get_deglitch(void __iomem *pio, unsigned pin)
+{
+	if ((__raw_readl(pio + PIO_IFSR) >> pin) & 0x1)
+		return !((__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1);
+
+	return false;
+}
+
 static void at91_mux_pio3_set_deglitch(void __iomem *pio, unsigned mask, bool is_on)
 {
 	if (is_on)
@@ -428,7 +436,8 @@ static bool at91_mux_pio3_get_debounce(void __iomem *pio, unsigned pin, u32 *div
 {
 	*div = __raw_readl(pio + PIO_SCDR);
 
-	return (__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1;
+	return ((__raw_readl(pio + PIO_IFSR) >> pin) & 0x1) &&
+	       ((__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1);
 }
 
 static void at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
@@ -438,9 +447,8 @@ static void at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
 		__raw_writel(mask, pio + PIO_IFSCER);
 		__raw_writel(div & PIO_SCDR_DIV, pio + PIO_SCDR);
 		__raw_writel(mask, pio + PIO_IFER);
-	} else {
-		__raw_writel(mask, pio + PIO_IFDR);
-	}
+	} else
+		__raw_writel(mask, pio + PIO_IFSCDR);
 }
 
 static bool at91_mux_pio3_get_pulldown(void __iomem *pio, unsigned pin)
@@ -478,7 +486,7 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
 	.mux_B_periph	= at91_mux_pio3_set_B_periph,
 	.mux_C_periph	= at91_mux_pio3_set_C_periph,
 	.mux_D_periph	= at91_mux_pio3_set_D_periph,
-	.get_deglitch	= at91_mux_get_deglitch,
+	.get_deglitch	= at91_mux_pio3_get_deglitch,
 	.set_deglitch	= at91_mux_pio3_set_deglitch,
 	.get_debounce	= at91_mux_pio3_get_debounce,
 	.set_debounce	= at91_mux_pio3_set_debounce,
-- 
1.7.9.5

[RFC PATCH 3/4] pinctrl: at91: improve pinconf_set/get function robustness

[Boris BREZILLON]

Reset caller's config variable before setting current config flags to avoid
erronous config return.

DEBOUNCE and DEGLITCH options are mutually exclusive. Return an error if they
are both defined in the config.
Do not call set_deglitch if DEBOUNCE is enabled to avoid reseting the IFSR
register (which will result in disabling the debounce filter).

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 drivers/pinctrl/pinctrl-at91.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 6624bce..ac9dbea 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -724,6 +724,7 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
 	dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__, __LINE__, pin_id, *config);
 	pio = pin_to_controller(info, pin_to_bank(pin_id));
 	pin = pin_id % MAX_NB_GPIO_PER_BANK;
+	*config = 0;
 
 	if (at91_mux_get_multidrive(pio, pin))
 		*config |= MULTI_DRIVE;
@@ -757,13 +758,20 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
 	if (config & PULL_UP && config & PULL_DOWN)
 		return -EINVAL;
 
-	at91_mux_set_pullup(pio, mask, config & PULL_UP);
-	at91_mux_set_multidrive(pio, mask, config & MULTI_DRIVE);
-	if (info->ops->set_deglitch)
-		info->ops->set_deglitch(pio, mask, config & DEGLITCH);
-	if (info->ops->set_debounce)
+	if (config & DEBOUNCE && config & DEGLITCH)
+		return -EINVAL;
+
+	if (config & DEBOUNCE) {
+		if (!info->ops->set_debounce)
+			return -ENOTSUPP;
+
 		info->ops->set_debounce(pio, mask, config & DEBOUNCE,
 				(config & DEBOUNCE_VAL) >> DEBOUNCE_VAL_SHIFT);
+	} else if (info->ops->set_deglitch)
+		info->ops->set_deglitch(pio, mask, config & DEGLITCH);
+
+	at91_mux_set_pullup(pio, mask, config & PULL_UP);
+	at91_mux_set_multidrive(pio, mask, config & MULTI_DRIVE);
 	if (info->ops->set_pulldown)
 		info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
 	if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
-- 
1.7.9.5

[RFC PATCH 4/4] pinctrl: at91: check for debounce time conflicts

[Boris BREZILLON]

On sam9x5 (pio3) PIO controller the debounce time config is shared across
all the pins of a given PIO bank (PIOA, PIOB, ...).

This patch adds checks before applying debounce config on a given pin.
If the pinctrl core tries to configure a debounce time on a given pin and
another pin on the same bank is already configured with a different
debounce time, the pin config with fail with -EINVAL.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 drivers/pinctrl/pinctrl-at91.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index ac9dbea..986e9bc 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -146,7 +146,7 @@ struct at91_pinctrl_mux_ops {
 	bool (*get_deglitch)(void __iomem *pio, unsigned pin);
 	void (*set_deglitch)(void __iomem *pio, unsigned mask, bool is_on);
 	bool (*get_debounce)(void __iomem *pio, unsigned pin, u32 *div);
-	void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on, u32 div);
+	int (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on, u32 div);
 	bool (*get_pulldown)(void __iomem *pio, unsigned pin);
 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
@@ -440,15 +440,26 @@ static bool at91_mux_pio3_get_debounce(void __iomem *pio, unsigned pin, u32 *div
 	       ((__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1);
 }
 
-static void at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
+static int at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
 				bool is_on, u32 div)
 {
 	if (is_on) {
+		div &= PIO_SCDR_DIV;
+
+		/* Check if another pin of this bank is already using debounce
+		 * option with a different debounce time */
+		if ((__raw_readl(pio + PIO_IFSR) &
+		     __raw_readl(pio + PIO_IFSCSR) & ~mask) &&
+		    (__raw_readl(pio + PIO_SCDR) & PIO_SCDR_DIV) != div)
+			return -EINVAL;
+
 		__raw_writel(mask, pio + PIO_IFSCER);
-		__raw_writel(div & PIO_SCDR_DIV, pio + PIO_SCDR);
+		__raw_writel(div, pio + PIO_SCDR);
 		__raw_writel(mask, pio + PIO_IFER);
 	} else
 		__raw_writel(mask, pio + PIO_IFSCDR);
+
+	return 0;
 }
 
 static bool at91_mux_pio3_get_pulldown(void __iomem *pio, unsigned pin)
@@ -750,6 +761,7 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
 	struct at91_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
 	unsigned mask;
 	void __iomem *pio;
+	int ret;
 
 	dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__, __LINE__, pin_id, config);
 	pio = pin_to_controller(info, pin_to_bank(pin_id));
@@ -765,8 +777,10 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
 		if (!info->ops->set_debounce)
 			return -ENOTSUPP;
 
-		info->ops->set_debounce(pio, mask, config & DEBOUNCE,
+		ret = info->ops->set_debounce(pio, mask, config & DEBOUNCE,
 				(config & DEBOUNCE_VAL) >> DEBOUNCE_VAL_SHIFT);
+		if (ret)
+			return ret;
 	} else if (info->ops->set_deglitch)
 		info->ops->set_deglitch(pio, mask, config & DEGLITCH);
 
-- 
1.7.9.5

[RFC PATCH alt 4/4] pinctrl: at91: rework debounce configuration

[Boris BREZILLON]

AT91 SoCs do not support per pin debounce time configuration.
Instead you have to configure a debounce time which will be used for all
pins of a given bank (PIOA, PIOB, ...).

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 .../bindings/pinctrl/atmel,at91-pinctrl.txt        |    9 ++-
 drivers/pinctrl/pinctrl-at91.c                     |   79 ++++++++++++++++----
 include/dt-bindings/pinctrl/at91.h                 |    1 -
 3 files changed, 73 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
index cf7c7bc..8a4cdeb 100644
--- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
@@ -78,6 +78,14 @@ PA31	TXD4
 
 => 0xffc00c3b
 
+Optional properties for iomux controller:
+- atmel,default-debounce-div: array of debounce divisors (one divisor per bank)
+  which describes the debounce timing in use for all pins of a given bank
+  configured with the DEBOUNCE option (see the following description).
+  Debounce timing is obtained with this formula:
+  Tdebounce = 2 * (debouncediv + 1) / Fslowclk
+  with Fslowclk = 32KHz
+
 Required properties for pin configuration node:
 - atmel,pins: 4 integers array, represents a group of pins mux and config
   setting. The format is atmel,pins = <PIN_BANK PIN_BANK_NUM PERIPH CONFIG>.
@@ -91,7 +99,6 @@ DEGLITCH	(1 << 2): indicate this pin need deglitch.
 PULL_DOWN	(1 << 3): indicate this pin need a pull down.
 DIS_SCHMIT	(1 << 4): indicate this pin need to disable schmit trigger.
 DEBOUNCE	(1 << 16): indicate this pin need debounce.
-DEBOUNCE_VAL	(0x3fff << 17): debounce val.
 
 NOTE:
 Some requirements for using atmel,at91rm9200-pinctrl binding:
diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index ac9dbea..2903758 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -62,8 +62,6 @@ static int gpio_banks;
 #define PULL_DOWN	(1 << 3)
 #define DIS_SCHMIT	(1 << 4)
 #define DEBOUNCE	(1 << 16)
-#define DEBOUNCE_VAL_SHIFT	17
-#define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
 
 /**
  * struct at91_pmx_func - describes AT91 pinmux functions
@@ -145,8 +143,10 @@ struct at91_pinctrl_mux_ops {
 	void (*mux_D_periph)(void __iomem *pio, unsigned mask);
 	bool (*get_deglitch)(void __iomem *pio, unsigned pin);
 	void (*set_deglitch)(void __iomem *pio, unsigned mask, bool is_on);
-	bool (*get_debounce)(void __iomem *pio, unsigned pin, u32 *div);
-	void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on, u32 div);
+	bool (*get_debounce)(void __iomem *pio, unsigned pin);
+	void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on);
+	u32 (*get_debounce_div)(void __iomem *pio);
+	void (*set_debounce_div)(void __iomem *pio, u32 div);
 	bool (*get_pulldown)(void __iomem *pio, unsigned pin);
 	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
 	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
@@ -432,25 +432,32 @@ static void at91_mux_pio3_set_deglitch(void __iomem *pio, unsigned mask, bool is
 	at91_mux_set_deglitch(pio, mask, is_on);
 }
 
-static bool at91_mux_pio3_get_debounce(void __iomem *pio, unsigned pin, u32 *div)
+static bool at91_mux_pio3_get_debounce(void __iomem *pio, unsigned pin)
 {
-	*div = __raw_readl(pio + PIO_SCDR);
-
 	return ((__raw_readl(pio + PIO_IFSR) >> pin) & 0x1) &&
 	       ((__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1);
 }
 
 static void at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
-				bool is_on, u32 div)
+				bool is_on)
 {
 	if (is_on) {
 		__raw_writel(mask, pio + PIO_IFSCER);
-		__raw_writel(div & PIO_SCDR_DIV, pio + PIO_SCDR);
 		__raw_writel(mask, pio + PIO_IFER);
 	} else
 		__raw_writel(mask, pio + PIO_IFSCDR);
 }
 
+static u32 at91_mux_pio3_get_debounce_div(void __iomem *pio)
+{
+	return __raw_readl(pio + PIO_SCDR) & PIO_SCDR_DIV;
+}
+
+static void at91_mux_pio3_set_debounce_div(void __iomem *pio, u32 div)
+{
+	__raw_writel(div & PIO_SCDR_DIV, pio + PIO_SCDR);
+}
+
 static bool at91_mux_pio3_get_pulldown(void __iomem *pio, unsigned pin)
 {
 	return !((__raw_readl(pio + PIO_PPDSR) >> pin) & 0x1);
@@ -490,6 +497,8 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
 	.set_deglitch	= at91_mux_pio3_set_deglitch,
 	.get_debounce	= at91_mux_pio3_get_debounce,
 	.set_debounce	= at91_mux_pio3_set_debounce,
+	.get_debounce_div = at91_mux_pio3_get_debounce_div,
+	.set_debounce_div = at91_mux_pio3_set_debounce_div,
 	.get_pulldown	= at91_mux_pio3_get_pulldown,
 	.set_pulldown	= at91_mux_pio3_set_pulldown,
 	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
@@ -719,7 +728,6 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
 	struct at91_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
 	void __iomem *pio;
 	unsigned pin;
-	int div;
 
 	dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__, __LINE__, pin_id, *config);
 	pio = pin_to_controller(info, pin_to_bank(pin_id));
@@ -734,8 +742,8 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
 
 	if (info->ops->get_deglitch && info->ops->get_deglitch(pio, pin))
 		*config |= DEGLITCH;
-	if (info->ops->get_debounce && info->ops->get_debounce(pio, pin, &div))
-		*config |= DEBOUNCE | (div << DEBOUNCE_VAL_SHIFT);
+	if (info->ops->get_debounce && info->ops->get_debounce(pio, pin))
+		*config |= DEBOUNCE;
 	if (info->ops->get_pulldown && info->ops->get_pulldown(pio, pin))
 		*config |= PULL_DOWN;
 	if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio, pin))
@@ -765,8 +773,7 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
 		if (!info->ops->set_debounce)
 			return -ENOTSUPP;
 
-		info->ops->set_debounce(pio, mask, config & DEBOUNCE,
-				(config & DEBOUNCE_VAL) >> DEBOUNCE_VAL_SHIFT);
+		info->ops->set_debounce(pio, mask, config & DEBOUNCE);
 	} else if (info->ops->set_deglitch)
 		info->ops->set_deglitch(pio, mask, config & DEGLITCH);
 
@@ -822,6 +829,39 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
 	}
 }
 
+static int at91_pinctrl_default_debounce_div(struct at91_pinctrl *info,
+					     struct device_node *np)
+{
+	int size;
+	int i;
+	int ret;
+
+	if (!info->ops->set_debounce_div ||
+	    !of_get_property(np, "atmel,default-debounce-div", &size))
+		return 0;
+
+	size /= sizeof(u32);
+	if (size != info->nbanks) {
+		dev_err(info->dev,
+			"atmel,default-debounce-div property should contain %d elements\n",
+			info->nbanks);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < info->nbanks; i++) {
+		u32 val;
+		ret = of_property_read_u32_index(np,
+						 "atmel,default-debounce-div",
+						 i, &val);
+		if (ret)
+			return ret;
+
+		info->ops->set_debounce_div(gpio_chips[i]->regbase, val);
+	}
+
+	return 0;
+}
+
 static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
 				 struct device_node *np)
 {
@@ -972,6 +1012,10 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
+	ret = at91_pinctrl_default_debounce_div(info, np);
+	if (ret)
+		return ret;
+
 	dev_dbg(&pdev->dev, "nmux = %d\n", info->nmux);
 
 	dev_dbg(&pdev->dev, "mux-mask\n");
@@ -982,6 +1026,13 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 		}
 	}
 
+	if (info->ops->get_debounce_div) {
+		dev_dbg(&pdev->dev, "default-debounce-div\n");
+		for (i = 0; i < info->nbanks; i++)
+			dev_dbg(&pdev->dev, "bank %d\t%d\n", i,
+				info->ops->get_debounce_div(gpio_chips[i]->regbase));
+	}
+
 	dev_dbg(&pdev->dev, "nfunctions = %d\n", info->nfunctions);
 	dev_dbg(&pdev->dev, "ngroups = %d\n", info->ngroups);
 	info->functions = devm_kzalloc(&pdev->dev, info->nfunctions * sizeof(struct at91_pmx_func),
diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h
index 0fee6ff..b478954 100644
--- a/include/dt-bindings/pinctrl/at91.h
+++ b/include/dt-bindings/pinctrl/at91.h
@@ -16,7 +16,6 @@
 #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
 #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
 #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
-#define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
 
 #define AT91_PINCTRL_PULL_UP_DEGLITCH	(AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DEGLITCH)
 
-- 
1.7.9.5

Re: [RFC PATCH alt 4/4] pinctrl: at91: rework debounce configuration

[Stephen Warren]

On 09/13/2013 01:53 AM, Boris BREZILLON wrote:
> AT91 SoCs do not support per pin debounce time configuration.
> Instead you have to configure a debounce time which will be used for all
> pins of a given bank (PIOA, PIOB, ...).

> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt

> +Optional properties for iomux controller:
> +- atmel,default-debounce-div: array of debounce divisors (one divisor per bank)
> +  which describes the debounce timing in use for all pins of a given bank
> +  configured with the DEBOUNCE option (see the following description).
> +  Debounce timing is obtained with this formula:
> +  Tdebounce = 2 * (debouncediv + 1) / Fslowclk
> +  with Fslowclk = 32KHz
> +
>  Required properties for pin configuration node:
>  - atmel,pins: 4 integers array, represents a group of pins mux and config
>    setting. The format is atmel,pins = <PIN_BANK PIN_BANK_NUM PERIPH CONFIG>.
> @@ -91,7 +99,6 @@ DEGLITCH	(1 << 2): indicate this pin need deglitch.
>  PULL_DOWN	(1 << 3): indicate this pin need a pull down.
>  DIS_SCHMIT	(1 << 4): indicate this pin need to disable schmit trigger.
>  DEBOUNCE	(1 << 16): indicate this pin need debounce.
> -DEBOUNCE_VAL	(0x3fff << 17): debounce val.

This change would break the DT ABI since it removes a feature that's
already present.

I suppose it's still up to the Atmel maintainers to decide whether this
is appropriate, or whether the impact to out-of-tree DT files would be
problematic.

Assuming the DT ABI can be broken, I think I'd prefer to do so, rather
than take "non-alt" patch 4/4, since a per-pin DEBOUNCE_VAL clearly
doesn't correctly model the HW, assuming the patch description is
correct. I don't think arguments re: the generic pinconf debounce
property hold; if the Linux-specific/internal generic property doesn't
apply, the DT binding should not be bent to adjust to it, but should
rather still represent the HW itself.

Re: [RFC PATCH alt 4/4] pinctrl: at91: rework debounce configuration

[boris brezillon]

Hello Stephen,

Le 14/09/2013 00:40, Stephen Warren a écrit :
> On 09/13/2013 01:53 AM, Boris BREZILLON wrote:
>> AT91 SoCs do not support per pin debounce time configuration.
>> Instead you have to configure a debounce time which will be used for all
>> pins of a given bank (PIOA, PIOB, ...).
>> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> +Optional properties for iomux controller:
>> +- atmel,default-debounce-div: array of debounce divisors (one divisor per bank)
>> +  which describes the debounce timing in use for all pins of a given bank
>> +  configured with the DEBOUNCE option (see the following description).
>> +  Debounce timing is obtained with this formula:
>> +  Tdebounce = 2 * (debouncediv + 1) / Fslowclk
>> +  with Fslowclk = 32KHz
>> +
>>   Required properties for pin configuration node:
>>   - atmel,pins: 4 integers array, represents a group of pins mux and config
>>     setting. The format is atmel,pins = <PIN_BANK PIN_BANK_NUM PERIPH CONFIG>.
>> @@ -91,7 +99,6 @@ DEGLITCH	(1 << 2): indicate this pin need deglitch.
>>   PULL_DOWN	(1 << 3): indicate this pin need a pull down.
>>   DIS_SCHMIT	(1 << 4): indicate this pin need to disable schmit trigger.
>>   DEBOUNCE	(1 << 16): indicate this pin need debounce.
>> -DEBOUNCE_VAL	(0x3fff << 17): debounce val.
> This change would break the DT ABI since it removes a feature that's
> already present.

I missed this point in my cons list.
This won't be an issue for in kernel DT definitions (nobody is currently 
using the
DEBOUCE option), but may be for out-of-tree DT definitions.

> I suppose it's still up to the Atmel maintainers to decide whether this
> is appropriate, or whether the impact to out-of-tree DT files would be
> problematic.
>
> Assuming the DT ABI can be broken, I think I'd prefer to do so, rather
> than take "non-alt" patch 4/4, since a per-pin DEBOUNCE_VAL clearly
> doesn't correctly model the HW, assuming the patch description is
> correct. I don't think arguments re: the generic pinconf debounce
> property hold; if the Linux-specific/internal generic property doesn't
> apply, the DT binding should not be bent to adjust to it, but should
> rather still represent the HW itself.

What about the last point in my list: "reconfigure debounce after startup" ?

Here is an example that may be problematic:

Let's say you have one device using multiple configuration of pins 
("default", "xxx", "yyy").
The "default" config needs a particular debounce time on a given pin and 
the "xxx" and "yyy"
configs need different debounce time on the same pin.

How would you solve this with this patch approach ?


Best Regards,

Boris

Re: [RFC PATCH alt 4/4] pinctrl: at91: rework debounce configuration

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:40 Fri 13 Sep     , Stephen Warren wrote:
> On 09/13/2013 01:53 AM, Boris BREZILLON wrote:
> > AT91 SoCs do not support per pin debounce time configuration.
> > Instead you have to configure a debounce time which will be used for all
> > pins of a given bank (PIOA, PIOB, ...).
> 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> 
> > +Optional properties for iomux controller:
> > +- atmel,default-debounce-div: array of debounce divisors (one divisor per bank)
> > +  which describes the debounce timing in use for all pins of a given bank
> > +  configured with the DEBOUNCE option (see the following description).
> > +  Debounce timing is obtained with this formula:
> > +  Tdebounce = 2 * (debouncediv + 1) / Fslowclk
> > +  with Fslowclk = 32KHz
> > +
> >  Required properties for pin configuration node:
> >  - atmel,pins: 4 integers array, represents a group of pins mux and config
> >    setting. The format is atmel,pins = <PIN_BANK PIN_BANK_NUM PERIPH CONFIG>.
> > @@ -91,7 +99,6 @@ DEGLITCH	(1 << 2): indicate this pin need deglitch.
> >  PULL_DOWN	(1 << 3): indicate this pin need a pull down.
> >  DIS_SCHMIT	(1 << 4): indicate this pin need to disable schmit trigger.
> >  DEBOUNCE	(1 << 16): indicate this pin need debounce.
> > -DEBOUNCE_VAL	(0x3fff << 17): debounce val.
> 
> This change would break the DT ABI since it removes a feature that's
> already present.
> 
> I suppose it's still up to the Atmel maintainers to decide whether this
> is appropriate, or whether the impact to out-of-tree DT files would be
> problematic.

I does ask Boris to break the DT ABI

as anyway no one use it and the current ABI is wrong

and as this is the new SoC the impact of out-of-tree board is limited if ever

Best Regards,
J.

Re: [RFC PATCH alt 4/4] pinctrl: at91: rework debounce configuration

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:53 Fri 13 Sep     , Boris BREZILLON wrote:
> AT91 SoCs do not support per pin debounce time configuration.
> Instead you have to configure a debounce time which will be used for all
> pins of a given bank (PIOA, PIOB, ...).
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> ---
>  .../bindings/pinctrl/atmel,at91-pinctrl.txt        |    9 ++-
>  drivers/pinctrl/pinctrl-at91.c                     |   79 ++++++++++++++++----
>  include/dt-bindings/pinctrl/at91.h                 |    1 -
>  3 files changed, 73 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> index cf7c7bc..8a4cdeb 100644
> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> @@ -78,6 +78,14 @@ PA31	TXD4
>  
>  => 0xffc00c3b
>  
> +Optional properties for iomux controller:
> +- atmel,default-debounce-div: array of debounce divisors (one divisor per bank)
> +  which describes the debounce timing in use for all pins of a given bank
> +  configured with the DEBOUNCE option (see the following description).
> +  Debounce timing is obtained with this formula:
> +  Tdebounce = 2 * (debouncediv + 1) / Fslowclk
> +  with Fslowclk = 32KHz

I known that I put the div in the original binding

but maybe we should just put the debounce timing in the DT and calculate the
div in C
> +
>  Required properties for pin configuration node:
>  - atmel,pins: 4 integers array, represents a group of pins mux and config
>    setting. The format is atmel,pins = <PIN_BANK PIN_BANK_NUM PERIPH CONFIG>.
> @@ -91,7 +99,6 @@ DEGLITCH	(1 << 2): indicate this pin need deglitch.
>  PULL_DOWN	(1 << 3): indicate this pin need a pull down.
>  DIS_SCHMIT	(1 << 4): indicate this pin need to disable schmit trigger.
>  DEBOUNCE	(1 << 16): indicate this pin need debounce.
> -DEBOUNCE_VAL	(0x3fff << 17): debounce val.
>  
>  NOTE:
>  Some requirements for using atmel,at91rm9200-pinctrl binding:
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index ac9dbea..2903758 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -62,8 +62,6 @@ static int gpio_banks;
>  #define PULL_DOWN	(1 << 3)
>  #define DIS_SCHMIT	(1 << 4)
>  #define DEBOUNCE	(1 << 16)
> -#define DEBOUNCE_VAL_SHIFT	17
> -#define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
>  
>  /**
>   * struct at91_pmx_func - describes AT91 pinmux functions
> @@ -145,8 +143,10 @@ struct at91_pinctrl_mux_ops {
>  	void (*mux_D_periph)(void __iomem *pio, unsigned mask);
>  	bool (*get_deglitch)(void __iomem *pio, unsigned pin);
>  	void (*set_deglitch)(void __iomem *pio, unsigned mask, bool is_on);
> -	bool (*get_debounce)(void __iomem *pio, unsigned pin, u32 *div);
> -	void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on, u32 div);
> +	bool (*get_debounce)(void __iomem *pio, unsigned pin);
> +	void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on);
> +	u32 (*get_debounce_div)(void __iomem *pio);
> +	void (*set_debounce_div)(void __iomem *pio, u32 div);
why do you split it?

if it's just get if on or not put NULL to div but do not add more function
pointer
>  	bool (*get_pulldown)(void __iomem *pio, unsigned pin);
>  	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
>  	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
> @@ -432,25 +432,32 @@ static void at91_mux_pio3_set_deglitch(void __iomem *pio, unsigned mask, bool is
>  	at91_mux_set_deglitch(pio, mask, is_on);
>  }
>  
> -static bool at91_mux_pio3_get_debounce(void __iomem *pio, unsigned pin, u32 *div)
> +static bool at91_mux_pio3_get_debounce(void __iomem *pio, unsigned pin)
>  {
> -	*div = __raw_readl(pio + PIO_SCDR);
> -
>  	return ((__raw_readl(pio + PIO_IFSR) >> pin) & 0x1) &&
>  	       ((__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1);
>  }
>  
>  static void at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
> -				bool is_on, u32 div)
> +				bool is_on)
>  {
>  	if (is_on) {
>  		__raw_writel(mask, pio + PIO_IFSCER);
> -		__raw_writel(div & PIO_SCDR_DIV, pio + PIO_SCDR);
>  		__raw_writel(mask, pio + PIO_IFER);
>  	} else
>  		__raw_writel(mask, pio + PIO_IFSCDR);
>  }
>  
> +static u32 at91_mux_pio3_get_debounce_div(void __iomem *pio)
> +{
> +	return __raw_readl(pio + PIO_SCDR) & PIO_SCDR_DIV;
> +}
> +
> +static void at91_mux_pio3_set_debounce_div(void __iomem *pio, u32 div)
> +{
> +	__raw_writel(div & PIO_SCDR_DIV, pio + PIO_SCDR);
> +}
> +
>  static bool at91_mux_pio3_get_pulldown(void __iomem *pio, unsigned pin)
>  {
>  	return !((__raw_readl(pio + PIO_PPDSR) >> pin) & 0x1);
> @@ -490,6 +497,8 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
>  	.set_deglitch	= at91_mux_pio3_set_deglitch,
>  	.get_debounce	= at91_mux_pio3_get_debounce,
>  	.set_debounce	= at91_mux_pio3_set_debounce,
> +	.get_debounce_div = at91_mux_pio3_get_debounce_div,
> +	.set_debounce_div = at91_mux_pio3_set_debounce_div,
>  	.get_pulldown	= at91_mux_pio3_get_pulldown,
>  	.set_pulldown	= at91_mux_pio3_set_pulldown,
>  	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
> @@ -719,7 +728,6 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
>  	struct at91_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>  	void __iomem *pio;
>  	unsigned pin;
> -	int div;
>  
>  	dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__, __LINE__, pin_id, *config);
>  	pio = pin_to_controller(info, pin_to_bank(pin_id));
> @@ -734,8 +742,8 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
>  
>  	if (info->ops->get_deglitch && info->ops->get_deglitch(pio, pin))
>  		*config |= DEGLITCH;
> -	if (info->ops->get_debounce && info->ops->get_debounce(pio, pin, &div))
> -		*config |= DEBOUNCE | (div << DEBOUNCE_VAL_SHIFT);
> +	if (info->ops->get_debounce && info->ops->get_debounce(pio, pin))
> +		*config |= DEBOUNCE;
>  	if (info->ops->get_pulldown && info->ops->get_pulldown(pio, pin))
>  		*config |= PULL_DOWN;
>  	if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio, pin))
> @@ -765,8 +773,7 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
>  		if (!info->ops->set_debounce)
>  			return -ENOTSUPP;
>  
> -		info->ops->set_debounce(pio, mask, config & DEBOUNCE,
> -				(config & DEBOUNCE_VAL) >> DEBOUNCE_VAL_SHIFT);
> +		info->ops->set_debounce(pio, mask, config & DEBOUNCE);
>  	} else if (info->ops->set_deglitch)
>  		info->ops->set_deglitch(pio, mask, config & DEGLITCH);
>  
> @@ -822,6 +829,39 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
>  	}
>  }
>  
> +static int at91_pinctrl_default_debounce_div(struct at91_pinctrl *info,
> +					     struct device_node *np)
> +{
> +	int size;
> +	int i;
> +	int ret;
> +
> +	if (!info->ops->set_debounce_div ||
> +	    !of_get_property(np, "atmel,default-debounce-div", &size))
> +		return 0;
> +
> +	size /= sizeof(u32);
> +	if (size != info->nbanks) {
> +		dev_err(info->dev,
> +			"atmel,default-debounce-div property should contain %d elements\n",
> +			info->nbanks);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < info->nbanks; i++) {
> +		u32 val;
> +		ret = of_property_read_u32_index(np,
> +						 "atmel,default-debounce-div",
> +						 i, &val);
> +		if (ret)
> +			return ret;
> +
> +		info->ops->set_debounce_div(gpio_chips[i]->regbase, val);
> +	}
> +
> +	return 0;
> +}
> +
>  static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
>  				 struct device_node *np)
>  {
> @@ -972,6 +1012,10 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
>  	if (ret)
>  		return ret;
>  
> +	ret = at91_pinctrl_default_debounce_div(info, np);
> +	if (ret)
> +		return ret;
> +
>  	dev_dbg(&pdev->dev, "nmux = %d\n", info->nmux);
>  
>  	dev_dbg(&pdev->dev, "mux-mask\n");
> @@ -982,6 +1026,13 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
>  		}
>  	}
>  
> +	if (info->ops->get_debounce_div) {
> +		dev_dbg(&pdev->dev, "default-debounce-div\n");
> +		for (i = 0; i < info->nbanks; i++)
> +			dev_dbg(&pdev->dev, "bank %d\t%d\n", i,
> +				info->ops->get_debounce_div(gpio_chips[i]->regbase));
> +	}
> +
>  	dev_dbg(&pdev->dev, "nfunctions = %d\n", info->nfunctions);
>  	dev_dbg(&pdev->dev, "ngroups = %d\n", info->ngroups);
>  	info->functions = devm_kzalloc(&pdev->dev, info->nfunctions * sizeof(struct at91_pmx_func),
> diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h
> index 0fee6ff..b478954 100644
> --- a/include/dt-bindings/pinctrl/at91.h
> +++ b/include/dt-bindings/pinctrl/at91.h
> @@ -16,7 +16,6 @@
>  #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
>  #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
>  #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
> -#define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
>  
>  #define AT91_PINCTRL_PULL_UP_DEGLITCH	(AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DEGLITCH)
>  
> -- 
> 1.7.9.5
> 

Re: [RFC PATCH 1/4] pinctrl: at91: fix typos

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:45 Fri 13 Sep     , Boris BREZILLON wrote:
> Fix AT91_PINCTRL_DEBOUNCE_VAL dt macro typo.
> Fix at91_pinctrl_mux_ops callback typos.
> 
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> ---
>  drivers/pinctrl/pinctrl-at91.c     |    6 +++---
>  include/dt-bindings/pinctrl/at91.h |    2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 19afb9a..50b555a 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -144,11 +144,11 @@ struct at91_pinctrl_mux_ops {
>  	void (*mux_C_periph)(void __iomem *pio, unsigned mask);
>  	void (*mux_D_periph)(void __iomem *pio, unsigned mask);
>  	bool (*get_deglitch)(void __iomem *pio, unsigned pin);
> -	void (*set_deglitch)(void __iomem *pio, unsigned mask, bool in_on);
> +	void (*set_deglitch)(void __iomem *pio, unsigned mask, bool is_on);
>  	bool (*get_debounce)(void __iomem *pio, unsigned pin, u32 *div);
> -	void (*set_debounce)(void __iomem *pio, unsigned mask, bool in_on, u32 div);
> +	void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on, u32 div);
>  	bool (*get_pulldown)(void __iomem *pio, unsigned pin);
> -	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool in_on);
> +	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
>  	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
>  	void (*disable_schmitt_trig)(void __iomem *pio, unsigned mask);
>  	/* irq */
> diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h
> index d7988b4..0fee6ff 100644
> --- a/include/dt-bindings/pinctrl/at91.h
> +++ b/include/dt-bindings/pinctrl/at91.h
> @@ -16,7 +16,7 @@
>  #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
>  #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
>  #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
> -#define AT91_PINCTRL_DEBOUNCE_VA(x)	(x << 17)
> +#define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
>  
>  #define AT91_PINCTRL_PULL_UP_DEGLITCH	(AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DEGLITCH)
>  
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 2/4] pinctrl: at91: fix sam9x5 debounce/deglitch functions

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:47 Fri 13 Sep     , Boris BREZILLON wrote:
> Replace at91_mux_get_deglitch with at91_mux_pio3_get_deglitch when using
> sam9x5 (pio3) IP.
> at91_mux_get_deglitch only test the activation of the "Input Filter" which
> may be overloaded by the activation of the "Input Filter Slow Clock" to use
> the input filter as a debounce filter instead of a deglitch filter.
> 
> Fix at91_mux_pio3_get_debounce to test the activation of the Input Filter
> before testing the activation of the debounce filter (Input Filter Slow
> Clock depends on Input Filter).
> 
> Fix at91_mux_pio3_set_debounce function to avoid disabling the deglitch
> filter ("Input Filter") when debounce filter is disabled.
> 
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> ---
>  drivers/pinctrl/pinctrl-at91.c |   18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 50b555a..6624bce 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -417,6 +417,14 @@ static void at91_mux_set_deglitch(void __iomem *pio, unsigned mask, bool is_on)
>  	__raw_writel(mask, pio + (is_on ? PIO_IFER : PIO_IFDR));
>  }
>  
> +static bool at91_mux_pio3_get_deglitch(void __iomem *pio, unsigned pin)
> +{
> +	if ((__raw_readl(pio + PIO_IFSR) >> pin) & 0x1)
> +		return !((__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1);
> +
> +	return false;
> +}
> +
>  static void at91_mux_pio3_set_deglitch(void __iomem *pio, unsigned mask, bool is_on)
>  {
>  	if (is_on)
> @@ -428,7 +436,8 @@ static bool at91_mux_pio3_get_debounce(void __iomem *pio, unsigned pin, u32 *div
>  {
>  	*div = __raw_readl(pio + PIO_SCDR);
>  
> -	return (__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1;
> +	return ((__raw_readl(pio + PIO_IFSR) >> pin) & 0x1) &&
> +	       ((__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1);
>  }
>  
>  static void at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
> @@ -438,9 +447,8 @@ static void at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
>  		__raw_writel(mask, pio + PIO_IFSCER);
>  		__raw_writel(div & PIO_SCDR_DIV, pio + PIO_SCDR);
>  		__raw_writel(mask, pio + PIO_IFER);
> -	} else {
> -		__raw_writel(mask, pio + PIO_IFDR);
> -	}
> +	} else
> +		__raw_writel(mask, pio + PIO_IFSCDR);
>  }
>  
>  static bool at91_mux_pio3_get_pulldown(void __iomem *pio, unsigned pin)
> @@ -478,7 +486,7 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
>  	.mux_B_periph	= at91_mux_pio3_set_B_periph,
>  	.mux_C_periph	= at91_mux_pio3_set_C_periph,
>  	.mux_D_periph	= at91_mux_pio3_set_D_periph,
> -	.get_deglitch	= at91_mux_get_deglitch,
> +	.get_deglitch	= at91_mux_pio3_get_deglitch,
>  	.set_deglitch	= at91_mux_pio3_set_deglitch,
>  	.get_debounce	= at91_mux_pio3_get_debounce,
>  	.set_debounce	= at91_mux_pio3_set_debounce,
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 3/4] pinctrl: at91: improve pinconf_set/get function robustness

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:49 Fri 13 Sep     , Boris BREZILLON wrote:
> Reset caller's config variable before setting current config flags to avoid
> erronous config return.
> 
> DEBOUNCE and DEGLITCH options are mutually exclusive. Return an error if they
> are both defined in the config.
> Do not call set_deglitch if DEBOUNCE is enabled to avoid reseting the IFSR
> register (which will result in disabling the debounce filter).
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> ---
>  drivers/pinctrl/pinctrl-at91.c |   18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 6624bce..ac9dbea 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -724,6 +724,7 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
>  	dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__, __LINE__, pin_id, *config);
>  	pio = pin_to_controller(info, pin_to_bank(pin_id));
>  	pin = pin_id % MAX_NB_GPIO_PER_BANK;
> +	*config = 0;
>  
>  	if (at91_mux_get_multidrive(pio, pin))
>  		*config |= MULTI_DRIVE;
> @@ -757,13 +758,20 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
>  	if (config & PULL_UP && config & PULL_DOWN)
>  		return -EINVAL;
>  
> -	at91_mux_set_pullup(pio, mask, config & PULL_UP);
> -	at91_mux_set_multidrive(pio, mask, config & MULTI_DRIVE);
> -	if (info->ops->set_deglitch)
> -		info->ops->set_deglitch(pio, mask, config & DEGLITCH);
> -	if (info->ops->set_debounce)
> +	if (config & DEBOUNCE && config & DEGLITCH)
> +		return -EINVAL;
here ok
> +
> +	if (config & DEBOUNCE) {
> +		if (!info->ops->set_debounce)
> +			return -ENOTSUPP;
a warning is better here than an error
> +
>  		info->ops->set_debounce(pio, mask, config & DEBOUNCE,
>  				(config & DEBOUNCE_VAL) >> DEBOUNCE_VAL_SHIFT);
> +	} else if (info->ops->set_deglitch)
> +		info->ops->set_deglitch(pio, mask, config & DEGLITCH);
> +


> +	at91_mux_set_pullup(pio, mask, config & PULL_UP);
> +	at91_mux_set_multidrive(pio, mask, config & MULTI_DRIVE);

>  	if (info->ops->set_pulldown)
>  		info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
>  	if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH alt 4/4] pinctrl: at91: rework debounce configuration

[boris brezillon]

Hello Jean-Christophe,

Le 14/09/2013 18:37, Jean-Christophe PLAGNIOL-VILLARD a écrit :
> On 09:53 Fri 13 Sep     , Boris BREZILLON wrote:
>> AT91 SoCs do not support per pin debounce time configuration.
>> Instead you have to configure a debounce time which will be used for all
>> pins of a given bank (PIOA, PIOB, ...).
>>
>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>> ---
>>   .../bindings/pinctrl/atmel,at91-pinctrl.txt        |    9 ++-
>>   drivers/pinctrl/pinctrl-at91.c                     |   79 ++++++++++++++++----
>>   include/dt-bindings/pinctrl/at91.h                 |    1 -
>>   3 files changed, 73 insertions(+), 16 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> index cf7c7bc..8a4cdeb 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
>> @@ -78,6 +78,14 @@ PA31	TXD4
>>   
>>   => 0xffc00c3b
>>   
>> +Optional properties for iomux controller:
>> +- atmel,default-debounce-div: array of debounce divisors (one divisor per bank)
>> +  which describes the debounce timing in use for all pins of a given bank
>> +  configured with the DEBOUNCE option (see the following description).
>> +  Debounce timing is obtained with this formula:
>> +  Tdebounce = 2 * (debouncediv + 1) / Fslowclk
>> +  with Fslowclk = 32KHz
> I known that I put the div in the original binding
>
> but maybe we should just put the debounce timing in the DT and calculate the
> div in C

Sure, I can do this: retrieve a debounce time (in usec ?) and compute the
according div value.

>> +
>>   Required properties for pin configuration node:
>>   - atmel,pins: 4 integers array, represents a group of pins mux and config
>>     setting. The format is atmel,pins = <PIN_BANK PIN_BANK_NUM PERIPH CONFIG>.
>> @@ -91,7 +99,6 @@ DEGLITCH	(1 << 2): indicate this pin need deglitch.
>>   PULL_DOWN	(1 << 3): indicate this pin need a pull down.
>>   DIS_SCHMIT	(1 << 4): indicate this pin need to disable schmit trigger.
>>   DEBOUNCE	(1 << 16): indicate this pin need debounce.
>> -DEBOUNCE_VAL	(0x3fff << 17): debounce val.
>>   
>>   NOTE:
>>   Some requirements for using atmel,at91rm9200-pinctrl binding:
>> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
>> index ac9dbea..2903758 100644
>> --- a/drivers/pinctrl/pinctrl-at91.c
>> +++ b/drivers/pinctrl/pinctrl-at91.c
>> @@ -62,8 +62,6 @@ static int gpio_banks;
>>   #define PULL_DOWN	(1 << 3)
>>   #define DIS_SCHMIT	(1 << 4)
>>   #define DEBOUNCE	(1 << 16)
>> -#define DEBOUNCE_VAL_SHIFT	17
>> -#define DEBOUNCE_VAL	(0x3fff << DEBOUNCE_VAL_SHIFT)
>>   
>>   /**
>>    * struct at91_pmx_func - describes AT91 pinmux functions
>> @@ -145,8 +143,10 @@ struct at91_pinctrl_mux_ops {
>>   	void (*mux_D_periph)(void __iomem *pio, unsigned mask);
>>   	bool (*get_deglitch)(void __iomem *pio, unsigned pin);
>>   	void (*set_deglitch)(void __iomem *pio, unsigned mask, bool is_on);
>> -	bool (*get_debounce)(void __iomem *pio, unsigned pin, u32 *div);
>> -	void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on, u32 div);
>> +	bool (*get_debounce)(void __iomem *pio, unsigned pin);
>> +	void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on);
>> +	u32 (*get_debounce_div)(void __iomem *pio);
>> +	void (*set_debounce_div)(void __iomem *pio, u32 div);
> why do you split it?
>
> if it's just get if on or not put NULL to div but do not add more function
> pointer

I not sure I got your point.
Are you suggesting we should store the default bank bebounce div values 
in struct at91_pinctrl
(during probe process) and pass these values each time the set_debounce 
function is called ?

IMHO if we split the logic (split debounce activation and debounce time 
definition) we should split
these callbacks:
  - one callback to enable debounce option on a given pin
  - one callback to configure the debounce time for a given bank

If you keep the div parameter in the debounce enable/disable callback 
you will reconfigure the div
register (PIO_SCDR_DIV) each time you enable the debounce option, which 
is kind of useless
because the div value will never change.

>>   	bool (*get_pulldown)(void __iomem *pio, unsigned pin);
>>   	void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
>>   	bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
>> @@ -432,25 +432,32 @@ static void at91_mux_pio3_set_deglitch(void __iomem *pio, unsigned mask, bool is
>>   	at91_mux_set_deglitch(pio, mask, is_on);
>>   }
>>   
>> -static bool at91_mux_pio3_get_debounce(void __iomem *pio, unsigned pin, u32 *div)
>> +static bool at91_mux_pio3_get_debounce(void __iomem *pio, unsigned pin)
>>   {
>> -	*div = __raw_readl(pio + PIO_SCDR);
>> -
>>   	return ((__raw_readl(pio + PIO_IFSR) >> pin) & 0x1) &&
>>   	       ((__raw_readl(pio + PIO_IFSCSR) >> pin) & 0x1);
>>   }
>>   
>>   static void at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
>> -				bool is_on, u32 div)
>> +				bool is_on)
>>   {
>>   	if (is_on) {
>>   		__raw_writel(mask, pio + PIO_IFSCER);
>> -		__raw_writel(div & PIO_SCDR_DIV, pio + PIO_SCDR);
>>   		__raw_writel(mask, pio + PIO_IFER);
>>   	} else
>>   		__raw_writel(mask, pio + PIO_IFSCDR);
>>   }
>>   
>> +static u32 at91_mux_pio3_get_debounce_div(void __iomem *pio)
>> +{
>> +	return __raw_readl(pio + PIO_SCDR) & PIO_SCDR_DIV;
>> +}
>> +
>> +static void at91_mux_pio3_set_debounce_div(void __iomem *pio, u32 div)
>> +{
>> +	__raw_writel(div & PIO_SCDR_DIV, pio + PIO_SCDR);
>> +}
>> +
>>   static bool at91_mux_pio3_get_pulldown(void __iomem *pio, unsigned pin)
>>   {
>>   	return !((__raw_readl(pio + PIO_PPDSR) >> pin) & 0x1);
>> @@ -490,6 +497,8 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
>>   	.set_deglitch	= at91_mux_pio3_set_deglitch,
>>   	.get_debounce	= at91_mux_pio3_get_debounce,
>>   	.set_debounce	= at91_mux_pio3_set_debounce,
>> +	.get_debounce_div = at91_mux_pio3_get_debounce_div,
>> +	.set_debounce_div = at91_mux_pio3_set_debounce_div,
>>   	.get_pulldown	= at91_mux_pio3_get_pulldown,
>>   	.set_pulldown	= at91_mux_pio3_set_pulldown,
>>   	.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
>> @@ -719,7 +728,6 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
>>   	struct at91_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>>   	void __iomem *pio;
>>   	unsigned pin;
>> -	int div;
>>   
>>   	dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__, __LINE__, pin_id, *config);
>>   	pio = pin_to_controller(info, pin_to_bank(pin_id));
>> @@ -734,8 +742,8 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
>>   
>>   	if (info->ops->get_deglitch && info->ops->get_deglitch(pio, pin))
>>   		*config |= DEGLITCH;
>> -	if (info->ops->get_debounce && info->ops->get_debounce(pio, pin, &div))
>> -		*config |= DEBOUNCE | (div << DEBOUNCE_VAL_SHIFT);
>> +	if (info->ops->get_debounce && info->ops->get_debounce(pio, pin))
>> +		*config |= DEBOUNCE;
>>   	if (info->ops->get_pulldown && info->ops->get_pulldown(pio, pin))
>>   		*config |= PULL_DOWN;
>>   	if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio, pin))
>> @@ -765,8 +773,7 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
>>   		if (!info->ops->set_debounce)
>>   			return -ENOTSUPP;
>>   
>> -		info->ops->set_debounce(pio, mask, config & DEBOUNCE,
>> -				(config & DEBOUNCE_VAL) >> DEBOUNCE_VAL_SHIFT);
>> +		info->ops->set_debounce(pio, mask, config & DEBOUNCE);
>>   	} else if (info->ops->set_deglitch)
>>   		info->ops->set_deglitch(pio, mask, config & DEGLITCH);
>>   
>> @@ -822,6 +829,39 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,
>>   	}
>>   }
>>   
>> +static int at91_pinctrl_default_debounce_div(struct at91_pinctrl *info,
>> +					     struct device_node *np)
>> +{
>> +	int size;
>> +	int i;
>> +	int ret;
>> +
>> +	if (!info->ops->set_debounce_div ||
>> +	    !of_get_property(np, "atmel,default-debounce-div", &size))
>> +		return 0;
>> +
>> +	size /= sizeof(u32);
>> +	if (size != info->nbanks) {
>> +		dev_err(info->dev,
>> +			"atmel,default-debounce-div property should contain %d elements\n",
>> +			info->nbanks);
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < info->nbanks; i++) {
>> +		u32 val;
>> +		ret = of_property_read_u32_index(np,
>> +						 "atmel,default-debounce-div",
>> +						 i, &val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		info->ops->set_debounce_div(gpio_chips[i]->regbase, val);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int at91_pinctrl_mux_mask(struct at91_pinctrl *info,
>>   				 struct device_node *np)
>>   {
>> @@ -972,6 +1012,10 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
>>   	if (ret)
>>   		return ret;
>>   
>> +	ret = at91_pinctrl_default_debounce_div(info, np);
>> +	if (ret)
>> +		return ret;
>> +
>>   	dev_dbg(&pdev->dev, "nmux = %d\n", info->nmux);
>>   
>>   	dev_dbg(&pdev->dev, "mux-mask\n");
>> @@ -982,6 +1026,13 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
>>   		}
>>   	}
>>   
>> +	if (info->ops->get_debounce_div) {
>> +		dev_dbg(&pdev->dev, "default-debounce-div\n");
>> +		for (i = 0; i < info->nbanks; i++)
>> +			dev_dbg(&pdev->dev, "bank %d\t%d\n", i,
>> +				info->ops->get_debounce_div(gpio_chips[i]->regbase));
>> +	}
>> +
>>   	dev_dbg(&pdev->dev, "nfunctions = %d\n", info->nfunctions);
>>   	dev_dbg(&pdev->dev, "ngroups = %d\n", info->ngroups);
>>   	info->functions = devm_kzalloc(&pdev->dev, info->nfunctions * sizeof(struct at91_pmx_func),
>> diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h
>> index 0fee6ff..b478954 100644
>> --- a/include/dt-bindings/pinctrl/at91.h
>> +++ b/include/dt-bindings/pinctrl/at91.h
>> @@ -16,7 +16,6 @@
>>   #define AT91_PINCTRL_PULL_DOWN		(1 << 3)
>>   #define AT91_PINCTRL_DIS_SCHMIT		(1 << 4)
>>   #define AT91_PINCTRL_DEBOUNCE		(1 << 16)
>> -#define AT91_PINCTRL_DEBOUNCE_VAL(x)	(x << 17)
>>   
>>   #define AT91_PINCTRL_PULL_UP_DEGLITCH	(AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DEGLITCH)
>>   
>> -- 
>> 1.7.9.5
>>

Re: [RFC PATCH alt 4/4] pinctrl: at91: rework debounce configuration

[Stephen Warren]

On 09/14/2013 01:08 AM, boris brezillon wrote:
> Hello Stephen,
> 
> Le 14/09/2013 00:40, Stephen Warren a écrit :
>> On 09/13/2013 01:53 AM, Boris BREZILLON wrote:
>>> AT91 SoCs do not support per pin debounce time configuration.
>>> Instead you have to configure a debounce time which will be used for all
>>> pins of a given bank (PIOA, PIOB, ...).
...
>>>   Required properties for pin configuration node:
...
>>> -DEBOUNCE_VAL    (0x3fff << 17): debounce val.
>>
>> This change would break the DT ABI since it removes a feature that's
>> already present.
...
>> I suppose it's still up to the Atmel maintainers to decide whether this
>> is appropriate, or whether the impact to out-of-tree DT files would be
>> problematic.
>>
>> Assuming the DT ABI can be broken, I think I'd prefer to do so, rather
>> than take "non-alt" patch 4/4, since a per-pin DEBOUNCE_VAL clearly
>> doesn't correctly model the HW, assuming the patch description is
>> correct. I don't think arguments re: the generic pinconf debounce
>> property hold; if the Linux-specific/internal generic property doesn't
>> apply, the DT binding should not be bent to adjust to it, but should
>> rather still represent the HW itself.
> 
> What about the last point in my list: "reconfigure debounce after
> startup" ?
> 
> Here is an example that may be problematic:
> 
> Let's say you have one device using multiple configuration of pins
> ("default", "xxx", "yyy").
> The "default" config needs a particular debounce time on a given pin and
> the "xxx" and "yyy"
> configs need different debounce time on the same pin.
> 
> How would you solve this with this patch approach ?

Each state has a different pin configuration node, and hence can specify
a different debounce value. This patch has no impact on that (it just
changes whether the state-specific node specifies the debounce value in
a single standalone property, or encodes it into each entry in the pins
property, all within the same node).

Re: [RFC PATCH alt 4/4] pinctrl: at91: rework debounce configuration

[boris brezillon]

Hello Stephen,

On 16/09/2013 18:41, Stephen Warren wrote:
> On 09/14/2013 01:08 AM, boris brezillon wrote:
>> Hello Stephen,
>>
>> Le 14/09/2013 00:40, Stephen Warren a écrit :
>>> On 09/13/2013 01:53 AM, Boris BREZILLON wrote:
>>>> AT91 SoCs do not support per pin debounce time configuration.
>>>> Instead you have to configure a debounce time which will be used for all
>>>> pins of a given bank (PIOA, PIOB, ...).
> ...
>>>>    Required properties for pin configuration node:
> ...
>>>> -DEBOUNCE_VAL    (0x3fff << 17): debounce val.
>>> This change would break the DT ABI since it removes a feature that's
>>> already present.
> ...
>>> I suppose it's still up to the Atmel maintainers to decide whether this
>>> is appropriate, or whether the impact to out-of-tree DT files would be
>>> problematic.
>>>
>>> Assuming the DT ABI can be broken, I think I'd prefer to do so, rather
>>> than take "non-alt" patch 4/4, since a per-pin DEBOUNCE_VAL clearly
>>> doesn't correctly model the HW, assuming the patch description is
>>> correct. I don't think arguments re: the generic pinconf debounce
>>> property hold; if the Linux-specific/internal generic property doesn't
>>> apply, the DT binding should not be bent to adjust to it, but should
>>> rather still represent the HW itself.
>> What about the last point in my list: "reconfigure debounce after
>> startup" ?
>>
>> Here is an example that may be problematic:
>>
>> Let's say you have one device using multiple configuration of pins
>> ("default", "xxx", "yyy").
>> The "default" config needs a particular debounce time on a given pin and
>> the "xxx" and "yyy"
>> configs need different debounce time on the same pin.
>>
>> How would you solve this with this patch approach ?
> Each state has a different pin configuration node, and hence can specify
> a different debounce value. This patch has no impact on that (it just
> changes whether the state-specific node specifies the debounce value in
> a single standalone property, or encodes it into each entry in the pins
> property, all within the same node).
Actually it does: this patch removes the debounce time setting option from
the pin config description. The only thing you can do is enable or 
disable the
debounce filter.

The atmel,default-debounce-div property is not part of the pin group (or 
pin state)
node, it is a global property you define for the whole pinctrl 
controller (pinctrl node
property):

pinctrl {
         atmel,default-debounce-div=<100 /* PIOA div <=> ~3 ms */
                                                          50   /* PIOB 
div */
                                                          ...>;

         function {
             group {
                 atmel,pins=<...>;
             };
         };
};

I can get the debounce time option in a separate property (as you're 
suggesting):

pinctrl {
     function {
         group {
             atmel,debounce=<1000>; /* debounce in usec */
             atmel,pins=<...>;
         };
     };
};

but it won't solve the primary issue, that is all the pin on a given 
bank (PIOA1 PIOA2, ...)
share the same debounce time.

Please tell me if I misunderstood your suggestion.

Best Regards,

Boris

Re: [RFC PATCH alt 4/4] pinctrl: at91: rework debounce configuration

[Stephen Warren]

On 09/16/2013 11:18 AM, boris brezillon wrote:
> Hello Stephen,
> 
> On 16/09/2013 18:41, Stephen Warren wrote:
>> On 09/14/2013 01:08 AM, boris brezillon wrote:
...
>>> Let's say you have one device using multiple configuration of pins
>>> ("default", "xxx", "yyy").
>>> The "default" config needs a particular debounce time on a given pin and
>>> the "xxx" and "yyy"
>>> configs need different debounce time on the same pin.
>>>
>>> How would you solve this with this patch approach ?
>>
>> Each state has a different pin configuration node, and hence can specify
>> a different debounce value. ...
...
> Actually it does: this patch removes the debounce time setting option from
> the pin config description. The only thing you can do is enable or
> disable the debounce filter.
> 
> The atmel,default-debounce-div property is not part of the pin group (or
> pin state) node, it is a global property you define for the whole pinctrl
> controller (pinctrl node
> property):
> 
> pinctrl {
>         atmel,default-debounce-div=<100...
...

Oh, I see. Sorry.

It seems this HW has been designed to just set that configuration up
once and be done with it.

There really isn't a way to make anything more complex or dynamic work
correctly. If you try to put the debounce config into a per-state node
rather than the top-level, how will you reconcile any conflicts? What if
the gpio-keys default state's node says 10ms, whereas the SDHCI
controller's default state's node says 100ms? Both those states need to
be active at the same time. The only way to avoid conflict resolution
issues like that is to prevent them, and just put the debounce config at
the top-level, and hence program it once when the driver starts, i.e.
make the DT exactly match the HW capabilities.

Re: [RFC PATCH 1/4] pinctrl: at91: fix typos

[Linus Walleij]

On Fri, Sep 13, 2013 at 9:45 AM, Boris BREZILLON
<b.brezillon@overkiz.com> wrote:

> Fix AT91_PINCTRL_DEBOUNCE_VAL dt macro typo.
> Fix at91_pinctrl_mux_ops callback typos.
>
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Patch applied with Jean-Christophe's ACK.

Yours,
Linus Walleij

Re: [RFC PATCH 2/4] pinctrl: at91: fix sam9x5 debounce/deglitch functions

[Linus Walleij]

On Fri, Sep 13, 2013 at 9:47 AM, Boris BREZILLON
<b.brezillon@overkiz.com> wrote:

> Replace at91_mux_get_deglitch with at91_mux_pio3_get_deglitch when using
> sam9x5 (pio3) IP.
> at91_mux_get_deglitch only test the activation of the "Input Filter" which
> may be overloaded by the activation of the "Input Filter Slow Clock" to use
> the input filter as a debounce filter instead of a deglitch filter.
>
> Fix at91_mux_pio3_get_debounce to test the activation of the Input Filter
> before testing the activation of the debounce filter (Input Filter Slow
> Clock depends on Input Filter).
>
> Fix at91_mux_pio3_set_debounce function to avoid disabling the deglitch
> filter ("Input Filter") when debounce filter is disabled.
>
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Patch applied with Jean-Christophe's ACK.

Yours,
Linus Walleij