Re: [PATCH v2 7/7] staging: mt7621-gpio: implement high level and low level irqs

[NeilBrown <neil@brown.name>]

[-- Attachment #1.1: Type: text/plain, Size: 5249 bytes --]

On Mon, Jun 11 2018, Sergio Paracuellos wrote:

> This chip support high level and low level interrupts. Those
> have to be implemented also to get a complete and clean driver.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/staging/mt7621-gpio/gpio-mt7621.c | 51 +++++++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 54c18c1..39874cb 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -38,6 +38,8 @@
>   * @bank: gpio bank number for the chip
>   * @rising: mask for rising irqs
>   * @falling: mask for falling irqs
> + * @hlevel: mask for high level irqs
> + * @llevel: mask for low level irqs
>   */
>  struct mtk_gc {
>  	struct gpio_chip chip;
> @@ -45,6 +47,8 @@ struct mtk_gc {
>  	int bank;
>  	u32 rising;
>  	u32 falling;
> +	u32 hlevel;
> +	u32 llevel;
>  };
>  
>  /**
> @@ -184,7 +188,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
>  	int bank = pin / MTK_BANK_WIDTH;
>  	struct mtk_gc *rg = &gpio_data->gc_map[bank];
>  	unsigned long flags;
> -	u32 rise, fall;
> +	u32 rise, fall, high, low;
>  
>  	if (!rg)
>  		return;
> @@ -192,8 +196,12 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
>  	spin_lock_irqsave(&rg->lock, flags);
>  	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
>  	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> +	high = mtk_gpio_r32(rg, GPIO_REG_HLVL);
> +	low = mtk_gpio_r32(rg, GPIO_REG_LLVL);
>  	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
>  	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
> +	mtk_gpio_w32(rg, GPIO_REG_HLVL, high | (PIN_MASK(pin) & rg->hlevel));
> +	mtk_gpio_w32(rg, GPIO_REG_LLVL, low | (PIN_MASK(pin) & rg->llevel));
>  	spin_unlock_irqrestore(&rg->lock, flags);
>  }
>  
> @@ -205,7 +213,7 @@ mediatek_gpio_irq_mask(struct irq_data *d)
>  	int bank = pin / MTK_BANK_WIDTH;
>  	struct mtk_gc *rg = &gpio_data->gc_map[bank];
>  	unsigned long flags;
> -	u32 rise, fall;
> +	u32 rise, fall, high, low;
>  
>  	if (!rg)
>  		return;
> @@ -213,8 +221,12 @@ mediatek_gpio_irq_mask(struct irq_data *d)
>  	spin_lock_irqsave(&rg->lock, flags);
>  	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
>  	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> +	high = mtk_gpio_r32(rg, GPIO_REG_HLVL);
> +	low = mtk_gpio_r32(rg, GPIO_REG_LLVL);
>  	mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
>  	mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
> +	mtk_gpio_w32(rg, GPIO_REG_HLVL, high & ~PIN_MASK(pin));
> +	mtk_gpio_w32(rg, GPIO_REG_LLVL, low & ~PIN_MASK(pin));
>  	spin_unlock_irqrestore(&rg->lock, flags);
>  }
>  
> @@ -231,21 +243,34 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
>  		return -1;
>  
>  	if (type == IRQ_TYPE_PROBE) {
> -		if ((rg->rising | rg->falling) & mask)
> +		if ((rg->rising | rg->falling |
> +		     rg->hlevel | rg->llevel) & mask)
>  			return 0;
>  
> -		type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
> +		type = (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> +			IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
>  	}

This doesn't look right.
IRQ_TYPE_PROBE isn't very well documented and there aren't a lot of
examples of usage, but I think the idea is that if the interrupt type
is already set, then leave it along, otherwise choose a sane default,
which is IRQ_TYPE_EDGE_BOTH
(aka IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING) in all the cases
that I've looked at.

Certainly setting the type to RISING and FALLING and LOW and HIGH cannot
be right as that would cause constant interrupts.

>  
> -	if (type & IRQ_TYPE_EDGE_RISING)
> -		rg->rising |= mask;
> -	else
> -		rg->rising &= ~mask;
> -
> -	if (type & IRQ_TYPE_EDGE_FALLING)
> -		rg->falling |= mask;
> -	else
> -		rg->falling &= ~mask;
> +	if (type & IRQ_TYPE_EDGE_RISING ||
> +	    type & IRQ_TYPE_EDGE_FALLING) {
> +		if (type & IRQ_TYPE_EDGE_RISING)
> +			rg->rising |= mask;
> +		else
> +			rg->rising &= ~mask;
> +
> +		if (type & IRQ_TYPE_EDGE_FALLING)
> +			rg->falling |= mask;
> +		else
> +			rg->falling &= ~mask;
> +	} else {
> +		if (type & IRQ_TYPE_LEVEL_HIGH) {
> +			rg->hlevel |= mask;
> +			rg->llevel &= ~mask;
> +		} else {
> +			rg->llevel |= mask;
> +			rg->hlevel &= ~mask;
> +		}
> +	}

I wonder if we should be clearing the mask bit for hlevel and llevel
when setting either edge - and clearing the edge bits when setting a
level.

Actually, you might have been right about using a switch statement.
Several drivers have
  switch (type & IRQ_TYPE_SENSE_MASK) {
or similar.
The cope with the possiblity that both RISING and FALLING are set, they
have a case for IRQ_TYPE_EDGE_BOTH.
Maybe do
 rg->rising &= ~mask;
 rg->falling &= ~mask;
 rg->hlevel &= ~mask;
 rg->llevel &= ~mask;
 switch (type & IRQ_TYPE_SENSE_MASK) {
 case IRQ_TYPE_EDGE_BOTH:
   rg->rising |= mask;
   rg->falling |= mask;
   break;
 ....
 }


The current code works for my test-cases, but maybe it could be better.

Thanks,
NeilBrown

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel