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

[Jiang Qiu]

å 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>>> Signed-off-by: qiujiang <qiujiang@xxxxxxxxxx>
>> 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.
>
> .
>