| Message ID | 1459926480-32966-4-git-send-email-qiujiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 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 > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 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. > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :) -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 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:). > . > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 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 > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(+)