linux-kernel - Re: [PATCH 3/4 v4] GPIO: gpio-dwapb: Support Debounce

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 16 Sep 2014 11:37:49 +0300
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Weike Chen <alvin.chen@...el.com>
Cc:	Linus Walleij <linus.walleij@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	atull <atull@...nsource.altera.com>, linux-gpio@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	Boon Leong Ong <boon.leong.ong@...el.com>,
	Hock Leong Kweh <hock.leong.kweh@...el.com>,
	Darren Hart <dvhart@...ux.intel.com>,
	Sebastian Andrzej Siewior <sebastian@...akpoint.cc>,
	Mika Westerberg <mika.westerberg@...el.com>,
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 3/4 v4] GPIO: gpio-dwapb: Support Debounce

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@...el.com>
> Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@...el.com>

This line is wrong. How come?

Couple of minor comments below, and after addressing them, please, use 
Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

> Signed-off-by: Weike Chen <alvin.chen@...el.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);
>  


-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/