Re: [PATCH v5 1/3] gpio: designware: convert device node to fwnode

[Jiang Qiu]

å 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
>
> .
>