Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941539AbcJYJmK (ORCPT ); Tue, 25 Oct 2016 05:42:10 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:45772 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933360AbcJYJmI (ORCPT ); Tue, 25 Oct 2016 05:42:08 -0400 Subject: Re: [Patch v5 04/12] irqchip: xilinx: Add support for parent intc To: Marc Zyngier , , , , , , References: <1476723176-39891-1-git-send-email-Zubair.Kakakhel@imgtec.com> <1476723176-39891-5-git-send-email-Zubair.Kakakhel@imgtec.com> <32f5f17d-7864-c782-7a6f-03660b7ab055@arm.com> CC: , , , , , , , From: Zubair Lutfullah Kakakhel Message-ID: <581adf44-388c-f8e5-8437-59d7ace2fa8f@imgtec.com> Date: Tue, 25 Oct 2016 10:42:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <32f5f17d-7864-c782-7a6f-03660b7ab055@arm.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [192.168.154.45] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2770 Lines: 115 Hi, Thanks for the review. Some comments in-line. On 10/21/2016 10:48 AM, Marc Zyngier wrote: > On 17/10/16 17:52, Zubair Lutfullah Kakakhel wrote: >> The MIPS based xilfpga platform has the following IRQ structure >> >> Peripherals --> xilinx_intcontroller -> mips_cpu_int controller >> >> Add support for the driver to chain the irq handler >> >> Signed-off-by: Zubair Lutfullah Kakakhel >> >> --- >> V4 -> V5 >> Rebased to v4.9-rc1 >> Missing curly braces >> >> V3 -> V4 >> Clean up if/else when a parent is found >> Pass irqchip structure to handler as data >> >> V2 -> V3 >> Reused existing parent node instead of finding again. >> Cleanup up handler based on review >> >> V1 -> V2 >> >> No change >> --- >> drivers/irqchip/irq-xilinx-intc.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c >> index 45e5154..dbf8b0c 100644 >> --- a/drivers/irqchip/irq-xilinx-intc.c >> +++ b/drivers/irqchip/irq-xilinx-intc.c >> @@ -15,6 +15,7 @@ >> #include >> #include >> #include >> +#include >> >> /* No one else should require these constants, so define them locally here. */ >> #define ISR 0x00 /* Interrupt Status Register */ >> @@ -154,11 +155,23 @@ static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw) >> .map = xintc_map, >> }; >> >> +static void xil_intc_irq_handler(struct irq_desc *desc) >> +{ >> + u32 pending; >> + >> + do { >> + pending = xintc_get_irq(); >> + if (pending == -1U) >> + break; >> + generic_handle_irq(pending); >> + } while (true); > > This is missing the chained_irq_enter()/exit() calls, which will lead to > races or lockups on the root irqchip. > I 'll fix it up in the next series. >> +} >> + >> static int __init xilinx_intc_of_init(struct device_node *intc, >> struct device_node *parent) >> { >> u32 nr_irq; >> - int ret; >> + int ret, irq; >> struct xintc_irq_chip *irqc; >> >> if (xintc_irqc) { >> @@ -221,7 +234,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc, >> goto err_alloc; >> } >> >> - irq_set_default_host(root_domain); >> + if (parent) { >> + irq = irq_of_parse_and_map(intc, 0); >> + if (irq) >> + irq_set_chained_handler_and_data(irq, >> + xil_intc_irq_handler, >> + irqc); >> + > > Shouldn't you return an error if irq is zero? > I'll add the following for the error case pr_err("%s: Parent exists but interrupts property not defined\n" , __func__); goto err_alloc; Thanks ZubairLK >> + } else { >> + irq_set_default_host(root_domain); >> + } >> >> return 0; >> >> > > Thanks, > > M. >