Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1584647 > unrolled thread
| Started by | Andrew Jeffery <andrew@aj.id.au> |
|---|---|
| First post | 2017-02-20 15:50 +0100 |
| Last post | 2017-02-23 02:00 +0100 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH] gpio: aspeed: Add debounce support Andrew Jeffery <andrew@aj.id.au> - 2017-02-20 15:50 +0100
Re: [PATCH] gpio: aspeed: Add debounce support Joel Stanley <joel@jms.id.au> - 2017-02-22 05:20 +0100
Re: [PATCH] gpio: aspeed: Add debounce support Joel Stanley <joel@jms.id.au> - 2017-02-22 14:50 +0100
Re: [PATCH] gpio: aspeed: Add debounce support Andrew Jeffery <andrew@aj.id.au> - 2017-02-23 02:00 +0100
| From | Andrew Jeffery <andrew@aj.id.au> |
|---|---|
| Date | 2017-02-20 15:50 +0100 |
| Subject | [PATCH] gpio: aspeed: Add debounce support |
| Message-ID | <tcXrQ-5Up-9@gated-at.bofh.it> |
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@aj.id.au>
---
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
@@ -9,14 +9,17 @@
* 2 of the License, or (at your option) any later version.
*/
-#include <linux/module.h>
-#include <linux/kernel.h>
+#include <asm/div64.h>
+#include <linux/clk.h>
+#include <linux/gpio/driver.h>
+#include <linux/hashtable.h>
#include <linux/init.h>
#include <linux/io.h>
-#include <linux/spinlock.h>
-#include <linux/platform_device.h>
-#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
struct aspeed_bank_props {
unsigned int bank;
@@ -29,59 +32,81 @@ struct aspeed_gpio_config {
const struct aspeed_bank_props *props;
};
+struct aspeed_debounce_timer {
+ unsigned int offset;
+ unsigned int timer;
+ struct hlist_node node;
+};
+
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);
+ unsigned int timer_users[3];
+ struct clk *clk;
};
struct aspeed_gpio_bank {
uint16_t val_regs;
uint16_t irq_regs;
+ uint16_t debounce_regs;
const char names[4][3];
};
+static const int debounce_timers[3] = { 0x50, 0x54, 0x58 };
+
static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
{
.val_regs = 0x0000,
.irq_regs = 0x0008,
+ .debounce_regs = 0x0040,
.names = { "A", "B", "C", "D" },
},
{
.val_regs = 0x0020,
.irq_regs = 0x0028,
+ .debounce_regs = 0x0048,
.names = { "E", "F", "G", "H" },
},
{
.val_regs = 0x0070,
.irq_regs = 0x0098,
+ .debounce_regs = 0x00b0,
.names = { "I", "J", "K", "L" },
},
{
.val_regs = 0x0078,
.irq_regs = 0x00e8,
+ .debounce_regs = 0x0100,
.names = { "M", "N", "O", "P" },
},
{
.val_regs = 0x0080,
.irq_regs = 0x0118,
+ .debounce_regs = 0x0130,
.names = { "Q", "R", "S", "T" },
},
{
.val_regs = 0x0088,
.irq_regs = 0x0148,
+ .debounce_regs = 0x0160,
.names = { "U", "V", "W", "X" },
},
{
.val_regs = 0x01E0,
.irq_regs = 0x0178,
+ .debounce_regs = 0x0190,
.names = { "Y", "Z", "AA", "AB" },
},
{
.val_regs = 0x01E8,
.irq_regs = 0x01A8,
+ .debounce_regs = 0x01c0,
.names = { "AC", "", "", "" },
},
};
@@ -99,6 +124,13 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
#define GPIO_IRQ_TYPE2 0x0c
#define GPIO_IRQ_STATUS 0x10
+#define GPIO_DEBOUNCE_SEL1 0x00
+#define GPIO_DEBOUNCE_SEL2 0x04
+
+#define _GPIO_SET_DEBOUNCE(t, o, i) ((!!((t + 1) & BIT(i))) << GPIO_OFFSET(o))
+#define GPIO_SET_DEBOUNCE1(t, o) _GPIO_SET_DEBOUNCE(t, o, 1)
+#define GPIO_SET_DEBOUNCE2(t, o) _GPIO_SET_DEBOUNCE(t, o, 0)
+
static const struct aspeed_gpio_bank *to_bank(unsigned int offset)
{
unsigned int bank = GPIO_BANK(offset);
@@ -144,6 +176,7 @@ static inline bool have_input(struct aspeed_gpio *gpio, unsigned int offset)
}
#define have_irq(g, o) have_input((g), (o))
+#define have_debounce(g, o) have_input((g), (o))
static inline bool have_output(struct aspeed_gpio *gpio, unsigned int offset)
{
@@ -506,6 +539,200 @@ static void aspeed_gpio_free(struct gpio_chip *chip, unsigned int offset)
pinctrl_free_gpio(chip->base + offset);
}
+static inline void __iomem *bank_debounce_reg(struct aspeed_gpio *gpio,
+ const struct aspeed_gpio_bank *bank,
+ unsigned int reg)
+{
+ return gpio->base + bank->debounce_regs + reg;
+}
+
+static inline int usecs_to_cycles(struct aspeed_gpio *gpio, unsigned long usecs,
+ u32 *cycles)
+{
+ u64 rate;
+ u64 n;
+
+ rate = clk_get_rate(gpio->clk);
+ if (!rate)
+ return -ENODEV;
+
+ n = rate * usecs;
+ do_div(n, 1000000);
+
+ if (n >= U32_MAX)
+ return -ERANGE;
+
+ /* At least as long as the requested time */
+ *cycles = n + 1;
+
+ return 0;
+}
+
+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) {
+ struct aspeed_debounce_timer *dt, *newdt;
+ 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;
+
+ 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 */
+ 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);
+
+ WARN_ON(gpio->timer_users[i] == UINT_MAX);
+ 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);
+
+ 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);
+ } else {
+ struct aspeed_debounce_timer *dt;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ /* Unregister any record of timer use by @offset */
+ hash_for_each_possible(gpio->offset_timer, dt, node, offset) {
+ if (offset == dt->offset) {
+ WARN_ON(!gpio->timer_users[dt->timer]);
+
+ gpio->timer_users[dt->timer]--;
+ hash_del(&dt->node);
+ devm_kfree(chip->parent, dt);
+ break;
+ }
+ }
+
+ addr = bank_debounce_reg(gpio, bank, GPIO_DEBOUNCE_SEL1);
+ val = ioread32(addr);
+ iowrite32((val & ~mask), addr);
+
+ addr = bank_debounce_reg(gpio, bank, GPIO_DEBOUNCE_SEL2);
+ val = ioread32(addr);
+ iowrite32((val & ~mask), addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+ }
+
+ return rc;
+
+err:
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return rc;
+}
+
+static int aspeed_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+ unsigned long config)
+{
+ unsigned long param = pinconf_to_config_param(config);
+ u32 arg = pinconf_to_config_argument(config);
+ int rc;
+
+ switch (param) {
+ case PIN_CONFIG_INPUT_DEBOUNCE:
+ rc = aspeed_gpio_set_debounce(chip, offset, arg);
+ break;
+ case PIN_CONFIG_BIAS_DISABLE:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ rc = pinctrl_gpio_set_config(offset, config);
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+ case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+ /* Fallthrough for emulation, as per datasheet */
+ default:
+ rc = -ENOTSUPP;
+ }
+
+ return rc;
+}
+
/*
* Any banks not specified in a struct aspeed_bank_props array are assumed to
* have the properties:
@@ -560,13 +787,19 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
return PTR_ERR(gpio->base);
spin_lock_init(&gpio->lock);
+ hash_init(gpio->offset_timer);
gpio_id = of_match_node(aspeed_gpio_of_table, pdev->dev.of_node);
if (!gpio_id)
return -EINVAL;
+ gpio->clk = of_clk_get(pdev->dev.of_node, 0);
+ if (IS_ERR(gpio->clk))
+ return PTR_ERR(gpio->clk);
+
gpio->config = gpio_id->data;
+ gpio->chip.parent = &pdev->dev;
gpio->chip.ngpio = gpio->config->nr_gpios;
gpio->chip.parent = &pdev->dev;
gpio->chip.direction_input = aspeed_gpio_dir_in;
@@ -576,6 +809,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
gpio->chip.free = aspeed_gpio_free;
gpio->chip.get = aspeed_gpio_get;
gpio->chip.set = aspeed_gpio_set;
+ gpio->chip.set_config = aspeed_gpio_set_config;
gpio->chip.label = dev_name(&pdev->dev);
gpio->chip.base = -1;
gpio->chip.irq_need_valid_mask = true;
--
2.9.3
[toc] | [next] | [standalone]
| From | Joel Stanley <joel@jms.id.au> |
|---|---|
| Date | 2017-02-22 05:20 +0100 |
| Message-ID | <tdwzf-4i1-1@gated-at.bofh.it> |
| In reply to | #1584647 |
On Tue, Feb 21, 2017 at 1:10 AM, Andrew Jeffery <andrew@aj.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@aj.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 :)
> + 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);
[toc] | [prev] | [next] | [standalone]
| From | Joel Stanley <joel@jms.id.au> |
|---|---|
| Date | 2017-02-22 14:50 +0100 |
| Message-ID | <tdFsR-2p2-7@gated-at.bofh.it> |
| In reply to | #1585914 |
On Wed, Feb 22, 2017 at 2:46 PM, Joel Stanley <joel@jms.id.au> 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);
[toc] | [prev] | [next] | [standalone]
| From | Andrew Jeffery <andrew@aj.id.au> |
|---|---|
| Date | 2017-02-23 02:00 +0100 |
| Message-ID | <tdPVg-1JC-3@gated-at.bofh.it> |
| In reply to | #1585914 |
[Multipart message — attachments visible in raw view] — view raw
On Wed, 2017-02-22 at 14:46 +1030, Joel Stanley wrote:
> > On Tue, Feb 21, 2017 at 1:10 AM, Andrew Jeffery <andrew@aj.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@aj.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);
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web