From: Linus Walleij <[email protected]>
Currently the kernel is ambigously treating GPIOs and interrupts
from a GPIO controller: GPIOs and interrupts are treated as
orthogonal. This unfortunately makes it unclear how to actually
retrieve and request a GPIO line or interrupt from a GPIO
controller in the device tree probe path.
In the non-DT probe path it is clear that the GPIO number has to
be passed to the consuming device, and if it is to be used as
an interrupt, the consumer shall performa a gpio_to_irq() mapping
and request the resulting IRQ number.
In the DT boot path consumers may have been given one or more
interrupts from the interrupt-controller side of the GPIO chip
in an abstract way, such that the driver is not aware of the
fact that a GPIO chip is backing this interrupt, and the GPIO
side of the same line is never requested with gpio_request().
A typical case for this is ethernet chips which just take some
interrupt line which may be a "hard" interrupt line (such as an
external interrupt line from an SoC) or a cascaded interrupt
connected to a GPIO line.
This has the following undesired effects:
- The GPIOlib subsystem is not aware that the line is in use
and willingly lets some other consumer perform gpio_request()
on it, leading to a complex resource conflict if it occurs.
- The GPIO debugfs file claims this GPIO line is "free".
- The line direction of the interrupt GPIO line is not
explicitly set as input, even though it is obvious that such
a line need to be set up in this way, often making the system
depend on boot-on defaults for this kind of settings.
To solve this dilemma, perform an interrupt consistency check
when adding a GPIO chip: if the chip is both gpio-controller and
interrupt-controller, walk all children of the device tree,
check if these in turn reference the interrupt-controller, and
if they do, loop over the interrupts used by that child and
perform gpio_request() and gpio_direction_input() on these,
making them unreachable from the GPIO side.
The patch has been devised by Linus Walleij and Lars Poeschel.
Changelog V2:
- To be able to parse custom interrupts properties from the
device tree, get a reference to the drivers irq_domain
and use the xlate function to parse the proptery and
get the irq number. This is tested with
#interrupt-cells = 1, 2, and 3 and multiple interrupts
per property.
Cc: [email protected]
Cc: Javier Martinez Canillas <[email protected]>
Cc: Enric Balletbo i Serra <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc: Santosh Shilimkar <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Balaji T K <[email protected]>
Cc: Tony Lindgren <[email protected]>
Cc: Jon Hunter <[email protected]>
Signed-off-by: Lars Poeschel <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 665f953..b42cdd7 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -10,7 +10,6 @@
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*/
-
#include <linux/device.h>
#include <linux/errno.h>
#include <linux/module.h>
@@ -19,6 +18,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
#include <linux/pinctrl/pinctrl.h>
#include <linux/slab.h>
@@ -127,6 +127,185 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
EXPORT_SYMBOL(of_gpio_simple_xlate);
/**
+ * of_gpio_scan_irq_lines() - internal function to recursively scan the device
+ * tree and request or free the GPIOs that are to be used as IRQ lines
+ * @node: node to start the scanning at
+ * @gcn: device node of the GPIO chip
+ * @irq_domain: the irq_domain for the GPIO chip
+ * @intsize: size of one single interrupt in the device tree for the GPIO
+ * chip. It is the same as #interrupt-cells.
+ * @gc: GPIO chip instantiated from same node
+ * @request: wheter the function should request(true) or free(false) the
+ * irq lines
+ *
+ * This is a internal function that calls itself to recursively scan the device
+ * tree. It scans for uses of the device_node gcn as an interrupt-controller.
+ * If it finds some, it requests the corresponding gpio lines that are to be
+ * used as irq lines and sets them as input.
+ *
+ * If the request parameter is 0 it frees the gpio lines.
+ * For more information see documentation of of_gpiochip_reserve_irq_lines
+ * function.
+ */
+static void of_gpio_scan_irq_lines(const struct device_node *const node,
+ struct device_node *const gcn,
+ struct irq_domain *const irq_domain,
+ const u32 intsize,
+ const struct gpio_chip * const gc,
+ bool request)
+{
+ struct device_node *child;
+ struct device_node *irq_parent;
+ const __be32 *intspec;
+ u32 intlen;
+ int ret;
+ int i;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+
+ if (node == NULL)
+ return;
+
+ for_each_child_of_node(node, child) {
+ of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
+ request);
+ /* Check if we have an IRQ parent, else continue */
+ irq_parent = of_irq_find_parent(child);
+ if (!irq_parent)
+ continue;
+
+ /* Is it so that this very GPIO chip is the parent? */
+ if (irq_parent != gcn) {
+ of_node_put(irq_parent);
+ continue;
+ }
+ of_node_put(irq_parent);
+
+ pr_debug("gpiochip OF: node %s references GPIO interrupt lines\n",
+ child->name);
+
+ /* Get the interrupts property */
+ intspec = of_get_property(child, "interrupts", &intlen);
+ if (intspec == NULL)
+ continue;
+ intlen /= sizeof(*intspec);
+
+ for (i = 0; i < intlen; i += intsize) {
+ /*
+ * Find out the local IRQ number. This corresponds to
+ * the GPIO line offset for a GPIO chip.
+ */
+ if (irq_domain && irq_domain->ops->xlate)
+ irq_domain->ops->xlate(irq_domain, gcn,
+ intspec + i, intsize,
+ &hwirq, &type);
+ else
+ hwirq = intspec[0];
+
+ hwirq = be32_to_cpu(hwirq);
+ pr_debug("gpiochip OF: node %s references GPIO %lu (%lu)\n",
+ child->name, gc->base + hwirq, hwirq);
+
+ if (request) {
+ /*
+ * This child is making a reference to this
+ * chip through the interrupts property, so
+ * reserve these GPIO lines and set them as
+ * input.
+ */
+ ret = gpio_request(gc->base + hwirq,
+ child->name);
+ if (ret)
+ pr_err("gpiolib OF: could not request IRQ GPIO %lu (%lu) for node %s (%d)\n",
+ gc->base + hwirq, hwirq,
+ child->name, ret);
+ ret = gpio_direction_input(gc->base + hwirq);
+ if (ret)
+ pr_err("gpiolib OF: could not set IRQ GPIO %lu (%lu) as input for node %s (%d)\n",
+ gc->base + hwirq, hwirq,
+ child->name, ret);
+ } else {
+ gpio_free(gc->base + hwirq);
+ }
+ }
+ }
+}
+
+/**
+ * of_gpiochip_reserve_irq_lines() - request or free GPIO IRQ lines
+ * @np: device node of the GPIO chip
+ * @gc: GPIO chip instantiated from same node
+ * @request: wheter the function should request(1) or free(0) the irq lines
+ *
+ * This function should not be used directly, use the macros
+ * of_gpiochip_request_irq_lines or of_gpiochip_free_gpio_lines instead.
+ *
+ * For the case of requesting the irq lines (request == 1) this function is
+ * called after instantiating a GPIO chip from a device tree node to assert
+ * that all interrupts derived from the chip are consistently requested as
+ * GPIO lines, if the GPIO chip is BOTH a gpio-controller AND an
+ * interrupt-controller.
+ *
+ * If another node in the device tree is referencing the interrupt-controller
+ * portions of the GPIO chip, such that it is using a GPIO line as some
+ * arbitrary interrupt source, the following holds:
+ *
+ * - That line must NOT be used anywhere else in the device tree as a
+ * <&gpio N> reference, or GPIO and interrupt usage may conflict.
+ *
+ * Conversely, if a node is using a line as a direct reference to a GPIO line,
+ * no node in the tree may use that line as an interrupt.
+ *
+ * If another node is referencing a GPIO line, and also want to use that line
+ * as an interrupt source, it is necessary for this driver to use the
+ * gpio_to_irq() kernel interface.
+ *
+ * For the case of freeing the irq lines (request == 0) this function simply
+ * uses the same device tree information used to request the irq lines to call
+ * gpiochip_free on that GPIOs.
+ */
+static void of_gpiochip_reserve_irq_lines(struct device_node *np,
+ struct gpio_chip *gc, bool request)
+{
+ struct device_node *root;
+ const __be32 *tmp;
+ struct irq_domain *irq_domain;
+ u32 intsize;
+
+ /*
+ * If this chip is not tagged as interrupt-controller, there is
+ * no problem so we just exit.
+ */
+ if (!of_property_read_bool(np, "interrupt-controller"))
+ return;
+
+ /*
+ * Proceed to check the consistency of all references to this
+ * GPIO chip.
+ */
+ root = of_find_node_by_path("/");
+ if (!root)
+ return;
+
+ tmp = of_get_property(np, "#interrupt-cells", NULL);
+ if (tmp == NULL)
+ intsize = 1;
+ else
+ intsize = be32_to_cpu(*tmp);
+
+ irq_domain = irq_find_host(np);
+
+ of_gpio_scan_irq_lines(root, np, irq_domain, intsize, gc, request);
+ of_node_put(root);
+}
+
+#define of_gpiochip_request_irq_lines(np, gc) \
+ of_gpiochip_reserve_irq_lines(np, gc, true)
+
+#define of_gpiochip_free_irq_lines(np, gc) \
+ of_gpiochip_reserve_irq_lines(np, gc, false)
+
+/**
* of_mm_gpiochip_add - Add memory mapped GPIO chip (bank)
* @np: device node of the GPIO chip
* @mm_gc: pointer to the of_mm_gpio_chip allocated structure
@@ -170,6 +349,8 @@ int of_mm_gpiochip_add(struct device_node *np,
if (ret)
goto err2;
+ of_gpiochip_request_irq_lines(np, gc);
+
return 0;
err2:
iounmap(mm_gc->regs);
@@ -231,12 +412,14 @@ void of_gpiochip_add(struct gpio_chip *chip)
chip->of_xlate = of_gpio_simple_xlate;
}
+ of_gpiochip_request_irq_lines(chip->of_node, chip);
of_gpiochip_add_pin_range(chip);
of_node_get(chip->of_node);
}
void of_gpiochip_remove(struct gpio_chip *chip)
{
+ of_gpiochip_free_irq_lines(chip->of_node, chip);
gpiochip_remove_pin_ranges(chip);
if (chip->of_node)
--
1.7.10.4
Hi Lars, Linus,
On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
> From: Linus Walleij <[email protected]>
>
> Currently the kernel is ambigously treating GPIOs and interrupts
> from a GPIO controller: GPIOs and interrupts are treated as
> orthogonal. This unfortunately makes it unclear how to actually
> retrieve and request a GPIO line or interrupt from a GPIO
> controller in the device tree probe path.
>
> In the non-DT probe path it is clear that the GPIO number has to
> be passed to the consuming device, and if it is to be used as
> an interrupt, the consumer shall performa a gpio_to_irq() mapping
> and request the resulting IRQ number.
>
> In the DT boot path consumers may have been given one or more
> interrupts from the interrupt-controller side of the GPIO chip
> in an abstract way, such that the driver is not aware of the
> fact that a GPIO chip is backing this interrupt, and the GPIO
> side of the same line is never requested with gpio_request().
> A typical case for this is ethernet chips which just take some
> interrupt line which may be a "hard" interrupt line (such as an
> external interrupt line from an SoC) or a cascaded interrupt
> connected to a GPIO line.
>
> This has the following undesired effects:
>
> - The GPIOlib subsystem is not aware that the line is in use
> and willingly lets some other consumer perform gpio_request()
> on it, leading to a complex resource conflict if it occurs.
>
> - The GPIO debugfs file claims this GPIO line is "free".
>
> - The line direction of the interrupt GPIO line is not
> explicitly set as input, even though it is obvious that such
> a line need to be set up in this way, often making the system
> depend on boot-on defaults for this kind of settings.
>
> To solve this dilemma, perform an interrupt consistency check
> when adding a GPIO chip: if the chip is both gpio-controller and
> interrupt-controller, walk all children of the device tree,
> check if these in turn reference the interrupt-controller, and
> if they do, loop over the interrupts used by that child and
> perform gpio_request() and gpio_direction_input() on these,
> making them unreachable from the GPIO side.
>
> The patch has been devised by Linus Walleij and Lars Poeschel.
>
> Changelog V2:
> - To be able to parse custom interrupts properties from the
> device tree, get a reference to the drivers irq_domain
> and use the xlate function to parse the proptery and
> get the irq number. This is tested with
> #interrupt-cells = 1, 2, and 3 and multiple interrupts
> per property.
This looks much better now, but I still can imagine potential problems.
See my comments inline.
> Cc: [email protected]
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Enric Balletbo i Serra <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
> Cc: Santosh Shilimkar <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: Balaji T K <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Cc: Jon Hunter <[email protected]>
> Signed-off-by: Lars Poeschel <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 665f953..b42cdd7 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -10,7 +10,6 @@
> * the Free Software Foundation; either version 2 of the License, or
> * (at your option) any later version.
> */
> -
> #include <linux/device.h>
> #include <linux/errno.h>
> #include <linux/module.h>
> @@ -19,6 +18,7 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_gpio.h>
> +#include <linux/of_irq.h>
> #include <linux/pinctrl/pinctrl.h>
> #include <linux/slab.h>
>
> @@ -127,6 +127,185 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
> EXPORT_SYMBOL(of_gpio_simple_xlate);
>
> /**
> + * of_gpio_scan_irq_lines() - internal function to recursively scan the
> device + * tree and request or free the GPIOs that are to be used as
> IRQ lines + * @node: node to start the scanning at
> + * @gcn: device node of the GPIO chip
> + * @irq_domain: the irq_domain for the GPIO chip
> + * @intsize: size of one single interrupt in the device tree for the
> GPIO + * chip. It is the same as #interrupt-cells.
> + * @gc: GPIO chip instantiated from same node
> + * @request: wheter the function should request(true) or free(false)
> the + * irq lines
> + *
> + * This is a internal function that calls itself to recursively scan
> the device + * tree. It scans for uses of the device_node gcn as an
> interrupt-controller. + * If it finds some, it requests the
> corresponding gpio lines that are to be + * used as irq lines and sets
> them as input.
> + *
> + * If the request parameter is 0 it frees the gpio lines.
> + * For more information see documentation of
> of_gpiochip_reserve_irq_lines + * function.
> + */
> +static void of_gpio_scan_irq_lines(const struct device_node *const
> node, + struct device_node *const gcn,
> + struct irq_domain *const irq_domain,
> + const u32 intsize,
> + const struct gpio_chip * const gc,
> + bool request)
> +{
> + struct device_node *child;
> + struct device_node *irq_parent;
> + const __be32 *intspec;
> + u32 intlen;
> + int ret;
> + int i;
> + irq_hw_number_t hwirq;
> + unsigned int type;
> +
> + if (node == NULL)
> + return;
> +
> + for_each_child_of_node(node, child) {
> + of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> + request);
> + /* Check if we have an IRQ parent, else continue */
> + irq_parent = of_irq_find_parent(child);
> + if (!irq_parent)
> + continue;
> +
> + /* Is it so that this very GPIO chip is the parent? */
> + if (irq_parent != gcn) {
> + of_node_put(irq_parent);
> + continue;
> + }
> + of_node_put(irq_parent);
> +
> + pr_debug("gpiochip OF: node %s references GPIO interrupt lines\n",
> + child->name);
> +
> + /* Get the interrupts property */
> + intspec = of_get_property(child, "interrupts", &intlen);
> + if (intspec == NULL)
> + continue;
> + intlen /= sizeof(*intspec);
> +
> + for (i = 0; i < intlen; i += intsize) {
> + /*
> + * Find out the local IRQ number. This corresponds to
> + * the GPIO line offset for a GPIO chip.
I'm still not convinced that this assumption is correct. This code will
behave erraticaly in cases where it is not true, requesting innocent GPIO
pins.
> + */
> + if (irq_domain && irq_domain->ops->xlate)
> + irq_domain->ops->xlate(irq_domain, gcn,
> + intspec + i, intsize,
> + &hwirq, &type);
> + else
> + hwirq = intspec[0];
Is it a correct fallback when irq_domain is NULL?
> +
> + hwirq = be32_to_cpu(hwirq);
Is this conversion correct? I don't think hwirq could be big endian here
(unless running on a big endian CPU).
> + pr_debug("gpiochip OF: node %s references GPIO %lu (%lu)\n",
> + child->name, gc->base + hwirq, hwirq);
> +
> + if (request) {
> + /*
> + * This child is making a reference to this
> + * chip through the interrupts property, so
> + * reserve these GPIO lines and set them as
> + * input.
> + */
> + ret = gpio_request(gc->base + hwirq,
> + child->name);
> + if (ret)
> + pr_err("gpiolib OF: could not request IRQ GPIO %lu (%lu) for node
> %s (%d)\n", + gc->base + hwirq, hwirq,
> + child->name, ret);
> + ret = gpio_direction_input(gc->base + hwirq);
> + if (ret)
> + pr_err("gpiolib OF: could not set IRQ GPIO %lu (%lu) as input for
> node %s (%d)\n", + gc->base + hwirq, hwirq,
> + child->name, ret);
> + } else {
> + gpio_free(gc->base + hwirq);
> + }
> + }
> + }
> +}
> +
> +/**
> + * of_gpiochip_reserve_irq_lines() - request or free GPIO IRQ lines
> + * @np: device node of the GPIO chip
> + * @gc: GPIO chip instantiated from same node
> + * @request: wheter the function should request(1) or free(0) the irq
> lines + *
> + * This function should not be used directly, use the macros
> + * of_gpiochip_request_irq_lines or of_gpiochip_free_gpio_lines
> instead. + *
> + * For the case of requesting the irq lines (request == 1) this
> function is + * called after instantiating a GPIO chip from a device
> tree node to assert + * that all interrupts derived from the chip are
> consistently requested as + * GPIO lines, if the GPIO chip is BOTH a
> gpio-controller AND an + * interrupt-controller.
> + *
> + * If another node in the device tree is referencing the
> interrupt-controller + * portions of the GPIO chip, such that it is
> using a GPIO line as some + * arbitrary interrupt source, the following
> holds:
> + *
> + * - That line must NOT be used anywhere else in the device tree as a
> + * <&gpio N> reference, or GPIO and interrupt usage may conflict.
> + *
> + * Conversely, if a node is using a line as a direct reference to a
> GPIO line, + * no node in the tree may use that line as an interrupt.
> + *
> + * If another node is referencing a GPIO line, and also want to use
> that line + * as an interrupt source, it is necessary for this driver
> to use the + * gpio_to_irq() kernel interface.
> + *
> + * For the case of freeing the irq lines (request == 0) this function
> simply + * uses the same device tree information used to request the
> irq lines to call + * gpiochip_free on that GPIOs.
> + */
> +static void of_gpiochip_reserve_irq_lines(struct device_node *np,
> + struct gpio_chip *gc, bool request)
> +{
> + struct device_node *root;
> + const __be32 *tmp;
> + struct irq_domain *irq_domain;
> + u32 intsize;
> +
> + /*
> + * If this chip is not tagged as interrupt-controller, there is
> + * no problem so we just exit.
> + */
> + if (!of_property_read_bool(np, "interrupt-controller"))
> + return;
> +
> + /*
> + * Proceed to check the consistency of all references to this
> + * GPIO chip.
> + */
> + root = of_find_node_by_path("/");
> + if (!root)
> + return;
> +
> + tmp = of_get_property(np, "#interrupt-cells", NULL);
> + if (tmp == NULL)
> + intsize = 1;
> + else
> + intsize = be32_to_cpu(*tmp);
> +
> + irq_domain = irq_find_host(np);
I'm not sure you can do too much if irq_find_host() fails to find the
domain you are looking for. I guess you can just bail out in this case.
However...
I believe this imposes some ordering requirement between GPIO and IRQ chip
initialization. For this code to work correctly, all GPIO/IRQ controller
drivers would have to register the IRQ controller part first and only then
the GPIO chip.
Best regards,
Tomasz
On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> Hi Lars, Linus,
>
> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>> From: Linus Walleij <[email protected]>
>>
>> Currently the kernel is ambigously treating GPIOs and interrupts
>> from a GPIO controller: GPIOs and interrupts are treated as
>> orthogonal. This unfortunately makes it unclear how to actually
>> retrieve and request a GPIO line or interrupt from a GPIO
>> controller in the device tree probe path.
>>
>> In the non-DT probe path it is clear that the GPIO number has to
>> be passed to the consuming device, and if it is to be used as
>> an interrupt, the consumer shall performa a gpio_to_irq() mapping
>> and request the resulting IRQ number.
>>
>> In the DT boot path consumers may have been given one or more
>> interrupts from the interrupt-controller side of the GPIO chip
>> in an abstract way, such that the driver is not aware of the
>> fact that a GPIO chip is backing this interrupt, and the GPIO
>> side of the same line is never requested with gpio_request().
>> A typical case for this is ethernet chips which just take some
>> interrupt line which may be a "hard" interrupt line (such as an
>> external interrupt line from an SoC) or a cascaded interrupt
>> connected to a GPIO line.
>>
>> This has the following undesired effects:
>>
>> - The GPIOlib subsystem is not aware that the line is in use
>> and willingly lets some other consumer perform gpio_request()
>> on it, leading to a complex resource conflict if it occurs.
>>
>> - The GPIO debugfs file claims this GPIO line is "free".
>>
>> - The line direction of the interrupt GPIO line is not
>> explicitly set as input, even though it is obvious that such
>> a line need to be set up in this way, often making the system
>> depend on boot-on defaults for this kind of settings.
That last point should simply be taken care of by the IRQ driver in the
relevant callbacks.
>> To solve this dilemma, perform an interrupt consistency check
>> when adding a GPIO chip: if the chip is both gpio-controller and
>> interrupt-controller, walk all children of the device tree,
It seems a little odd to solve this only for DT. What about the non-DT case?
>> check if these in turn reference the interrupt-controller, and
>> if they do, loop over the interrupts used by that child and
>> perform gpio_request() and gpio_direction_input() on these,
>> making them unreachable from the GPIO side.
What about bindings that require a GPIO to be specified, yet don't allow
an IRQ to be specified, and the driver internally does perform
gpio_to_irq() on it? I don't think one can detect that case.
Isn't it better to have the IRQ chip's .request() operation convert the
IRQ to a GPIO if relevant (which it can do since it has specific
knowledge of the HW) and take ownership of the GPIO at that level?
That would cover both the exceptions I pointed out above.
I vaguely recall seeing patches along those lines before, but there must
have been some other problem pointed out...
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> +static void of_gpio_scan_irq_lines(const struct device_node *const
>> + for (i = 0; i < intlen; i += intsize) {
>> + /*
>> + * Find out the local IRQ number. This corresponds to
>> + * the GPIO line offset for a GPIO chip.
>
> I'm still not convinced that this assumption is correct. This code will
> behave erraticaly in cases where it is not true, requesting innocent GPIO
> pins.
>
>> + */
>> + if (irq_domain && irq_domain->ops->xlate)
>> + irq_domain->ops->xlate(irq_domain, gcn,
>> + intspec + i, intsize,
>> + &hwirq, &type);
>> + else
>> + hwirq = intspec[0];
>
> Is it a correct fallback when irq_domain is NULL?
Indeed this fallback is dangerous. The /only/ way to parse an IRQ
specifier is with binding-specific knowledge, which is obtained by
calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
operation simply has to be deferred; we can't just guess and hope.
>
>> +
>> + hwirq = be32_to_cpu(hwirq);
>
> Is this conversion correct? I don't think hwirq could be big endian here
> (unless running on a big endian CPU).
I think that should be inside the else branch above.
On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <[email protected]> wrote:
>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>>> To solve this dilemma, perform an interrupt consistency check
>>> when adding a GPIO chip: if the chip is both gpio-controller and
>>> interrupt-controller, walk all children of the device tree,
>
> It seems a little odd to solve this only for DT. What about the non-DT case?
DT is the hardware configuration system that lets you request
the same resource in two ways, i.e. it allows one and the same
node to be both gpio-controller and interrupt-controller, and
start handing out the same line as both GPIO and IRQ
independently.
I asked if ACPI had this ambiguity, and the answer appears to
be either "no" (which I suspect) or just "nobody knows" :-/
In either way, checking the consistency of ACPI IRQs vs
GPIOs will be fundamentally different, should it have the same
problem, and does it appear we can certainly refactor this to
be shared, should there be something to gain from.
Yours,
Linus Walleij
On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <[email protected]> wrote:
[Me]
>>> check if these in turn reference the interrupt-controller, and
>>> if they do, loop over the interrupts used by that child and
>>> perform gpio_request() and gpio_direction_input() on these,
>>> making them unreachable from the GPIO side.
>
> What about bindings that require a GPIO to be specified, yet don't allow
> an IRQ to be specified, and the driver internally does perform
> gpio_to_irq() on it? I don't think one can detect that case.
This is still allowed. Consumers that prefer to have a GPIO
passed and convert it to IRQ by that call can still do so,
they will know what they're doing and will not cause the
double-command situation that we're trying to solve.
> Isn't it better to have the IRQ chip's .request() operation convert the
> IRQ to a GPIO if relevant (which it can do since it has specific
> knowledge of the HW) and take ownership of the GPIO at that level?
We tried this in the OMAP case, but apart from that the OMAP
driver blew up so we had to revert the patches, it also means
the same code needs to go into each and every driver
instead of solving the dilemma centrally like this.
> I vaguely recall seeing patches along those lines before, but there must
> have been some other problem pointed out...
You bet. It turns out these patches break the case which you
just described above, whereas this patch does not.
OMAP had drivers that used gpio_to_irq() *and* it had drivers
that used the GPIO controller node as interrupt parent.
So when they fixes .request() as per above, the latter started
working properly whereas the former started breaking.
Yours,
Linus Walleij
On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
> On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> >> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >>
> >> +static void of_gpio_scan_irq_lines(const struct device_node *const
> >>
> >> + for (i = 0; i < intlen; i += intsize) {
> >> + /*
> >> + * Find out the local IRQ number. This corresponds to
> >> + * the GPIO line offset for a GPIO chip.
> >
> > I'm still not convinced that this assumption is correct. This code
> > will behave erraticaly in cases where it is not true, requesting
> > innocent GPIO pins.
Do you have an idea how we can destroy your doubts?
Either irq_chips nor irq_domains provide some sort of translation function
for this.
Is there a driver in the kernel that has different gpio- vs. irq-namespaces
where I can have a look at? How does platform code solve this translation?
The irq_to_gpio functions in include/linux/gpio.h seem deprecated. They
just return -EINVAL.
For me it seems, that there is no such device inside the kernel yet.
Correct me if I'm wrong. If such a device comes to surface, we're in
trouble. We will need some device-specific translation function then.
Is it the time to introduce an additional pointer for such a function now
and nobody uses it? Or wait until such a device arises and introduce the
pointer then?
> >> + */
> >> + if (irq_domain && irq_domain->ops->xlate)
> >> + irq_domain->ops->xlate(irq_domain, gcn,
> >> + intspec + i, intsize,
> >> + &hwirq, &type);
> >> + else
> >> + hwirq = intspec[0];
> >
> > Is it a correct fallback when irq_domain is NULL?
>
> Indeed this fallback is dangerous. The /only/ way to parse an IRQ
> specifier is with binding-specific knowledge, which is obtained by
> calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
> operation simply has to be deferred; we can't just guess and hope.
At least the of irq mapping code make this assumption also:
kernel/irq/irqdomain.c:483
It should be valid for us here too.
The additional assumption that I made is that if irq_domain == NULL (not
only xlate), that we can use intspec[0] either.
> >> +
> >> + hwirq = be32_to_cpu(hwirq);
> >
> > Is this conversion correct? I don't think hwirq could be big endian
> > here (unless running on a big endian CPU).
>
> I think that should be inside the else branch above.
No it has to be in both branches as it is. Device tree data is big endian.
The conversion is converting big endian data (from device tree in both
cases) to cpu endianess and not coverting TO big endian.
My test machine is a arm in little endian mode and it provided wrong values
if I did not do the conversion.
What I am a bit unsure about is if the xlate function is expecting the
intspec pointer to point to big endian device tree data or data already
converted to cpu endianess. For the standard xlate functions
irq_domain_xlate_[one|two|onetwo]cell it does not matter.
On 2013-08-21 15:38, Lars Poeschel wrote:
> +static void of_gpio_scan_irq_lines(const struct device_node *const node,
> + struct device_node *const gcn,
> + struct irq_domain *const irq_domain,
> + const u32 intsize,
> + const struct gpio_chip * const gc,
> + bool request)
> +{
> + struct device_node *child;
> + struct device_node *irq_parent;
> + const __be32 *intspec;
> + u32 intlen;
> + int ret;
> + int i;
> + irq_hw_number_t hwirq;
> + unsigned int type;
> +
> + if (node == NULL)
> + return;
> +
> + for_each_child_of_node(node, child) {
> + of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> + request);
> + /* Check if we have an IRQ parent, else continue */
> + irq_parent = of_irq_find_parent(child);
Hi!
This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
fact that the function is undefined when !defined(CONFIG_OF_IRQ) &&
defined(CONFIG_OF).
Defining the empty of_irq_find_parent in include/linux/of_irq.h when
!defined(CONFIG_OF_IRQ) instead of the current case when
!defined(CONFIG_OF) would solve the immediate compilation problem.
However, when !defined(CONFIG_OF_IRQ) (i.e. SPARC in this case) the
whole tree walking will never accomplish anything, so it would be good
if of_gpiochip_reserve_irq_lines is just an empty dummy or something
like that when !defined(CONFIG_OF_IRQ).
Cheers,
Andreas Larsson
On 08/21/2013 05:27 PM, Linus Walleij wrote:
> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <[email protected]> wrote:
>>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>
>>>> To solve this dilemma, perform an interrupt consistency check
>>>> when adding a GPIO chip: if the chip is both gpio-controller and
>>>> interrupt-controller, walk all children of the device tree,
>>
>> It seems a little odd to solve this only for DT. What about the non-DT case?
>
> DT is the hardware configuration system that lets you request
> the same resource in two ways, i.e. it allows one and the same
> node to be both gpio-controller and interrupt-controller, and
> start handing out the same line as both GPIO and IRQ
> independently.
Huh? What stops systems using board files and platform data from having
this issue?
On 08/22/2013 03:01 AM, Lars Poeschel wrote:
> On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
>> On 08/21/2013 03:49 PM, Tomasz Figa wrote:
>>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>>>> + */
>>>> + if (irq_domain && irq_domain->ops->xlate)
>>>> + irq_domain->ops->xlate(irq_domain, gcn,
>>>> + intspec + i, intsize,
>>>> + &hwirq, &type);
>>>> + else
>>>> + hwirq = intspec[0];
>>>
>>> Is it a correct fallback when irq_domain is NULL?
>>
>> Indeed this fallback is dangerous. The /only/ way to parse an IRQ
>> specifier is with binding-specific knowledge, which is obtained by
>> calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
>> operation simply has to be deferred; we can't just guess and hope.
>
> At least the of irq mapping code make this assumption also:
> kernel/irq/irqdomain.c:483
> It should be valid for us here too.
> The additional assumption that I made is that if irq_domain == NULL (not
> only xlate), that we can use intspec[0] either.
OK, I guess it's likely this won't cause any additional issue then. I
suspect most IRQ domains use within the context of device tree already
provide an explicit xlate op anyway; for example irq_domain_simple_ops
points at the default irq_domain_xlate_onetwocell.
>>>> +
>>>> + hwirq = be32_to_cpu(hwirq);
>>>
>>> Is this conversion correct? I don't think hwirq could be big endian
>>> here (unless running on a big endian CPU).
>>
>> I think that should be inside the else branch above.
>
> No it has to be in both branches as it is. Device tree data is big endian.
> The conversion is converting big endian data (from device tree in both
> cases) to cpu endianess and not coverting TO big endian.
> My test machine is a arm in little endian mode and it provided wrong values
> if I did not do the conversion.
> What I am a bit unsure about is if the xlate function is expecting the
> intspec pointer to point to big endian device tree data or data already
> converted to cpu endianess. For the standard xlate functions
> irq_domain_xlate_[one|two|onetwo]cell it does not matter.
The xlate function assumes that data is already converted to CPU-endian.
See:
irq_of_parse_and_map() ->
of_irq_map_one() ->
of_irq_map_raw() ->
out_irq->specifier[i] = of_read_number(intspec +i, 1);
irq_create_of_mapping()
(of_read_number does the be32_to_cpu() internally)
On 08/21/2013 05:36 PM, Linus Walleij wrote:
> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <[email protected]> wrote:
> [Me]
>>>> check if these in turn reference the interrupt-controller, and
>>>> if they do, loop over the interrupts used by that child and
>>>> perform gpio_request() and gpio_direction_input() on these,
>>>> making them unreachable from the GPIO side.
>>
>> What about bindings that require a GPIO to be specified, yet don't allow
>> an IRQ to be specified, and the driver internally does perform
>> gpio_to_irq() on it? I don't think one can detect that case.
>
> This is still allowed. Consumers that prefer to have a GPIO
> passed and convert it to IRQ by that call can still do so,
> they will know what they're doing and will not cause the
> double-command situation that we're trying to solve.
Why not? There are certainly drivers in the kernel which request a GPIO
as both a GPIO and as an (dual-edge) interrupt, so that they can read
the GPIO input whenever the IRQ goes off, in order to determine the pin
state. This is safer against high-latency or lost interrupts.
On Thursday 22 of August 2013 15:08:12 Stephen Warren wrote:
> On 08/22/2013 03:01 AM, Lars Poeschel wrote:
> > On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
> >> On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> >>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >>>>
> >>>> + */
> >>>> + if (irq_domain && irq_domain->ops->xlate)
> >>>> + irq_domain->ops->xlate(irq_domain,
gcn,
> >>>> + intspec +
i, intsize,
> >>>> + &hwirq,
&type);
> >>>> + else
> >>>> + hwirq = intspec[0];
> >>>
> >>> Is it a correct fallback when irq_domain is NULL?
> >>
> >> Indeed this fallback is dangerous. The /only/ way to parse an IRQ
> >> specifier is with binding-specific knowledge, which is obtained by
> >> calling irq_domain->ops->xlate(). If the IRQ domain can't be found,
> >> this operation simply has to be deferred; we can't just guess and
> >> hope.>
> > At least the of irq mapping code make this assumption also:
> > kernel/irq/irqdomain.c:483
> > It should be valid for us here too.
> > The additional assumption that I made is that if irq_domain == NULL
> > (not only xlate), that we can use intspec[0] either.
>
> OK, I guess it's likely this won't cause any additional issue then. I
> suspect most IRQ domains use within the context of device tree already
> provide an explicit xlate op anyway; for example irq_domain_simple_ops
> points at the default irq_domain_xlate_onetwocell.
We got away from the problem I pointed in my reply. If irq_domain == NULL,
there is no way to translate specifier to hwirq (and in what domain such
hwirq would be in anyway?).
> >>>> +
> >>>> + hwirq = be32_to_cpu(hwirq);
> >>>
> >>> Is this conversion correct? I don't think hwirq could be big endian
> >>> here (unless running on a big endian CPU).
> >>
> >> I think that should be inside the else branch above.
> >
> > No it has to be in both branches as it is. Device tree data is big
> > endian. The conversion is converting big endian data (from device
> > tree in both cases) to cpu endianess and not coverting TO big endian.
> > My test machine is a arm in little endian mode and it provided wrong
> > values if I did not do the conversion.
> > What I am a bit unsure about is if the xlate function is expecting the
> > intspec pointer to point to big endian device tree data or data
> > already
> > converted to cpu endianess. For the standard xlate functions
> > irq_domain_xlate_[one|two|onetwo]cell it does not matter.
>
> The xlate function assumes that data is already converted to CPU-endian.
> See:
>
> irq_of_parse_and_map() ->
> of_irq_map_one() ->
> of_irq_map_raw() ->
> out_irq->specifier[i] = of_read_number(intspec +i, 1);
> irq_create_of_mapping()
>
> (of_read_number does the be32_to_cpu() internally)
So basically to be correct, the code here would need to read the specifier
into internal buffer using a helper like of_read_number() or maybe even
of_property_read_u32_array() and then pass this buffer to xlate().
Best regards,
Tomasz
On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote:
> On 08/21/2013 05:36 PM, Linus Walleij wrote:
> > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
> > <[email protected]> wrote: [Me]
> >
> >>>> check if these in turn reference the interrupt-controller, and
> >>>> if they do, loop over the interrupts used by that child and
> >>>> perform gpio_request() and gpio_direction_input() on these,
> >>>> making them unreachable from the GPIO side.
> >>
> >> What about bindings that require a GPIO to be specified, yet don't
> >> allow an IRQ to be specified, and the driver internally does perform
> >> gpio_to_irq() on it? I don't think one can detect that case.
> >
> > This is still allowed. Consumers that prefer to have a GPIO
> > passed and convert it to IRQ by that call can still do so,
> > they will know what they're doing and will not cause the
> > double-command situation that we're trying to solve.
>
> Why not? There are certainly drivers in the kernel which request a GPIO
> as both a GPIO and as an (dual-edge) interrupt, so that they can read
> the GPIO input whenever the IRQ goes off, in order to determine the pin
> state. This is safer against high-latency or lost interrupts.
This is the point! They REQUEST the GPIO. They can then do whatever they
like with this GPIO then, even additionally use it as interrupt.
In the devicetree case the interrupts are not requested. This is what we
are trying to address with the patch. The device using this interrupt has
no idea where this interrupt comes from. Is it a gpio-interrupt or not?
Does it have to request a gpio before using this interrupt or not?
On Thursday 22 August 2013 at 22:53:09, Stephen Warren wrote:
> On 08/21/2013 05:27 PM, Linus Walleij wrote:
> > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <[email protected]>
wrote:
> >>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
> >>>> To solve this dilemma, perform an interrupt consistency check
> >>>> when adding a GPIO chip: if the chip is both gpio-controller and
> >>>> interrupt-controller, walk all children of the device tree,
> >>
> >> It seems a little odd to solve this only for DT. What about the
> >> non-DT case?
> >
> > DT is the hardware configuration system that lets you request
> > the same resource in two ways, i.e. it allows one and the same
> > node to be both gpio-controller and interrupt-controller, and
> > start handing out the same line as both GPIO and IRQ
> > independently.
>
> Huh? What stops systems using board files and platform data from having
> this issue?
I am not 100% sure, Linus knows better.
I think nothing stops them from having this issue, but board files are
gentle and request the GPIO before doing gpio_to_irq, because they know
that they are using a gpio based interrupt.
You can read the whole story here:
http://www.mail-archive.com/[email protected]/msg91405.html
Things get interesting after the first mail from Alexander Holler, who is
the first having a problem with the patch in the link.
On Thu, Aug 22, 2013 at 10:53 PM, Stephen Warren <[email protected]> wrote:
> On 08/21/2013 05:27 PM, Linus Walleij wrote:
>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <[email protected]> wrote:
>>>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>>
>>>>> To solve this dilemma, perform an interrupt consistency check
>>>>> when adding a GPIO chip: if the chip is both gpio-controller and
>>>>> interrupt-controller, walk all children of the device tree,
>>>
>>> It seems a little odd to solve this only for DT. What about the non-DT case?
>>
>> DT is the hardware configuration system that lets you request
>> the same resource in two ways, i.e. it allows one and the same
>> node to be both gpio-controller and interrupt-controller, and
>> start handing out the same line as both GPIO and IRQ
>> independently.
>
> Huh? What stops systems using board files and platform data from having
> this issue?
It can't be stopped but I consider it a bug if they do, as the proper
way to handle such GPIO lines is the sequence:
request_gpio(gpio);
request_irq(gpio_to_irq(gpio));
But I was mainly contrasting against ACPI, where the problem appears
to not exist.
Yours,
Linus Walleij
On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren <[email protected]> wrote:
> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <[email protected]> wrote:
>> [Me]
>>>>> check if these in turn reference the interrupt-controller, and
>>>>> if they do, loop over the interrupts used by that child and
>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>> making them unreachable from the GPIO side.
>>>
>>> What about bindings that require a GPIO to be specified, yet don't allow
>>> an IRQ to be specified, and the driver internally does perform
>>> gpio_to_irq() on it? I don't think one can detect that case.
>>
>> This is still allowed. Consumers that prefer to have a GPIO
>> passed and convert it to IRQ by that call can still do so,
>> they will know what they're doing and will not cause the
>> double-command situation that we're trying to solve.
>
> Why not? There are certainly drivers in the kernel which request a GPIO
> as both a GPIO and as an (dual-edge) interrupt, so that they can read
> the GPIO input whenever the IRQ goes off, in order to determine the pin
> state. This is safer against high-latency or lost interrupts.
Yes? Are we talking past each other here?
This is a perfectly OK thing to do as long as it is done like
this:
request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));
Pass only the GPIO in the device tree and this works just fine.
The use case after that we do not interfer with.
Yours,
Linus Walleij
On 08/23/2013 03:40 AM, Lars Poeschel wrote:
> On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote:
>> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
>>> <[email protected]> wrote: [Me]
>>>
>>>>>> check if these in turn reference the interrupt-controller, and
>>>>>> if they do, loop over the interrupts used by that child and
>>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>>> making them unreachable from the GPIO side.
>>>>
>>>> What about bindings that require a GPIO to be specified, yet don't
>>>> allow an IRQ to be specified, and the driver internally does perform
>>>> gpio_to_irq() on it? I don't think one can detect that case.
>>>
>>> This is still allowed. Consumers that prefer to have a GPIO
>>> passed and convert it to IRQ by that call can still do so,
>>> they will know what they're doing and will not cause the
>>> double-command situation that we're trying to solve.
>>
>> Why not? There are certainly drivers in the kernel which request a GPIO
>> as both a GPIO and as an (dual-edge) interrupt, so that they can read
>> the GPIO input whenever the IRQ goes off, in order to determine the pin
>> state. This is safer against high-latency or lost interrupts.
>
> This is the point! They REQUEST the GPIO. They can then do whatever they
> like with this GPIO then, even additionally use it as interrupt.
> In the devicetree case the interrupts are not requested. This is what we
> are trying to address with the patch. The device using this interrupt has
> no idea where this interrupt comes from. Is it a gpio-interrupt or not?
> Does it have to request a gpio before using this interrupt or not?
If the kernel automatically requests the GPIO because it's referenced as
an interrupt then surely if the driver also requests it, then it will
fail, because the GPIO is already requested. Or, is there an explicit
check for that?
Is the problem you're trying to solve really an actual problem? I'm not
convinced it's necessary for the GPIO to show up as requested if the pin
is used as an IRQ input?
On 08/23/2013 12:38 PM, Linus Walleij wrote:
> On Thu, Aug 22, 2013 at 10:53 PM, Stephen Warren <[email protected]> wrote:
>> On 08/21/2013 05:27 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <[email protected]> wrote:
>>>>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>>>
>>>>>> To solve this dilemma, perform an interrupt consistency check
>>>>>> when adding a GPIO chip: if the chip is both gpio-controller and
>>>>>> interrupt-controller, walk all children of the device tree,
>>>>
>>>> It seems a little odd to solve this only for DT. What about the non-DT case?
>>>
>>> DT is the hardware configuration system that lets you request
>>> the same resource in two ways, i.e. it allows one and the same
>>> node to be both gpio-controller and interrupt-controller, and
>>> start handing out the same line as both GPIO and IRQ
>>> independently.
>>
>> Huh? What stops systems using board files and platform data from having
>> this issue?
>
> It can't be stopped but I consider it a bug if they do, as the proper
> way to handle such GPIO lines is the sequence:
>
> request_gpio(gpio);
> request_irq(gpio_to_irq(gpio));
Back in the old days of ARM board files, there were many boards that
didn't do this. I guess that doesn't make it any less of a bug, but it
certainly implies to me that solving this in a way that caters to that
bug being present will be a lot more useful.
On 08/23/2013 12:45 PM, Linus Walleij wrote:
> On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren <[email protected]> wrote:
>> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <[email protected]> wrote:
>>> [Me]
>>>>>> check if these in turn reference the interrupt-controller, and
>>>>>> if they do, loop over the interrupts used by that child and
>>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>>> making them unreachable from the GPIO side.
>>>>
>>>> What about bindings that require a GPIO to be specified, yet don't allow
>>>> an IRQ to be specified, and the driver internally does perform
>>>> gpio_to_irq() on it? I don't think one can detect that case.
>>>
>>> This is still allowed. Consumers that prefer to have a GPIO
>>> passed and convert it to IRQ by that call can still do so,
>>> they will know what they're doing and will not cause the
>>> double-command situation that we're trying to solve.
>>
>> Why not? There are certainly drivers in the kernel which request a GPIO
>> as both a GPIO and as an (dual-edge) interrupt, so that they can read
>> the GPIO input whenever the IRQ goes off, in order to determine the pin
>> state. This is safer against high-latency or lost interrupts.
>
> Yes? Are we talking past each other here?
>
> This is a perfectly OK thing to do as long as it is done like
> this:
>
> request_gpio(gpio);
> gpio_direction_input(gpio);
> request_irq(gpio_to_irq(gpio));
But I'm not aware that there's a rule saying it's illegal to:
request_irq(gpio_to_irq(gpio));
request_gpio(gpio);
gpio_direction_input(gpio);
> Pass only the GPIO in the device tree and this works just fine.
And I wouldn't be surprised if there were DTs that had separate GPIO and
interrupt entries for the same pin. In fact, it's arguably technically
more correct to do that than just list the GPIO, and then hope the OS
will be able to convert it to the correct IRQ. Then, drivers wouldn't
have any reason to believe they needed a specific IRQ-vs-GPIO request
ordering.
On Friday 23 of August 2013 13:52:20 Stephen Warren wrote:
> On 08/23/2013 12:45 PM, Linus Walleij wrote:
> > On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren
<[email protected]> wrote:
> >> On 08/21/2013 05:36 PM, Linus Walleij wrote:
> >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
> >>> <[email protected]> wrote: [Me]
> >>>
> >>>>>> check if these in turn reference the interrupt-controller, and
> >>>>>> if they do, loop over the interrupts used by that child and
> >>>>>> perform gpio_request() and gpio_direction_input() on these,
> >>>>>> making them unreachable from the GPIO side.
> >>>>
> >>>> What about bindings that require a GPIO to be specified, yet don't
> >>>> allow an IRQ to be specified, and the driver internally does
> >>>> perform gpio_to_irq() on it? I don't think one can detect that
> >>>> case.
> >>>
> >>> This is still allowed. Consumers that prefer to have a GPIO
> >>> passed and convert it to IRQ by that call can still do so,
> >>> they will know what they're doing and will not cause the
> >>> double-command situation that we're trying to solve.
> >>
> >> Why not? There are certainly drivers in the kernel which request a
> >> GPIO
> >> as both a GPIO and as an (dual-edge) interrupt, so that they can read
> >> the GPIO input whenever the IRQ goes off, in order to determine the
> >> pin
> >> state. This is safer against high-latency or lost interrupts.
> >
> > Yes? Are we talking past each other here?
> >
> > This is a perfectly OK thing to do as long as it is done like
> > this:
> >
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > request_irq(gpio_to_irq(gpio));
>
> But I'm not aware that there's a rule saying it's illegal to:
>
> request_irq(gpio_to_irq(gpio));
> request_gpio(gpio);
> gpio_direction_input(gpio);
Well, at least on Samsung platforms it is illegal to do so, because
gpio_direction_input() would override the interrupt+input function set by
setup done in request_irq() with normal input function, thus breaking the
interrupt.
We are still to implement some sanity check to disallow (or ignore) this
if the pin is already configured as an interrupt.
Best regards,
Tomasz
On 08/23/2013 01:55 PM, Tomasz Figa wrote:
> On Friday 23 of August 2013 13:52:20 Stephen Warren wrote:
>> On 08/23/2013 12:45 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren
> <[email protected]> wrote:
>>>> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>>>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
>>>>> <[email protected]> wrote: [Me]
>>>>>
>>>>>>>> check if these in turn reference the interrupt-controller, and
>>>>>>>> if they do, loop over the interrupts used by that child and
>>>>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>>>>> making them unreachable from the GPIO side.
>>>>>>
>>>>>> What about bindings that require a GPIO to be specified, yet don't
>>>>>> allow an IRQ to be specified, and the driver internally does
>>>>>> perform gpio_to_irq() on it? I don't think one can detect that
>>>>>> case.
>>>>>
>>>>> This is still allowed. Consumers that prefer to have a GPIO
>>>>> passed and convert it to IRQ by that call can still do so,
>>>>> they will know what they're doing and will not cause the
>>>>> double-command situation that we're trying to solve.
>>>>
>>>> Why not? There are certainly drivers in the kernel which request a
>>>> GPIO
>>>> as both a GPIO and as an (dual-edge) interrupt, so that they can read
>>>> the GPIO input whenever the IRQ goes off, in order to determine the
>>>> pin
>>>> state. This is safer against high-latency or lost interrupts.
>>>
>>> Yes? Are we talking past each other here?
>>>
>>> This is a perfectly OK thing to do as long as it is done like
>>> this:
>>>
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>> request_irq(gpio_to_irq(gpio));
>>
>> But I'm not aware that there's a rule saying it's illegal to:
>>
>> request_irq(gpio_to_irq(gpio));
>> request_gpio(gpio);
>> gpio_direction_input(gpio);
>
> Well, at least on Samsung platforms it is illegal to do so, because
> gpio_direction_input() would override the interrupt+input function set by
> setup done in request_irq() with normal input function, thus breaking the
> interrupt.
Assuming that Linux has no general rule that requires a specific order,
isn't that simply a bug in the Samsung platforms? After all, a
completely generic cross-platform driver, which could touch both GPIO
and IRQ, would be written to any Linux-imposed rules, not any
Samsung-platform-imposed rules.
> We are still to implement some sanity check to disallow (or ignore) this
> if the pin is already configured as an interrupt.
OK good, so it sounds like this is a temporary issue.
On Friday 23 August 2013 at 21:48:43, Stephen Warren wrote:
> On 08/23/2013 03:40 AM, Lars Poeschel wrote:
> > On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote:
> >> On 08/21/2013 05:36 PM, Linus Walleij wrote:
> >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
> >>> <[email protected]> wrote: [Me]
> >>>
> >>>>>> check if these in turn reference the interrupt-controller, and
> >>>>>> if they do, loop over the interrupts used by that child and
> >>>>>> perform gpio_request() and gpio_direction_input() on these,
> >>>>>> making them unreachable from the GPIO side.
> >>>>
> >>>> What about bindings that require a GPIO to be specified, yet don't
> >>>> allow an IRQ to be specified, and the driver internally does
> >>>> perform gpio_to_irq() on it? I don't think one can detect that
> >>>> case.
> >>>
> >>> This is still allowed. Consumers that prefer to have a GPIO
> >>> passed and convert it to IRQ by that call can still do so,
> >>> they will know what they're doing and will not cause the
> >>> double-command situation that we're trying to solve.
> >>
> >> Why not? There are certainly drivers in the kernel which request a
> >> GPIO as both a GPIO and as an (dual-edge) interrupt, so that they
> >> can read the GPIO input whenever the IRQ goes off, in order to
> >> determine the pin state. This is safer against high-latency or lost
> >> interrupts.
> >
> > This is the point! They REQUEST the GPIO. They can then do whatever
> > they like with this GPIO then, even additionally use it as interrupt.
> > In the devicetree case the interrupts are not requested. This is what
> > we are trying to address with the patch. The device using this
> > interrupt has no idea where this interrupt comes from. Is it a
> > gpio-interrupt or not? Does it have to request a gpio before using
> > this interrupt or not?
>
> If the kernel automatically requests the GPIO because it's referenced as
> an interrupt then surely if the driver also requests it, then it will
> fail, because the GPIO is already requested. Or, is there an explicit
> check for that?
There is no explicit check, because it is not wanted! Drivers that only
want the interrupt do not know that it is gpio-backed and therefore can not
request it.
Drivers wanting to explicit use a gpio for interrupt, request the gpio in
the device tree. They can then turn that gpio into an interrupt.
> Is the problem you're trying to solve really an actual problem? I'm not
> convinced it's necessary for the GPIO to show up as requested if the pin
> is used as an IRQ input?
It is definitely an actual problem. Please read the thread I already sent!
Surely it is not necessary for the GPIO to show up as requested but it does
not harm it is even neat if it shows up. But it HAS to be requested so that
no other entity can request it and change it's configuration i.e. turn it
to an output.
On Friday 23 August 2013 at 21:52:20, Stephen Warren wrote:
> On 08/23/2013 12:45 PM, Linus Walleij wrote:
> > On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren
<[email protected]> wrote:
> >> On 08/21/2013 05:36 PM, Linus Walleij wrote:
> >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
> >>> <[email protected]> wrote: [Me]
> >>>
> >>>>>> check if these in turn reference the interrupt-controller, and
> >>>>>> if they do, loop over the interrupts used by that child and
> >>>>>> perform gpio_request() and gpio_direction_input() on these,
> >>>>>> making them unreachable from the GPIO side.
> >>>>
> >>>> What about bindings that require a GPIO to be specified, yet don't
> >>>> allow an IRQ to be specified, and the driver internally does
> >>>> perform gpio_to_irq() on it? I don't think one can detect that
> >>>> case.
> >>>
> >>> This is still allowed. Consumers that prefer to have a GPIO
> >>> passed and convert it to IRQ by that call can still do so,
> >>> they will know what they're doing and will not cause the
> >>> double-command situation that we're trying to solve.
> >>
> >> Why not? There are certainly drivers in the kernel which request a
> >> GPIO as both a GPIO and as an (dual-edge) interrupt, so that they
> >> can read the GPIO input whenever the IRQ goes off, in order to
> >> determine the pin state. This is safer against high-latency or lost
> >> interrupts.
> >
> > Yes? Are we talking past each other here?
> >
> > This is a perfectly OK thing to do as long as it is done like
> > this:
> >
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > request_irq(gpio_to_irq(gpio));
>
> But I'm not aware that there's a rule saying it's illegal to:
>
> request_irq(gpio_to_irq(gpio));
> request_gpio(gpio);
> gpio_direction_input(gpio);
But I'd consider this as a bug. What if the scheduler interrupts you right
after you requested (and got assigned) the interrupt and another entity
requests your gpio? Then you'd have a resource conflict, because you are
not the owner of the gpio you requested an interrupt for.
Hi Andreas!
On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
> On 2013-08-21 15:38, Lars Poeschel wrote:
> > +static void of_gpio_scan_irq_lines(const struct device_node *const
> > node, + struct device_node *const gcn,
> > + struct irq_domain *const irq_domain,
> > + const u32 intsize,
> > + const struct gpio_chip * const gc,
> > + bool request)
> > +{
> > + struct device_node *child;
> > + struct device_node *irq_parent;
> > + const __be32 *intspec;
> > + u32 intlen;
> > + int ret;
> > + int i;
> > + irq_hw_number_t hwirq;
> > + unsigned int type;
> > +
> > + if (node == NULL)
> > + return;
> > +
> > + for_each_child_of_node(node, child) {
> > + of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> > + request);
> > + /* Check if we have an IRQ parent, else continue */
> > + irq_parent = of_irq_find_parent(child);
>
> Hi!
>
> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
> fact that the function is undefined when !defined(CONFIG_OF_IRQ) &&
> defined(CONFIG_OF).
>
> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
> !defined(CONFIG_OF_IRQ) instead of the current case when
> !defined(CONFIG_OF) would solve the immediate compilation problem.
Is this a bug and should be fixed ?
> However, when !defined(CONFIG_OF_IRQ) (i.e. SPARC in this case) the
> whole tree walking will never accomplish anything, so it would be good
> if of_gpiochip_reserve_irq_lines is just an empty dummy or something
> like that when !defined(CONFIG_OF_IRQ).
You are right. I'll consider this in the next spin.
Thanks,
Lars
On 2013-08-26 12:56, Lars Poeschel wrote:
> Hi Andreas!
>
> On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
>> On 2013-08-21 15:38, Lars Poeschel wrote:
>>> +static void of_gpio_scan_irq_lines(const struct device_node *const
>>> node, + struct device_node *const gcn,
>>> + struct irq_domain *const irq_domain,
>>> + const u32 intsize,
>>> + const struct gpio_chip * const gc,
>>> + bool request)
>>> +{
>>> + struct device_node *child;
>>> + struct device_node *irq_parent;
>>> + const __be32 *intspec;
>>> + u32 intlen;
>>> + int ret;
>>> + int i;
>>> + irq_hw_number_t hwirq;
>>> + unsigned int type;
>>> +
>>> + if (node == NULL)
>>> + return;
>>> +
>>> + for_each_child_of_node(node, child) {
>>> + of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
>>> + request);
>>> + /* Check if we have an IRQ parent, else continue */
>>> + irq_parent = of_irq_find_parent(child);
>>
>> Hi!
>>
>> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
>> fact that the function is undefined when !defined(CONFIG_OF_IRQ) &&
>> defined(CONFIG_OF).
>>
>> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
>> !defined(CONFIG_OF_IRQ) instead of the current case when
>> !defined(CONFIG_OF) would solve the immediate compilation problem.
>
> Is this a bug and should be fixed ?
Well, at least as soon as anyone tries to use in a context that does not
exclude SPARC it creates a bug, so I would say so. There is no reason
for SPARC to fall between the chairs. This is the first case I am aware
of that triggers this.
Cheers,
Andreas
On Monday 26 August 2013 at 13:29:07, Andreas Larsson wrote:
> On 2013-08-26 12:56, Lars Poeschel wrote:
> > Hi Andreas!
> >
> > On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
> >> On 2013-08-21 15:38, Lars Poeschel wrote:
> >>> +static void of_gpio_scan_irq_lines(const struct device_node *const
> >>> node, + struct device_node *const gcn,
> >>> + struct irq_domain *const irq_domain,
> >>> + const u32 intsize,
> >>> + const struct gpio_chip * const gc,
> >>> + bool request)
> >>> +{
> >>> + struct device_node *child;
> >>> + struct device_node *irq_parent;
> >>> + const __be32 *intspec;
> >>> + u32 intlen;
> >>> + int ret;
> >>> + int i;
> >>> + irq_hw_number_t hwirq;
> >>> + unsigned int type;
> >>> +
> >>> + if (node == NULL)
> >>> + return;
> >>> +
> >>> + for_each_child_of_node(node, child) {
> >>> + of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> >>> + request);
> >>> + /* Check if we have an IRQ parent, else continue */
> >>> + irq_parent = of_irq_find_parent(child);
> >>
> >> Hi!
> >>
> >> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to
> >> the fact that the function is undefined when !defined(CONFIG_OF_IRQ)
> >> && defined(CONFIG_OF).
> >>
> >> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
> >> !defined(CONFIG_OF_IRQ) instead of the current case when
> >> !defined(CONFIG_OF) would solve the immediate compilation problem.
> >
> > Is this a bug and should be fixed ?
>
> Well, at least as soon as anyone tries to use in a context that does not
> exclude SPARC it creates a bug, so I would say so. There is no reason
> for SPARC to fall between the chairs. This is the first case I am aware
> of that triggers this.
I also think this should be fixed.
Are you able to do a patch that fixes this and submit to the relevant
people?
Regards,
Lars
On 2013-08-26 16:04, Lars Poeschel wrote:
> On Monday 26 August 2013 at 13:29:07, Andreas Larsson wrote:
>> On 2013-08-26 12:56, Lars Poeschel wrote:
>>>> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to
>>>> the fact that the function is undefined when !defined(CONFIG_OF_IRQ)
>>>> && defined(CONFIG_OF).
>>>>
>>>> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
>>>> !defined(CONFIG_OF_IRQ) instead of the current case when
>>>> !defined(CONFIG_OF) would solve the immediate compilation problem.
>>>
>>> Is this a bug and should be fixed ?
>>
>> Well, at least as soon as anyone tries to use in a context that does not
>> exclude SPARC it creates a bug, so I would say so. There is no reason
>> for SPARC to fall between the chairs. This is the first case I am aware
>> of that triggers this.
>
> I also think this should be fixed.
> Are you able to do a patch that fixes this and submit to the relevant
> people?
Sure!
Cheers,
Andreas
On 08/26/2013 04:45 AM, Lars Poeschel wrote:
> On Friday 23 August 2013 at 21:52:20, Stephen Warren wrote:
>> On 08/23/2013 12:45 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren
> <[email protected]> wrote:
>>>> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>>>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
>>>>> <[email protected]> wrote: [Me]
>>>>>
>>>>>>>> check if these in turn reference the interrupt-controller, and
>>>>>>>> if they do, loop over the interrupts used by that child and
>>>>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>>>>> making them unreachable from the GPIO side.
>>>>>>
>>>>>> What about bindings that require a GPIO to be specified, yet don't
>>>>>> allow an IRQ to be specified, and the driver internally does
>>>>>> perform gpio_to_irq() on it? I don't think one can detect that
>>>>>> case.
>>>>>
>>>>> This is still allowed. Consumers that prefer to have a GPIO
>>>>> passed and convert it to IRQ by that call can still do so,
>>>>> they will know what they're doing and will not cause the
>>>>> double-command situation that we're trying to solve.
>>>>
>>>> Why not? There are certainly drivers in the kernel which request a
>>>> GPIO as both a GPIO and as an (dual-edge) interrupt, so that they
>>>> can read the GPIO input whenever the IRQ goes off, in order to
>>>> determine the pin state. This is safer against high-latency or lost
>>>> interrupts.
>>>
>>> Yes? Are we talking past each other here?
>>>
>>> This is a perfectly OK thing to do as long as it is done like
>>> this:
>>>
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>> request_irq(gpio_to_irq(gpio));
>>
>> But I'm not aware that there's a rule saying it's illegal to:
>>
>> request_irq(gpio_to_irq(gpio));
>> request_gpio(gpio);
>> gpio_direction_input(gpio);
>
> But I'd consider this as a bug. What if the scheduler interrupts you right
> after you requested (and got assigned) the interrupt and another entity
> requests your gpio? Then you'd have a resource conflict, because you are
> not the owner of the gpio you requested an interrupt for.
How is that any different from two drivers both attempting to claim a
GPIO, and there being a race to get their first?
Presumably, all this happens in the device's/driver's probe(), and hence
if the gpio_request() fails, then probe() fails, and undoes all allocations.
Your argument can equally be applied to the first case where the GPIO is
requested first, then the IRQ later. What if driver A requests the GPIO
and then attempts to request the IRQ, yet the scheduler causes driver B
to *just* request the (same) IRQ (because it only cares about using it
as an IRQ and doesn't even know it's a GPIO). Then, you have the exact
same problem in reverse.
The only possible way to solve this is for either request_irq() or
request_gpio() to take complete ownership of the GPIO that backs the IRQ
if there is one, yet allow request_gpio by the same driver on that same
GPIO to still succeed, so that the order doesn't matter; whatever the
driver first always claims the GPIO and disallows any other driver from
claiming "part of the GPIO".
Yet, in your other reply you explicitly said there isn't a check for
this (the same driver claiming both the IRQ and GPIO).
Perhaps it would help to lay out exactly the problem this is trying to
solve and why it solves it again.
On Fri, Aug 23, 2013 at 9:49 PM, Stephen Warren <[email protected]> wrote:
> On 08/23/2013 12:38 PM, Linus Walleij wrote:
>> It can't be stopped but I consider it a bug if they do, as the proper
>> way to handle such GPIO lines is the sequence:
>>
>> request_gpio(gpio);
>> request_irq(gpio_to_irq(gpio));
>
> Back in the old days of ARM board files, there were many boards that
> didn't do this. I guess that doesn't make it any less of a bug, but it
> certainly implies to me that solving this in a way that caters to that
> bug being present will be a lot more useful.
I was more thinking along the lines of trying to avoid the
unpleasant lack of control when doing this with platform
data and hard-coded GPIO numbers by restricting the way
it can be done in the device tree.
Hoping that there were no offenders in there already ...
I guess it has to hit linux-next before we know that.
Yours,
Linus Walleij
On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren <[email protected]> wrote:
> On 08/23/2013 12:45 PM, Linus Walleij wrote:
>> This is a perfectly OK thing to do as long as it is done like
>> this:
>>
>> request_gpio(gpio);
>> gpio_direction_input(gpio);
>> request_irq(gpio_to_irq(gpio));
>
> But I'm not aware that there's a rule saying it's illegal to:
>
> request_irq(gpio_to_irq(gpio));
> request_gpio(gpio);
> gpio_direction_input(gpio);
No but I think there should be one ... maybe I'm an oddball
but it seems natural to request a GPIO before tying
IRQs to fire off it.
Besides, the bug that this is trying to solve is due to the
fact that the OMAP driver require exactly the former order
for the IRQs to work. I think if the sequence matters it is
likely to be the first form, but it's a rough guess.
>> Pass only the GPIO in the device tree and this works just fine.
>
> And I wouldn't be surprised if there were DTs that had separate GPIO and
> interrupt entries for the same pin.
That is the situation we want to catch.
Don't you agree that it has some bit of ambiguity around it, like
the tree makes an assumtion that whether you ask for the GPIO
line or IRQ first does not matter, and leave it up to the driver to
"do something" if the order suddenly turns out the other way around,
but is important to the hardware?
I think we can't get away from the ambition to define this
semantic for all DT systems.
> In fact, it's arguably technically
> more correct to do that than just list the GPIO, and then hope the OS
> will be able to convert it to the correct IRQ. Then, drivers wouldn't
> have any reason to believe they needed a specific IRQ-vs-GPIO request
> ordering.
If that is more technically correct (hm I wonder what measure is
used for "correctness" here) I think we are back at the GPIO input
hogs to achive what OMAP needs. Using such hogs they can
let the gpiochip node hog a few pins and set them as input rather
than each and every driver having to do so.
Yours,
Linus Walleij
On 08/29/2013 01:00 PM, Linus Walleij wrote:
> On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren <[email protected]> wrote:
>> On 08/23/2013 12:45 PM, Linus Walleij wrote:
>
>>> This is a perfectly OK thing to do as long as it is done like
>>> this:
>>>
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>> request_irq(gpio_to_irq(gpio));
>>
>> But I'm not aware that there's a rule saying it's illegal to:
>>
>> request_irq(gpio_to_irq(gpio));
>> request_gpio(gpio);
>> gpio_direction_input(gpio);
>
> No but I think there should be one ... maybe I'm an oddball
> but it seems natural to request a GPIO before tying
> IRQs to fire off it.
What if there is no GPIO?
There are plenty of chips with dedicated IRQ input pins that can't be
read as GPIOs, or treated as GPIOs in any way.
If a driver only needs IRQ input functionality, it should just request
an IRQ and be done with it. There should be no need at all for the
driver to know that the IRQ might be routed into a GPIO controller, and
hence that the driver may (or may not) need to additionally request the
GPIO before requesting the IRQ.
In other words, request_irq() must do everything necessary for the input
signal to operate as an IRQ input, irrespective of whether it might be
possible to use that input signal as a GPIO.
Am Freitag, 30. August 2013, 14:08:41 schrieb Stephen Warren:
> On 08/29/2013 01:00 PM, Linus Walleij wrote:
> > On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren <[email protected]>
wrote:
> >> On 08/23/2013 12:45 PM, Linus Walleij wrote:
> >>> This is a perfectly OK thing to do as long as it is done like
> >>> this:
> >>>
> >>> request_gpio(gpio);
> >>> gpio_direction_input(gpio);
> >>> request_irq(gpio_to_irq(gpio));
> >>
> >> But I'm not aware that there's a rule saying it's illegal to:
> >>
> >> request_irq(gpio_to_irq(gpio));
> >> request_gpio(gpio);
> >> gpio_direction_input(gpio);
> >
> > No but I think there should be one ... maybe I'm an oddball
> > but it seems natural to request a GPIO before tying
> > IRQs to fire off it.
>
> What if there is no GPIO?
If there is no GPIO there is no gpio-controller and there is no problem.
> There are plenty of chips with dedicated IRQ input pins that can't be
> read as GPIOs, or treated as GPIOs in any way.
>
> If a driver only needs IRQ input functionality, it should just request
> an IRQ and be done with it. There should be no need at all for the
> driver to know that the IRQ might be routed into a GPIO controller, and
> hence that the driver may (or may not) need to additionally request the
> GPIO before requesting the IRQ.
Yes, you're right, but reality is different. Legacy drivers / board-files do:
request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));
> In other words, request_irq() must do everything necessary for the input
> signal to operate as an IRQ input, irrespective of whether it might be
> possible to use that input signal as a GPIO.
On Fri, Aug 30, 2013 at 10:08 PM, Stephen Warren <[email protected]> wrote:
> On 08/29/2013 01:00 PM, Linus Walleij wrote:
>> No but I think there should be one ... maybe I'm an oddball
>> but it seems natural to request a GPIO before tying
>> IRQs to fire off it.
>
> What if there is no GPIO?
>
> There are plenty of chips with dedicated IRQ input pins that can't be
> read as GPIOs, or treated as GPIOs in any way.
I'm not following this. In that case it is not tagged as
gpio-controller right? The patch is to gpiolib.c.
Do you mean chips where some part of it is GPIO and
another part is IRQ-only?
Should we not in that case create an MFD device that spawns
one GPIO device and then another irqchip device, or possibly
have the IRQ handling directly in the MFD device, as
ab8500-core.c does?
> If a driver only needs IRQ input functionality, it should just request
> an IRQ and be done with it. There should be no need at all for the
> driver to know that the IRQ might be routed into a GPIO controller, and
> hence that the driver may (or may not) need to additionally request the
> GPIO before requesting the IRQ.
This is what the patch is trying to achieve, for the DT use case.
> In other words, request_irq() must do everything necessary for the input
> signal to operate as an IRQ input, irrespective of whether it might be
> possible to use that input signal as a GPIO.
That is not the case for a bunch of OMAP drivers today, and their
attempt to fix that inside the irqchip callback backfired since it
was mutually exclusive with requesting the gpio first.
We have to encode some semantic into this, it's just a matter of
which one shall win, the APIs as they stand are ambiguous wrt
call sequence.
Yours,
Linus Walleij