Re: [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip

[Marc Zyngier]

On 2020-02-19 11:34, Alexandre Torgue wrote:
> Fix Marc email address.
>
> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
> > This patch adds level interrupt support to gpio irq chip.

A commit message should not contain "this patch".

> > GPIO hardware block is directly linked to EXTI block but EXTI handles
> > external interrupts only on edge. To be able to handle GPIO interrupt 
> > on
> > level a "hack" is done in gpio irq chip: parent interrupt (exti irq 
> > chip)
> > is retriggered following interrupt type and gpio line value.
> >
> > Signed-off-by: Alexandre Torgue <alexandre.torgue@xxxxxx>
> > Tested-by: Marek Vasut <marex@xxxxxxx>
> >
> > diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c 
> > b/drivers/pinctrl/stm32/pinctrl-stm32.c
> > index 2d5e0435af0a..dae236562543 100644
> > --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> > +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> > @@ -92,6 +92,7 @@ struct stm32_gpio_bank {
> >   	u32 bank_nr;
> >   	u32 bank_ioport_nr;
> >   	u32 pin_backup[STM32_GPIO_PINS_PER_BANK];
> > +	u32 irq_type[STM32_GPIO_PINS_PER_BANK];

Do you really need a u32 here? an array of u8 seems enough. After all,
you only need two bits of information per interrupts (level or not,
low or high).

> >   };
> >     struct stm32_pinctrl {
> > @@ -303,6 +304,46 @@ static const struct gpio_chip stm32_gpio_template 
> > = {
> >   	.get_direction		= stm32_gpio_get_direction,
> >   };
> >   +void stm32_gpio_irq_eoi(struct irq_data *d)
> > +{
> > +	struct stm32_gpio_bank *bank = d->domain->host_data;
> > +	int line;
> > +
> > +	irq_chip_eoi_parent(d);
> > +
> > +	/* If level interrupt type then retrig */
> > +	line = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
> > +	if ((line == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
> > +	    (line == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
> > +		irq_chip_retrigger_hierarchy(d);

s/line/level/

> > +};
> > +
> > +static int stm32_gpio_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +	struct stm32_gpio_bank *bank = d->domain->host_data;
> > +	u32 parent_type;
> > +
> > +	bank->irq_type[d->hwirq] = type;

It would make more sense if this this assignment was done *after*
sanitizing the type value.

> > +
> > +	switch (type) {
> > +	case IRQ_TYPE_EDGE_RISING:
> > +	case IRQ_TYPE_EDGE_FALLING:
> > +	case IRQ_TYPE_EDGE_BOTH:
> > +		parent_type = type;
> > +		break;
> > +	case IRQ_TYPE_LEVEL_HIGH:
> > +		parent_type = IRQ_TYPE_EDGE_RISING;
> > +		break;
> > +	case IRQ_TYPE_LEVEL_LOW:
> > +		parent_type = IRQ_TYPE_EDGE_FALLING;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return irq_chip_set_type_parent(d, parent_type);
> > +};
> > +
> >   static int stm32_gpio_irq_request_resources(struct irq_data 
> > *irq_data)
> >   {
> >   	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
> > @@ -332,11 +373,11 @@ static void 
> > stm32_gpio_irq_release_resources(struct irq_data *irq_data)
> >     static struct irq_chip stm32_gpio_irq_chip = {
> >   	.name		= "stm32gpio",
> > -	.irq_eoi	= irq_chip_eoi_parent,
> > +	.irq_eoi	= stm32_gpio_irq_eoi,
> >   	.irq_ack	= irq_chip_ack_parent,
> >   	.irq_mask	= irq_chip_mask_parent,
> >   	.irq_unmask	= irq_chip_unmask_parent,
> > -	.irq_set_type	= irq_chip_set_type_parent,
> > +	.irq_set_type	= stm32_gpio_set_type,
> >   	.irq_set_wake	= irq_chip_set_wake_parent,
> >   	.irq_request_resources = stm32_gpio_irq_request_resources,
> >   	.irq_release_resources = stm32_gpio_irq_release_resources,

Thanks,

        M.
--
Jazz is not dead. It just smells funny...