| Message ID | 1491577811-26989-2-git-send-email-alexandre.torgue@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 7, 2017 at 5:10 PM, Alexandre TORGUE <alexandre.torgue@st.com> wrote: > This patch ensures that pin is correctly set as gpio input when it is used > as an interrupt. > > Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com> (...) > +static int stm32_gpio_irq_request_resources(struct irq_data *irq_data) > +{ > + struct stm32_gpio_bank *bank = irq_data->domain->host_data; > + u32 ret; > + > + if (!gpiochip_is_requested(&bank->gpio_chip, irq_data->hwirq)) { > + ret = stm32_gpio_request(&bank->gpio_chip, irq_data->hwirq); > + if (ret) > + return ret; > + } This is wrong. You should only use gpiochip_lock_as_irq(), because of the following in Documentation/gpio/driver.txt: --------------- It is legal for any IRQ consumer to request an IRQ from any irqchip no matter if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and irq_chip are orthogonal, and offering their services independent of each other. (...) So always prepare the hardware and make it ready for action in respective callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having been called first. This orthogonality leads to ambiguities that we need to solve: if there is competition inside the subsystem which side is using the resource (a certain GPIO line and register for example) it needs to deny certain operations and keep track of usage inside of the gpiolib subsystem. This is why the API below exists. Locking IRQ usage ----------------- Input GPIOs can be used as IRQ signals. When this happens, a driver is requested to mark the GPIO as being used as an IRQ: int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock is released: void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset) When implementing an irqchip inside a GPIO driver, these two functions should typically be called in the .startup() and .shutdown() callbacks from the irqchip. When using the gpiolib irqchip helpers, these callback are automatically assigned. -------------- It is because of easy to make errors like this that I prefer that people try to use GPIOLIB_IRQCHIP helpers insteaf of rolling their own irqchip code. Yours, Linus Walleij
Hi Linus, On 04/24/2017 02:36 PM, Linus Walleij wrote: > On Fri, Apr 7, 2017 at 5:10 PM, Alexandre TORGUE > <alexandre.torgue@st.com> wrote: > >> This patch ensures that pin is correctly set as gpio input when it is used >> as an interrupt. >> >> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com> > (...) > >> +static int stm32_gpio_irq_request_resources(struct irq_data *irq_data) >> +{ >> + struct stm32_gpio_bank *bank = irq_data->domain->host_data; >> + u32 ret; >> + >> + if (!gpiochip_is_requested(&bank->gpio_chip, irq_data->hwirq)) { >> + ret = stm32_gpio_request(&bank->gpio_chip, irq_data->hwirq); >> + if (ret) >> + return ret; >> + } > > This is wrong. You should only use gpiochip_lock_as_irq(), because of the > following in Documentation/gpio/driver.txt: > > --------------- > It is legal for any IRQ consumer to request an IRQ from any irqchip no matter > if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and > irq_chip are orthogonal, and offering their services independent of each > other. > (...) > So always prepare the hardware and make it ready for action in respective > callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having > been called first. Ok, so actually the action to set pin in input mode is necessary in stm32_gpio_irq_request_resources. I'll fix it in V2. > > This orthogonality leads to ambiguities that we need to solve: if there is > competition inside the subsystem which side is using the resource (a certain > GPIO line and register for example) it needs to deny certain operations and > keep track of usage inside of the gpiolib subsystem. This is why the API > below exists. > > Locking IRQ usage > ----------------- > Input GPIOs can be used as IRQ signals. When this happens, a driver is requested > to mark the GPIO as being used as an IRQ: > > int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) > > This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock > is released: > > void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset) > > When implementing an irqchip inside a GPIO driver, these two functions should > typically be called in the .startup() and .shutdown() callbacks from the > irqchip. > > When using the gpiolib irqchip helpers, these callback are automatically > assigned. > -------------- > > It is because of easy to make errors like this that I prefer that people try > to use GPIOLIB_IRQCHIP helpers insteaf of rolling their own irqchip code. I understand. It was difficult in our case due to design. Thanks for review. Alex > > Yours, > Linus Walleij >
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c index abc405b..c8825e5 100644 --- a/drivers/pinctrl/stm32/pinctrl-stm32.c +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c @@ -207,12 +207,35 @@ static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) .to_irq = stm32_gpio_to_irq, }; +static int stm32_gpio_irq_request_resources(struct irq_data *irq_data) +{ + struct stm32_gpio_bank *bank = irq_data->domain->host_data; + u32 ret; + + if (!gpiochip_is_requested(&bank->gpio_chip, irq_data->hwirq)) { + ret = stm32_gpio_request(&bank->gpio_chip, irq_data->hwirq); + if (ret) + return ret; + } + + return stm32_gpio_direction_input(&bank->gpio_chip, irq_data->hwirq); +} + +static void stm32_gpio_irq_release_resources(struct irq_data *irq_data) +{ + struct stm32_gpio_bank *bank = irq_data->domain->host_data; + + stm32_gpio_free(&bank->gpio_chip, irq_data->hwirq); +} + static struct irq_chip stm32_gpio_irq_chip = { .name = "stm32gpio", .irq_eoi = irq_chip_eoi_parent, .irq_mask = irq_chip_mask_parent, .irq_unmask = irq_chip_unmask_parent, .irq_set_type = irq_chip_set_type_parent, + .irq_request_resources = stm32_gpio_irq_request_resources, + .irq_release_resources = stm32_gpio_irq_release_resources, }; static int stm32_gpio_domain_translate(struct irq_domain *d,
This patch ensures that pin is correctly set as gpio input when it is used as an interrupt. Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>