| Message ID | 1409161588-19417-3-git-send-email-alvin.chen@intel.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, 27 Aug 2014, 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> > Signed-off-by: Weike Chen <alvin.chen@intel.com> > --- > drivers/gpio/gpio-dwapb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 155a64b..e0ab988 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -36,6 +36,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 > @@ -64,6 +65,23 @@ struct dwapb_gpio { > const struct dwapb_gpio_platform_data *pdata; > }; > > +static inline u32 dwapb_read(struct dwapb_gpio *gpio, unsigned int offset) > +{ > + struct bgpio_chip *bgc = &gpio->ports[0].bgc; > + void __iomem *reg_base = gpio->regs; > + > + return bgc->read_reg(reg_base + offset); > +} > + > +static inline void dwapb_write(struct dwapb_gpio *gpio, unsigned int offset, > + u32 val) > +{ > + struct bgpio_chip *bgc = &gpio->ports[0].bgc; > + void __iomem *reg_base = gpio->regs; > + > + bgc->write_reg(reg_base + offset, val); > +} > + Hello, I don't understand the reason for adding dwapb_read and dwapb_write here. The rest of the driver is using readl and writel. I'd rather not see two different methods being used in the same driver for register access. Maybe I'm missing something, but if we need to add dwapb_read/write, then it should be used for all register access. Alan > static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset) > { > struct bgpio_chip *bgc = to_bgpio_chip(gc); > @@ -209,6 +227,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) > { > struct irq_desc *desc = irq_to_desc(irq); > @@ -342,6 +383,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > > port->bgc.gc.ngpio = pp->ngpio; > port->bgc.gc.base = pp->gpio_base; > + port->bgc.gc.set_debounce = dwapb_gpio_set_debounce; > > /* > * Only port A can provide interrupts in all configurations of the IP. > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-08-28 at 10:11 -0500, atull wrote: > On Wed, 27 Aug 2014, Weike Chen wrote: [] > > +static inline u32 dwapb_read(struct dwapb_gpio *gpio, unsigned int offset) > > +{ > > + struct bgpio_chip *bgc = &gpio->ports[0].bgc; > > + void __iomem *reg_base = gpio->regs; > > + > > + return bgc->read_reg(reg_base + offset); > > +} > > + > > +static inline void dwapb_write(struct dwapb_gpio *gpio, unsigned int offset, > > + u32 val) > > +{ > > + struct bgpio_chip *bgc = &gpio->ports[0].bgc; > > + void __iomem *reg_base = gpio->regs; > > + > > + bgc->write_reg(reg_base + offset, val); > > +} > > + > > Hello, > > I don't understand the reason for adding dwapb_read and dwapb_write here. > The rest of the driver is using readl and writel. I'd rather not see two > different methods being used in the same driver for register access. > Maybe I'm missing something, but if we need to add dwapb_read/write, then > it should be used for all register access. Alan, it was my proposal. I rather agree that is better to convert all accesses to that. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
> > > > I don't understand the reason for adding dwapb_read and dwapb_write here. > > The rest of the driver is using readl and writel. I'd rather not see > > two different methods being used in the same driver for register access. > > Maybe I'm missing something, but if we need to add dwapb_read/write, > > then it should be used for all register access. > > Alan, it was my proposal. I rather agree that is better to convert all accesses to > that. OK, I will convert all readl&writel to dwapb_read&dwapb_write.
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 155a64b..e0ab988 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -36,6 +36,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 @@ -64,6 +65,23 @@ struct dwapb_gpio { const struct dwapb_gpio_platform_data *pdata; }; +static inline u32 dwapb_read(struct dwapb_gpio *gpio, unsigned int offset) +{ + struct bgpio_chip *bgc = &gpio->ports[0].bgc; + void __iomem *reg_base = gpio->regs; + + return bgc->read_reg(reg_base + offset); +} + +static inline void dwapb_write(struct dwapb_gpio *gpio, unsigned int offset, + u32 val) +{ + struct bgpio_chip *bgc = &gpio->ports[0].bgc; + void __iomem *reg_base = gpio->regs; + + bgc->write_reg(reg_base + offset, val); +} + static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset) { struct bgpio_chip *bgc = to_bgpio_chip(gc); @@ -209,6 +227,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) { struct irq_desc *desc = irq_to_desc(irq); @@ -342,6 +383,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, port->bgc.gc.ngpio = pp->ngpio; port->bgc.gc.base = pp->gpio_base; + port->bgc.gc.set_debounce = dwapb_gpio_set_debounce; /* * Only port A can provide interrupts in all configurations of the IP.