[PATCH v5 0/3] "gpio: designware: add gpio-signaled acpi event support for power button"

[PATCH v5 0/3] "gpio: designware: add gpio-signaled acpi event support for power button"

[qiujiang]

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

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

[qiujiang]

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

[PATCH v5 2/3] gpio: designware: add acpi binding

[qiujiang]

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 <andy.shevchenko@gmail.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: qiujiang <qiujiang@huawei.com>
---
 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 support@picochip.com
  */
+#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

[PATCH v5 3/3] gpio: designware: add gpio-signaled acpi event support

[qiujiang]

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 <mika.westerberg@linux.intel.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: qiujiang <qiujiang@huawei.com>
---
 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

Re: [PATCH v5 0/3] "gpio: designware: add gpio-signaled acpi event support for power button"

[Linus Walleij]

On Fri, Mar 4, 2016 at 2:44 PM, qiujiang <qiujiang@huawei.com> 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

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

[Alan Tull]

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

Re: [PATCH v5 0/3] "gpio: designware: add gpio-signaled acpi event support for power button"

[Alan Tull]

On Tue, Mar 8, 2016 at 8:26 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Mar 4, 2016 at 2:44 PM, qiujiang <qiujiang@huawei.com> 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

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

[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")) {

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

Re: [PATCH v5 2/3] gpio: designware: add acpi binding

[Andy Shevchenko]

On Fri, Mar 4, 2016 at 9:44 AM, qiujiang <qiujiang@huawei.com> 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 <andy.shevchenko@gmail.com>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: qiujiang <qiujiang@huawei.com>

>  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

Re: [PATCH v5 3/3] gpio: designware: add gpio-signaled acpi event support

[Andy Shevchenko]

On Fri, Mar 4, 2016 at 9:44 AM, qiujiang <qiujiang@huawei.com> 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 <mika.westerberg@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: qiujiang <qiujiang@huawei.com>

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

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

[Jiang Qiu]

在 2016/3/11 3:09, Alan Tull 写道:
> 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>
>> ---
>>  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
>>
> .
>

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

[Jiang Qiu]

在 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")) {
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

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

[Jiang Qiu]

在 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")) { 
>> 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.
>

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

[Linus Walleij]

On Fri, Mar 4, 2016 at 8: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>

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

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

[Mika Westerberg]

On Tue, Mar 22, 2016 at 11:38:26AM +0100, Linus Walleij wrote:
> On Fri, Mar 4, 2016 at 8: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>
> 
> 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.

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

[Jiang Qiu]

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

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

[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

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

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