Re: [PATCH V3 4/4] gpio: tegra: Add support for gpio debounce

[Alexandre Courbot]

On Wed, Apr 20, 2016 at 10:30 PM, Laxman Dewangan <ldewangan@xxxxxxxxxx> 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.
>
> Signed-off-by: Laxman Dewangan <ldewangan@xxxxxxxxxx>
>
> ---
> Changes from V1:
> - Write debounce count before enable.
> - Make sure the debounce count do not have any boot residuals.
>
> Changes from V2:
> - Only access register fo debounce when SoC support debounce.
> ---
>  drivers/gpio/gpio-tegra.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index 36e865f..1f8ec24 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -76,11 +76,14 @@ struct tegra_gpio_bank {
>         u32 int_enb[4];
>         u32 int_lvl[4];
>         u32 wake_enb[4];
> +       u32 dbc_enb[4];
>  #endif
> +       u32 dbc_cnt[4];
>         struct tegra_gpio_info *tgi;
>  };
>
>  struct tegra_gpio_soc_config {
> +       bool debounce_supported;
>         u32 bank_stride;
>         u32 upper_offset;
>  };
> @@ -184,6 +187,35 @@ static int tegra_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
>         return 0;
>  }
>
> +static int tegra_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
> +                                  unsigned int debounce)
> +{
> +       struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> +       unsigned int debounce_ms = DIV_ROUND_UP(debounce, 1000);
> +       int port = GPIO_PORT(offset);
> +       int bank = GPIO_BANK(offset);
> +
> +       if (!debounce_ms) {
> +               tegra_gpio_mask_write(tgi, GPIO_MSK_DBC_EN(tgi, 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.
> +        */
> +       if (tgi->bank_info[bank].dbc_cnt[port] < debounce_ms) {
> +               tegra_gpio_writel(tgi, debounce_ms, GPIO_DBC_CNT(tgi, offset));
> +               tgi->bank_info[bank].dbc_cnt[port] = debounce_ms;
> +       }
> +
> +       tegra_gpio_mask_write(tgi, GPIO_MSK_DBC_EN(tgi, offset), offset, 1);
> +
> +       return 0;
> +}
> +
>  static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
>         struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> @@ -199,6 +231,7 @@ static struct gpio_chip tegra_gpio_chip = {
>         .get                    = tegra_gpio_get,
>         .direction_output       = tegra_gpio_direction_output,
>         .set                    = tegra_gpio_set,
> +       .set_debounce           = tegra_gpio_set_debounce,
>         .to_irq                 = tegra_gpio_to_irq,
>         .base                   = 0,
>  };
> @@ -363,6 +396,14 @@ static int tegra_gpio_resume(struct device *dev)
>                         unsigned int gpio = (b<<5) | (p<<3);
>                         tegra_gpio_writel(tgi, bank->cnf[p],
>                                           GPIO_CNF(tgi, gpio));
> +
> +                       if (tgi->soc->debounce_supported) {
> +                               tegra_gpio_writel(tgi, bank->dbc_cnt[p],
> +                                                 GPIO_DBC_CNT(tgi, gpio));
> +                               tegra_gpio_writel(tgi, bank->dbc_enb[p],
> +                                                 GPIO_MSK_DBC_EN(tgi, gpio));
> +                       }
> +
>                         tegra_gpio_writel(tgi, bank->out[p],
>                                           GPIO_OUT(tgi, gpio));
>                         tegra_gpio_writel(tgi, bank->oe[p],
> @@ -398,6 +439,13 @@ static int tegra_gpio_suspend(struct device *dev)
>                                                         GPIO_OUT(tgi, gpio));
>                         bank->oe[p] = tegra_gpio_readl(tgi,
>                                                        GPIO_OE(tgi, gpio));
> +                       if (tgi->soc->debounce_supported) {
> +                               bank->dbc_enb[p] = tegra_gpio_readl(tgi,
> +                                               GPIO_MSK_DBC_EN(tgi, gpio));
> +                               bank->dbc_enb[p] = (bank->dbc_enb[p] << 8) |
> +                                                       bank->dbc_enb[p];
> +                       }
> +
>                         bank->int_enb[p] = tegra_gpio_readl(tgi,
>                                                 GPIO_INT_ENB(tgi, gpio));
>                         bank->int_lvl[p] = tegra_gpio_readl(tgi,
> @@ -550,6 +598,9 @@ static int tegra_gpio_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, tgi);
>
> +       if (!config->debounce_supported)
> +               tgi->gc->set_debounce = NULL;

This last line is equivalent to doing

     tegra_gpio_chip.set_debounce = NULL

Which means that after that no one can reinstanciate this driver and
use debounce. Granted, this does not happen in real life, but the
purpose of the previous patch that removes all static variables is
supposedly to make this scenario possible. I think you can easily fix
this though: make tgi->gc a non-pointer member, do tgi->gc =
tegra_gpio_chip to copy the initial data, and then set
tgi->gc.set_debounce to NULL if needed.

tegra_gpio_chip can then be made constant, and maybe even __initdata?