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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Jun 10, 2018 at 07:04:56PM +1000, NeilBrown wrote:
> On Sat, Jun 09 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@xxxxxxxxx>
> > ---
> >  drivers/staging/mt7621-gpio/gpio-mt7621.c | 57 +++++++++++++++++++++++++------
> >  1 file changed, 46 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > index 5d082c8..b0cbb30 100644
> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > @@ -37,6 +37,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;
> > @@ -44,6 +46,8 @@ struct mtk_gc {
> >  	int bank;
> >  	u32 rising;
> >  	u32 falling;
> > +	u32 hlevel;
> > +	u32 llevel;
> >  };
> >  
> >  /**
> > @@ -114,7 +118,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;
> > @@ -122,10 +126,16 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
> >  	spin_lock_irqsave(&rg->lock, flags);
> >  	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> >  	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> > +	high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank));
> > +	low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank));
> >  	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank),
> >  		     rise | (PIN_MASK(pin) & rg->rising));
> >  	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank),
> >  		     fall | (PIN_MASK(pin) & rg->falling));
> > +	mtk_gpio_w32(rg, GPIO_REG_HLVL(bank),
> > +		     high | (PIN_MASK(pin) & rg->hlevel));
> > +	mtk_gpio_w32(rg, GPIO_REG_LLVL(bank),
> > +		     low | (PIN_MASK(pin) & rg->llevel));
> >  	spin_unlock_irqrestore(&rg->lock, flags);
> >  }
> >  
> > @@ -137,7 +147,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;
> > @@ -145,8 +155,12 @@ mediatek_gpio_irq_mask(struct irq_data *d)
> >  	spin_lock_irqsave(&rg->lock, flags);
> >  	rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> >  	fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> > +	high = mtk_gpio_r32(rg, GPIO_REG_HLVL(bank));
> > +	low = mtk_gpio_r32(rg, GPIO_REG_LLVL(bank));
> >  	mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin));
> >  	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin));
> > +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), high & ~PIN_MASK(pin));
> > +	mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), low & ~PIN_MASK(pin));
> >  	spin_unlock_irqrestore(&rg->lock, flags);
> >  }
> >  
> > @@ -163,21 +177,42 @@ 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);
> >  	}
> >  
> > -	if (type & IRQ_TYPE_EDGE_RISING)
> > +	switch (type) {
> > +	case IRQ_TYPE_EDGE_RISING:
> >  		rg->rising |= mask;
> > -	else
> > -		rg->rising &= ~mask;
> > -
> > -	if (type & IRQ_TYPE_EDGE_FALLING)
> > -		rg->falling |= mask;
> > -	else
> >  		rg->falling &= ~mask;
> > +		rg->hlevel &= ~mask;
> > +		rg->llevel &= ~mask;
> > +		break;
> > +	case IRQ_TYPE_EDGE_FALLING:
> > +		rg->falling |= mask;
> > +		rg->rising &= ~mask;
> > +		rg->hlevel &= ~mask;
> > +		rg->llevel &= ~mask;
> > +		break;
> > +	case IRQ_TYPE_LEVEL_HIGH:
> > +		rg->hlevel |= mask;
> > +		rg->rising &= ~mask;
> > +		rg->falling &= mask;
> > +		rg->llevel &= ~mask;
> > +		break;
> > +	case IRQ_TYPE_LEVEL_LOW:
> > +		rg->llevel |= mask;
> > +		rg->rising &= ~mask;
> > +		rg->falling &= mask;
> > +		rg->hlevel &= ~mask;
> > +		break;
> 
> Changing this to a switch statement is definitely the wrong thing to do.
> Various combinations of bits are possible.
> It only makes sense to have HIGH *or* LOW, but it makes lots of sense to
> have both RISING and FALLING.

Yes, you are right. I'll fix up this to allow both RISING and FALLING as
it was before.

> It probably doesn't make sense to have both a LEVEL and an EDGE, but I'm
> not certain.

The chip support both so it seems that it must be also implemented.

> 
> Thanks,
> NeilBrown

Best regards,
    Sergio Paracuellos

> 
> > +	default:
> > +		return -EINVAL;
> > +	}
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.7.4


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



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux