Re: [PATCH] gpio: aspeed: Add debounce support

From: Joel Stanley
Date: Tue Feb 21 2017 - 23:17:27 EST


On Tue, Feb 21, 2017 at 1:10 AM, Andrew Jeffery <andrew@xxxxxxxx> wrote:
> Each GPIO in the Aspeed GPIO controller can choose one of four input
> debounce states: to not debounce an input or to select from one of three
> programmable debounce timer values. Each GPIO in a four-bank-set is
> assigned one bit in each of two debounce configuration registers
> dedicated to the set, and selects a debounce state by configuring the
> two bits to select one of the four options.
>
> The limitation on debounce timer values is managed by mapping offsets
> onto a configured timer value and keeping count of the number of users
> a timer has. Timer values are configured on a first-come-first-served
> basis.
>
> A small twist in the hardware design is that the debounce configuration
> register numbering is reversed with respect to the binary representation
> of the debounce timer of interest (i.e. debounce register 1 represents
> bit 1, and debounce register 2 represents bit 0 of the timer numbering).
>
> Open-drain/open-source support also introduced, but merely by deferring
> to the gpiolib emulation (as per the datasheet).
>
> Tested on an AST2500EVB.
>
> Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>

Some questions below. Looks good overall.

> ---
> drivers/gpio/gpio-aspeed.c | 244 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 239 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index fb16cc771c0d..164a75272151 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c


> struct aspeed_gpio {
> struct gpio_chip chip;
> spinlock_t lock;
> void __iomem *base;
> int irq;
> const struct aspeed_gpio_config *config;
> +
> + /* Debouncing */
> + DECLARE_HASHTABLE(offset_timer, 5);

Ohh a hash table. I've not seen one of these in a driver before :)

> + unsigned int timer_users[3];
> + struct clk *clk;
> };
>

> +static int aspeed_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
> + unsigned long usecs)
> +{
> + const struct aspeed_gpio_bank *bank = to_bank(offset);
> + struct aspeed_gpio *gpio = gpiochip_get_data(chip);
> + const u32 mask = GPIO_BIT(offset);
> + unsigned long flags;
> + void __iomem *addr;
> + u32 val;
> + int rc;
> +
> + if (!have_debounce(gpio, offset))
> + return -ENOTSUPP;
> +
> + if (usecs) {

if (usecs) is the "turn on debounce", and the else is "disable debounce"?

It would be easier to read if we had a enable_debounce and disable_debounce.

> + struct aspeed_debounce_timer *dt, *newdt;

I thought this was device tree and got really confused. Perhaps 'timer' instead?

> + u32 requested_cycles;
> + int i;
> +
> + /*
> + * TODO: Look at improving these:
> + *
> + * - we always delete users of timers even if the call is for a
> + * duplicate debounce period for an offset
> + * - we don't reuse struct aspeed_debounce_timer allocations if
> + * the offset is already using a timer
> + */
> +
> + rc = usecs_to_cycles(gpio, usecs, &requested_cycles);
> + if (rc < 0)
> + return rc;

Should this produce a warning (as you do below when we're out of timers)?

> +
> + newdt = devm_kmalloc(chip->parent, sizeof(*dt), GFP_KERNEL);
> + if (!newdt)
> + return -ENOMEM;
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + /* Check if the @offset is already using a timer */
> + hash_for_each_possible(gpio->offset_timer, dt, node, offset) {
> + if (offset == dt->offset) {
> + hash_del(&dt->node);
> + WARN_ON(!gpio->timer_users[dt->timer]);
> + gpio->timer_users[dt->timer]--;
> + devm_kfree(chip->parent, dt);
> + break;
> + }
> + }
> +
> + /*
> + * Check if a timer is already configured for the requested
> + * debounce period. If so, just add @offset as a user of this
> + * timer
> + */
> + for (i = 0; i < ARRAY_SIZE(debounce_timers); i++) {
> + u32 cycles;
> +
> + addr = gpio->base + debounce_timers[i];
> + cycles = ioread32(addr);
> +
> + if (requested_cycles == cycles)
> + break;
> + }
> +
> + /* Otherwise, allocate a timer */

Is "otherwise" in reference to the check above? Is the break above
supposed to be a goto?

> + if (i == ARRAY_SIZE(debounce_timers)) {
> + for (i = 0; i < ARRAY_SIZE(gpio->timer_users); i++) {
> + if (gpio->timer_users[i] == 0)
> + break;
> + }
> +
> + /* We have run out of timers */
> + if (i == ARRAY_SIZE(gpio->timer_users)) {
> + dev_warn(chip->parent,
> + "Debounce timers exhausted, cannot debounce for period %luus\n",
> + usecs);
> + rc = -EPERM;
> + goto err;
> + }
> +
> + /* Timer update */
> + addr = gpio->base + debounce_timers[i];
> + iowrite32(requested_cycles, addr);
> + }
> +
> + /* Register @offset as a user of timer i */
> + newdt->offset = offset;
> + newdt->timer = i;
> + hash_add(gpio->offset_timer, &newdt->node, offset);

I got a bit lost with gpio->offset_timer and offset. Is offset
referring to a GPIO pin?

> +
> + WARN_ON(gpio->timer_users[i] == UINT_MAX);

Should we just bail out here? Also printing a message to say Danger
Will Robinson would be more informative.

> + gpio->timer_users[i]++;
> +
> + /* Configure @offset to use timer i */
> + addr = bank_debounce_reg(gpio, bank, GPIO_DEBOUNCE_SEL1);
> + val = ioread32(addr);
> + iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE1(i, offset), addr);

Is the mask clearing the bit and GPIO_SET_DEBOUNCE1 setting it again?

We're pretty deep in macro land by now.

> +
> + addr = bank_debounce_reg(gpio, bank, GPIO_DEBOUNCE_SEL2);
> + val = ioread32(addr);
> + iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE2(i, offset), addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);