Re: [RFC PATCH] USB: misc: Add usb_hub_pwr driver

[Matthias Kaehlcke]

Hi Peter,

sorry for the delayed reply, I got distracted by other things and ran into
issues with my mail setup.

On Thu, Sep 03, 2020 at 01:46:18AM +0000, Peter Chen wrote:
> On 20-09-02 10:45:36, Matthias Kaehlcke wrote:
> > > 
> > > Hi Matthias,
> > > 
> > > I did similar several years ago [1], but the concept (power sequence)
> > > doesn't be accepted by power maintainer.
> > 
> > Yeah, I saw that, I think I even reviewed or tested some early version
> > of it :)
> > 
> > > Your patch introduce an new way to fix this long-term issue, I have an
> > > idea to fix it more generally.
> > 
> > > - Create a table (say usb_pm_table) for USB device which needs to do
> > > initial power on and power management during suspend suspend/resume based
> > > on VID and PID, example: usb/core/quirks.c
> > > - After hub (both roothub and intermediate hub) device is created, search
> > > the DT node under this hub, and see if the device is in usb_pm_table. If
> > > it is in it, create a platform device, say usb-power-supply, and the
> > > related driver is like your usb_hub_psupply.c, the parent of this device
> > > is controller device.
> > 
> > This part isn't clear to me. How would the DT look like? Would it have a
> > single node per physical hub chip or one node for each 'logical' hub?
> > Similarly, would there be a single plaform device or multiple?
> 
> One power supply platform device for one physical device, and DT only
> describes physical device. HUB driver only probes non-SS HUB port to
> avoid create two power supply device for SS HUB, there should be no
> SS-only HUB.

I agree that there should be only one platform device per physical device.
Probing only the non-SS hub should work to avoid multiple instances, however
it doesn't work for the extended use case, where the hub is powered off
during system suspend, but only when no wakeup capable devices are connected
to any of the 'logical' hubs. For this to work the driver that controls the
regulators, GPIOs, ... needs to have knowledge of all 'logical' hubs.

I just sent v1 of this driver, which reworks things a bit, but for now
there is still one platform device instantiated through the DT, and
one DT entry for every 'logical' hub.

I'm open to keep discussing alternative designs, as long as they can also
cover the use case of conditionally powering the hub off during system
suspend. We can probably continue the discussion on v1, unless it takes
me longer than