| Message ID | CACh+v5Os_FHh39oXGt=srw0s7bVkpnpH0pQ4ugit_k5BL_k-0A@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: > Hello Nicolas, Jean-Christophe, > > As I was trying to enable the touchscreen on the at91sam9261ek with > device-tree support, I ran into an issue. The touchscreen driver needs > to know the state of the pendown gpio and also needs it as an > interrupt source. > > The problem is that when a gpio is used as an interrupt, it's > requested by the pinctrl driver during the xlate stage, marking it > unavaliable for the other driver. > It looks like the at91 pinctrl driver is the only one to use > gpio_request() in the xlate stage. Maybe we should remove this: You should only request it as a GPIO and then use gpio_to_irq to get the related IRQ. Because what is done here, is to solve the case where only the irq is request, and in this specific case we need to request the pin as a GPIO. Best Regards, Boris > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > index a7549c4..cf91a35 100644 > --- a/drivers/pinctrl/pinctrl-at91.c > +++ b/drivers/pinctrl/pinctrl-at91.c > @@ -1463,14 +1463,6 @@ static int at91_gpio_irq_domain_xlate(struct > irq_domain *d, > *out_hwirq = intspec[0]; > *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK; > > - ret = gpio_request(pin, ctrlr->full_name); > - if (ret) > - return ret; > - > - ret = gpio_direction_input(pin); > - if (ret) > - return ret; > - > return 0; > } > > Jean-Jacques
Hi Boris, 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: >> >> Hello Nicolas, Jean-Christophe, >> >> As I was trying to enable the touchscreen on the at91sam9261ek with >> device-tree support, I ran into an issue. The touchscreen driver needs >> to know the state of the pendown gpio and also needs it as an >> interrupt source. >> >> The problem is that when a gpio is used as an interrupt, it's >> requested by the pinctrl driver during the xlate stage, marking it >> unavaliable for the other driver. >> It looks like the at91 pinctrl driver is the only one to use >> gpio_request() in the xlate stage. Maybe we should remove this: > > > You should only request it as a GPIO and then use gpio_to_irq to get the > related IRQ. > Because what is done here, is to solve the case where only the irq > is request, and in this specific case we need to request the pin as a > GPIO. > That's what I did first, and was about to submit the patch for the touchscreen driver. However it doesn't feel right. Being able to get the state of a gpio that is also an interrupt seems very useful to me, not only for a touchscreen controller. I understand why it's being done here. It's a matter of being sure that the GPIO is an input and that it'll not be configured otherwise latter. But: 1) I'm wondering why the atmel pinctrl is the only one to do that. 2) I believe that configuration of the direction can be done by describing the GPIO in the DT. (pinctrl-at91.c line 592). 3) If all the GPIOs are described in the DT with a proper pinmux description, i believe the exclusion is also handled. It's probably not the case today though. > > Best Regards, > > Boris > >> >> diff --git a/drivers/pinctrl/pinctrl-at91.c >> b/drivers/pinctrl/pinctrl-at91.c >> index a7549c4..cf91a35 100644 >> --- a/drivers/pinctrl/pinctrl-at91.c >> +++ b/drivers/pinctrl/pinctrl-at91.c >> @@ -1463,14 +1463,6 @@ static int at91_gpio_irq_domain_xlate(struct >> irq_domain *d, >> *out_hwirq = intspec[0]; >> *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK; >> >> - ret = gpio_request(pin, ctrlr->full_name); >> - if (ret) >> - return ret; >> - >> - ret = gpio_direction_input(pin); >> - if (ret) >> - return ret; >> - >> return 0; >> } >> >> Jean-Jacques > >
On 11:29 Mon 13 Jan , Jean-Jacques Hiblot wrote: > Hello Nicolas, Jean-Christophe, > > As I was trying to enable the touchscreen on the at91sam9261ek with > device-tree support, I ran into an issue. The touchscreen driver needs > to know the state of the pendown gpio and also needs it as an > interrupt source. > > The problem is that when a gpio is used as an interrupt, it's > requested by the pinctrl driver during the xlate stage, marking it > unavaliable for the other driver. > It looks like the at91 pinctrl driver is the only one to use > gpio_request() in the xlate stage. Maybe we should remove this: > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > index a7549c4..cf91a35 100644 > --- a/drivers/pinctrl/pinctrl-at91.c > +++ b/drivers/pinctrl/pinctrl-at91.c > @@ -1463,14 +1463,6 @@ static int at91_gpio_irq_domain_xlate(struct > irq_domain *d, > *out_hwirq = intspec[0]; > *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK; > > - ret = gpio_request(pin, ctrlr->full_name); > - if (ret) > - return ret; > - > - ret = gpio_direction_input(pin); > - if (ret) > - return ret; > - Nack the gpio is request automaticaly as the driver NEVER need to known that is a GPIO Best Regards, J. > return 0; > } > > Jean-Jacques > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 12:05 Mon 13 Jan , Jean-Jacques Hiblot wrote: > Hi Boris, > > 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: > > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: > >> > >> Hello Nicolas, Jean-Christophe, > >> > >> As I was trying to enable the touchscreen on the at91sam9261ek with > >> device-tree support, I ran into an issue. The touchscreen driver needs > >> to know the state of the pendown gpio and also needs it as an > >> interrupt source. > >> > >> The problem is that when a gpio is used as an interrupt, it's > >> requested by the pinctrl driver during the xlate stage, marking it > >> unavaliable for the other driver. > >> It looks like the at91 pinctrl driver is the only one to use > >> gpio_request() in the xlate stage. Maybe we should remove this: > > > > > > You should only request it as a GPIO and then use gpio_to_irq to get the > > related IRQ. > > Because what is done here, is to solve the case where only the irq > > is request, and in this specific case we need to request the pin as a > > GPIO. > > > > That's what I did first, and was about to submit the patch for the > touchscreen driver. > However it doesn't feel right. Being able to get the state of a gpio > that is also an interrupt seems very useful to me, not only for a > touchscreen controller. > > I understand why it's being done here. It's a matter of being sure > that the GPIO is an input and that it'll not be configured otherwise > latter. > But: > 1) I'm wondering why the atmel pinctrl is the only one to do that. because this the only to start to do it right I had a very long discussion woth LinusW and Grant the Gpio need to stop to use gpio_to_irq & co for irq. I even send a proposition to do this work across the kernel t the CE-Linux for funding > 2) I believe that configuration of the direction can be done by > describing the GPIO in the DT. (pinctrl-at91.c line 592). No pinctrl is for pinmux ONLY not gpio direction & co Best Regards, J. > 3) If all the GPIOs are described in the DT with a proper pinmux > description, i believe the exclusion is also handled. It's probably > not the case today though. > > > > > Best Regards, > > > > Boris > > > >> > >> diff --git a/drivers/pinctrl/pinctrl-at91.c > >> b/drivers/pinctrl/pinctrl-at91.c > >> index a7549c4..cf91a35 100644 > >> --- a/drivers/pinctrl/pinctrl-at91.c > >> +++ b/drivers/pinctrl/pinctrl-at91.c > >> @@ -1463,14 +1463,6 @@ static int at91_gpio_irq_domain_xlate(struct > >> irq_domain *d, > >> *out_hwirq = intspec[0]; > >> *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK; > >> > >> - ret = gpio_request(pin, ctrlr->full_name); > >> - if (ret) > >> - return ret; > >> - > >> - ret = gpio_direction_input(pin); > >> - if (ret) > >> - return ret; > >> - > >> return 0; > >> } > >> > >> Jean-Jacques > > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > On 12:05 Mon 13 Jan , Jean-Jacques Hiblot wrote: >> Hi Boris, >> >> 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: >> > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: >> >> >> >> Hello Nicolas, Jean-Christophe, >> >> >> >> As I was trying to enable the touchscreen on the at91sam9261ek with >> >> device-tree support, I ran into an issue. The touchscreen driver needs >> >> to know the state of the pendown gpio and also needs it as an >> >> interrupt source. >> >> >> >> The problem is that when a gpio is used as an interrupt, it's >> >> requested by the pinctrl driver during the xlate stage, marking it >> >> unavaliable for the other driver. >> >> It looks like the at91 pinctrl driver is the only one to use >> >> gpio_request() in the xlate stage. Maybe we should remove this: >> > >> > >> > You should only request it as a GPIO and then use gpio_to_irq to get the >> > related IRQ. >> > Because what is done here, is to solve the case where only the irq >> > is request, and in this specific case we need to request the pin as a >> > GPIO. >> > >> >> That's what I did first, and was about to submit the patch for the >> touchscreen driver. >> However it doesn't feel right. Being able to get the state of a gpio >> that is also an interrupt seems very useful to me, not only for a >> touchscreen controller. >> >> I understand why it's being done here. It's a matter of being sure >> that the GPIO is an input and that it'll not be configured otherwise >> latter. >> But: >> 1) I'm wondering why the atmel pinctrl is the only one to do that. > > because this the only to start to do it right > I had a very long discussion woth LinusW and Grant the Gpio need to stop to > use gpio_to_irq & co for irq. How can you get the value of the gpio that is also an interrupt source then ? Can you give a short example? > > I even send a proposition to do this work across the kernel t the CE-Linux for > funding >> 2) I believe that configuration of the direction can be done by >> describing the GPIO in the DT. (pinctrl-at91.c line 592). > No pinctrl is for pinmux ONLY not gpio direction & co > > Best Regards, > J. > >> 3) If all the GPIOs are described in the DT with a proper pinmux >> description, i believe the exclusion is also handled. It's probably >> not the case today though. > >> >> > >> > Best Regards, >> > >> > Boris >> > >> >> >> >> diff --git a/drivers/pinctrl/pinctrl-at91.c >> >> b/drivers/pinctrl/pinctrl-at91.c >> >> index a7549c4..cf91a35 100644 >> >> --- a/drivers/pinctrl/pinctrl-at91.c >> >> +++ b/drivers/pinctrl/pinctrl-at91.c >> >> @@ -1463,14 +1463,6 @@ static int at91_gpio_irq_domain_xlate(struct >> >> irq_domain *d, >> >> *out_hwirq = intspec[0]; >> >> *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK; >> >> >> >> - ret = gpio_request(pin, ctrlr->full_name); >> >> - if (ret) >> >> - return ret; >> >> - >> >> - ret = gpio_direction_input(pin); >> >> - if (ret) >> >> - return ret; >> >> - >> >> return 0; >> >> } >> >> >> >> Jean-Jacques >> > >> > >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 14:04 Wed 15 Jan , Jean-Jacques Hiblot wrote: > 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > > On 12:05 Mon 13 Jan , Jean-Jacques Hiblot wrote: > >> Hi Boris, > >> > >> 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: > >> > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: > >> >> > >> >> Hello Nicolas, Jean-Christophe, > >> >> > >> >> As I was trying to enable the touchscreen on the at91sam9261ek with > >> >> device-tree support, I ran into an issue. The touchscreen driver needs > >> >> to know the state of the pendown gpio and also needs it as an > >> >> interrupt source. > >> >> > >> >> The problem is that when a gpio is used as an interrupt, it's > >> >> requested by the pinctrl driver during the xlate stage, marking it > >> >> unavaliable for the other driver. > >> >> It looks like the at91 pinctrl driver is the only one to use > >> >> gpio_request() in the xlate stage. Maybe we should remove this: > >> > > >> > > >> > You should only request it as a GPIO and then use gpio_to_irq to get the > >> > related IRQ. > >> > Because what is done here, is to solve the case where only the irq > >> > is request, and in this specific case we need to request the pin as a > >> > GPIO. > >> > > >> > >> That's what I did first, and was about to submit the patch for the > >> touchscreen driver. > >> However it doesn't feel right. Being able to get the state of a gpio > >> that is also an interrupt seems very useful to me, not only for a > >> touchscreen controller. > >> > >> I understand why it's being done here. It's a matter of being sure > >> that the GPIO is an input and that it'll not be configured otherwise > >> latter. > >> But: > >> 1) I'm wondering why the atmel pinctrl is the only one to do that. > > > > because this the only to start to do it right > > I had a very long discussion woth LinusW and Grant the Gpio need to stop to > > use gpio_to_irq & co for irq. > How can you get the value of the gpio that is also an interrupt source then ? > Can you give a short example? you just have to check the irq source failing or raising but on 9261 impossible the gpio does not have such detail but anyway this is the invert you need to get the information from the IRQ no the way arround so the gpio driver may need to be updated to provide such information to the touch driver Best Regards, J. > > > > > I even send a proposition to do this work across the kernel t the CE-Linux for > > funding > >> 2) I believe that configuration of the direction can be done by > >> describing the GPIO in the DT. (pinctrl-at91.c line 592). > > No pinctrl is for pinmux ONLY not gpio direction & co > > > > Best Regards, > > J. > > > >> 3) If all the GPIOs are described in the DT with a proper pinmux > >> description, i believe the exclusion is also handled. It's probably > >> not the case today though. > > > >> > >> > > >> > Best Regards, > >> > > >> > Boris > >> > > >> >> > >> >> diff --git a/drivers/pinctrl/pinctrl-at91.c > >> >> b/drivers/pinctrl/pinctrl-at91.c > >> >> index a7549c4..cf91a35 100644 > >> >> --- a/drivers/pinctrl/pinctrl-at91.c > >> >> +++ b/drivers/pinctrl/pinctrl-at91.c > >> >> @@ -1463,14 +1463,6 @@ static int at91_gpio_irq_domain_xlate(struct > >> >> irq_domain *d, > >> >> *out_hwirq = intspec[0]; > >> >> *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK; > >> >> > >> >> - ret = gpio_request(pin, ctrlr->full_name); > >> >> - if (ret) > >> >> - return ret; > >> >> - > >> >> - ret = gpio_direction_input(pin); > >> >> - if (ret) > >> >> - return ret; > >> >> - > >> >> return 0; > >> >> } > >> >> > >> >> Jean-Jacques > >> > > >> > > >> > >> _______________________________________________ > >> linux-arm-kernel mailing list > >> linux-arm-kernel@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > On 14:04 Wed 15 Jan , Jean-Jacques Hiblot wrote: >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: >> > On 12:05 Mon 13 Jan , Jean-Jacques Hiblot wrote: >> >> Hi Boris, >> >> >> >> 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: >> >> > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: >> >> >> >> >> >> Hello Nicolas, Jean-Christophe, >> >> >> >> >> >> As I was trying to enable the touchscreen on the at91sam9261ek with >> >> >> device-tree support, I ran into an issue. The touchscreen driver needs >> >> >> to know the state of the pendown gpio and also needs it as an >> >> >> interrupt source. >> >> >> >> >> >> The problem is that when a gpio is used as an interrupt, it's >> >> >> requested by the pinctrl driver during the xlate stage, marking it >> >> >> unavaliable for the other driver. >> >> >> It looks like the at91 pinctrl driver is the only one to use >> >> >> gpio_request() in the xlate stage. Maybe we should remove this: >> >> > >> >> > >> >> > You should only request it as a GPIO and then use gpio_to_irq to get the >> >> > related IRQ. >> >> > Because what is done here, is to solve the case where only the irq >> >> > is request, and in this specific case we need to request the pin as a >> >> > GPIO. >> >> > >> >> >> >> That's what I did first, and was about to submit the patch for the >> >> touchscreen driver. >> >> However it doesn't feel right. Being able to get the state of a gpio >> >> that is also an interrupt seems very useful to me, not only for a >> >> touchscreen controller. >> >> >> >> I understand why it's being done here. It's a matter of being sure >> >> that the GPIO is an input and that it'll not be configured otherwise >> >> latter. >> >> But: >> >> 1) I'm wondering why the atmel pinctrl is the only one to do that. >> > >> > because this the only to start to do it right >> > I had a very long discussion woth LinusW and Grant the Gpio need to stop to >> > use gpio_to_irq & co for irq. >> How can you get the value of the gpio that is also an interrupt source then ? >> Can you give a short example? > > you just have to check the irq source > > failing or raising > > but on 9261 impossible the gpio does not have such detail > > but anyway this is the invert you need to get the information from the IRQ no the way > arround Should I modify the touchscreen driver to use irq_to_gpio() in this case then ? or is this also not proper ? > > so the gpio driver may need to be updated to provide such information to the > touch driver > > Best Regards, > J. >> >> > >> > I even send a proposition to do this work across the kernel t the CE-Linux for >> > funding >> >> 2) I believe that configuration of the direction can be done by >> >> describing the GPIO in the DT. (pinctrl-at91.c line 592). >> > No pinctrl is for pinmux ONLY not gpio direction & co >> > >> > Best Regards, >> > J. >> > >> >> 3) If all the GPIOs are described in the DT with a proper pinmux >> >> description, i believe the exclusion is also handled. It's probably >> >> not the case today though. >> > >> >> >> >> > >> >> > Best Regards, >> >> > >> >> > Boris >> >> > >> >> >> >> >> >> diff --git a/drivers/pinctrl/pinctrl-at91.c >> >> >> b/drivers/pinctrl/pinctrl-at91.c >> >> >> index a7549c4..cf91a35 100644 >> >> >> --- a/drivers/pinctrl/pinctrl-at91.c >> >> >> +++ b/drivers/pinctrl/pinctrl-at91.c >> >> >> @@ -1463,14 +1463,6 @@ static int at91_gpio_irq_domain_xlate(struct >> >> >> irq_domain *d, >> >> >> *out_hwirq = intspec[0]; >> >> >> *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK; >> >> >> >> >> >> - ret = gpio_request(pin, ctrlr->full_name); >> >> >> - if (ret) >> >> >> - return ret; >> >> >> - >> >> >> - ret = gpio_direction_input(pin); >> >> >> - if (ret) >> >> >> - return ret; >> >> >> - >> >> >> return 0; >> >> >> } >> >> >> >> >> >> Jean-Jacques >> >> > >> >> > >> >> >> >> _______________________________________________ >> >> linux-arm-kernel mailing list >> >> linux-arm-kernel@lists.infradead.org >> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wednesday 15 January 2014 14:44:24 Jean-Jacques Hiblot wrote: > 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > > On 14:04 Wed 15 Jan , Jean-Jacques Hiblot wrote: > >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > >> > because this the only to start to do it right > >> > I had a very long discussion woth LinusW and Grant the Gpio need to stop to > >> > use gpio_to_irq & co for irq. > >> How can you get the value of the gpio that is also an interrupt source then ? > >> Can you give a short example? > > > > you just have to check the irq source > > > > failing or raising > > > > but on 9261 impossible the gpio does not have such detail > > > > but anyway this is the invert you need to get the information from the IRQ no the way > > arround > > Should I modify the touchscreen driver to use irq_to_gpio() in this > case then ? or is this also not proper ? irq_to_gpio no longer exists. Arnd
On 14:44 Wed 15 Jan , Jean-Jacques Hiblot wrote: > 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > > On 14:04 Wed 15 Jan , Jean-Jacques Hiblot wrote: > >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > >> > On 12:05 Mon 13 Jan , Jean-Jacques Hiblot wrote: > >> >> Hi Boris, > >> >> > >> >> 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: > >> >> > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: > >> >> >> > >> >> >> Hello Nicolas, Jean-Christophe, > >> >> >> > >> >> >> As I was trying to enable the touchscreen on the at91sam9261ek with > >> >> >> device-tree support, I ran into an issue. The touchscreen driver needs > >> >> >> to know the state of the pendown gpio and also needs it as an > >> >> >> interrupt source. > >> >> >> > >> >> >> The problem is that when a gpio is used as an interrupt, it's > >> >> >> requested by the pinctrl driver during the xlate stage, marking it > >> >> >> unavaliable for the other driver. > >> >> >> It looks like the at91 pinctrl driver is the only one to use > >> >> >> gpio_request() in the xlate stage. Maybe we should remove this: > >> >> > > >> >> > > >> >> > You should only request it as a GPIO and then use gpio_to_irq to get the > >> >> > related IRQ. > >> >> > Because what is done here, is to solve the case where only the irq > >> >> > is request, and in this specific case we need to request the pin as a > >> >> > GPIO. > >> >> > > >> >> > >> >> That's what I did first, and was about to submit the patch for the > >> >> touchscreen driver. > >> >> However it doesn't feel right. Being able to get the state of a gpio > >> >> that is also an interrupt seems very useful to me, not only for a > >> >> touchscreen controller. > >> >> > >> >> I understand why it's being done here. It's a matter of being sure > >> >> that the GPIO is an input and that it'll not be configured otherwise > >> >> latter. > >> >> But: > >> >> 1) I'm wondering why the atmel pinctrl is the only one to do that. > >> > > >> > because this the only to start to do it right > >> > I had a very long discussion woth LinusW and Grant the Gpio need to stop to > >> > use gpio_to_irq & co for irq. > >> How can you get the value of the gpio that is also an interrupt source then ? > >> Can you give a short example? > > > > you just have to check the irq source > > > > failing or raising > > > > but on 9261 impossible the gpio does not have such detail > > > > but anyway this is the invert you need to get the information from the IRQ no the way > > arround > > Should I modify the touchscreen driver to use irq_to_gpio() in this > case then ? or is this also not proper ? > no as said by arnd irq_to_gpio does not exsit that's why I said the irq need to provide you the information as it's a raising or failing irq Best Regards, J.
2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > On 14:44 Wed 15 Jan , Jean-Jacques Hiblot wrote: >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: >> > On 14:04 Wed 15 Jan , Jean-Jacques Hiblot wrote: >> >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: >> >> > On 12:05 Mon 13 Jan , Jean-Jacques Hiblot wrote: >> >> >> Hi Boris, >> >> >> >> >> >> 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: >> >> >> > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: >> >> >> >> >> >> >> >> Hello Nicolas, Jean-Christophe, >> >> >> >> >> >> >> >> As I was trying to enable the touchscreen on the at91sam9261ek with >> >> >> >> device-tree support, I ran into an issue. The touchscreen driver needs >> >> >> >> to know the state of the pendown gpio and also needs it as an >> >> >> >> interrupt source. >> >> >> >> >> >> >> >> The problem is that when a gpio is used as an interrupt, it's >> >> >> >> requested by the pinctrl driver during the xlate stage, marking it >> >> >> >> unavaliable for the other driver. >> >> >> >> It looks like the at91 pinctrl driver is the only one to use >> >> >> >> gpio_request() in the xlate stage. Maybe we should remove this: >> >> >> > >> >> >> > >> >> >> > You should only request it as a GPIO and then use gpio_to_irq to get the >> >> >> > related IRQ. >> >> >> > Because what is done here, is to solve the case where only the irq >> >> >> > is request, and in this specific case we need to request the pin as a >> >> >> > GPIO. >> >> >> > >> >> >> >> >> >> That's what I did first, and was about to submit the patch for the >> >> >> touchscreen driver. >> >> >> However it doesn't feel right. Being able to get the state of a gpio >> >> >> that is also an interrupt seems very useful to me, not only for a >> >> >> touchscreen controller. >> >> >> >> >> >> I understand why it's being done here. It's a matter of being sure >> >> >> that the GPIO is an input and that it'll not be configured otherwise >> >> >> latter. >> >> >> But: >> >> >> 1) I'm wondering why the atmel pinctrl is the only one to do that. >> >> > >> >> > because this the only to start to do it right >> >> > I had a very long discussion woth LinusW and Grant the Gpio need to stop to >> >> > use gpio_to_irq & co for irq. >> >> How can you get the value of the gpio that is also an interrupt source then ? >> >> Can you give a short example? >> > >> > you just have to check the irq source >> > >> > failing or raising >> > >> > but on 9261 impossible the gpio does not have such detail >> > >> > but anyway this is the invert you need to get the information from the IRQ no the way >> > arround >> >> Should I modify the touchscreen driver to use irq_to_gpio() in this >> case then ? or is this also not proper ? >> > no as said by arnd irq_to_gpio does not exsit > > that's why I said the irq need to provide you the information as it's a > raising or failing irq Even when this kind of information is available it's not enough to know for sure the state of the gpio. Think of short pulses such as the glitches you have when you press a button. > > Best Regards, > J.
On 15:41 Wed 15 Jan , Jean-Jacques Hiblot wrote: > 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > > On 14:44 Wed 15 Jan , Jean-Jacques Hiblot wrote: > >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > >> > On 14:04 Wed 15 Jan , Jean-Jacques Hiblot wrote: > >> >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > >> >> > On 12:05 Mon 13 Jan , Jean-Jacques Hiblot wrote: > >> >> >> Hi Boris, > >> >> >> > >> >> >> 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: > >> >> >> > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: > >> >> >> >> > >> >> >> >> Hello Nicolas, Jean-Christophe, > >> >> >> >> > >> >> >> >> As I was trying to enable the touchscreen on the at91sam9261ek with > >> >> >> >> device-tree support, I ran into an issue. The touchscreen driver needs > >> >> >> >> to know the state of the pendown gpio and also needs it as an > >> >> >> >> interrupt source. > >> >> >> >> > >> >> >> >> The problem is that when a gpio is used as an interrupt, it's > >> >> >> >> requested by the pinctrl driver during the xlate stage, marking it > >> >> >> >> unavaliable for the other driver. > >> >> >> >> It looks like the at91 pinctrl driver is the only one to use > >> >> >> >> gpio_request() in the xlate stage. Maybe we should remove this: > >> >> >> > > >> >> >> > > >> >> >> > You should only request it as a GPIO and then use gpio_to_irq to get the > >> >> >> > related IRQ. > >> >> >> > Because what is done here, is to solve the case where only the irq > >> >> >> > is request, and in this specific case we need to request the pin as a > >> >> >> > GPIO. > >> >> >> > > >> >> >> > >> >> >> That's what I did first, and was about to submit the patch for the > >> >> >> touchscreen driver. > >> >> >> However it doesn't feel right. Being able to get the state of a gpio > >> >> >> that is also an interrupt seems very useful to me, not only for a > >> >> >> touchscreen controller. > >> >> >> > >> >> >> I understand why it's being done here. It's a matter of being sure > >> >> >> that the GPIO is an input and that it'll not be configured otherwise > >> >> >> latter. > >> >> >> But: > >> >> >> 1) I'm wondering why the atmel pinctrl is the only one to do that. > >> >> > > >> >> > because this the only to start to do it right > >> >> > I had a very long discussion woth LinusW and Grant the Gpio need to stop to > >> >> > use gpio_to_irq & co for irq. > >> >> How can you get the value of the gpio that is also an interrupt source then ? > >> >> Can you give a short example? > >> > > >> > you just have to check the irq source > >> > > >> > failing or raising > >> > > >> > but on 9261 impossible the gpio does not have such detail > >> > > >> > but anyway this is the invert you need to get the information from the IRQ no the way > >> > arround > >> > >> Should I modify the touchscreen driver to use irq_to_gpio() in this > >> case then ? or is this also not proper ? > >> > > no as said by arnd irq_to_gpio does not exsit > > > > that's why I said the irq need to provide you the information as it's a > > raising or failing irq > > Even when this kind of information is available it's not enough to > know for sure the state of the gpio. Think of short pulses such as the > glitches you have when you press a button. this why you have debounce in the gpio IP to solve this Best Regards, J. > > > > > Best Regards, > > J.
2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > On 15:41 Wed 15 Jan , Jean-Jacques Hiblot wrote: >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: >> > On 14:44 Wed 15 Jan , Jean-Jacques Hiblot wrote: >> >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: >> >> > On 14:04 Wed 15 Jan , Jean-Jacques Hiblot wrote: >> >> >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: >> >> >> > On 12:05 Mon 13 Jan , Jean-Jacques Hiblot wrote: >> >> >> >> Hi Boris, >> >> >> >> >> >> >> >> 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: >> >> >> >> > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: >> >> >> >> >> >> >> >> >> >> Hello Nicolas, Jean-Christophe, >> >> >> >> >> >> >> >> >> >> As I was trying to enable the touchscreen on the at91sam9261ek with >> >> >> >> >> device-tree support, I ran into an issue. The touchscreen driver needs >> >> >> >> >> to know the state of the pendown gpio and also needs it as an >> >> >> >> >> interrupt source. >> >> >> >> >> >> >> >> >> >> The problem is that when a gpio is used as an interrupt, it's >> >> >> >> >> requested by the pinctrl driver during the xlate stage, marking it >> >> >> >> >> unavaliable for the other driver. >> >> >> >> >> It looks like the at91 pinctrl driver is the only one to use >> >> >> >> >> gpio_request() in the xlate stage. Maybe we should remove this: >> >> >> >> > >> >> >> >> > >> >> >> >> > You should only request it as a GPIO and then use gpio_to_irq to get the >> >> >> >> > related IRQ. >> >> >> >> > Because what is done here, is to solve the case where only the irq >> >> >> >> > is request, and in this specific case we need to request the pin as a >> >> >> >> > GPIO. >> >> >> >> > >> >> >> >> >> >> >> >> That's what I did first, and was about to submit the patch for the >> >> >> >> touchscreen driver. >> >> >> >> However it doesn't feel right. Being able to get the state of a gpio >> >> >> >> that is also an interrupt seems very useful to me, not only for a >> >> >> >> touchscreen controller. >> >> >> >> >> >> >> >> I understand why it's being done here. It's a matter of being sure >> >> >> >> that the GPIO is an input and that it'll not be configured otherwise >> >> >> >> latter. >> >> >> >> But: >> >> >> >> 1) I'm wondering why the atmel pinctrl is the only one to do that. >> >> >> > >> >> >> > because this the only to start to do it right >> >> >> > I had a very long discussion woth LinusW and Grant the Gpio need to stop to >> >> >> > use gpio_to_irq & co for irq. >> >> >> How can you get the value of the gpio that is also an interrupt source then ? >> >> >> Can you give a short example? >> >> > >> >> > you just have to check the irq source >> >> > >> >> > failing or raising >> >> > >> >> > but on 9261 impossible the gpio does not have such detail >> >> > >> >> > but anyway this is the invert you need to get the information from the IRQ no the way >> >> > arround >> >> >> >> Should I modify the touchscreen driver to use irq_to_gpio() in this >> >> case then ? or is this also not proper ? >> >> >> > no as said by arnd irq_to_gpio does not exsit >> > >> > that's why I said the irq need to provide you the information as it's a >> > raising or failing irq >> >> Even when this kind of information is available it's not enough to >> know for sure the state of the gpio. Think of short pulses such as the >> glitches you have when you press a button. > > this why you have debounce in the gpio IP to solve this Unfortunately, the debounce filter in the IP not appropriate to handle the glitches such as what you have in the case of a button. It may be good enough to filter EMI induced pulses but not mechanical ones which are way too long. > > Best Regards, > J. >> >> > >> > Best Regards, >> > J.
On 13/01/2014 11:35, boris brezillon : > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: >> Hello Nicolas, Jean-Christophe, >> >> As I was trying to enable the touchscreen on the at91sam9261ek with >> device-tree support, I ran into an issue. The touchscreen driver needs >> to know the state of the pendown gpio and also needs it as an >> interrupt source. >> >> The problem is that when a gpio is used as an interrupt, it's >> requested by the pinctrl driver during the xlate stage, marking it >> unavaliable for the other driver. >> It looks like the at91 pinctrl driver is the only one to use >> gpio_request() in the xlate stage. Maybe we should remove this: > > You should only request it as a GPIO and then use gpio_to_irq to get the > related IRQ. > Because what is done here, is to solve the case where only the irq > is request, and in this specific case we need to request the pin as a > GPIO. Yes, this is what we do. It seems simple and obvious to me, but some may say that "you shall not do that, it is horrible!". Well... I always tend to choose a solution that works. It is one of my weaknesses, I admit ;-) Linus W. any advice on this, before we hit again one of those infinite threads that leads no progress at all? >> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c >> index a7549c4..cf91a35 100644 >> --- a/drivers/pinctrl/pinctrl-at91.c >> +++ b/drivers/pinctrl/pinctrl-at91.c >> @@ -1463,14 +1463,6 @@ static int at91_gpio_irq_domain_xlate(struct >> irq_domain *d, >> *out_hwirq = intspec[0]; >> *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK; >> >> - ret = gpio_request(pin, ctrlr->full_name); >> - if (ret) >> - return ret; >> - >> - ret = gpio_direction_input(pin); >> - if (ret) >> - return ret; >> - >> return 0; >> } >> >> Jean-Jacques > > >
On 16:30 Wed 15 Jan , Jean-Jacques Hiblot wrote: > 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > > On 15:41 Wed 15 Jan , Jean-Jacques Hiblot wrote: > >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > >> > On 14:44 Wed 15 Jan , Jean-Jacques Hiblot wrote: > >> >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > >> >> > On 14:04 Wed 15 Jan , Jean-Jacques Hiblot wrote: > >> >> >> 2014/1/15 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>: > >> >> >> > On 12:05 Mon 13 Jan , Jean-Jacques Hiblot wrote: > >> >> >> >> Hi Boris, > >> >> >> >> > >> >> >> >> 2014/1/13 boris brezillon <b.brezillon@overkiz.com>: > >> >> >> >> > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: > >> >> >> >> >> > >> >> >> >> >> Hello Nicolas, Jean-Christophe, > >> >> >> >> >> > >> >> >> >> >> As I was trying to enable the touchscreen on the at91sam9261ek with > >> >> >> >> >> device-tree support, I ran into an issue. The touchscreen driver needs > >> >> >> >> >> to know the state of the pendown gpio and also needs it as an > >> >> >> >> >> interrupt source. > >> >> >> >> >> > >> >> >> >> >> The problem is that when a gpio is used as an interrupt, it's > >> >> >> >> >> requested by the pinctrl driver during the xlate stage, marking it > >> >> >> >> >> unavaliable for the other driver. > >> >> >> >> >> It looks like the at91 pinctrl driver is the only one to use > >> >> >> >> >> gpio_request() in the xlate stage. Maybe we should remove this: > >> >> >> >> > > >> >> >> >> > > >> >> >> >> > You should only request it as a GPIO and then use gpio_to_irq to get the > >> >> >> >> > related IRQ. > >> >> >> >> > Because what is done here, is to solve the case where only the irq > >> >> >> >> > is request, and in this specific case we need to request the pin as a > >> >> >> >> > GPIO. > >> >> >> >> > > >> >> >> >> > >> >> >> >> That's what I did first, and was about to submit the patch for the > >> >> >> >> touchscreen driver. > >> >> >> >> However it doesn't feel right. Being able to get the state of a gpio > >> >> >> >> that is also an interrupt seems very useful to me, not only for a > >> >> >> >> touchscreen controller. > >> >> >> >> > >> >> >> >> I understand why it's being done here. It's a matter of being sure > >> >> >> >> that the GPIO is an input and that it'll not be configured otherwise > >> >> >> >> latter. > >> >> >> >> But: > >> >> >> >> 1) I'm wondering why the atmel pinctrl is the only one to do that. > >> >> >> > > >> >> >> > because this the only to start to do it right > >> >> >> > I had a very long discussion woth LinusW and Grant the Gpio need to stop to > >> >> >> > use gpio_to_irq & co for irq. > >> >> >> How can you get the value of the gpio that is also an interrupt source then ? > >> >> >> Can you give a short example? > >> >> > > >> >> > you just have to check the irq source > >> >> > > >> >> > failing or raising > >> >> > > >> >> > but on 9261 impossible the gpio does not have such detail > >> >> > > >> >> > but anyway this is the invert you need to get the information from the IRQ no the way > >> >> > arround > >> >> > >> >> Should I modify the touchscreen driver to use irq_to_gpio() in this > >> >> case then ? or is this also not proper ? > >> >> > >> > no as said by arnd irq_to_gpio does not exsit > >> > > >> > that's why I said the irq need to provide you the information as it's a > >> > raising or failing irq > >> > >> Even when this kind of information is available it's not enough to > >> know for sure the state of the gpio. Think of short pulses such as the > >> glitches you have when you press a button. > > > > this why you have debounce in the gpio IP to solve this > > Unfortunately, the debounce filter in the IP not appropriate to handle > the glitches such as what you have in the case of a button. > It may be good enough to filter EMI induced pulses but not mechanical > ones which are way too long. so use deglitch one this will do the tric Best Regards, J.
On 11:35 Mon 13 Jan , boris brezillon wrote: > On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: > >Hello Nicolas, Jean-Christophe, > > > >As I was trying to enable the touchscreen on the at91sam9261ek with > >device-tree support, I ran into an issue. The touchscreen driver needs > >to know the state of the pendown gpio and also needs it as an > >interrupt source. > > > >The problem is that when a gpio is used as an interrupt, it's > >requested by the pinctrl driver during the xlate stage, marking it > >unavaliable for the other driver. > >It looks like the at91 pinctrl driver is the only one to use > >gpio_request() in the xlate stage. Maybe we should remove this: > > You should only request it as a GPIO and then use gpio_to_irq to get the > related IRQ. This here a HUGE issue in the hole kernel You should NEVER known ti's a gpio in the driver at all gpio_to_irq should never exist you need to only see the irq > Because what is done here, is to solve the case where only the irq > is request, and in this specific case we need to request the pin as a > GPIO. this need to be handled at irq level not drivers Best Regards, J.
On 15/01/2014 19:06, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 11:35 Mon 13 Jan , boris brezillon wrote: >> On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: >>> Hello Nicolas, Jean-Christophe, >>> >>> As I was trying to enable the touchscreen on the at91sam9261ek with >>> device-tree support, I ran into an issue. The touchscreen driver needs >>> to know the state of the pendown gpio and also needs it as an >>> interrupt source. >>> >>> The problem is that when a gpio is used as an interrupt, it's >>> requested by the pinctrl driver during the xlate stage, marking it >>> unavaliable for the other driver. >>> It looks like the at91 pinctrl driver is the only one to use >>> gpio_request() in the xlate stage. Maybe we should remove this: >> You should only request it as a GPIO and then use gpio_to_irq to get the >> related IRQ. > This here a HUGE issue in the hole kernel > > You should NEVER known ti's a gpio in the driver at all gpio_to_irq should never > exist you need to only see the irq > >> Because what is done here, is to solve the case where only the irq >> is request, and in this specific case we need to request the pin as a >> GPIO. > this need to be handled at irq level not drivers Then propose something (or at least give us a deadline for this new interrupt model to come out), because the ADS7843E touchscreen is not working anymore (at least on at91 platforms). What this driver needs is a level IRQ (not an edge IRQ). The code in ads7846_hard_irq<http://lxr.free-electrons.com/ident?i=ads7846_hard_irq> interrupt handler is here to transform an edge IRQ into a level IRQ. Is there a way to provide a generic framework to transform edge IRQs into level IRQs (because some GPIO controllers do not support level IRQs, and this is the case for the at91sam9261 one) ? Of cource the gpio_to_irq approach is not perfect, but at least it as the benefit to quickly fix the bug, and we will still be able to improve this later, when we have enough tools (or mechanisms) to do it. Best Regards, Boris > Best Regards, > J.
On 16/01/2014 09:54, boris brezillon : > On 15/01/2014 19:06, Jean-Christophe PLAGNIOL-VILLARD wrote: >> On 11:35 Mon 13 Jan , boris brezillon wrote: >>> On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: >>>> Hello Nicolas, Jean-Christophe, >>>> >>>> As I was trying to enable the touchscreen on the at91sam9261ek with >>>> device-tree support, I ran into an issue. The touchscreen driver needs >>>> to know the state of the pendown gpio and also needs it as an >>>> interrupt source. >>>> >>>> The problem is that when a gpio is used as an interrupt, it's >>>> requested by the pinctrl driver during the xlate stage, marking it >>>> unavaliable for the other driver. >>>> It looks like the at91 pinctrl driver is the only one to use >>>> gpio_request() in the xlate stage. Maybe we should remove this: >>> You should only request it as a GPIO and then use gpio_to_irq to get the >>> related IRQ. >> This here a HUGE issue in the hole kernel >> >> You should NEVER known ti's a gpio in the driver at all gpio_to_irq should never >> exist you need to only see the irq >> >>> Because what is done here, is to solve the case where only the irq >>> is request, and in this specific case we need to request the pin as a >>> GPIO. >> this need to be handled at irq level not drivers > > Then propose something (or at least give us a deadline for this new > interrupt > model to come out), because the ADS7843E touchscreen is not working anymore > (at least on at91 platforms). > > What this driver needs is a level IRQ (not an edge IRQ). The code in > ads7846_hard_irq<http://lxr.free-electrons.com/ident?i=ads7846_hard_irq> > interrupt handler is here to transform an edge IRQ into a level IRQ. > > Is there a way to provide a generic framework to transform edge IRQs > into level IRQs > (because some GPIO controllers do not support level IRQs, and this is > the case for the > at91sam9261 one) ? > > > Of cource the gpio_to_irq approach is not perfect, but at least it as > the benefit to quickly > fix the bug, and we will still be able to improve this later, when we > have enough tools > (or mechanisms) to do it. Moreover, I do not see the rationale behind the concept of "interrupt with an electrical value". An interrupt signals an event and this event can be a transition or a state but an electrical value is the responsibility of a GPIO line, not the other way around. I see a code that could give the value of an electrical line related to an interrupt as a twisted interpretation of the notion of "interrupt". It surprises me that it could be thought as an enhancement that an IRQ coming from a GPIO could give a value! Isn't it why other people are also using this simple distinction to use their GPIO/IRQ mechanism? Maybe this is why we are the only ones to completely forbid this possibility. So, let's fix the bug, submit the modification to mainline, make platform work and see if somebody can convince me that retrieving an electrical line status from a GPIO is a "bad" thing... Bye,
2014/1/16 Nicolas Ferre <nicolas.ferre@atmel.com>: > On 16/01/2014 09:54, boris brezillon : >> On 15/01/2014 19:06, Jean-Christophe PLAGNIOL-VILLARD wrote: >>> On 11:35 Mon 13 Jan , boris brezillon wrote: >>>> On 13/01/2014 11:29, Jean-Jacques Hiblot wrote: >>>>> Hello Nicolas, Jean-Christophe, >>>>> >>>>> As I was trying to enable the touchscreen on the at91sam9261ek with >>>>> device-tree support, I ran into an issue. The touchscreen driver needs >>>>> to know the state of the pendown gpio and also needs it as an >>>>> interrupt source. >>>>> >>>>> The problem is that when a gpio is used as an interrupt, it's >>>>> requested by the pinctrl driver during the xlate stage, marking it >>>>> unavaliable for the other driver. >>>>> It looks like the at91 pinctrl driver is the only one to use >>>>> gpio_request() in the xlate stage. Maybe we should remove this: >>>> You should only request it as a GPIO and then use gpio_to_irq to get the >>>> related IRQ. >>> This here a HUGE issue in the hole kernel >>> >>> You should NEVER known ti's a gpio in the driver at all gpio_to_irq should never >>> exist you need to only see the irq >>> >>>> Because what is done here, is to solve the case where only the irq >>>> is request, and in this specific case we need to request the pin as a >>>> GPIO. >>> this need to be handled at irq level not drivers >> >> Then propose something (or at least give us a deadline for this new >> interrupt >> model to come out), because the ADS7843E touchscreen is not working anymore >> (at least on at91 platforms). >> >> What this driver needs is a level IRQ (not an edge IRQ). The code in >> ads7846_hard_irq<http://lxr.free-electrons.com/ident?i=ads7846_hard_irq> >> interrupt handler is here to transform an edge IRQ into a level IRQ. >> >> Is there a way to provide a generic framework to transform edge IRQs >> into level IRQs >> (because some GPIO controllers do not support level IRQs, and this is >> the case for the >> at91sam9261 one) ? >> >> >> Of cource the gpio_to_irq approach is not perfect, but at least it as >> the benefit to quickly >> fix the bug, and we will still be able to improve this later, when we >> have enough tools >> (or mechanisms) to do it. > > Moreover, I do not see the rationale behind the concept of "interrupt > with an electrical value". An interrupt signals an event and this event > can be a transition or a state but an electrical value is the > responsibility of a GPIO line, not the other way around. > > I see a code that could give the value of an electrical line related to > an interrupt as a twisted interpretation of the notion of "interrupt". > It surprises me that it could be thought as an enhancement that an IRQ > coming from a GPIO could give a value! > > Isn't it why other people are also using this simple distinction to use > their GPIO/IRQ mechanism? Maybe this is why we are the only ones to > completely forbid this possibility. > > So, let's fix the bug, submit the modification to mainline, make > platform work and see if somebody can convince me that retrieving an > electrical line status from a GPIO is a "bad" thing... > I share your view on this. Linus, the root of the issue is the fact that a GPIO can't be requested twice. But IMO it's safe for a single device to request it more than once and use it for different purposes (irq and electrical signal value). Maybe we can rework the GPIO request to include this ownership information. I can post a draft implementation for this if you think it's worthwhile. Jean-Jacques > Bye, > -- > Nicolas Ferre
On Wed, Jan 15, 2014 at 6:28 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > On 13/01/2014 11:35, boris brezillon : >> You should only request it as a GPIO and then use gpio_to_irq to get the >> related IRQ. >> Because what is done here, is to solve the case where only the irq >> is request, and in this specific case we need to request the pin as a >> GPIO. > > Yes, this is what we do. > > It seems simple and obvious to me, but some may say that "you shall not > do that, it is horrible!". Well... I always tend to choose a solution > that works. It is one of my weaknesses, I admit ;-) > > Linus W. any advice on this, before we hit again one of those infinite > threads that leads no progress at all? So we recently established that it is actually OK for any IRQ consumer to request an IRQ from any irq_chip no matter if that is a combined GPIO+IRQ driver. This was concluded after a long discussion involving several parties. gpio_to_irq() is just a convenience function and should not be relied upon to have been called before the IRQ is used. The basic premise is that gpio_chip and irq_chip are orthogonal, and offering their services independent of each other. Especially in the device tree use case it is very hard to dictate that a certain semantic need to be followed to use a certain interrupt-controller to have dependencies to a certain gpio-controller. So they need to be orthogonal. First step is: always prepare the hardware and make it ready for action in respective callbacks from the gpio_chip API and irq_chip API. Do not rely on gpio_to_irq() having been called first anymore. This 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. We have added a new API to help with this situation: gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset) gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset) These should be called from the irq_chip startup() and .shutdown() callbacks to flag that the line is now in use by an IRQ. For example the GPIOlib core will deny the line to be set as output after this. If we need more infrastructure to help with this, I'm game. Clear as mud? ;-) Yours, Linus Walleij
On Thu, Jan 16, 2014 at 1:02 PM, Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote: > Linus, > the root of the issue is the fact that a GPIO can't be requested > twice. But IMO it's safe for a single device to request it more than > once and use it for different purposes (irq and electrical signal > value). Maybe we can rework the GPIO request to include this ownership > information. I can post a draft implementation for this if you think > it's worthwhile. You are right and actually Jean-Christophe's patch is a good start. You also need to use the gpio_lock_as_irq() API as shown in other patches to the subsystem e.g. this. commit eb7cce1ea96b6399672abce787598f6e7a4352c3 Also check my other mail in this thread. Yours, Linus Walleij
2014/1/22 Linus Walleij <linus.walleij@linaro.org>: > On Wed, Jan 15, 2014 at 6:28 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote: >> On 13/01/2014 11:35, boris brezillon : > >>> You should only request it as a GPIO and then use gpio_to_irq to get the >>> related IRQ. >>> Because what is done here, is to solve the case where only the irq >>> is request, and in this specific case we need to request the pin as a >>> GPIO. >> >> Yes, this is what we do. >> >> It seems simple and obvious to me, but some may say that "you shall not >> do that, it is horrible!". Well... I always tend to choose a solution >> that works. It is one of my weaknesses, I admit ;-) >> >> Linus W. any advice on this, before we hit again one of those infinite >> threads that leads no progress at all? > > So we recently established that it is actually OK for any IRQ > consumer to request an IRQ from any irq_chip no matter if > that is a combined GPIO+IRQ driver. This was concluded after > a long discussion involving several parties. > > gpio_to_irq() is just a convenience function and should not be > relied upon to have been called before the IRQ is used. > > The basic premise is that gpio_chip and irq_chip are > orthogonal, and offering their services independent of each > other. > > Especially in the device tree use case it is very hard to > dictate that a certain semantic need to be followed to use > a certain interrupt-controller to have dependencies to a > certain gpio-controller. So they need to be orthogonal. > > First step is: always prepare the hardware and make it > ready for action in respective callbacks from the gpio_chip API > and irq_chip API. Do not rely on gpio_to_irq() having been > called first anymore. > > This 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. > > We have added a new API to help with this situation: > > gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset) > gpio_unlock_as_irq(struct gpio_chip *chip, > unsigned int offset) > > These should be called from the irq_chip startup() and > .shutdown() callbacks to flag that the line is now in use by > an IRQ. For example the GPIOlib core will deny the line to > be set as output after this. > > If we need more infrastructure to help with this, I'm game. > > Clear as mud? ;-) it's actually crystal clear. > > Yours, > Linus Walleij
2014/1/22 Linus Walleij <linus.walleij@linaro.org>: > On Thu, Jan 16, 2014 at 1:02 PM, Jean-Jacques Hiblot > <jjhiblot@traphandler.com> wrote: > >> Linus, >> the root of the issue is the fact that a GPIO can't be requested >> twice. But IMO it's safe for a single device to request it more than >> once and use it for different purposes (irq and electrical signal >> value). Maybe we can rework the GPIO request to include this ownership >> information. I can post a draft implementation for this if you think >> it's worthwhile. > > You are right and actually Jean-Christophe's patch is a good > start. Hi Linus, I've been thinking hard on the gpio ownership and I'd like your opinion on the following assumptions: - a gpio used as an interrupt is always a gpio configured as an input. (corollary : a gpio used for interrupt cannot not be an ouput) - a gpio used as an input or interrupt only, could be safely accessed by multiple users. - a gpio used as an output should be used as such by only one user. Still other users should be allowed to read its value. If those assumptions are true, I believe they can lead to a new exclusion scheme: - ouput is mutually exclusive with interrupt but not with input - only 1 ouput user at a time. It would do away with our sharing issue and more (chained interrupt handlers on gpio interrupt, read/interrupt access through sysfs to a gpio requested by a driver) And I believe that most of the infrastructure is in place to implement this : - direction flags in gpio_request_one - gpio_lock_as_irq/gpio_unlock_as_irq Jean-Jacques > > You also need to use the gpio_lock_as_irq() API as shown in > other patches to the subsystem e.g. this. > commit eb7cce1ea96b6399672abce787598f6e7a4352c3 > > Also check my other mail in this thread. > > Yours, > Linus Walleij
On Thu, Jan 23, 2014 at 2:16 PM, Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote: > I've been thinking hard on the gpio ownership and I'd like your > opinion on the following assumptions: > > - a gpio used as an interrupt is always a gpio configured as an input. > (corollary : a gpio used for interrupt cannot not be an ouput) That is the assumption made by the core. Until the day someone comes up with a weird usecase... > - a gpio used as an input or interrupt only, could be safely accessed > by multiple users. Multiple consumers is the term we would use. But yes. > - a gpio used as an output should be used as such by only one user. > Still other users should be allowed to read its value. Sounds reasonable. > If those assumptions are true, I believe they can lead to a new > exclusion scheme: > - ouput is mutually exclusive with interrupt but not with input > - only 1 ouput user at a time. Logical conclusion from the above yes. > It would do away with our sharing issue and more (chained interrupt > handlers on gpio interrupt, read/interrupt access through sysfs to a > gpio requested by a driver) > > And I believe that most of the infrastructure is in place to implement this : > - direction flags in gpio_request_one > - gpio_lock_as_irq/gpio_unlock_as_irq So I think what I need to see is a proposed patch, whether it will hit a particular driver or the gpiolib core, but either would probably be interesting. Pls keep linux-gpio and Alexandre on CC for this discussion. Yours, Linus Walleij
diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c index a7549c4..cf91a35 100644 --- a/drivers/pinctrl/pinctrl-at91.c +++ b/drivers/pinctrl/pinctrl-at91.c @@ -1463,14 +1463,6 @@ static int at91_gpio_irq_domain_xlate(struct irq_domain *d, *out_hwirq = intspec[0]; *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK; - ret = gpio_request(pin, ctrlr->full_name); - if (ret) - return ret; - - ret = gpio_direction_input(pin); - if (ret) - return ret; - return 0; }