Re: [PATCH v10 1/3] gpio: dwapb: remove name from dwapb_port_property
在 4/29/2016 5:22 PM, Linus Walleij 写道: > On Thu, Apr 28, 2016 at 11:32 AM, Jiang Qiu wrote: > >> This patch removed the name property from dwapb_port_property. >> The name property is redundant, since we can get this info >> from dwapb_gpio dev node. >> >> Reviewed-by: Andy Shevchenko >> Signed-off-by: Jiang Qiu > > Hmmm just applied another version. Oh well, applying this > instead. Hi, Linux, many thanks for applying. > > Yours, > Linus Walleij > > . >
[PATCH v10 3/3] gpio: dwapb: add gpio-signaled acpi event support
This patch adds gpio-signaled acpi event support. It is used for
power button on hisilicon D02 board, an arm64 platform.
The corresponding DSDT file is defined as follows:
Device(GPI0) {
Name(_HID, "HISI0181")
Name(_ADR, 0)
Name(_UID, 0)
Name (_CRS, ResourceTemplate () {
Memory32Fixed (ReadWrite, 0x802e, 0x1)
Interrupt (ResourceConsumer, Level, ActiveHigh,
Exclusive,,,) {344}
})
Device(PRTa) {
Name (_DSD, Package () {
Package () {
Package () {"reg",0},
Package () {"snps,nr-gpios",32},
}
})
}
Name (_AEI, ResourceTemplate () {
GpioInt(Edge, ActiveLow, ExclusiveAndWake,
PullUp, , " \\_SB.GPI0") {8}
})
Method (_E08, 0x0, NotSerialized) {
Notify (\_SB.PWRB, 0x80)
}
}
Acked-by: Mika Westerberg
Reviewed-by: Andy Shevchenko
Signed-off-by: Jiang Qiu
---
drivers/gpio/gpio-dwapb.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 7517c2f..b235d70 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -7,6 +7,7 @@
*
* All enquiries to supp...@picochip.com
*/
+#include
#include
/* FIXME: for gpio_get_value(), replace this with direct register read */
#include
@@ -27,6 +28,8 @@
#include
#include
+#include "gpiolib.h"
+
#define GPIO_SWPORTA_DR0x00
#define GPIO_SWPORTA_DDR 0x04
#define GPIO_SWPORTB_DR0x0c
@@ -435,6 +438,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;
}
@@ -502,6 +509,9 @@ dwapb_gpio_get_pdata(struct device *dev)
dev_warn(dev, "no irq for port%d\n", pp->idx);
}
+ 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;
}
@@ -576,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)
{
@@ -670,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 v10 2/3] gpio: dwapb: convert device node to fwnode
This patch converts device node to fwnode for dwapb driver, so
as to provide a unified fwnode for DT and ACPI bindings.
Tested-by: Alan Tull
Acked-by: Andy Shevchenko
Signed-off-by: Jiang Qiu
---
drivers/gpio/gpio-dwapb.c| 36 +++-
drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
include/linux/platform_data/gpio-dwapb.h | 2 +-
3 files changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 772d743..7517c2f 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -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,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
}
#ifdef CONFIG_OF_GPIO
- port->gc.of_node = pp->node;
+ port->gc.of_node = to_of_node(pp->fwnode);
#endif
port->gc.ngpio = pp->ngpio;
port->gc.base = pp->gpio_base;
@@ -447,19 +448,15 @@ 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 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,18 +471,18 @@ 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 port%d\n", i);
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 port%d\n",
@@ -497,9 +494,10 @@ 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 &&
+ fwnode_property_read_bool(fwnode,
+ "interrupt-controller")) {
+ pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0);
if (!pp->irq)
dev_warn(dev, "no irq for port%d\n", pp->idx);
}
@@ -521,7 +519,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);
}
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
b/drivers/mfd/intel_quark_i2c_gpio.c
index a4ef99b..a24b35f 100644
--- a/drivers/mfd/intel_quark_i2c_
[PATCH v10 0/3] gpio: dwapb: add gpio-signaled acpi event support for power button
This patchset adds gpio-signaled acpi events support for power button on hisilicon D02 board. The three patches respectively: - remove name from dwapb_port_property - convert device node to fwnode - 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 v9 -> v10: - fixed one minor Changes v8 -> v9: - fixed a low-level compile warning Changes v7 -> v8: - fixed few minors Changes v6 -> v7: - add patch1 by Alan's suggestion Changes v5 -> v6: - merge patch 2~3 to one patch - small fixed from Alan's suggestion - fixed subject title reference commit history 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 Jiang Qiu (3): gpio: dwapb: remove name from dwapb_port_property gpio: dwapb: convert device node to fwnode gpio: dwapb: add gpio-signaled acpi event support drivers/gpio/gpio-dwapb.c| 77 +++- drivers/mfd/intel_quark_i2c_gpio.c | 3 +- include/linux/platform_data/gpio-dwapb.h | 3 +- 3 files changed, 47 insertions(+), 36 deletions(-) -- 1.9.1
[PATCH v10 1/3] gpio: dwapb: remove name from dwapb_port_property
This patch removed the name property from dwapb_port_property.
The name property is redundant, since we can get this info
from dwapb_gpio dev node.
Reviewed-by: Andy Shevchenko
Signed-off-by: Jiang Qiu
---
drivers/gpio/gpio-dwapb.c| 24 +++-
drivers/mfd/intel_quark_i2c_gpio.c | 1 -
include/linux/platform_data/gpio-dwapb.h | 1 -
3 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 597de1e..772d743 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -409,8 +409,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
err = bgpio_init(&port->gc, gpio->dev, 4, dat, set, NULL, dirout,
NULL, false);
if (err) {
- dev_err(gpio->dev, "failed to init gpio chip for %s\n",
- pp->name);
+ dev_err(gpio->dev, "failed to init gpio chip for port%d\n",
+ port->idx);
return err;
}
@@ -429,8 +429,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
err = gpiochip_add_data(&port->gc, port);
if (err)
- dev_err(gpio->dev, "failed to register gpiochip for %s\n",
- pp->name);
+ dev_err(gpio->dev, "failed to register gpiochip for port%d\n",
+ port->idx);
else
port->is_registered = true;
@@ -480,15 +480,16 @@ dwapb_gpio_get_pdata_of(struct device *dev)
if (of_property_read_u32(port_np, "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 for port%d\n", i);
return ERR_PTR(-EINVAL);
}
if (of_property_read_u32(port_np, "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 for port%d\n",
+i);
pp->ngpio = 32;
}
@@ -499,15 +500,12 @@ dwapb_gpio_get_pdata_of(struct device *dev)
if (pp->idx == 0 &&
of_property_read_bool(port_np, "interrupt-controller")) {
pp->irq = irq_of_parse_and_map(port_np, 0);
- if (!pp->irq) {
- dev_warn(dev, "no irq for bank %s\n",
-port_np->full_name);
- }
+ if (!pp->irq)
+ dev_warn(dev, "no irq for port%d\n", pp->idx);
}
pp->irq_shared = false;
pp->gpio_base = -1;
- pp->name= port_np->full_name;
}
return pdata;
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
b/drivers/mfd/intel_quark_i2c_gpio.c
index bdc5e27..a4ef99b 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -220,7 +220,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev,
struct mfd_cell *cell)
/* Set the properties for portA */
pdata->properties->node = NULL;
- pdata->properties->name = "intel-quark-x1000-gpio-portA";
pdata->properties->idx = 0;
pdata->properties->ngpio= INTEL_QUARK_MFD_NGPIO;
pdata->properties->gpio_base= INTEL_QUARK_MFD_GPIO_BASE;
diff --git a/include/linux/platform_data/gpio-dwapb.h
b/include/linux/platform_data/gpio-dwapb.h
index 28702c8..955b579 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -16,7 +16,6 @@
struct dwapb_port_property {
struct device_node *node;
- const char *name;
unsigned intidx;
unsigned intngpio;
unsigned intgpio_base;
--
1.9.1
Re: [PATCH v9 0/3] gpio: dwapb: add gpio-signaled acpi event support for power button
在 2016/4/21 0:10, Alan Tull 写道: > On Wed, Apr 20, 2016 at 8:06 AM, Andy Shevchenko > wrote: >> On Wed, Apr 20, 2016 at 10:13 AM, Jiang Qiu wrote: >>> This patchset adds gpio-signaled acpi events support for power button on >>> hisilicon >>> D02 board. >>> >>> The three patches respectively: >>> - remove name from dwapb_port_property >>> - convert device node to fwnode >>> - 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 v8 -> v9: >>>- fixed a low-level compile warning >> >> Please, don't send updates to often. You see, now I noticed one more >> thing you have to address. >> Usually the time between fixup series is something like 24h. Thus, >> waiting for v10 at least tomorrow or even next week. >> >> Linus, for my point of view next version should be fine and final. The >> current code is in a good shape already. > > Reviewed, tested v9, looks good. > > Alan Tull > Alan, Thanks a lot. :) > >> >>> >>> Changes v7 -> v8: >>>- fixed few minors >>> >>> Changes v6 -> v7: >>>- add patch1 by Alan's suggestion >>> >>> Changes v5 -> v6: >>>- merge patch 2~3 to one patch >>>- small fixed from Alan's suggestion >>>- fixed subject title reference commit history >>> >>> 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 >>> >>> Jiang Qiu (3): >>> gpio: dwapb: remove name from dwapb_port_property >>> gpio: dwapb: convert device node to fwnode >>> gpio: dwapb: add gpio-signaled acpi event support >>> >>> drivers/gpio/gpio-dwapb.c| 78 >>> +++- >>> drivers/mfd/intel_quark_i2c_gpio.c | 3 +- >>> include/linux/platform_data/gpio-dwapb.h | 3 +- >>> 3 files changed, 48 insertions(+), 36 deletions(-) >>> >>> -- >>> 1.9.1 >>> >> >> >> >> -- >> With Best Regards, >> Andy Shevchenko > > . >
Re: [PATCH v9 0/3] gpio: dwapb: add gpio-signaled acpi event support for power button
在 2016/4/20 21:06, Andy Shevchenko 写道: > On Wed, Apr 20, 2016 at 10:13 AM, Jiang Qiu wrote: >> This patchset adds gpio-signaled acpi events support for power button on >> hisilicon >> D02 board. >> >> The three patches respectively: >> - remove name from dwapb_port_property >> - convert device node to fwnode >> - 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 v8 -> v9: >>- fixed a low-level compile warning > > Please, don't send updates to often. You see, now I noticed one more > thing you have to address. > Usually the time between fixup series is something like 24h. Thus, > waiting for v10 at least tomorrow or even next week. > Thanks for the reminder. I made a mistake when I finished my test and merge the last code because my careless. I'm too anxious to correct it :) > Linus, for my point of view next version should be fine and final. The > current code is in a good shape already. > >> >> Changes v7 -> v8: >>- fixed few minors >> >> Changes v6 -> v7: >>- add patch1 by Alan's suggestion >> >> Changes v5 -> v6: >>- merge patch 2~3 to one patch >>- small fixed from Alan's suggestion >>- fixed subject title reference commit history >> >> 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 >> >> Jiang Qiu (3): >> gpio: dwapb: remove name from dwapb_port_property >> gpio: dwapb: convert device node to fwnode >> gpio: dwapb: add gpio-signaled acpi event support >> >> drivers/gpio/gpio-dwapb.c| 78 >> +++- >> drivers/mfd/intel_quark_i2c_gpio.c | 3 +- >> include/linux/platform_data/gpio-dwapb.h | 3 +- >> 3 files changed, 48 insertions(+), 36 deletions(-) >> >> -- >> 1.9.1 >> > > >
Re: [PATCH v9 2/3] gpio: dwapb: convert device node to fwnode
在 2016/4/20 21:04, Andy Shevchenko 写道:
> On Wed, Apr 20, 2016 at 10:13 AM, Jiang Qiu wrote:
>> This patch converts device node to fwnode for dwapb driver, so
>> as to provide a unified fwnode for DT and ACPI bindings.
>>
>> Tested-by: Alan Tull
>> Acked-by: Andy Shevchenko
>> Signed-off-by: Jiang Qiu
>> ---
>> drivers/gpio/gpio-dwapb.c| 37
>>
>> drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
>> include/linux/platform_data/gpio-dwapb.h | 2 +-
>> 3 files changed, 20 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
>> index 772d743..92bc204 100644
>> --- a/drivers/gpio/gpio-dwapb.c
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -22,6 +22,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -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;
>
> Someone commented on this that it's simple
> …of_node = to_of_node(…);
>
Yes, the to_of_node() function has checked as like above, as follow:
static inline struct device_node *to_of_node(struct fwnode_handle *fwnode)
{
return is_of_node(fwnode) ?
container_of(fwnode, struct device_node, fwnode) : NULL;
}
Next version, I will fixed it.
>> #endif
>> port->gc.ngpio = pp->ngpio;
>> port->gc.base = pp->gpio_base;
>> @@ -447,19 +449,15 @@ 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 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,18 +472,18 @@ 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 port%d\n",
>> i);
>> 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,
>>
[PATCH v9 3/3] gpio: dwapb: add gpio-signaled acpi event support
This patch adds gpio-signaled acpi event support. It is used for
power button on hisilicon D02 board, an arm64 platform.
The corresponding DSDT file is defined as follows:
Device(GPI0) {
Name(_HID, "HISI0181")
Name(_ADR, 0)
Name(_UID, 0)
Name (_CRS, ResourceTemplate () {
Memory32Fixed (ReadWrite, 0x802e, 0x1)
Interrupt (ResourceConsumer, Level, ActiveHigh,
Exclusive,,,) {344}
})
Device(PRTa) {
Name (_DSD, Package () {
Package () {
Package () {"reg",0},
Package () {"snps,nr-gpios",32},
}
})
}
Name (_AEI, ResourceTemplate () {
GpioInt(Edge, ActiveLow, ExclusiveAndWake,
PullUp, , " \\_SB.GPI0") {8}
})
Method (_E08, 0x0, NotSerialized) {
Notify (\_SB.PWRB, 0x80)
}
}
Acked-by: Mika Westerberg
Reviewed-by: Andy Shevchenko
Signed-off-by: Jiang Qiu
---
drivers/gpio/gpio-dwapb.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 92bc204..c411e95 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -7,6 +7,7 @@
*
* All enquiries to supp...@picochip.com
*/
+#include
#include
/* FIXME: for gpio_get_value(), replace this with direct register read */
#include
@@ -27,6 +28,8 @@
#include
#include
+#include "gpiolib.h"
+
#define GPIO_SWPORTA_DR0x00
#define GPIO_SWPORTA_DDR 0x04
#define GPIO_SWPORTB_DR0x0c
@@ -436,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;
}
@@ -503,6 +510,9 @@ dwapb_gpio_get_pdata(struct device *dev)
dev_warn(dev, "no irq for port%d\n", pp->idx);
}
+ 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;
}
@@ -577,6 +587,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 +687,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 v9 2/3] gpio: dwapb: convert device node to fwnode
This patch converts device node to fwnode for dwapb driver, so
as to provide a unified fwnode for DT and ACPI bindings.
Tested-by: Alan Tull
Acked-by: Andy Shevchenko
Signed-off-by: Jiang Qiu
---
drivers/gpio/gpio-dwapb.c| 37
drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
include/linux/platform_data/gpio-dwapb.h | 2 +-
3 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 772d743..92bc204 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -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;
@@ -447,19 +449,15 @@ 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 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,18 +472,18 @@ 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 port%d\n", i);
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 port%d\n",
@@ -497,9 +495,10 @@ 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 &&
+ fwnode_property_read_bool(fwnode,
+ "interrupt-controller")) {
+ pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0);
if (!pp->irq)
dev_warn(dev, "no irq for port%d\n", pp->idx);
}
@@ -521,7 +520,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);
}
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
b/drivers/mfd/intel_quark_i2c_gpio.c
index a4ef99b..a24b35f
[PATCH v9 0/3] gpio: dwapb: add gpio-signaled acpi event support for power button
This patchset adds gpio-signaled acpi events support for power button on hisilicon D02 board. The three patches respectively: - remove name from dwapb_port_property - convert device node to fwnode - 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 v8 -> v9: - fixed a low-level compile warning Changes v7 -> v8: - fixed few minors Changes v6 -> v7: - add patch1 by Alan's suggestion Changes v5 -> v6: - merge patch 2~3 to one patch - small fixed from Alan's suggestion - fixed subject title reference commit history 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 Jiang Qiu (3): gpio: dwapb: remove name from dwapb_port_property gpio: dwapb: convert device node to fwnode gpio: dwapb: add gpio-signaled acpi event support drivers/gpio/gpio-dwapb.c| 78 +++- drivers/mfd/intel_quark_i2c_gpio.c | 3 +- include/linux/platform_data/gpio-dwapb.h | 3 +- 3 files changed, 48 insertions(+), 36 deletions(-) -- 1.9.1
[PATCH v9 1/3] gpio: dwapb: remove name from dwapb_port_property
This patch removed the name property from dwapb_port_property.
The name property is redundant, since we can get this info
from dwapb_gpio dev node.
Reviewed-by: Andy Shevchenko
Signed-off-by: Jiang Qiu
---
drivers/gpio/gpio-dwapb.c| 24 +++-
drivers/mfd/intel_quark_i2c_gpio.c | 1 -
include/linux/platform_data/gpio-dwapb.h | 1 -
3 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 597de1e..772d743 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -409,8 +409,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
err = bgpio_init(&port->gc, gpio->dev, 4, dat, set, NULL, dirout,
NULL, false);
if (err) {
- dev_err(gpio->dev, "failed to init gpio chip for %s\n",
- pp->name);
+ dev_err(gpio->dev, "failed to init gpio chip for port%d\n",
+ port->idx);
return err;
}
@@ -429,8 +429,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
err = gpiochip_add_data(&port->gc, port);
if (err)
- dev_err(gpio->dev, "failed to register gpiochip for %s\n",
- pp->name);
+ dev_err(gpio->dev, "failed to register gpiochip for port%d\n",
+ port->idx);
else
port->is_registered = true;
@@ -480,15 +480,16 @@ dwapb_gpio_get_pdata_of(struct device *dev)
if (of_property_read_u32(port_np, "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 for port%d\n", i);
return ERR_PTR(-EINVAL);
}
if (of_property_read_u32(port_np, "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 for port%d\n",
+i);
pp->ngpio = 32;
}
@@ -499,15 +500,12 @@ dwapb_gpio_get_pdata_of(struct device *dev)
if (pp->idx == 0 &&
of_property_read_bool(port_np, "interrupt-controller")) {
pp->irq = irq_of_parse_and_map(port_np, 0);
- if (!pp->irq) {
- dev_warn(dev, "no irq for bank %s\n",
-port_np->full_name);
- }
+ if (!pp->irq)
+ dev_warn(dev, "no irq for port%d\n", pp->idx);
}
pp->irq_shared = false;
pp->gpio_base = -1;
- pp->name= port_np->full_name;
}
return pdata;
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
b/drivers/mfd/intel_quark_i2c_gpio.c
index bdc5e27..a4ef99b 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -220,7 +220,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev,
struct mfd_cell *cell)
/* Set the properties for portA */
pdata->properties->node = NULL;
- pdata->properties->name = "intel-quark-x1000-gpio-portA";
pdata->properties->idx = 0;
pdata->properties->ngpio= INTEL_QUARK_MFD_NGPIO;
pdata->properties->gpio_base= INTEL_QUARK_MFD_GPIO_BASE;
diff --git a/include/linux/platform_data/gpio-dwapb.h
b/include/linux/platform_data/gpio-dwapb.h
index 28702c8..955b579 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -16,7 +16,6 @@
struct dwapb_port_property {
struct device_node *node;
- const char *name;
unsigned intidx;
unsigned intngpio;
unsigned intgpio_base;
--
1.9.1
[PATCH v8 2/3] gpio: dwapb: convert device node to fwnode
This patch converts device node to fwnode for dwapb driver, so
as to provide a unified fwnode for DT and ACPI bindings.
Tested-by: Alan Tull
Acked-by: Andy Shevchenko
Signed-off-by: Jiang Qiu
---
drivers/gpio/gpio-dwapb.c| 37
drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
include/linux/platform_data/gpio-dwapb.h | 2 +-
3 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index ea04172..7258cc5 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -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;
@@ -447,19 +449,15 @@ 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 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,18 +472,18 @@ 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 port%d\n", i);
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 port%d\n",
@@ -497,9 +495,10 @@ 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 &&
+ fwnode_property_read_bool(fwnode,
+ "interrupt-controller")) {
+ pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0);
if (!pp->irq)
dev_warn(dev, "no irq for port%d\n", pp->idx);
}
@@ -521,7 +520,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);
}
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
b/drivers/mfd/intel_quark_i2c_gpio.c
index a4ef99b..a24b35f
[PATCH v8 3/3] gpio: dwapb: add gpio-signaled acpi event support
This patch adds gpio-signaled acpi event support. It is used for
power button on hisilicon D02 board, an arm64 platform.
The corresponding DSDT file is defined as follows:
Device(GPI0) {
Name(_HID, "HISI0181")
Name(_ADR, 0)
Name(_UID, 0)
Name (_CRS, ResourceTemplate () {
Memory32Fixed (ReadWrite, 0x802e, 0x1)
Interrupt (ResourceConsumer, Level, ActiveHigh,
Exclusive,,,) {344}
})
Device(PRTa) {
Name (_DSD, Package () {
Package () {
Package () {"reg",0},
Package () {"snps,nr-gpios",32},
}
})
}
Name (_AEI, ResourceTemplate () {
GpioInt(Edge, ActiveLow, ExclusiveAndWake,
PullUp, , " \\_SB.GPI0") {8}
})
Method (_E08, 0x0, NotSerialized) {
Notify (\_SB.PWRB, 0x80)
}
}
Acked-by: Mika Westerberg
Reviewed-by: Andy Shevchenko
Signed-off-by: Jiang Qiu
---
drivers/gpio/gpio-dwapb.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 7258cc5..b2b83d6 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -7,6 +7,7 @@
*
* All enquiries to supp...@picochip.com
*/
+#include
#include
/* FIXME: for gpio_get_value(), replace this with direct register read */
#include
@@ -27,6 +28,8 @@
#include
#include
+#include "gpiolib.h"
+
#define GPIO_SWPORTA_DR0x00
#define GPIO_SWPORTA_DDR 0x04
#define GPIO_SWPORTB_DR0x0c
@@ -436,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;
}
@@ -503,6 +510,9 @@ dwapb_gpio_get_pdata(struct device *dev)
dev_warn(dev, "no irq for port%d\n", pp->idx);
}
+ 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;
}
@@ -577,6 +587,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 +687,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 v8 1/3] gpio: dwapb: remove name from dwapb_port_property
This patch removed the name property from dwapb_port_property.
The name property is redundant, because we can get this info
from dwapb_gpio dev node.
Reviewed-by: Andy Shevchenko
Signed-off-by: Jiang Qiu
---
drivers/gpio/gpio-dwapb.c| 24 +++-
drivers/mfd/intel_quark_i2c_gpio.c | 1 -
include/linux/platform_data/gpio-dwapb.h | 1 -
3 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 597de1e..ea04172 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -409,8 +409,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
err = bgpio_init(&port->gc, gpio->dev, 4, dat, set, NULL, dirout,
NULL, false);
if (err) {
- dev_err(gpio->dev, "failed to init gpio chip for %s\n",
- pp->name);
+ dev_err(gpio->dev, "failed to init gpio chip for port%\n",
+ port->idx);
return err;
}
@@ -429,8 +429,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
err = gpiochip_add_data(&port->gc, port);
if (err)
- dev_err(gpio->dev, "failed to register gpiochip for %s\n",
- pp->name);
+ dev_err(gpio->dev, "failed to register gpiochip for port%d\n",
+ port->idx);
else
port->is_registered = true;
@@ -480,15 +480,16 @@ dwapb_gpio_get_pdata_of(struct device *dev)
if (of_property_read_u32(port_np, "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 for port%d\n", i);
return ERR_PTR(-EINVAL);
}
if (of_property_read_u32(port_np, "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 for port%d\n",
+i);
pp->ngpio = 32;
}
@@ -499,15 +500,12 @@ dwapb_gpio_get_pdata_of(struct device *dev)
if (pp->idx == 0 &&
of_property_read_bool(port_np, "interrupt-controller")) {
pp->irq = irq_of_parse_and_map(port_np, 0);
- if (!pp->irq) {
- dev_warn(dev, "no irq for bank %s\n",
-port_np->full_name);
- }
+ if (!pp->irq)
+ dev_warn(dev, "no irq for port%d\n", pp->idx);
}
pp->irq_shared = false;
pp->gpio_base = -1;
- pp->name= port_np->full_name;
}
return pdata;
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
b/drivers/mfd/intel_quark_i2c_gpio.c
index bdc5e27..a4ef99b 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -220,7 +220,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev,
struct mfd_cell *cell)
/* Set the properties for portA */
pdata->properties->node = NULL;
- pdata->properties->name = "intel-quark-x1000-gpio-portA";
pdata->properties->idx = 0;
pdata->properties->ngpio= INTEL_QUARK_MFD_NGPIO;
pdata->properties->gpio_base= INTEL_QUARK_MFD_GPIO_BASE;
diff --git a/include/linux/platform_data/gpio-dwapb.h
b/include/linux/platform_data/gpio-dwapb.h
index 28702c8..955b579 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -16,7 +16,6 @@
struct dwapb_port_property {
struct device_node *node;
- const char *name;
unsigned intidx;
unsigned intngpio;
unsigned intgpio_base;
--
1.9.1
[PATCH v8 0/3] gpio: dwapb: add gpio-signaled acpi event support for power button
This patchset adds gpio-signaled acpi events support for power button on hisilicon D02 board. The three patches respectively: - remove name from dwapb_port_property - convert device node to fwnode - 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 v7 -> v8: - fixed few minors Changes v6 -> v7: - add patch1 by Alan's suggestion Changes v5 -> v6: - merge patch 2~3 to one patch - small fixed from Alan's suggestion - fixed subject title reference commit history 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 Jiang Qiu (3): gpio: dwapb: remove name from dwapb_port_property gpio: dwapb: convert device node to fwnode gpio: dwapb: add gpio-signaled acpi event support drivers/gpio/gpio-dwapb.c| 78 +++- drivers/mfd/intel_quark_i2c_gpio.c | 3 +- include/linux/platform_data/gpio-dwapb.h | 3 +- 3 files changed, 48 insertions(+), 36 deletions(-) -- 1.9.1
Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support
在 2016/4/15 15:40, Linus Walleij 写道: > On Tue, Apr 12, 2016 at 8:46 AM, Mika Westerberg > wrote: >> On Mon, Apr 11, 2016 at 08:43:22PM +0800, Jiang Qiu wrote: >>>> Currently it just complains if something goes wrong. The GPIO driver >>>> itself can still work just fine (including interrupts). >>>> >>>> I'm fine to change it to return an error code. >>> Agree, if add a error code for acpi_gpiochip_request_interrupts(), it looks >>> more pretty. >>> >>> However, this function is common for other part, maybe cause any other >>> effects if I >>> do this change, did you think so? >> I'm thinking what the callers are going to do with the error code. >> Basically it means that we were not able to attach and configure ACPI >> event GPIOs. It does not prevent GPIO drivers from functioning so they >> probably just print out some warning message and continue probing, and >> we already warn in acpi_gpiochip_request_interrupts() if something fails. >> >> Unless Linus W insists, let's just keep it as is for now :) > I'm fine with it, don' worry. > > I'm just waiting for this patch set to mature so I can apply > it. Many thanks, I will fix these minor mentioned by Andy and get ready for the new version ASAP. Regards, Jiang > > Yours, > Linus Walleij > > . >
Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support
在 2016/4/12 14:46, Mika Westerberg 写道: > On Mon, Apr 11, 2016 at 08:43:22PM +0800, Jiang Qiu wrote: >>> Currently it just complains if something goes wrong. The GPIO driver >>> itself can still work just fine (including interrupts). >>> >>> I'm fine to change it to return an error code. >> Agree, if add a error code for acpi_gpiochip_request_interrupts(), it looks >> more pretty. >> >> However, this function is common for other part, maybe cause any other >> effects if I >> do this change, did you think so? > I'm thinking what the callers are going to do with the error code. > Basically it means that we were not able to attach and configure ACPI > event GPIOs. It does not prevent GPIO drivers from functioning so they > probably just print out some warning message and continue probing, and > we already warn in acpi_gpiochip_request_interrupts() if something fails. > > Unless Linus W insists, let's just keep it as is for now :) Fine to me, thanks:). > . >
Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support
在 2016/4/8 16:38, Mika Westerberg 写道:
> On Fri, Apr 08, 2016 at 10:26:28AM +0200, Linus Walleij wrote:
>> On Wed, Apr 6, 2016 at 9:08 AM, qiujiang wrote:
>>
>>> This patch adds gpio-signaled acpi event support. It is used for
>>> power button on hisilicon D02 board, an arm64 platform.
>>>
>>> The corresponding DSDT file is defined as follows:
>>> Device(GPI0) {
>>> Name(_HID, "HISI0181")
>>> Name(_ADR, 0)
>>> Name(_UID, 0)
>>>
>>> Name (_CRS, ResourceTemplate () {
>>> Memory32Fixed (ReadWrite, 0x802e, 0x1)
>>> Interrupt (ResourceConsumer, Level, ActiveHigh,
>>> Exclusive,,,) {344}
>>> })
>>>
>>> Device(PRTa) {
>>> Name (_DSD, Package () {
>>> Package () {
>>> Package () {"reg",0},
>>> Package () {"snps,nr-gpios",32},
>>> }
>>> })
>>> }
>>>
>>> Name (_AEI, ResourceTemplate () {
>>> GpioInt(Edge, ActiveLow, ExclusiveAndWake,
>>> PullUp, , " \\_SB.GPI0") {8}
>>> })
>>>
>>> Method (_E08, 0x0, NotSerialized) {
>>> Notify (\_SB.PWRB, 0x80)
>>> }
>>> }
>>>
>>> Acked-by: Mika Westerberg
>>> Reviewed-by: Andy Shevchenko
>>> Signed-off-by: qiujiang
>> Admittedly I'm an ACPI novice and need help with deciding
>> about ACPI, but I mostly trust Mika to know these things right.
>>
>> About this:
>>
>>> + /* Add GPIO-signaled ACPI event support */
>>> + if (pp->irq)
>>> + acpi_gpiochip_request_interrupts(&port->gc);
>> It's weird to me that the driver already has a requested IRQ and
>> everything, now it has to request it again from ACPI.
> This is different thing, though.
>
> Calling acpi_gpiochip_request_interrupts() results _AEI ACPI method
> being evaluated that returns a list of GPIOs which are used as event
> sources. acpi_gpiochip_request_interrupts() then goes and installs
> interrupt handler per each GPIO in that list.
>
>> When I look into the acpi_gpiochip_request_interrupts()
>> I find it weird that it is void given how much can go wrong
>> inside it. Should it not return an errorcode?
> Currently it just complains if something goes wrong. The GPIO driver
> itself can still work just fine (including interrupts).
>
> I'm fine to change it to return an error code.
Agree, if add a error code for acpi_gpiochip_request_interrupts(), it looks
more pretty.
However, this function is common for other part, maybe cause any other effects
if I
do this change, did you think so?
>>> + if (has_acpi_companion(dev) && pp->idx == 0)
>>> + pp->irq = platform_get_irq(to_platform_device(dev),
>>> 0);
>> As it was already fetched here and then later requested,
>> we still have to call acpi_gpiochip_request_interrupts()
>> further down the road? That is confusing to me, can you
>> explain what is going on?
> See above.
>
> .
>
Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support
在 2016/4/8 16:26, Linus Walleij 写道:
> On Wed, Apr 6, 2016 at 9:08 AM, qiujiang wrote:
>
>> This patch adds gpio-signaled acpi event support. It is used for
>> power button on hisilicon D02 board, an arm64 platform.
>>
>> The corresponding DSDT file is defined as follows:
>> Device(GPI0) {
>> Name(_HID, "HISI0181")
>> Name(_ADR, 0)
>> Name(_UID, 0)
>>
>> Name (_CRS, ResourceTemplate () {
>> Memory32Fixed (ReadWrite, 0x802e, 0x1)
>> Interrupt (ResourceConsumer, Level, ActiveHigh,
>> Exclusive,,,) {344}
>> })
>>
>> Device(PRTa) {
>> Name (_DSD, Package () {
>> Package () {
>> Package () {"reg",0},
>> Package () {"snps,nr-gpios",32},
>> }
>> })
>> }
>>
>> Name (_AEI, ResourceTemplate () {
>> GpioInt(Edge, ActiveLow, ExclusiveAndWake,
>> PullUp, , " \\_SB.GPI0") {8}
>> })
>>
>> Method (_E08, 0x0, NotSerialized) {
>> Notify (\_SB.PWRB, 0x80)
>> }
>> }
>>
>> Acked-by: Mika Westerberg
>> Reviewed-by: Andy Shevchenko
>> Signed-off-by: qiujiang
> Admittedly I'm an ACPI novice and need help with deciding
> about ACPI, but I mostly trust Mika to know these things right.
>
> About this:
>
>> + /* Add GPIO-signaled ACPI event support */
>> + if (pp->irq)
>> + acpi_gpiochip_request_interrupts(&port->gc);
> It's weird to me that the driver already has a requested IRQ and
> everything, now it has to request it again from ACPI.
>
> When I look into the acpi_gpiochip_request_interrupts()
> I find it weird that it is void given how much can go wrong
> inside it. Should it not return an errorcode?
Just as Mika said, these are two different things:
platform_get_irq() requestedIRQ resource from interrupt subsystem and
create irq mapping, then gose ready for device, but dose not request
a handler immediately.
acpi_gpiochip_request_interrupts() parse the _AEI and _EVT object and
result awareness of what GPIO pin is used.Then, install a event handler
for each pin by request this pp->irq.
If something gose wrong when acpi_gpiochip_request_interrupts() process,
GPIO itself can still works fine.
>> + if (has_acpi_companion(dev) && pp->idx == 0)
>> + pp->irq = platform_get_irq(to_platform_device(dev),
>> 0);
> As it was already fetched here and then later requested,
> we still have to call acpi_gpiochip_request_interrupts()
> further down the road? That is confusing to me, can you
> explain what is going on?
>
> Yours,
> Linus Walleij
>
> .
>
Re: [PATCH v7 1/3] gpio: dwapb: remove name from dwapb_port_property
Hi Andy,
Thanks for your reply. See my comments inline.
Thanks
Jiang
在 2016/4/6 20:57, Andy Shevchenko 写道:
> On Wed, Apr 6, 2016 at 10:07 AM, qiujiang wrote:
>> This patch removed the name property from dwapb_port_property.
>> The name property is redundant because we can get those info
>> from dwapb_gpio dev and pp->idx property.
>
> Where idx is used in such replacements?
Actually, it is not used so far. As Alan mentioned, the only additional
info from the pp->name against dev is the port index. I present here to
prevent anyone from missing it.
If it is inappropriate, I will remove it.
>
>> --- a/drivers/gpio/gpio-dwapb.c
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -409,8 +409,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>> err = bgpio_init(&port->gc, gpio->dev, 4, dat, set, NULL, dirout,
>> NULL, false);
>> if (err) {
>> - dev_err(gpio->dev, "failed to init gpio chip for %s\n",
>> - pp->name);
>> + dev_err(gpio->dev, "failed to init gpio chip\n");
>
> Do we have any port index here available (expected value I suppose)?
The 3rd parameter 'offs' can be used, I will add it.
>
>> @@ -429,8 +428,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>>
>> err = gpiochip_add_data(&port->gc, port);
>> if (err)
>> - dev_err(gpio->dev, "failed to register gpiochip for %s\n",
>> - pp->name);
>> + dev_err(gpio->dev, "failed to register gpiochip\n");
>
> Ditto.
>
>
>> @@ -499,15 +498,12 @@ dwapb_gpio_get_pdata_of(struct device *dev)
>> if (pp->idx == 0 &&
>> of_property_read_bool(port_np, "interrupt-controller")) {
>> pp->irq = irq_of_parse_and_map(port_np, 0);
>> - if (!pp->irq) {
>> - dev_warn(dev, "no irq for bank %s\n",
>> -port_np->full_name);
>> - }
>> + if (!pp->irq)
>> + dev_warn(dev, "no irq for this bank\n");
>
> pp->idx here?
>
> dev_warn(dev, "no irq for port%d\n", pp->idx);
Here, pp->idx should always be zero, means portA, because only portA can be a
interrupt controller as dwapb gpio IP defined. So, I omited it.
>
>> --- a/drivers/mfd/intel_quark_i2c_gpio.c
>> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
>> @@ -220,7 +220,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev,
>> struct mfd_cell *cell)
>>
>> /* Set the properties for portA */
>> pdata->properties->node = NULL;
>> - pdata->properties->name = "intel-quark-x1000-gpio-portA";
>> pdata->properties->idx = 0;
>> pdata->properties->ngpio= INTEL_QUARK_MFD_NGPIO;
>> pdata->properties->gpio_base= INTEL_QUARK_MFD_GPIO_BASE;
>
> For Quark part:
> Acked-by: Andy Shevchenko
>
Re: [PATCH v7 2/3] gpio: dwapb: convert device node to fwnode
在 2016/4/6 21:01, Andy Shevchenko 写道:
> On Wed, Apr 6, 2016 at 10:07 AM, qiujiang wrote:
>> This patch converts device node to fwnode for dwapb driver, so
>> as to provide a unified fwnode for DT and ACPI bindings.
>>
>> Acked-by: Andy Shevchenko
>> Signed-off-by: qiujiang
>
>> static struct dwapb_platform_data *
>> -dwapb_gpio_get_pdata_of(struct device *dev)
>> +dwapb_gpio_get_pdata(struct device *dev)
>
> I don't remember if I had asked already
> does it fit now one row?
Hi Andy,
It dose not fit one row yet, checkpatch.pl will report a warning.
>
>> @@ -495,9 +493,10 @@ 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 &&
>
> is_of_fwnode() && ?
>
>> + fwnode_property_read_bool(fwnode,
>> + "interrupt-controller")) {
>> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode),
>> 0);
>> if (!pp->irq)
>> dev_warn(dev, "no irq for this bank\n");
>
Re: [PATCH v6 2/2] gpio: dwapb: add gpio-signaled acpi event support
在 2016/3/30 19:41, Andy Shevchenko 写道: > On Sat, Mar 26, 2016 at 4:31 AM, qiujiang wrote: >> This patch adds gpio-signaled acpi event support. It is used for >> power button on hisilicon D02 board, an arm64 platform. > > Yes, looks good, my tag stays. > >> @@ -502,10 +509,17 @@ dwapb_gpio_get_pdata(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; > > >> + >> if (dev->of_node) >> pp->name = to_of_node(fwnode)->full_name; > > This change doesn't belong the patch, though it should gone with the > two lines of pp->name assignment as per Alan's suggestion to get rid > of pp->name completely. Agree, I plan to take Alan's suggestion, thank you. > >> + >> + if (has_acpi_companion(dev)) >> + pp->name = >> acpi_dev_name(to_acpi_device_node(fwnode)); >
Re: [PATCH v6 1/2] gpio: dwapb: convert device node to fwnode
在 2016/3/30 19:38, Andy Shevchenko 写道:
> On Tue, Mar 29, 2016 at 7:30 PM, Alan Tull wrote:
>> On Fri, Mar 25, 2016 at 9:31 PM, qiujiang wrote:
>>
>>> - 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 &&
>>> + fwnode_property_read_bool(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;
>>> + if (dev->of_node)
>>> + pp->name = to_of_node(fwnode)->full_name;
>>
>> Hi Jiang,
>>
>> I tested lightly on a CycloneV and it worked fine (with device tree).
>>
>> One suggestion for both patches: you could remove name from struct
>> dwapb_port_property and get rid of pp->name and nobody would miss it.
>> All it is used for is some dev_err's so the device info gets printed
>> anyway. For example (if I leave the irq out of the DT)
>>
>> gpio-dwapb ff708000.gpio: no irq for bank
>> /soc/gpio@ff708000/gpio-controller@0
>>
>> is redundant. The only additional info here from the name is the port
>> index. That could be added to the messages without having to get the
>> name through the two property/of methods.
>>
>
> Good suggestion! That'll make patches and code cleaner.
>
> Perhaps separate prepended patch?
>
Hi Alan/Andy,
It sounds good, I will follow this suggestion and do a test. But, what's the
"separate prepended patch" mean?
Thanks, Jiang
Re: [PATCH v5 1/3] gpio: designware: convert device node to fwnode
在 2016/3/24 0:20, Alan Tull 写道:
> On Wed, Mar 23, 2016 at 6:41 AM, Jiang Qiu wrote:
>> 在 2016/3/11 4:27, Andy Shevchenko 写道:
>>> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull
>>> wrote:
>>>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang 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
>>>>> Signed-off-by: qiujiang
>>>>> @@ -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@802f {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> compatible = "snps,dw-apb-gpio";
>> reg = <0x0 0x802f 0x0 0x1>;
>>
>> 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, 0x802e, 0x1)
>> 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 ab
Re: [PATCH v5 1/3] gpio: designware: convert device node to fwnode
在 2016/3/11 4:27, Andy Shevchenko 写道:
> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull wrote:
>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang 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
>>> Signed-off-by: qiujiang
>>> @@ -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@802f {
#address-cells = <1>;
#size-cells = <0>;
compatible = "snps,dw-apb-gpio";
reg = <0x0 0x802f 0x0 0x1>;
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, 0x802e, 0x1)
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
在 2016/3/11 4:27, Andy Shevchenko 写道:
> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull wrote:
>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang 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
>>> Signed-off-by: qiujiang
>>> @@ -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@802e {
#address-cells = <1>;
#size-cells = <0>;
compatible = "snps,dw-apb-gpio";
reg = <0x0 0x802e 0x0 0x1>;
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
在 2016/3/11 4:27, Andy Shevchenko 写道:
> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull wrote:
>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang 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
>>> Signed-off-by: qiujiang
>>> @@ -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
在 2016/3/11 3:09, Alan Tull 写道:
> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang 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
>> Signed-off-by: qiujiang
>> ---
>> 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
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -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->ir
Re: [PATCH v4 1/2] gpio: designware: switch device node to fwnode and add acpi binding
在 2016/2/29 21:51, Andy Shevchenko 写道:
> On Mon, Feb 29, 2016 at 3:13 PM, qiujiang wrote:
>> This patch switches device node to fwnode and adds acpi
>> binding. As a result, DT and acpi bingdings are compatible
>> for this driver.
>
> I'm not sure it makes sense to add ACPI binding here. It more logical
> to me to add them in patch 2/2.
> And I see that it touches different subsystems.
>
>>
>> Acked-by: Mika Westerberg
>
> Due to above and Mika's Ack was only to ACPI parts, I think
> you may be split this also to two and we will have clearly logical set:
> 1. Convert to fwnode
> 2. Add ACPI bindings
> 3. ACPI event support.
>
> Does it sound okay to you?
>
> If you do that I give my
> Acked-by: Andy Shevchenko
> to intel_quark_i2c_gpio.c part and
> Reviewed-by: Andy Shevchenko
> to the rest.
>
>> Signed-off-by: qiujiang
>> ---
>> drivers/gpio/gpio-dwapb.c| 62
>> +++-
>> drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
>> include/linux/platform_data/gpio-dwapb.h | 2 +-
>> 3 files changed, 39 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
>> index 597de1e..7a37c65 100644
>> --- a/drivers/gpio/gpio-dwapb.c
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -7,6 +7,7 @@
>> *
>> * All enquiries to supp...@picochip.com
>> */
>> +#include
>
> +#include instead (see above).
>
>> #include
>> /* FIXME: for gpio_get_value(), replace this with direct register read */
>> #include
>
>> @@ -496,18 +492,27 @@ 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")) {
>
>> + if (has_acpi_companion(dev) && pp->idx == 0)
>> + pp->irq = platform_get_irq(to_platform_device(dev),
>> 0);
>> +
>
> To patch 2.
>
>> pp->irq_shared = false;
>> pp->gpio_base = -1;
>
>> - pp->name= port_np->full_name;
>> +
>> + if (dev->of_node)
>> + pp->name = to_of_node(fwnode)->full_name;
>> +
>
> And here you have to provide something in case of default / built-in
> device properties.
Sorry, I'm not very clear here about the case of default / built-in device
properties, can you give a example about what will the code look like ?
>
>
>> + if (has_acpi_companion(dev))
>> + pp->name =
>> acpi_dev_name(to_acpi_device_node(fwnode));
>
> To patch 2.
>
>> }
>>
>> return pdata;
>
>> @@ -580,6 +585,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)
>> {
>> @@ -674,6 +685,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,
>
> Ditto to the above hunks.
>
Re: [PATCH v4 1/2] gpio: designware: switch device node to fwnode and add acpi binding
在 2016/2/29 21:51, Andy Shevchenko 写道:
> On Mon, Feb 29, 2016 at 3:13 PM, qiujiang wrote:
>> This patch switches device node to fwnode and adds acpi
>> binding. As a result, DT and acpi bingdings are compatible
>> for this driver.
>
> I'm not sure it makes sense to add ACPI binding here. It more logical
> to me to add them in patch 2/2.
> And I see that it touches different subsystems.
>
>>
>> Acked-by: Mika Westerberg
>
> Due to above and Mika's Ack was only to ACPI parts, I think
> you may be split this also to two and we will have clearly logical set:
> 1. Convert to fwnode
> 2. Add ACPI bindings
> 3. ACPI event support.
>
> Does it sound okay to you?
>
Sure, I will do that ASAP, thank you.
> If you do that I give my
> Acked-by: Andy Shevchenko
> to intel_quark_i2c_gpio.c part and
> Reviewed-by: Andy Shevchenko
> to the rest.
>
>> Signed-off-by: qiujiang
>> ---
>> drivers/gpio/gpio-dwapb.c| 62
>> +++-
>> drivers/mfd/intel_quark_i2c_gpio.c | 2 +-
>> include/linux/platform_data/gpio-dwapb.h | 2 +-
>> 3 files changed, 39 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
>> index 597de1e..7a37c65 100644
>> --- a/drivers/gpio/gpio-dwapb.c
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -7,6 +7,7 @@
>> *
>> * All enquiries to supp...@picochip.com
>> */
>> +#include
>
> +#include instead (see above).
>
>> #include
>> /* FIXME: for gpio_get_value(), replace this with direct register read */
>> #include
>
>> @@ -496,18 +492,27 @@ 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")) {
>
>> + if (has_acpi_companion(dev) && pp->idx == 0)
>> + pp->irq = platform_get_irq(to_platform_device(dev),
>> 0);
>> +
>
> To patch 2.
>
>> pp->irq_shared = false;
>> pp->gpio_base = -1;
>
>> - pp->name= port_np->full_name;
>> +
>> + if (dev->of_node)
>> + pp->name = to_of_node(fwnode)->full_name;
>> +
>
> And here you have to provide something in case of default / built-in
> device properties.
>
>
>> + if (has_acpi_companion(dev))
>> + pp->name =
>> acpi_dev_name(to_acpi_device_node(fwnode));
>
> To patch 2.
>
>> }
>>
>> return pdata;
>
>> @@ -580,6 +585,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)
>> {
>> @@ -674,6 +685,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,
>
> Ditto to the above hunks.
>
Re: [PATCH v3 1/2] gpio: designware: switch device node to fwnode
在 2016/2/25 21:43, Andy Shevchenko 写道: > On Thu, Feb 25, 2016 at 1:58 PM, Jiang Qiu wrote: >> 在 2016/2/24 21:46, Andy Shevchenko 写道: >>> On Wed, Feb 24, 2016 at 2:33 PM, qiujiang wrote: > >>> - why do you use fwnode_*() instead of device_property_*() calls? >>> What prevents us to move to device property API directly? >> Yes, it looks more reasonable by using devce_property. Howerver, >> device_get_child_node_count was used here to find each child node. This >> API output the fwnode_handle for each child node directly, but device >> property APIs need 'dev' data instead. Actually, the effects of fwnode_*() >> and device_*() are the same. So, I used fwnode_*() APIs here. > > Right, looks okay then. > >>>> - node = dev->of_node; >>>> - if (!IS_ENABLED(CONFIG_OF_GPIO) || !node) >>>> + if (!IS_ENABLED(CONFIG_OF_GPIO) || !(dev->of_node)) >>>> return ERR_PTR(-ENODEV); >>> >>> So, since you converted to fwnode, do you still need this check? >>> >> Although this patch coverted device node to fwnode, only DTs binding was >> supported here, and patch2 support ACPI will remove this check. > > Yes, but like I said below device_get_child_node_count() will take > care of that, will it? Right, device_get_child_node_count() will take of it, this should be removed. > >>>> >>>> - nports = of_get_child_count(node); >>>> + nports = device_get_child_node_count(dev); >>>> if (nports == 0) >>>> return ERR_PTR(-ENODEV); >>> >>> ...I think this one fail if it will not found any child. >> This one fail? yes, it will return to failure. >> I am not very clear here. > > See above. Here, device_get_child_node_count will return ZERO if there is not any child. So, I think this will work ok, will it? >
Re: [PATCH v3 2/2] gpio: designware: add gpio-signaled acpi events support for power button
在 2016/2/24 21:49, Andy Shevchenko 写道:
> On Wed, Feb 24, 2016 at 2:33 PM, qiujiang wrote:
>> This patch modifies the designware gpio controller driver to
>> support the gpio-signaled acpi events. This is used for power
>> button on hisilicon D02 board(an arm64 platform).
>
>> @@ -434,6 +436,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));
>
> Redundant parens.
OK, fixed it in next version, thank you.
>
>> @@ -447,7 +453,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;
>> @@ -455,9 +461,6 @@ dwapb_gpio_get_pdata_of(struct device *dev)
>> int nports;
>> int i;
>>
>
>> - if (!IS_ENABLED(CONFIG_OF_GPIO) || !(dev->of_node))
>> - return ERR_PTR(-ENODEV);
>> -
>
> I think it belongs to patch 1.
If these code remove to patch1, it will contain ACPI support and patch2
implement GPIO-signaled acpi events support only.
Maybe this patchset partition looks more clearly, I think.
>
>> @@ -479,15 +482,13 @@ dwapb_gpio_get_pdata_of(struct device *dev)
>>
>> if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) ||
>> pp->idx >= DWAPB_MAX_PORTS) {
>> - dev_err(dev, "missing/invalid port index for %s\n",
>> - to_of_node(fwnode)->full_name);
>> + dev_err(dev, "missing/invalid port index\n");
>
> Ditto.
>
>> - dev_info(dev, "failed to get number of gpios for
>> %s\n",
>> -to_of_node(fwnode)->full_name);
>> + dev_info(dev, "failed to get number of gpios\n");
>
> Ditto.
>
>> @@ -495,7 +496,7 @@ dwapb_gpio_get_pdata_of(struct device *dev)
>> * Only port A can provide interrupts in all configurations
>> of
>> * the IP.
>> */
>> - if (pp->idx == 0 &&
>> + if (dev->of_node && pp->idx == 0 &&
>
> Why is it needed?
Different APIs was used to parse interrupt resource for DT and ACPI, a unified
way
platform_get_irq looks like more resonable, I will fixed it in the next version.
>
>
Re: [PATCH v3 1/2] gpio: designware: switch device node to fwnode
在 2016/2/24 21:46, Andy Shevchenko 写道:
> On Wed, Feb 24, 2016 at 2:33 PM, qiujiang wrote:
>> This patch switch device node to fwnode in dwapb_port_property,
>> so as to apply a unified data structure for DT and ACPI.
>>
>> This change also needs to be done in intel_quark_i2c_gpio driver,
>> since it depends on gpio-dwapb driver.
>>
>> Acked-by: Mika Westerberg
>> Signed-off-by: qiujiang
>
> Yes, something like this.
> Though I have questions:
> - why do you use fwnode_*() instead of device_property_*() calls?
> What prevents us to move to device property API directly?
Yes, it looks more reasonable by using devce_property. Howerver,
device_get_child_node_count was used here to find each child node. This
API output the fwnode_handle for each child node directly, but device
property APIs need 'dev' data instead. Actually, the effects of fwnode_*()
and device_*() are the same. So, I used fwnode_*() APIs here.
If there is any other more way to traverse child nodes, let me know.
Thank you.
>
>> - 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);
>
> Are they equivalent?
Yes, they are equivalent.
>
>> @@ -415,7 +415,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>> }
>>
>> #ifdef CONFIG_OF_GPIO
>> - port->gc.of_node = pp->node;
>> + port->gc.of_node = to_of_node(pp->fwnode);
>
> If fwnode is not OF one?
> Perhaps, something like ... = is_of_node() ? to_of_node() : NULL;
>
The way you suggested is more resonable, I will fixed it in next version.
>
>> - node = dev->of_node;
>> - if (!IS_ENABLED(CONFIG_OF_GPIO) || !node)
>> + if (!IS_ENABLED(CONFIG_OF_GPIO) || !(dev->of_node))
>> return ERR_PTR(-ENODEV);
>
> So, since you converted to fwnode, do you still need this check?
>
Although this patch coverted device node to fwnode, only DTs binding was
supported here, and patch2 support ACPI will remove this check.
>>
>> - nports = of_get_child_count(node);
>> + nports = device_get_child_node_count(dev);
>> if (nports == 0)
>> return ERR_PTR(-ENODEV);
>
> ...I think this one fail if it will not found any child.
This one fail? yes, it will return to failure.
I am not very clear here.
>
>> - if (of_property_read_u32(port_np, "reg", &pp->idx) ||
>> + if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) ||
>
> device_property_*() ?
>
>> pp->idx >= DWAPB_MAX_PORTS) {
>> dev_err(dev, "missing/invalid port index for %s\n",
>> - port_np->full_name);
>> + to_of_node(fwnode)->full_name);
>
> If it's not OF?
This is checked above, and patch2 will remove it.
>
>> - if (of_property_read_u32(port_np, "snps,nr-gpios",
>> + if (fwnode_property_read_u32(fwnode, "snps,nr-gpios",
>
> Ditto.
>
>> &pp->ngpio)) {
>> dev_info(dev, "failed to get number of gpios for
>> %s\n",
>> -port_np->full_name);
>> +to_of_node(fwnode)->full_name);
>
> Ditto.
>
>> if (pp->idx == 0 &&
>> - of_property_read_bool(port_np, "interrupt-controller")) {
>> - pp->irq = irq_of_parse_and_map(port_np, 0);
>> + of_property_read_bool(to_of_node(fwnode),
>> + "interrupt-controller")) {
>
> device_property_*() ?
>
>> + 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;
>
>
Re: [RFC PATCH] GPIO/ACPI: DesignWare: Add GPIO-signaled ACPI events support for power button
在 2016/2/16 17:24, Mika Westerberg 写道:
> On Fri, Feb 05, 2016 at 02:25:42PM +0800, qiujiang wrote:
>> This patch modifies the DesignWare GPIO controller driver to
>> support the GPIO-signaled ACPI Events. This is used for power
>> button function on ARM server.
>>
>> To make it work, the _AEI and _EVT object must be defined in
>> the corresponding GPIO driver's dsdt table in UEFI. At the same
>> time, ACPI daemon component is also necessary.
>
> In general ACPI parts look ok to me. I would like to see ACPI DSDT table
> entry for this device though.
>
Thanks for review, the DSDT table for this driver is as follow:
Device(GPI0) {
Name(_HID, "HISI0181")
Name(_ADR, 0) // _ADR: Address
Name(_UID, 0)
Name (_CRS, ResourceTemplate () {
Memory32Fixed (ReadWrite, 0x802e, 0x1)
Interrupt (ResourceConsumer, Level, ActiveHigh,
Exclusive,,,) {344}
})
Device(PRTa) {
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"reg",0},
Package () {"snps,nr-gpios",32},
}
})
}
Name (_AEI, ResourceTemplate () {
GpioInt(Edge, ActiveLow, ExclusiveAndWake, PullUp, ,
" \\_SB.GPI0") {8}
})
Method (_E08, 0x0, NotSerialized) {
Notify (\_SB.PWRB, 0x80)
}
}
> There are few minor comments below.
>
>> Signed-off-by: qiujiang
>> ---
>> drivers/gpio/gpio-dwapb.c| 75
>>
>> include/linux/platform_data/gpio-dwapb.h | 2 +-
>> 2 files changed, 49 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
>> index fcd5b0a..bfed2e9 100644
>> --- a/drivers/gpio/gpio-dwapb.c
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -23,6 +23,11 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> +
>> +#include "gpiolib.h"
>> +
>>
>> #define GPIO_SWPORTA_DR 0x00
>> #define GPIO_SWPORTA_DDR0x04
>> @@ -296,14 +301,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio
>> *gpio,
>> struct dwapb_port_property *pp)
>> {
>> struct gpio_chip *gc = &port->bgc.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;
>>
>> @@ -421,7 +426,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>> }
>>
>> #ifdef CONFIG_OF_GPIO
>> -port->bgc.gc.of_node = pp->node;
>> +port->bgc.gc.of_node = to_of_node(pp->fwnode);
>> #endif
>> port->bgc.gc.ngpio = pp->ngpio;
>> port->bgc.gc.base = pp->gpio_base;
>> @@ -440,6 +445,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->bgc.gc));
>> +
>> return err;
>> }
>>
>> @@ -453,19 +462,15 @@ 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 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);
>>
>> @@ -480,21 +485,19 @@ dwapb_gpio_get_pdata_of(struct device *dev)
>> pdata->nports = nports;
>>
>> i = 0;
>> -for_each_child_of_node(node, port_np) {
>> -pp = &pdata->properties[i++];
>> -pp->node = port_np;
>> +device_for_each_child_node(dev, fwnode) {
>> +pp = &pdata->properties[i];
>> +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) {
>> -
Re: [RFC PATCH] GPIO/ACPI: DesignWare: Add GPIO-signaled ACPI events support for power button
在 2016/2/16 2:25, Linus Walleij 写道: > Mika can you help out looking at this patch. Tell me if you need a copy > of the whole patch, I'm not smart with ACPI. > > On Fri, Feb 5, 2016 at 7:25 AM, qiujiang wrote: > >> This patch modifies the DesignWare GPIO controller driver to >> support the GPIO-signaled ACPI Events. This is used for power >> button function on ARM server. >> >> To make it work, the _AEI and _EVT object must be defined in >> the corresponding GPIO driver's dsdt table in UEFI. At the same >> time, ACPI daemon component is also necessary. >> >> Signed-off-by: qiujiang > > (...) >> --- a/drivers/gpio/gpio-dwapb.c >> +++ b/drivers/gpio/gpio-dwapb.c >> @@ -23,6 +23,11 @@ >> #include >> #include >> #include >> +#include >> +#include > > You should only need Here, I don't understand well. I used driver.h instead acpi.h and gpio.h, but it came to compile failed. > >> +#include "gpiolib.h" > > I guess this is for some acpi_gpiochip* functions that ACPI GPIO > drivers need like this: > Yes, this is for function the following acpi_gpiochip* function. These functions were declared in the gpiolib.h but gpiolib-acpi.h. >> + /* Add GPIO-signaled ACPI event support */ >> + if (pp->irq) >> + acpi_gpiochip_request_interrupts(&(port->bgc.gc)); > > Hm, maybe these should be in "gpiolib-acpi.h" or so. > > Overall the patch looks sane to me, but I need some ACPI > person to tell. > > Yours, > Linus Walleij > > . >
Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver
Hi Andy,
Sorry for late relpy because Chinese new year holiday. See my replies below.
Best Regards
Jiang
在 2016/2/5 23:55, Andy Shevchenko 写道:
On Fri, Feb 5, 2016 at 9:11 AM, qiujiang wrote:
This patch added ACPI support for DesignWare SPI mmio driver. It
was based the corresponding DT driver and compatible for this two
way. This patch has been tested on Hisilicon D02 board. It relies
on the GPIO patchset.
My comments below.
As Mark mentioned, I want to ask you how to use this spi-dw-mmio driver for
ACPI binding? Dose it need any other extra patchset?
@@ -84,8 +87,6 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
dws->num_cs = num_cs;
if (pdev->dev.of_node) {
- int i;
-
for (i = 0; i < dws->num_cs; i++) {
int cs_gpio = of_get_named_gpio(pdev->dev.of_node,
"cs-gpios", i);
It seems the driver was never validated with more than one chip select.
Perhaps someone has to switch to use of_spi_register_master() here.
of_spi_register_master() will be executed in the spi_register_master(),
but it just saved the
cs_gpios to the spi_master and not used it any more.
@@ -104,6 +105,18 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
}
}
+ if (ACPI_COMPANION(&pdev->dev)) {
+ for (i = 0; i < dws->num_cs; i++) {
+ snprintf(propname, sizeof(propname), "cs%d", i);
+ gpiod = devm_gpiod_get(&pdev->dev,
+ propname, GPIOD_ASIS);
+ if (IS_ERR(gpiod)) {
+ dev_err(&pdev->dev, "Get gpio desc failed!\n");
+ return PTR_ERR(gpiod);
+ }
+ }
+ }
Like Mark noticed there is also same issue. Do you indeed check the
configuration with different chip select signals?
As a spi master driver, it seems that multi-chip select must be
supported, so this check is
necessary, I think.
Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver
Hi Mark,
Many thanks for your review, I'm so sorry for late reply because The
Chinese
new year holiday. See my replies below.
Best Regards
Jiang
在 2016/2/5 19:09, Mark Brown 写道:
On Fri, Feb 05, 2016 at 03:11:20PM +0800, qiujiang wrote:
This patch added ACPI support for DesignWare SPI mmio driver. It
was based the corresponding DT driver and compatible for this two
way. This patch has been tested on Hisilicon D02 board. It relies
on the GPIO patchset.
Intel are heavy users of this driver on their systems which also use
ACPI. Have you discussed this binding with them? I've copied Andy and
Jarkko who've worked on the driver recently.
I'm going to ask Andy to get some ideas that how to use this spi-dw-mmio
driver by ACPI binding.
Please use subject lines matching the style for the subsystem. This
makes it easier for people to identify relevant patches.
Thanks for the reminder, I will fix it in the next version.
+ char propname[32];
That's a magic number, where did it come from and why is it a magic
nummber?
I'm sorry for here without any comments. This number define is come from
gpiolib.c. It means the max size of gpio property name. The reference code
located in line 1815 of gpiolib.c.
+ if (ACPI_COMPANION(&pdev->dev)) {
+ for (i = 0; i < dws->num_cs; i++) {
+ snprintf(propname, sizeof(propname), "cs%d", i);
+ gpiod = devm_gpiod_get(&pdev->dev,
+ propname, GPIOD_ASIS);
+ if (IS_ERR(gpiod)) {
+ dev_err(&pdev->dev, "Get gpio desc failed!\n");
+ return PTR_ERR(gpiod);
+ }
+ }
+ }
I'm not seeing anywhere where we store the GPIO in this loop. It is
therefore unclear to me how the chip select is going to work?
In DT binding, of_get_named_gpio and devm_gpio_request were used to
parse gpio
pins defined in DTs and then request these pins. Similarly, for ACPI,
devm_gpiod_get
can do that two operation in a single function. It is a unified
interface to ACPI and DT
binding.
If the gpiod is valid, the corresponding gpio pins has been requested.
We do not need
to save this gpiod any more.
which gpio pin was used is defined in spi_device, named cs_gpio, the
configuration to the
gpio pins will be done in the setup callback routine of each device.
What the spi master
should do is just request these pins to the gpio subsystem.
+static const struct acpi_device_id dw_spi_mmio_acpi_match[] = {
+ {"HISI0171", 0},
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, dw_spi_mmio_acpi_match);
I really do wish ACPI had some more sensible system for allocating
device IDs so the tables were a little more legible. :(
This is really a question, I will do this feedback to ACPI maintainers.
