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

[Alexandre Torgue]

Hi Marc

On 2/19/20 1:07 PM, Marc Zyngier wrote:
> 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".

Ok I'll change in v3

> > > 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).

I agree. No need to have u32.

> > > Â };
> > > ÂÂÂ 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/

correct

> > > +};
> > > +
> > > +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.

Ok.

> > > +
> > > +ÂÂÂ 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.