Hi Peter, On Tue, Sep 15, 2020 at 07:05:38AM +0000, Peter Chen wrote: > > > > > + hub->cfg.power_off_in_suspend = > > of_property_read_bool(dev->of_node, "power-off-in-suspend"); > > > > + hub->cfg.wakeup_source = of_property_read_bool(dev->of_node, > > > > +"wakeup-source"); > > > > > > Do you really need these two properties? If the device (and its > > > children if existed) has wakeup enabled, you keep power in suspend, > > > otherwise, you could close it, any exceptions? > > > > That would work for my use case, but I'm not sure it's a universally good > > configuration. > > > > I don't have a specific USB device in mind, but you could have a device that > > shouldn't lose it's context during suspend or keep operating autonomously (e.g. > > a sensor with a large buffer collecting samples). Not sure if something like this > > exists in the real though. > > > > I'm not an expert, but it seems there are USB controllers with wakeup support > > which is always enabled. A board with such a controller then couldn't have a > > policy to power down the hub regardless of wakeup capable devices being > > connected. > > > > Whether or not it is a wakeup_source, it could get through its or its children's > /sys/../power/wakeup value, you have already used usb_wakeup_enabled_descendants > to know it. I conceptually agree, but in practice there are some conflicting details: wakeup for the hubs on my system is by default disabled, yet USB wakeup works regardless, so the flag doesn't really provide useful information. I guess we could still use it if there is no better way, but it doesn't seem ideal. Similar for udev->bus->controller, according to sysfs it doesn't even have wakeup support. Please let me know if there is a reliable way to check if wakeup is enabled on the controller of a device. > If the onboard HUB needs to reflect wakeup signal, it should not power off its regulator. > > For another property power-off-in-suspend, I think it is also a user option, > but not a hardware feature. Ok, I think you are suggesting a sysfs attribute instead of a DT property, that sounds good to me.
