Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1372235 > unrolled thread

[PATCH v7 0/3] gpio: dwapb: add gpio-signaled acpi event support for power button

Back to article view | Back to linux.kernel

Contents

  [PATCH v7 0/3] gpio: dwapb: add gpio-signaled acpi event support for power button qiujiang <qiujiang@huawei.com> - 2016-04-06 09:00 +0200
    [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support qiujiang <qiujiang@huawei.com> - 2016-04-06 09:00 +0200
      Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support Linus Walleij <linus.walleij@linaro.org> - 2016-04-08 10:30 +0200
        Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-04-08 10:40 +0200
          Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support Jiang Qiu <qiujiang@huawei.com> - 2016-04-11 14:50 +0200
            Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-04-12 08:50 +0200
              Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support Jiang Qiu <qiujiang@huawei.com> - 2016-04-12 09:00 +0200
              Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support Linus Walleij <linus.walleij@linaro.org> - 2016-04-15 09:50 +0200
                Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support Jiang Qiu <qiujiang@huawei.com> - 2016-04-15 10:00 +0200
          Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support Linus Walleij <linus.walleij@linaro.org> - 2016-04-11 15:10 +0200
        Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support Jiang Qiu <qiujiang@huawei.com> - 2016-04-11 14:40 +0200
    [PATCH v7 1/3] gpio: dwapb: remove name from dwapb_port_property qiujiang <qiujiang@huawei.com> - 2016-04-06 09:00 +0200
      Re: [PATCH v7 1/3] gpio: dwapb: remove name from dwapb_port_property Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-04-06 15:00 +0200
        Re: [PATCH v7 1/3] gpio: dwapb: remove name from dwapb_port_property Jiang Qiu <qiujiang@huawei.com> - 2016-04-07 13:50 +0200
    Re: [PATCH v7 0/3] gpio: dwapb: add gpio-signaled acpi event support  for power button Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-04-06 15:10 +0200

#1372235 — [PATCH v7 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 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

qiujiang (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                | 76 ++++++++++++++++++--------------
 drivers/mfd/intel_quark_i2c_gpio.c       |  3 +-
 include/linux/platform_data/gpio-dwapb.h |  3 +-
 3 files changed, 46 insertions(+), 36 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1372236 — [PATCH v7 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, 0x802e0000, 0x10000)
		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 <mika.westerberg@linux.intel.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: qiujiang <qiujiang@huawei.com>
---
 drivers/gpio/gpio-dwapb.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 3c4d8e6..1cd8c20 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -7,6 +7,7 @@
  *
  * All enquiries to support@picochip.com
  */
+#include <linux/acpi.h>
 #include <linux/gpio/driver.h>
 /* FIXME: for gpio_get_value(), replace this with direct register read */
 #include <linux/gpio.h>
@@ -27,6 +28,8 @@
 #include <linux/platform_data/gpio-dwapb.h>
 #include <linux/slab.h>
 
+#include "gpiolib.h"
+
 #define GPIO_SWPORTA_DR		0x00
 #define GPIO_SWPORTA_DDR	0x04
 #define GPIO_SWPORTB_DR		0x0c
@@ -434,6 +437,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;
 }
 
@@ -501,6 +508,9 @@ dwapb_gpio_get_pdata(struct device *dev)
 				dev_warn(dev, "no irq for this bank\n");
 		}
 
+		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;
 	}
@@ -575,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)
 {
@@ -669,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,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1374075 — Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support

On Wed, Apr 6, 2016 at 9:08 AM, qiujiang <qiujiang@huawei.com> 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, 0x802e0000, 0x10000)
>                 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 <mika.westerberg@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: qiujiang <qiujiang@huawei.com>

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?

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

[toc] | [prev] | [next] | [standalone]

#1374080 — Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support

On Fri, Apr 08, 2016 at 10:26:28AM +0200, Linus Walleij wrote:
> On Wed, Apr 6, 2016 at 9:08 AM, qiujiang <qiujiang@huawei.com> 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, 0x802e0000, 0x10000)
> >                 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 <mika.westerberg@linux.intel.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: qiujiang <qiujiang@huawei.com>
> 
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1375793 — 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 <qiujiang@huawei.com> 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, 0x802e0000, 0x10000)
>>>                 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 <mika.westerberg@linux.intel.com>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Signed-off-by: qiujiang <qiujiang@huawei.com>
>> 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.
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1376508 — Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support

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

[toc] | [prev] | [next] | [standalone]

#1376514 — 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:).
> .
>

[toc] | [prev] | [next] | [standalone]

#1379518 — Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support

On Tue, Apr 12, 2016 at 8:46 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> 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.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1379521 — 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
> <mika.westerberg@linux.intel.com> 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
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1375823 — Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support

On Fri, Apr 8, 2016 at 10:38 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

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

Aha OK I see. Thanks for explaining!

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1375783 — 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 <qiujiang@huawei.com> 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, 0x802e0000, 0x10000)
>>                 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 <mika.westerberg@linux.intel.com>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: qiujiang <qiujiang@huawei.com>
> 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
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1372237 — [PATCH v7 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 those info
from dwapb_gpio dev and pp->idx property.

Signed-off-by: qiujiang <qiujiang@huawei.com>
---
 drivers/gpio/gpio-dwapb.c                | 22 +++++++++-------------
 drivers/mfd/intel_quark_i2c_gpio.c       |  1 -
 include/linux/platform_data/gpio-dwapb.h |  1 -
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 597de1e..91afec8 100644
--- 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");
 		return err;
 	}
 
@@ -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");
 	else
 		port->is_registered = true;
 
@@ -480,15 +478,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 +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->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 int	idx;
 	unsigned int	ngpio;
 	unsigned int	gpio_base;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1372501 — Re: [PATCH v7 1/3] gpio: dwapb: remove name from dwapb_port_property

On Wed, Apr 6, 2016 at 10:07 AM, qiujiang <qiujiang@huawei.com> 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?

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

> @@ -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);

> --- 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 <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1373305 — 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 <qiujiang@huawei.com> 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 <andy.shevchenko@gmail.com>
> 

[toc] | [prev] | [next] | [standalone]

#1372514 — Re: [PATCH v7 0/3] gpio: dwapb: add gpio-signaled acpi event support for power button

On Wed, Apr 6, 2016 at 10:07 AM, qiujiang <qiujiang@huawei.com> 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"

Thanks for new version. Still few minors, but looks pretty match good!

>
> 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
>
> qiujiang (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                | 76 ++++++++++++++++++--------------
>  drivers/mfd/intel_quark_i2c_gpio.c       |  3 +-
>  include/linux/platform_data/gpio-dwapb.h |  3 +-
>  3 files changed, 46 insertions(+), 36 deletions(-)
>
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web