From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lina Iyer Subject: Re: [PATCH v3 6/9] drivers: pinctrl: msm: setup GPIO irqchip hierarchy Date: Tue, 12 Mar 2019 10:35:09 -0600 Message-ID: <20190312163509.GB8553@codeaurora.org> References: <20190222221850.26939-1-ilina@codeaurora.org> <20190222221850.26939-7-ilina@codeaurora.org> <6452d538-5714-7e3a-1537-2dd1c4976653@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Return-path: Content-Disposition: inline In-Reply-To: <6452d538-5714-7e3a-1537-2dd1c4976653@arm.com> Sender: linux-kernel-owner@vger.kernel.org To: Marc Zyngier Cc: swboyd@chromium.org, evgreen@chromium.org, linux-kernel@vger.kernel.org, rplsssn@codeaurora.org, linux-arm-msm@vger.kernel.org, thierry.reding@gmail.com, bjorn.andersson@linaro.org, dianders@chromium.org, linus.walleij@linaro.org List-Id: linux-arm-msm@vger.kernel.org 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 >> Signed-off-by: Lina Iyer [...] >> @@ -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