Re: [PATCH V4 3/5] gpio: gpio-xilinx: Add interrupt support
From: Linus Walleij <hidden>
Date: 2021-01-07 09:27:59
Also in:
linux-arm-kernel, lkml
On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli [off-list ref] wrote:
Adds interrupt support to the Xilinx GPIO driver so that rising and falling edge line events can be supported. Since interrupt support is an optional feature in the Xilinx IP, the driver continues to support devices which have no interrupt provided. Depends on OF_GPIO framework for of_xlate function to translate gpiospec to the GPIO number and flags. Signed-off-by: Robert Hancock <redacted> Signed-off-by: Shubhrajyoti Datta <redacted> Signed-off-by: Srinivas Neeli <redacted>
(...)
config GPIO_XILINX
tristate "Xilinx GPIO support"
+ select GPIOLIB_IRQCHIP
+ select IRQ_DOMAIN_HIERARCHYWhy do you select IRQ_DOMAIN_HIERARCHY? You don't seem to use it.
+#include <linux/of_gpio.h>
No modern driver should include this header. I suspect you can just drop it. See drivers/gpio/TODO
quoted hunk ↗ jump to hunk
+ /* Update cells with gpio-cells value */ + if (of_property_read_u32(np, "#gpio-cells", &cells)) + dev_dbg(&pdev->dev, "Missing gpio-cells property\n"); + /* * Check device node and parent device node for device width * and assume default width of 32@@ -343,6 +545,7 @@ static int xgpio_probe(struct platform_device *pdev) chip->gc.parent = &pdev->dev; chip->gc.direction_input = xgpio_dir_in; chip->gc.direction_output = xgpio_dir_out; + chip->gc.of_gpio_n_cells = cells;
This looks a bit dangerous. What actually happens if this is something like 3. The translate function is not going to work. I would just dev_err() and bail out if #gpio-cells != 2. Other than that this looks good! Yours, Linus Walleij