Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1363371 > unrolled thread
| Started by | Jiang Qiu <qiujiang@huawei.com> |
|---|---|
| First post | 2016-03-23 12:50 +0100 |
| Last post | 2016-03-24 02:30 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v5 1/3] gpio: designware: convert device node to fwnode Jiang Qiu <qiujiang@huawei.com> - 2016-03-23 12:50 +0100
Re: [PATCH v5 1/3] gpio: designware: convert device node to fwnode Alan Tull <delicious.quinoa@gmail.com> - 2016-03-23 17:30 +0100
Re: [PATCH v5 1/3] gpio: designware: convert device node to fwnode Jiang Qiu <qiujiang@huawei.com> - 2016-03-24 02:30 +0100
| From | Jiang Qiu <qiujiang@huawei.com> |
|---|---|
| Date | 2016-03-23 12:50 +0100 |
| Subject | Re: [PATCH v5 1/3] gpio: designware: convert device node to fwnode |
| Message-ID | <rfPsu-2Nq-17@gated-at.bofh.it> |
在 2016/3/11 4:27, Andy Shevchenko 写道:
> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> 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@gmail.com>
>>> Signed-off-by: qiujiang <qiujiang@huawei.com>
>>> @@ -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.
>
[toc] | [next] | [standalone]
| From | Alan Tull <delicious.quinoa@gmail.com> |
|---|---|
| Date | 2016-03-23 17:30 +0100 |
| Message-ID | <rfTPt-66l-25@gated-at.bofh.it> |
| In reply to | #1363371 |
On Wed, Mar 23, 2016 at 6:41 AM, Jiang Qiu <qiujiang@huawei.com> wrote:
> 在 2016/3/11 4:27, Andy Shevchenko 写道:
>> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> 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@gmail.com>
>>>> Signed-off-by: qiujiang <qiujiang@huawei.com>
>>>> @@ -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
>
> 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.
>>
>
>
[toc] | [prev] | [next] | [standalone]
| From | Jiang Qiu <qiujiang@huawei.com> |
|---|---|
| Date | 2016-03-24 02:30 +0100 |
| Message-ID | <rg2g2-3vC-9@gated-at.bofh.it> |
| In reply to | #1363522 |
在 2016/3/24 0:20, Alan Tull 写道:
> On Wed, Mar 23, 2016 at 6:41 AM, Jiang Qiu <qiujiang@huawei.com> wrote:
>> 在 2016/3/11 4:27, Andy Shevchenko 写道:
>>> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>>>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> 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@gmail.com>
>>>>> Signed-off-by: qiujiang <qiujiang@huawei.com>
>>>>> @@ -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
>
> .
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web