Message-ID: <1487811486.21631.6.camel@aj.id.au>
Date:   Thu, 23 Feb 2017 11:28:06 +1030
From:   Andrew Jeffery <andrew@...id.au>
To:     Joel Stanley <joel@....id.au>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Alexandre Courbot <gnurou@...il.com>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>
Subject: Re: [PATCH] gpio: aspeed: Add debounce support

On Wed, 2017-02-22 at 14:46 +1030, Joel Stanley wrote:
> > On Tue, Feb 21, 2017 at 1:10 AM, Andrew Jeffery <andrew@...id.au> 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@...id.au>
> 
> 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 :)

Right - so we have up to 232-ish GPIOs (AST2500), any of which might
need debouncing.  Chances are only a few will ever configure debouncing
and I didn't want to allocate an array to map the offset to the timer
index it was using. 

However, I should have actually done the calculations.

Assuming we just use u8s for the timer indexes that would give us 232
bytes in the naive case of just allocating a 232 element array.

A bucket exponent 5 of here will give us 32 buckets, which wind up
being pointers to struct hlist_node.  This then already claims 128
bytes of space on our 32bit ARM chip, and each bucket entry weighs 16
bytes as it stands. Therefore if debounce is configured for only 8
gpios we reach the space usage of the naive case of allocating 232 u8s.

We could compress struct aspeed_debounce_timer a little bit by making
timer and offset u8s, which gives 10 bytes and buys us another 4 gpios
before hitting the naive case size, but I don't think that's a huge
win.

I think I've talked myself into just using the naive u8 array approach.
Thoughts?

> 
> > +       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"?

Yes.

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

So you want something like:

    bool disable_debounce = (usecs == 0);
    bool enable_debounce = !disable_debounce;

    if (enable_debounce) {
        ...;
    }
    if (disable_debounce) {
        ...;
    }

That seems a little verbose? What about just a comment? Zero meaning
disabled came from the interface documentation.

> 
> > +               struct aspeed_debounce_timer *dt, *newdt;
> 
> I thought this was device tree and got really confused. Perhaps 'timer' instead?

Sure

> 
> > +               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)?

As in dev_warn()? Yeah, that would probably be a good idea.

> 
> > +
> > +               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?

Well, the index of a GPIO pin in the context of the current controller,
but yes, effectively 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.

Yeah, bailing out is a better move. I'll also add a formatted message.

> 
> > +               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?

Well it's not necessarily setting it again. The bit value depends on
which timer was selected.

> 
> We're pretty deep in macro land by now.

The mapping between the timer index, the timer's configuration value
and the register bit pattern of the configuration value is pretty
horrid. Better to hide the ugly calculation in a macro than open-coding 
it here, especially as we need to do it twice (again right below), but
slightly differently each time.

Cheers,

Andrew

> 
> > +
> > +               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);
Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)

Powered by blists - more mailing lists