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

Sat, 14 Sep 2013 23:23:37 -0700 

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.brezil...@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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to