Messages in this thread | | | Date | Tue, 12 Mar 2019 10:35:09 -0600 | From | Lina Iyer <> | Subject | Re: [PATCH v3 6/9] drivers: pinctrl: msm: setup GPIO irqchip hierarchy |
| |
On Mon, Mar 11 2019 at 04:55 -0600, Marc Zyngier wrote: >On 22/02/2019 22:18, Lina Iyer wrote: >> To allow GPIOs to wakeup the system from suspend or deep idle, the >> wakeup capable GPIOs are setup in hierarchy with interrupts from the >> wakeup-parent irqchip. >> >> In older SoC's, the TLMM will handover detection to the parent irqchip >> and in newer SoC's, the parent irqchip may also be active as well as the >> TLMM and therefore the GPIOs need to be masked at TLMM to avoid >> duplicate interrupts. To enable both these configurations to exist, >> allow the parent irqchip to dictate the TLMM irqchip's behavior when >> masking/unmasking the interrupt. >> >> Co-developed-by: Stephen Boyd <swboyd@chromium.org> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
[...]
>> @@ -986,6 +1093,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) >> chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl); >> >> pctrl->irq_chip.name = "msmgpio"; >> + pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent; >> pctrl->irq_chip.irq_mask = msm_gpio_irq_mask; >> pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask; >> pctrl->irq_chip.irq_ack = msm_gpio_irq_ack; >> @@ -994,6 +1102,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) >> pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres; >> pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres; >> >> + chip->irq.chip = &pctrl->irq_chip; >> + chip->irq.domain_ops = &msm_gpio_domain_ops; >> + chip->irq.handler = handle_edge_irq; >> + chip->irq.default_type = IRQ_TYPE_NONE; > >I know you're only moving this around, but can you explain why you're >setting IRQ_TYPE_NONE in combination with handle_edge_irq? The two >really should go together. If this doesn't work for some reason, please >document it. > Yes, it was a carry-over from the existing code. I am not sure why that seems to be a common practice among GPIO drivers. IRQ_TYPE_EDGE_RISING would be a better option ?
>> + >> + dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0); >> + if (dn) { >> + chip->irq.parent_domain = irq_find_matching_host(dn, >> + DOMAIN_BUS_WAKEUP); >> + of_node_put(dn); >> + if (!chip->irq.parent_domain) >> + return -EPROBE_DEFER; >> + >> + chip->to_irq = msm_gpio_to_irq; >> + } >> + >> ret = gpiochip_add_data(&pctrl->chip, pctrl); >> if (ret) { >> dev_err(pctrl->dev, "Failed register gpiochip\n"); >> @@ -1015,26 +1139,17 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) >> dev_name(pctrl->dev), 0, 0, chip->ngpio); >> if (ret) { >> dev_err(pctrl->dev, "Failed to add pin range\n"); >> - gpiochip_remove(&pctrl->chip); >> - return ret; >> + goto fail; >> } >> } >> >> - ret = gpiochip_irqchip_add(chip, >> - &pctrl->irq_chip, >> - 0, >> - handle_edge_irq, >> - IRQ_TYPE_NONE); >> - if (ret) { >> - dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n"); >> - gpiochip_remove(&pctrl->chip); >> - return -ENOSYS; >> - } >> - >> gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq, >> msm_gpio_irq_handler); >> >> return 0; >> +fail: >> + gpiochip_remove(&pctrl->chip); >> + return ret; >> } >>
Thanks, Lina
| |