lkml.org 
[lkml]   [2016]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 3/3] gpio: tegra: Add support for gpio debounce
    From
    Date
    On 04/18/2016 02:46 AM, Laxman Dewangan wrote:
    > NVIDIA's Tegra210 support the HW debounce in the GPIO
    > controller for all its GPIO pins.
    >
    > Add support for setting debounce timing by implementing the
    > set_debounce callback of gpiochip.

    > diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c

    > +static int tegra_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
    > + unsigned int debounce)
    > +{
    > + unsigned int max_dbc;
    > + unsigned int debounce_ms = DIV_ROUND_UP(debounce, 1000);
    > +
    > + if (!debounce_ms) {
    > + tegra_gpio_mask_write(GPIO_MSK_DBC_EN(offset), offset, 0);
    > + return 0;
    > + }
    > +
    > + debounce_ms = min(debounce_ms, 255U);
    > +
    > + /* There is only one debounce count register per port and hence
    > + * set the maximum of current and requested debounce time.
    > + */
    > + max_dbc = tegra_gpio_readl(GPIO_DBC_CNT(offset));

    What if the system boots with random values in that register, or some
    code that runs before the kernel programs large values into the
    register? That would (incorrectly) impose a lower bound on the possible
    values the kernel driver can impose. Perhaps the kernel should clear the
    DBC_CNT registers at probe(), or should store a shadow copy of the
    DBC_CNT register, use that value here rather than re-reading the
    registers, and clear that SW shadow at probe().

    > + max_dbc = max(max_dbc, debounce_ms);

    I wonder if there should be more discussion of how to honor conflicting
    requests. Perhaps we should only allow exactly equal values (someone
    might strictly care about latency, and increasing the latency of GPIO X1
    just because GPIO X5 wanted a longer debounce period might not be
    acceptable). Does the GPIO subsystem define explicit semantics for this
    case?

    > + tegra_gpio_mask_write(GPIO_MSK_DBC_EN(offset), offset, 1);
    > + tegra_gpio_writel(max_dbc, GPIO_DBC_CNT(offset));

    I think DBC_CNT should be written first; the debounce process uses that
    data to configure itself. The process shouldn't be enabled before it's
    configured.

    > @@ -327,6 +358,9 @@ static int tegra_gpio_resume(struct device *dev)
    > tegra_gpio_writel(bank->oe[p], GPIO_OE(gpio));
    > tegra_gpio_writel(bank->int_lvl[p], GPIO_INT_LVL(gpio));
    > tegra_gpio_writel(bank->int_enb[p], GPIO_INT_ENB(gpio));
    > + tegra_gpio_writel(bank->dbc_enb[p],
    > + GPIO_MSK_DBC_EN(gpio));
    > + tegra_gpio_writel(bank->dbc_cnt[p], GPIO_DBC_CNT(gpio));

    dbg_cnt should be restored before dbc_en, for the same reason as above.

    \
     
     \ /
      Last update: 2016-04-18 19:01    [W:5.780 / U:0.756 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site