| Message ID | 1410859335-11080-4-git-send-email-alvin.chen@intel.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, 2014-09-16 at 02:22 -0700, Weike Chen wrote: > This patch enables 'debounce' for the designware GPIO, and > it is based on Josef Ahmad's previous work. > > Reviewed-by: Hock Leong Kweh <hock.leong.kweh@intel.com> > Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com> This line is wrong. How come? Couple of minor comments below, and after addressing them, please, use Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Weike Chen <alvin.chen@intel.com> > --- > drivers/gpio/gpio-dwapb.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 534945c..9a76e3c 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -37,6 +37,7 @@ > #define GPIO_INTTYPE_LEVEL 0x38 > #define GPIO_INT_POLARITY 0x3c > #define GPIO_INTSTATUS 0x40 > +#define GPIO_PORTA_DEBOUNCE 0x48 > #define GPIO_PORTA_EOI 0x4c > #define GPIO_EXT_PORTA 0x50 > #define GPIO_EXT_PORTB 0x54 > @@ -235,6 +236,29 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) > return 0; > } > > +static int dwapb_gpio_set_debounce(struct gpio_chip *gc, > + unsigned offset, unsigned debounce) > +{ > + struct bgpio_chip *bgc = to_bgpio_chip(gc); > + struct dwapb_gpio_port *port = > + container_of(bgc, struct dwapb_gpio_port, bgc); Does it make sense to introduce an inline helper like static inline struct dwapb_gpio_port *to_dwapb_gpio_port(struct bgpio_chip *gc) {...} ? There is also another place where it could be used. > + struct dwapb_gpio *gpio = port->gpio; > + unsigned long flags, val_deb; > + unsigned long mask = bgc->pin2mask(bgc, offset); > + > + spin_lock_irqsave(&bgc->lock, flags); > + > + val_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE); > + if (debounce) > + dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, mask | val_deb); May you put value on the first place? Like 'val_deb | mask'. > + else > + dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, ~mask & val_deb); Ditto. > + > + spin_unlock_irqrestore(&bgc->lock, flags); > + > + return 0; > +} > + > static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) > { > u32 worked; > @@ -373,6 +397,10 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > port->bgc.gc.ngpio = pp->ngpio; > port->bgc.gc.base = pp->gpio_base; > > + /* Only port A support debounce */ > + if (pp->idx == 0) > + port->bgc.gc.set_debounce = dwapb_gpio_set_debounce; > + > if (pp->irq) > dwapb_configure_irqs(gpio, port, pp); >
> > + struct bgpio_chip *bgc = to_bgpio_chip(gc); > > + struct dwapb_gpio_port *port = > > + container_of(bgc, struct dwapb_gpio_port, bgc); > > Does it make sense to introduce an inline helper like > > static inline struct dwapb_gpio_port *to_dwapb_gpio_port(struct bgpio_chip > *gc) {...} > > ? OK. > There is also another place where it could be used. > > > + struct dwapb_gpio *gpio = port->gpio; > > + unsigned long flags, val_deb; > > + unsigned long mask = bgc->pin2mask(bgc, offset); > > + > > + spin_lock_irqsave(&bgc->lock, flags); > > + > > + val_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE); > > + if (debounce) > > + dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, mask | val_deb); > > May you put value on the first place? Like 'val_deb | mask'. OK. > > + else > > + dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, ~mask & val_deb); > > Ditto. > OK. > > +
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 534945c..9a76e3c 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -37,6 +37,7 @@ #define GPIO_INTTYPE_LEVEL 0x38 #define GPIO_INT_POLARITY 0x3c #define GPIO_INTSTATUS 0x40 +#define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 #define GPIO_EXT_PORTB 0x54 @@ -235,6 +236,29 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) return 0; } +static int dwapb_gpio_set_debounce(struct gpio_chip *gc, + unsigned offset, unsigned debounce) +{ + struct bgpio_chip *bgc = to_bgpio_chip(gc); + struct dwapb_gpio_port *port = + container_of(bgc, struct dwapb_gpio_port, bgc); + struct dwapb_gpio *gpio = port->gpio; + unsigned long flags, val_deb; + unsigned long mask = bgc->pin2mask(bgc, offset); + + spin_lock_irqsave(&bgc->lock, flags); + + val_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE); + if (debounce) + dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, mask | val_deb); + else + dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, ~mask & val_deb); + + spin_unlock_irqrestore(&bgc->lock, flags); + + return 0; +} + static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) { u32 worked; @@ -373,6 +397,10 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, port->bgc.gc.ngpio = pp->ngpio; port->bgc.gc.base = pp->gpio_base; + /* Only port A support debounce */ + if (pp->idx == 0) + port->bgc.gc.set_debounce = dwapb_gpio_set_debounce; + if (pp->irq) dwapb_configure_irqs(gpio, port, pp);