On 04/25/2018 11:14 AM, Dmitry Torokhov wrote: > On Wed, Apr 25, 2018 at 10:00:31AM -0500, Rob Herring wrote: >> On Tue, Apr 24, 2018 at 5:58 PM, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: >>> Hi Linus, Rafael, all >>> >>> Our GPIO controller driver: gpio-brcmstb.c has a shutdown callback which >>> gets invoked when the system is brought into poweroff aka S5. So far so >>> good, except that we also wish to use gpio_keys.c as a possible wake-up >>> source, so we may have a number of GPIO pins declared as gpio-keys that >>> allow the system to wake-up from deep slumber. >>> >>> Recently we noticed that we could easily get into a state where >>> gpio-brcmstb.c::brcmstb_gpio_shutdown() gets called first, and then >>> gpio_keys.c::gpio_keys_suspend() gets called later, which is too late to >>> have the enable_irq_wake() call do anything sensible since we have >>> suspend its parent interrupt controller before. This is completely >>> expected unfortunately because these two drivers are both platform >>> device instances with no connection to one another except via Device >>> Tree and the use of the GPIOLIB APIs. >>> >>> First solution is to make sure that gpio-keys nodes are declared in >>> Device Tree *before* the GPIO controller. This works because Device Tree >>> nodes are probed in the order in which they are declared in Device Tree >>> and that directly influences the order in which platform devices are >>> created. Problem with that is that this is easy to miss and it may not >>> work with overlays, kexec reconstructing DT etc. etc. >> >> I'm going to make of_platform_populate randomize the order it creates devices... >> >>> Another possible solution would be have the GPIO controller nodes have >>> the GPIO consumers nodes such as gpio-keys, gpio-leds etc., and that >>> would allow the Linux device driver model to create an appropriate >>> child/parent relationship. This would unfortunately require Device Tree >>> changes everywhere to make that consistent, and it would be a special >>> case, because not all GPIO consumers are eligible as child nodes of >>> their parent GPIO controller, there are plenty of other consumers that >>> are not suitable for being moved under a parent GPIO controller node. >>> This would also mean that we need to "probe" GPIO controller nodes to >>> populate their child nodes (e.g: of_platform_bus_populate). >>> >>> I am thinking a more generic solution might involve some more complex >>> tracking of the provider <-> consumer, but there is room for breakage. >> >> That's what device connections are for. It probably just needs the >> GPIO core to create the links. (but I've not looked into it at all). > > Not all APIs accept device as parameter to easily create links. But I > wonder, for cases like this, if we could not simply move the device to > the end of the dpm list after successful binding it to a driver. The > assumption that when GOPIs or other resources are not ready they'll > return -EPROBE_DEFER and probing would fail. Dmitry, do you see any reason why we are enabling the gpio_keys.c button interrupts for wake-up during suspend/resume only, and not right from the probe() function? button->wakeup is effectively read-only past the probe function, if we moved the logic to enable/disable the interrupts that would greatly simplify things. I am assuming whomever added that functionality must have been worried about spurious wake-up events somehow and wanted to do it as late as possible? Thanks -- Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
