This patchset adds gpio-signaled acpi events support for power button on hisilicon
D02 board.
The three patches respectively:
- convert device node to fwnode
- add acpi binding
- add gpio-signaled acpi event support
This patchset is based on
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
branch "devel"
Changes v4 -> v5:
- split into three patchs
- add Andy's ACKs
Changes v3 -> v4:
- re-organize this two patchs by Andy's suggestion
Changes v2 -> v3:
- fixed the build error reported by Kbuild test robot
Changes v1 -> v2:
- rebase to branch "devel" of Linus Walleij's repository
- split in two patch as suggested by Andy S
- add Mika's ACKs
qiujiang (3):
gpio: designware: convert device node to fwnode
gpio: designware: add acpi binding
gpio: designware: add gpio-signaled acpi event support
drivers/gpio/gpio-dwapb.c | 69 ++++++++++++++++++++------------
drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
include/linux/platform_data/gpio-dwapb.h | 2 +-
3 files changed, 46 insertions(+), 27 deletions(-)
--
1.9.1
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 <[email protected]>
Signed-off-by: qiujiang <[email protected]>
---
drivers/gpio/gpio-dwapb.c | 43 +++++++++++++++-----------------
drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
include/linux/platform_data/gpio-dwapb.h | 2 +-
3 files changed, 22 insertions(+), 25 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 597de1e..49f6e5d 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -22,6 +22,7 @@
#include <linux/of_address.h>
#include <linux/of_irq.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/spinlock.h>
#include <linux/platform_data/gpio-dwapb.h>
#include <linux/slab.h>
@@ -290,14 +291,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
struct dwapb_port_property *pp)
{
struct gpio_chip *gc = &port->gc;
- struct device_node *node = pp->node;
+ struct fwnode_handle *fwnode = pp->fwnode;
struct irq_chip_generic *irq_gc = NULL;
unsigned int hwirq, ngpio = gc->ngpio;
struct irq_chip_type *ct;
int err, i;
- gpio->domain = irq_domain_add_linear(node, ngpio,
- &irq_generic_chip_ops, gpio);
+ gpio->domain = irq_domain_create_linear(fwnode, ngpio,
+ &irq_generic_chip_ops, gpio);
if (!gpio->domain)
return;
@@ -415,7 +416,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
}
#ifdef CONFIG_OF_GPIO
- port->gc.of_node = pp->node;
+ port->gc.of_node = is_of_node(pp->fwnode) ?
+ to_of_node(pp->fwnode) : NULL;
#endif
port->gc.ngpio = pp->ngpio;
port->gc.base = pp->gpio_base;
@@ -449,17 +451,13 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
static struct dwapb_platform_data *
dwapb_gpio_get_pdata_of(struct device *dev)
{
- struct device_node *node, *port_np;
+ struct fwnode_handle *fwnode;
struct dwapb_platform_data *pdata;
struct dwapb_port_property *pp;
int nports;
int i;
- node = dev->of_node;
- if (!IS_ENABLED(CONFIG_OF_GPIO) || !node)
- return ERR_PTR(-ENODEV);
-
- nports = of_get_child_count(node);
+ nports = device_get_child_node_count(dev);
if (nports == 0)
return ERR_PTR(-ENODEV);
@@ -474,21 +472,19 @@ dwapb_gpio_get_pdata_of(struct device *dev)
pdata->nports = nports;
i = 0;
- for_each_child_of_node(node, port_np) {
+ device_for_each_child_node(dev, fwnode) {
pp = &pdata->properties[i++];
- pp->node = port_np;
+ pp->fwnode = fwnode;
- if (of_property_read_u32(port_np, "reg", &pp->idx) ||
+ if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) ||
pp->idx >= DWAPB_MAX_PORTS) {
- dev_err(dev, "missing/invalid port index for %s\n",
- port_np->full_name);
+ dev_err(dev, "missing/invalid port index\n");
return ERR_PTR(-EINVAL);
}
- if (of_property_read_u32(port_np, "snps,nr-gpios",
+ if (fwnode_property_read_u32(fwnode, "snps,nr-gpios",
&pp->ngpio)) {
- dev_info(dev, "failed to get number of gpios for %s\n",
- port_np->full_name);
+ dev_info(dev, "failed to get number of gpios\n");
pp->ngpio = 32;
}
@@ -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")) {
+ pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0);
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;
}
return pdata;
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index 0421374..265bd3c 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -227,7 +227,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
return -ENOMEM;
/* Set the properties for portA */
- pdata->properties->node = NULL;
+ pdata->properties->fwnode = NULL;
pdata->properties->name = "intel-quark-x1000-gpio-portA";
pdata->properties->idx = 0;
pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO;
diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
index 28702c8..c5bd1f2 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -15,7 +15,7 @@
#define GPIO_DW_APB_H
struct dwapb_port_property {
- struct device_node *node;
+ struct fwnode_handle *fwnode;
const char *name;
unsigned int idx;
unsigned int ngpio;
--
1.9.1
This patch adds the support for the gpio-signaled acpi event.
This is used for power button on hisilicon D02 board, which
is an arm64 platform.
To support this function, _AEI and _Exx objects must be
defined in the corresponding GPIO device as follows:
Name (_AEI, ResourceTemplate () {
GpioInt(Edge, ActiveLow, ExclusiveAndWake, PullUp,
, " \\_SB.GPI0") {8}
})
Method (_E08, 0x0, NotSerialized) {
Notify (\_SB.PWRB, 0x80)
}
Acked-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: qiujiang <[email protected]>
---
drivers/gpio/gpio-dwapb.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 2ae506f..043e1c2 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -28,6 +28,8 @@
#include <linux/platform_data/gpio-dwapb.h>
#include <linux/slab.h>
+#include "gpiolib.h"
+
#define GPIO_SWPORTA_DR 0x00
#define GPIO_SWPORTA_DDR 0x04
#define GPIO_SWPORTB_DR 0x0c
@@ -437,6 +439,10 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
else
port->is_registered = true;
+ /* Add GPIO-signaled ACPI event support */
+ if (pp->irq)
+ acpi_gpiochip_request_interrupts(&port->gc);
+
return err;
}
--
1.9.1
This patch adds acpi binding for designware gpio driver,
because it is used to support power button on hisilicon
d02 board.
These two bindings, DT and acpi, are compatible in this
driver.
Reviewed-by: Andy Shevchenko <[email protected]>
Acked-by: Mika Westerberg <[email protected]>
Signed-off-by: qiujiang <[email protected]>
---
drivers/gpio/gpio-dwapb.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 49f6e5d..2ae506f 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -7,6 +7,7 @@
*
* All enquiries to [email protected]
*/
+#include <linux/acpi.h>
#include <linux/gpio/driver.h>
/* FIXME: for gpio_get_value(), replace this with direct register read */
#include <linux/gpio.h>
@@ -449,7 +450,7 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
}
static struct dwapb_platform_data *
-dwapb_gpio_get_pdata_of(struct device *dev)
+dwapb_gpio_get_pdata(struct device *dev)
{
struct fwnode_handle *fwnode;
struct dwapb_platform_data *pdata;
@@ -502,9 +503,17 @@ dwapb_gpio_get_pdata_of(struct device *dev)
}
}
+ if (has_acpi_companion(dev) && pp->idx == 0)
+ pp->irq = platform_get_irq(to_platform_device(dev), 0);
+
pp->irq_shared = false;
pp->gpio_base = -1;
- pp->name = to_of_node(fwnode)->full_name;
+
+ if (dev->of_node)
+ pp->name = to_of_node(fwnode)->full_name;
+
+ if (has_acpi_companion(dev))
+ pp->name = acpi_dev_name(to_acpi_device_node(fwnode));
}
return pdata;
@@ -520,7 +529,7 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
struct dwapb_platform_data *pdata = dev_get_platdata(dev);
if (!pdata) {
- pdata = dwapb_gpio_get_pdata_of(dev);
+ pdata = dwapb_gpio_get_pdata(dev);
if (IS_ERR(pdata))
return PTR_ERR(pdata);
}
@@ -577,6 +586,12 @@ static const struct of_device_id dwapb_of_match[] = {
};
MODULE_DEVICE_TABLE(of, dwapb_of_match);
+static const struct acpi_device_id dwapb_acpi_match[] = {
+ {"HISI0181", 0},
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, dwapb_acpi_match);
+
#ifdef CONFIG_PM_SLEEP
static int dwapb_gpio_suspend(struct device *dev)
{
@@ -671,6 +686,7 @@ static struct platform_driver dwapb_gpio_driver = {
.name = "gpio-dwapb",
.pm = &dwapb_gpio_pm_ops,
.of_match_table = of_match_ptr(dwapb_of_match),
+ .acpi_match_table = ACPI_PTR(dwapb_acpi_match),
},
.probe = dwapb_gpio_probe,
.remove = dwapb_gpio_remove,
--
1.9.1
On Fri, Mar 4, 2016 at 2:44 PM, qiujiang <[email protected]> wrote:
> This patchset adds gpio-signaled acpi events support for power button on hisilicon
> D02 board.
>
> The three patches respectively:
> - convert device node to fwnode
> - add acpi binding
> - add gpio-signaled acpi event support
>
> This patchset is based on
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
> branch "devel"
>
> Changes v4 -> v5:
> - split into three patchs
> - add Andy's ACKs
>
> Changes v3 -> v4:
> - re-organize this two patchs by Andy's suggestion
>
> Changes v2 -> v3:
> - fixed the build error reported by Kbuild test robot
>
> Changes v1 -> v2:
> - rebase to branch "devel" of Linus Walleij's repository
> - split in two patch as suggested by Andy S
> - add Mika's ACKs
>
> qiujiang (3):
> gpio: designware: convert device node to fwnode
> gpio: designware: add acpi binding
> gpio: designware: add gpio-signaled acpi event support
I noticed that the original driver authors Jamie and Alan have not been
properly CC:ed on this patch. Jamie, Alan: can you look at this and
ACK it if you like what you see?
Yours,
Linus Walleij
On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <[email protected]> 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 <[email protected]>
> Signed-off-by: qiujiang <[email protected]>
> ---
> drivers/gpio/gpio-dwapb.c | 43 +++++++++++++++-----------------
> drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
> include/linux/platform_data/gpio-dwapb.h | 2 +-
> 3 files changed, 22 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 597de1e..49f6e5d 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -22,6 +22,7 @@
> #include <linux/of_address.h>
> #include <linux/of_irq.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/spinlock.h>
> #include <linux/platform_data/gpio-dwapb.h>
> #include <linux/slab.h>
> @@ -290,14 +291,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> struct dwapb_port_property *pp)
> {
> struct gpio_chip *gc = &port->gc;
> - struct device_node *node = pp->node;
> + struct fwnode_handle *fwnode = pp->fwnode;
> struct irq_chip_generic *irq_gc = NULL;
> unsigned int hwirq, ngpio = gc->ngpio;
> struct irq_chip_type *ct;
> int err, i;
>
> - gpio->domain = irq_domain_add_linear(node, ngpio,
> - &irq_generic_chip_ops, gpio);
> + gpio->domain = irq_domain_create_linear(fwnode, ngpio,
> + &irq_generic_chip_ops, gpio);
> if (!gpio->domain)
> return;
>
> @@ -415,7 +416,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
> }
>
> #ifdef CONFIG_OF_GPIO
> - port->gc.of_node = pp->node;
> + port->gc.of_node = is_of_node(pp->fwnode) ?
> + to_of_node(pp->fwnode) : NULL;
> #endif
> port->gc.ngpio = pp->ngpio;
> port->gc.base = pp->gpio_base;
> @@ -449,17 +451,13 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
> static struct dwapb_platform_data *
> dwapb_gpio_get_pdata_of(struct device *dev)
> {
> - struct device_node *node, *port_np;
> + struct fwnode_handle *fwnode;
> struct dwapb_platform_data *pdata;
> struct dwapb_port_property *pp;
> int nports;
> int i;
>
> - node = dev->of_node;
> - if (!IS_ENABLED(CONFIG_OF_GPIO) || !node)
> - return ERR_PTR(-ENODEV);
> -
> - nports = of_get_child_count(node);
> + nports = device_get_child_node_count(dev);
> if (nports == 0)
> return ERR_PTR(-ENODEV);
>
> @@ -474,21 +472,19 @@ dwapb_gpio_get_pdata_of(struct device *dev)
> pdata->nports = nports;
>
> i = 0;
> - for_each_child_of_node(node, port_np) {
> + device_for_each_child_node(dev, fwnode) {
> pp = &pdata->properties[i++];
> - pp->node = port_np;
> + pp->fwnode = fwnode;
>
> - if (of_property_read_u32(port_np, "reg", &pp->idx) ||
> + if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) ||
> pp->idx >= DWAPB_MAX_PORTS) {
> - dev_err(dev, "missing/invalid port index for %s\n",
> - port_np->full_name);
> + dev_err(dev, "missing/invalid port index\n");
> return ERR_PTR(-EINVAL);
> }
>
> - if (of_property_read_u32(port_np, "snps,nr-gpios",
> + if (fwnode_property_read_u32(fwnode, "snps,nr-gpios",
> &pp->ngpio)) {
> - dev_info(dev, "failed to get number of gpios for %s\n",
> - port_np->full_name);
> + dev_info(dev, "failed to get number of gpios\n");
> pp->ngpio = 32;
> }
>
> @@ -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?
Alan
> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0);
> 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;
> }
>
> return pdata;
> diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
> index 0421374..265bd3c 100644
> --- a/drivers/mfd/intel_quark_i2c_gpio.c
> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> @@ -227,7 +227,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> return -ENOMEM;
>
> /* Set the properties for portA */
> - pdata->properties->node = NULL;
> + pdata->properties->fwnode = NULL;
> pdata->properties->name = "intel-quark-x1000-gpio-portA";
> pdata->properties->idx = 0;
> pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO;
> diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
> index 28702c8..c5bd1f2 100644
> --- a/include/linux/platform_data/gpio-dwapb.h
> +++ b/include/linux/platform_data/gpio-dwapb.h
> @@ -15,7 +15,7 @@
> #define GPIO_DW_APB_H
>
> struct dwapb_port_property {
> - struct device_node *node;
> + struct fwnode_handle *fwnode;
> const char *name;
> unsigned int idx;
> unsigned int ngpio;
> --
> 1.9.1
>
On Tue, Mar 8, 2016 at 8:26 PM, Linus Walleij <[email protected]> wrote:
> On Fri, Mar 4, 2016 at 2:44 PM, qiujiang <[email protected]> wrote:
>
>> This patchset adds gpio-signaled acpi events support for power button on hisilicon
>> D02 board.
>>
>> The three patches respectively:
>> - convert device node to fwnode
>> - add acpi binding
>> - add gpio-signaled acpi event support
>>
>> This patchset is based on
>> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
>> branch "devel"
>>
>> Changes v4 -> v5:
>> - split into three patchs
>> - add Andy's ACKs
>>
>> Changes v3 -> v4:
>> - re-organize this two patchs by Andy's suggestion
>>
>> Changes v2 -> v3:
>> - fixed the build error reported by Kbuild test robot
>>
>> Changes v1 -> v2:
>> - rebase to branch "devel" of Linus Walleij's repository
>> - split in two patch as suggested by Andy S
>> - add Mika's ACKs
>>
>> qiujiang (3):
>> gpio: designware: convert device node to fwnode
>> gpio: designware: add acpi binding
>> gpio: designware: add gpio-signaled acpi event support
>
> I noticed that the original driver authors Jamie and Alan have not been
> properly CC:ed on this patch. Jamie, Alan: can you look at this and
> ACK it if you like what you see?
>
> Yours,
> Linus Walleij
Hi Linus,
I briefly tested the patches on a ARM CycloneV board and didn't see
any issues. I looked over the patches (not super familiar with ACPI)
and just had one question (on another thread).
Thanks,
Alan
On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <[email protected]> wrote:
> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <[email protected]> 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 <[email protected]>
>> Signed-off-by: qiujiang <[email protected]>
>> @@ -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")) {
>
> 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.
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 4, 2016 at 9:44 AM, qiujiang <[email protected]> wrote:
> This patch adds acpi binding for designware gpio driver,
> because it is used to support power button on hisilicon
> d02 board.
>
> These two bindings, DT and acpi, are compatible in this
> driver.
>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Acked-by: Mika Westerberg <[email protected]>
> Signed-off-by: qiujiang <[email protected]>
> static struct dwapb_platform_data *
> -dwapb_gpio_get_pdata_of(struct device *dev)
> +dwapb_gpio_get_pdata(struct device *dev)
Rename is a part of patch 1
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 4, 2016 at 9:44 AM, qiujiang <[email protected]> wrote:
> This patch adds the support for the gpio-signaled acpi event.
> This is used for power button on hisilicon D02 board, which
> is an arm64 platform.
>
> To support this function, _AEI and _Exx objects must be
> defined in the corresponding GPIO device as follows:
>
> Name (_AEI, ResourceTemplate () {
> GpioInt(Edge, ActiveLow, ExclusiveAndWake, PullUp,
> , " \\_SB.GPI0") {8}
> })
> Method (_E08, 0x0, NotSerialized) {
> Notify (\_SB.PWRB, 0x80)
> }
>
> Acked-by: Mika Westerberg <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: qiujiang <[email protected]>
This one fine to me.
> ---
> drivers/gpio/gpio-dwapb.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 2ae506f..043e1c2 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -28,6 +28,8 @@
> #include <linux/platform_data/gpio-dwapb.h>
> #include <linux/slab.h>
>
> +#include "gpiolib.h"
> +
> #define GPIO_SWPORTA_DR 0x00
> #define GPIO_SWPORTA_DDR 0x04
> #define GPIO_SWPORTB_DR 0x0c
> @@ -437,6 +439,10 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
> else
> port->is_registered = true;
>
> + /* Add GPIO-signaled ACPI event support */
> + if (pp->irq)
> + acpi_gpiochip_request_interrupts(&port->gc);
> +
> return err;
> }
>
> --
> 1.9.1
>
--
With Best Regards,
Andy Shevchenko
在 2016/3/11 3:09, Alan Tull 写道:
> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <[email protected]> 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 <[email protected]>
>> Signed-off-by: qiujiang <[email protected]>
>> ---
>> drivers/gpio/gpio-dwapb.c | 43 +++++++++++++++-----------------
>> drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
>> include/linux/platform_data/gpio-dwapb.h | 2 +-
>> 3 files changed, 22 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
>> index 597de1e..49f6e5d 100644
>> --- a/drivers/gpio/gpio-dwapb.c
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -22,6 +22,7 @@
>> #include <linux/of_address.h>
>> #include <linux/of_irq.h>
>> #include <linux/platform_device.h>
>> +#include <linux/property.h>
>> #include <linux/spinlock.h>
>> #include <linux/platform_data/gpio-dwapb.h>
>> #include <linux/slab.h>
>> @@ -290,14 +291,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>> struct dwapb_port_property *pp)
>> {
>> struct gpio_chip *gc = &port->gc;
>> - struct device_node *node = pp->node;
>> + struct fwnode_handle *fwnode = pp->fwnode;
>> struct irq_chip_generic *irq_gc = NULL;
>> unsigned int hwirq, ngpio = gc->ngpio;
>> struct irq_chip_type *ct;
>> int err, i;
>>
>> - gpio->domain = irq_domain_add_linear(node, ngpio,
>> - &irq_generic_chip_ops, gpio);
>> + gpio->domain = irq_domain_create_linear(fwnode, ngpio,
>> + &irq_generic_chip_ops, gpio);
>> if (!gpio->domain)
>> return;
>>
>> @@ -415,7 +416,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>> }
>>
>> #ifdef CONFIG_OF_GPIO
>> - port->gc.of_node = pp->node;
>> + port->gc.of_node = is_of_node(pp->fwnode) ?
>> + to_of_node(pp->fwnode) : NULL;
>> #endif
>> port->gc.ngpio = pp->ngpio;
>> port->gc.base = pp->gpio_base;
>> @@ -449,17 +451,13 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
>> static struct dwapb_platform_data *
>> dwapb_gpio_get_pdata_of(struct device *dev)
>> {
>> - struct device_node *node, *port_np;
>> + struct fwnode_handle *fwnode;
>> struct dwapb_platform_data *pdata;
>> struct dwapb_port_property *pp;
>> int nports;
>> int i;
>>
>> - node = dev->of_node;
>> - if (!IS_ENABLED(CONFIG_OF_GPIO) || !node)
>> - return ERR_PTR(-ENODEV);
>> -
>> - nports = of_get_child_count(node);
>> + nports = device_get_child_node_count(dev);
>> if (nports == 0)
>> return ERR_PTR(-ENODEV);
>>
>> @@ -474,21 +472,19 @@ dwapb_gpio_get_pdata_of(struct device *dev)
>> pdata->nports = nports;
>>
>> i = 0;
>> - for_each_child_of_node(node, port_np) {
>> + device_for_each_child_node(dev, fwnode) {
>> pp = &pdata->properties[i++];
>> - pp->node = port_np;
>> + pp->fwnode = fwnode;
>>
>> - if (of_property_read_u32(port_np, "reg", &pp->idx) ||
>> + if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) ||
>> pp->idx >= DWAPB_MAX_PORTS) {
>> - dev_err(dev, "missing/invalid port index for %s\n",
>> - port_np->full_name);
>> + dev_err(dev, "missing/invalid port index\n");
>> return ERR_PTR(-EINVAL);
>> }
>>
>> - if (of_property_read_u32(port_np, "snps,nr-gpios",
>> + if (fwnode_property_read_u32(fwnode, "snps,nr-gpios",
>> &pp->ngpio)) {
>> - dev_info(dev, "failed to get number of gpios for %s\n",
>> - port_np->full_name);
>> + dev_info(dev, "failed to get number of gpios\n");
>> pp->ngpio = 32;
>> }
>>
>> @@ -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?
>
> Alan
Agree, "to_of_node" should be never used since it coverted to fwnode.
I will give a more reasonable solution in the next version.
Thanks, Jiang
>
>> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0);
>> 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;
>> }
>>
>> return pdata;
>> diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
>> index 0421374..265bd3c 100644
>> --- a/drivers/mfd/intel_quark_i2c_gpio.c
>> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
>> @@ -227,7 +227,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
>> return -ENOMEM;
>>
>> /* Set the properties for portA */
>> - pdata->properties->node = NULL;
>> + pdata->properties->fwnode = NULL;
>> pdata->properties->name = "intel-quark-x1000-gpio-portA";
>> pdata->properties->idx = 0;
>> pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO;
>> diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
>> index 28702c8..c5bd1f2 100644
>> --- a/include/linux/platform_data/gpio-dwapb.h
>> +++ b/include/linux/platform_data/gpio-dwapb.h
>> @@ -15,7 +15,7 @@
>> #define GPIO_DW_APB_H
>>
>> struct dwapb_port_property {
>> - struct device_node *node;
>> + struct fwnode_handle *fwnode;
>> const char *name;
>> unsigned int idx;
>> unsigned int ngpio;
>> --
>> 1.9.1
>>
> .
>
在 2016/3/11 4:27, Andy Shevchenko 写道:
> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <[email protected]> wrote:
>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <[email protected]> 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 <[email protected]>
>>> Signed-off-by: qiujiang <[email protected]>
>>> @@ -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")) {
Yes, agreed, "to_of_node" will never appear in this patch : )
>> 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.
Agreed.
>
>>> 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.
Agreed.
Thanks, Jiang
在 2016/3/11 4:27, Andy Shevchenko 写道:
> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <[email protected]> wrote:
>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <[email protected]> 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 <[email protected]>
>>> Signed-off-by: qiujiang <[email protected]>
>>> @@ -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")) {
>> 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.
Hi Alan, Andy,
There is some trouble to replace the interface irq_of_parse_and_map and take
fwnode on input.
As far as I know, there are two common APIs to parse interrupt resource:
1) irq_of_parse_and_map() - It is a DT specific API, and have to use of_node as
input.
2) platform_get_irq() - It could be used to DT as well as ACPI, It needs platform_
device as input. In this driver, the DTs is defined as this:
pc_gpio0: gpio@802e0000 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "snps,dw-apb-gpio";
reg = <0x0 0x802e0000 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 312 4>;
};
};
The compatible string, which is "snps,dw-apb-gpio", is only for the parent node.
That is mean, there is no platform device for the child node, but the interrupt
resource is defined in child node.
Is there any other APIs can be used to parse interrupts resource using fwnode
as input?
Thanks, Jiang
>>> 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.
>
On Fri, Mar 4, 2016 at 8:44 AM, qiujiang <[email protected]> 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 <[email protected]>
> Signed-off-by: qiujiang <[email protected]>
I need Mika's and maybe also Rafael's ACKs on this before
I merge any of the series.
> --- a/drivers/mfd/intel_quark_i2c_gpio.c
> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> @@ -227,7 +227,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> return -ENOMEM;
>
> /* Set the properties for portA */
> - pdata->properties->node = NULL;
> + pdata->properties->fwnode = NULL;
And apparently also the MFD maintainer needs to be involved to ACK this,
so add Lee Jones on subsequent postings.
Yours,
Linus Walleij
On Tue, Mar 22, 2016 at 11:38:26AM +0100, Linus Walleij wrote:
> On Fri, Mar 4, 2016 at 8:44 AM, qiujiang <[email protected]> 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 <[email protected]>
> > Signed-off-by: qiujiang <[email protected]>
>
> I need Mika's and maybe also Rafael's ACKs on this before
> I merge any of the series.
Hmm, I thought I already acked the ACPI GPIO signaled event patch? It
still looks fine to me.
在 2016/3/11 4:27, Andy Shevchenko 写道:
> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <[email protected]> wrote:
>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <[email protected]> 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 <[email protected]>
>>> Signed-off-by: qiujiang <[email protected]>
>>> @@ -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.
>
On Wed, Mar 23, 2016 at 6:41 AM, Jiang Qiu <[email protected]> wrote:
> 在 2016/3/11 4:27, Andy Shevchenko 写道:
>> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <[email protected]> wrote:
>>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: qiujiang <[email protected]>
>>>> @@ -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.
>>
>
>
在 2016/3/24 0:20, Alan Tull 写道:
> On Wed, Mar 23, 2016 at 6:41 AM, Jiang Qiu <[email protected]> wrote:
>> 在 2016/3/11 4:27, Andy Shevchenko 写道:
>>> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <[email protected]> wrote:
>>>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <[email protected]> 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 <[email protected]>
>>>>> Signed-off-by: qiujiang <[email protected]>
>>>>> @@ -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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> .
>