Messages in this thread | | | Subject | Re: [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip | From | Alexandre Torgue <> | Date | Wed, 19 Feb 2020 13:59:09 +0100 |
| |
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@st.com> >>> Tested-by: Marek Vasut <marex@denx.de> >>> >>> 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.
| |