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

From: Joel Stanley
Date: Wed Feb 22 2017 - 08:49:44 EST


On Wed, Feb 22, 2017 at 2:46 PM, Joel Stanley <joel@xxxxxxxxx> wrote:
>> + /*
>> + * 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?

I realised later that the if i == ARRAY_SIZE test below will always be
false when the break was hit, so in effect we are doing a goto
"Register @offset as a user of timer i".

I think adding a goto would make it easier to read.

If you're not a fan then perhaps tweak the comment?

Cheers,

Joel

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