From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932097AbcCWLmd (ORCPT ); Wed, 23 Mar 2016 07:42:33 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:47694 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751925AbcCWLmZ (ORCPT ); Wed, 23 Mar 2016 07:42:25 -0400 Reply-To: Subject: Re: [PATCH v5 1/3] gpio: designware: convert device node to fwnode References: <1457077474-124064-1-git-send-email-qiujiang@huawei.com> <1457077474-124064-2-git-send-email-qiujiang@huawei.com> To: Andy Shevchenko , Alan Tull , Mika Westerberg CC: Linus Walleij , Alexandre Courbot , linux-kernel , "linux-gpio@vger.kernel.org" , "linux-acpi@vger.kernel.org" , , , From: Jiang Qiu Message-ID: <56F28107.6090305@huawei.com> Date: Wed, 23 Mar 2016 19:41:59 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.74.160.152] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A0B0202.56F28113.03DB,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: f77887ba74eededfbe9aee81f90f8638 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 2016/3/11 4:27, Andy Shevchenko 写道: > On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull wrote: >> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang wrote: >>> This patch converts device node to fwnode in >>> dwapb_port_property for designware gpio driver, >>> so as to provide a unified data structure for DT >>> and ACPI bindings. >>> >>> Acked-by: Andy Shevchenko >>> Signed-off-by: qiujiang >>> @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) >>> * Only port A can provide interrupts in all configurations of >>> * the IP. >>> */ >>> - if (pp->idx == 0 && >>> - of_property_read_bool(port_np, "interrupt-controller")) { >>> - pp->irq = irq_of_parse_and_map(port_np, 0); >>> + if (dev->of_node && pp->idx == 0 && >>> + of_property_read_bool(to_of_node(fwnode), >>> + "interrupt-controller")) { >> Hi Qiujiang, >> >> Is there a reason to use "of_property_read_bool" here instead of >> "device_property_read_bool" or similar? > Yeah, this patch looks unfinished. > > This should be > if (pp->idx == 0 && fwnode_property_read_bool(fwnode, > "interrupt-controller")) { Hi, Alan, Andy, Mika, Many thanks for help me review this patchset. I tried to use a unified interface to parse the interrupts resource in DT and ACPI, but it looks difficult because of the hierarchy device node structure as follow: pc_gpio1: gpio@802f0000 { #address-cells = <1>; #size-cells = <0>; compatible = "snps,dw-apb-gpio"; reg = <0x0 0x802f0000 0x0 0x10000>; porta: gpio-controller@0 { compatible = "snps,dw-apb-gpio-port"; gpio-controller; #gpio-cells = <2>; snps,nr-gpios = <32>; reg = <0>; interrupt-controller; #interrupt-cells = <2>; interrupts = <0 313 4>; }; }; According to the designware gpio databook, each GPIO controller includes 4 ports (porta,b,c,d), only porta can be a interrupt controller. So, I moved the interrupts resource to the parent node from porta in ACPI. Device(GPI0) { Name(_HID, "HISI0181") Name(_ADR, 0) // _ADR: Address Name(_UID, 0) Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0x802e0000, 0x10000) Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) {344} //moved here }) Device(PRTa) { Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () {"reg",0}, Package () {"snps,nr-gpios",32}, } }) } } That is to say, if GPI0 should be a interrupt controller, the child node PRTa must be present first, then add the interrupt resource to the parent node GPI0 scope. Dose this proposal sounds ok? if yes, we can do that for DT. If not, there can only keep two branches to parse the IRQ resource, and the code looks strange. That would be great if I can get some help from you. Best Regards Jiang >> Alan >> >>> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); > But here should be common method called which takes fwnode on input. > >>> if (!pp->irq) { >>> dev_warn(dev, "no irq for bank %s\n", >>> - port_np->full_name); >>> + to_of_node(fwnode)->full_name); >>> } >>> } >>> >>> pp->irq_shared = false; >>> pp->gpio_base = -1; >>> - pp->name = port_np->full_name; >>> + pp->name = to_of_node(fwnode)->full_name; > Also those two should be device property source agnostic. That's what > I tried to tell earlier. >