在 2016/3/24 0:20, Alan Tull 写道:
> On Wed, Mar 23, 2016 at 6:41 AM, Jiang Qiu <qiujiang@xxxxxxxxxx> wrote:
>> 在 2016/3/11 4:27, Andy Shevchenko 写道:
>>> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@xxxxxxxxx> wrote:
>>>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@xxxxxxxxxx> 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 <andy.shevchenko@xxxxxxxxx>
>>>>> Signed-off-by: qiujiang <qiujiang@xxxxxxxxxx>
>>>>> @@ -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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> .
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
