Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753193AbcCXBZf (ORCPT ); Wed, 23 Mar 2016 21:25:35 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:52606 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751117AbcCXBZd (ORCPT ); Wed, 23 Mar 2016 21:25:33 -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> <56F28107.6090305@huawei.com> To: Alan Tull CC: Andy Shevchenko , Mika Westerberg , Linus Walleij , Alexandre Courbot , linux-kernel , "linux-gpio@vger.kernel.org" , "linux-acpi@vger.kernel.org" , , , , atull From: Jiang Qiu Message-ID: <56F341CD.2050300@huawei.com> Date: Thu, 24 Mar 2016 09:24:29 +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.0A090201.56F341DE.00C0,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 Content-Length: 5443 Lines: 134 在 2016/3/24 0:20, Alan Tull 写道: > On Wed, Mar 23, 2016 at 6:41 AM, Jiang Qiu wrote: >> 在 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. > Hi Jiang, > > Are you suggesting a change for the DT to make it similar to the ACPI > case? DT changes create unexpected breakages when people upgrade > their kernel even if the change is minor. How bad will the code look > if you implement it as the two separate code paths as you suggest? > > Alan Agreed. It would better do not make any change for DT if possible. If keeping the two separate code paths, as presented above, I have to do those check like "if (dev->of_node)" and covert back the fwnode to of_node, so as to parse IRQ resource in DT. Andy think this patch looks unfinished if used to_of_node. Andy, do you think it acceptable if keeping two separate paths for DT and ACPI? >> 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. >>> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > . >