Hi Linus, 2017-08-07 21:42 GMT+09:00 Linus Walleij <linus.walleij@xxxxxxxxxx>: > On Thu, Aug 3, 2017 at 1:06 PM, Masahiro Yamada > <yamada.masahiro@xxxxxxxxxxxxx> wrote: > >> This GPIO controller device is used on UniPhier SoCs. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> > (...) >> +static const u32 uniphier_gpio_irq_parent_hwirqs[] = { >> + 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, >> + 186, 187, 188, 189, 190, 249, 250, 251, >> +}; > (..) >> + /* parent is UniPhier AIDET */ >> + parent_fwspec.fwnode = domain->parent->fwnode; >> + parent_fwspec.param_count = 2; >> + parent_fwspec.param[0] = uniphier_gpio_irq_parent_hwirqs[hwirq]; >> + parent_fwspec.param[1] = type; >> + >> + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, >> + &parent_fwspec); > > This is not right. > > That parent IRQ needs to come from the device tree, not from a static array > like this, and it is definately to use hierarchical irqdomain. > > Please look at similar drivers merged recently for inspiration: > gpio/gpio-xgene-sb.c > pinctrl/stm32/pinctrl-stm32.c > I was waiting for such a comment! I was actually tackling to specify IRQ-mapping with "interrupts" property. Adding "interrupts" property in DT causes of_pupulate_default_populate() to assign virtual IRQ numbers before driver probing. So it does not work well with IRQ domain hierarchy. I send a question: https://lkml.org/lkml/2017/7/6/758 But, I did not get any answer. So, I thought the only solution is to specify the IRQ-remapping somehow in the driver. For gpio/gpio-xgene-sb.c, the driver code seems suspicious. As you see xgene_gpio_sb_probe(), it uses the first entry of "interrupts" for just getting the ->parent_irq_base. The assigned virtual IRQs will not be used. ret = platform_get_irq(pdev, 0); if (ret > 0) { priv->parent_irq_base = irq_get_irq_data(ret)->hwirq; parent_domain = irq_get_irq_data(ret)->domain; } Then, xgene_gpio_sb_domain_alloc() allocates different virtual IRQs expecting a particular binding between the parent. parent_fwspec.fwnode = domain->parent->fwnode; if (is_of_node(parent_fwspec.fwnode)) { parent_fwspec.param_count = 3; parent_fwspec.param[0] = 0;/* SPI */ /* Skip SGIs and PPIs*/ parent_fwspec.param[1] = hwirq + priv->parent_irq_base - 32; parent_fwspec.param[2] = fwspec->param[1]; } else if (is_fwnode_irqchip(parent_fwspec.fwnode)) { parent_fwspec.param_count = 2; parent_fwspec.param[0] = hwirq + priv->parent_irq_base; parent_fwspec.param[1] = fwspec->param[1]; } else return -EINVAL; It uses ->parent_irq_base derived from DT, but other parts are really hard-coded. For pinctrl/stm32/pinctrl-stm32.c, I do not see "interrupts", so it just straight maps the irq numbers. -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html