[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <53f72a8b241da3032a42b80c86b7c6ab@kernel.org>
Date: Wed, 19 Feb 2020 12:07:15 +0000
From: Marc Zyngier <maz@...nel.org>
To: Alexandre Torgue <alexandre.torgue@...com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>,
Linus Walleij <linus.walleij@...aro.org>, marex@...x.de,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to
gpio irq chip
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@...com>
>> Tested-by: Marek Vasut <marex@...x.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).
>> };
>> 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...
Powered by blists - more mailing lists