Messages in this thread | | | Date | Thu, 2 Sep 2021 23:42:18 +0200 | From | Mauro Carvalho Chehab <> | Subject | Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema |
| |
Em Thu, 2 Sep 2021 13:31:45 -0700 John Stultz <john.stultz@linaro.org> escreveu:
> On Thu, Sep 2, 2021 at 1:03 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > On Thu, Sep 02, 2021 at 11:29:49AM -0700, John Stultz wrote: > > > The current iteration (of which there have been many) of hikey hub > > > driver uses the role switch notification as its trigger. It's not > > > really controlling the role switching, just using that signal. That's > > > why the driver works as an intermediary/relay of the roleswitch > > > notification. > > > > Apologies too, my terminology wasn't very clear, I had little exposure to > > OTG so far. > > > > The hisi_hikey_usb driver doesn't control the role of the USB controller, > > however it deals with platform specific role switching stuff, like muxing > > the USB PHY to the hub (host mode) or directly to the type-C port (device > > mode), or controlling the power of the type-C port. > > True, though the exact configuration of powering type-c port, the mux > and the hub power that we want to set depends on the role. > > > > We had earlier efforts that had hacks to specific drivers as well as > > > attempts to add notifiers on role switches (but those were terribly > > > racy), so the intermediary/relay approach has been a great improvement > > > on reliability with little impact to other drivers. > > > > I can see how raciness can be a problem. I'm not proprosing to use > > notifiers in the driver that deals with the hub, from the hub's > > perspective it is connected to a host port and it shouldn't have to care > > about OTG. > > > > But the 'hub driver' could expose a synchronous interface that allows the > > hisi_hikey_usb driver to power the hub on and off (or keep it in reset). > > That would maintain the relay approach, but without having a driver that > > tries to do too many things at once. For example the onboard_usb_hub driver > > has the option to power the hub down during suspend if no wakeup capable > > devices are connected downstream, I'm not convinced that this and the > > handling of the mux should be done by the same driver. > > I'm not sure I'm totally following your proposal, so apologies if I > have it wrong. > It seems you're suggesting to move just the hub power logic out of the > hisi hub driver, and utilize the onboard_usb_hub driver for that? > > I guess that could be done, but I also feel like it's really just the > regulator control, which is a pretty small part of the hisi hub driver > logic. > Additionally, if you look at the logic that handles the different > setting configurations needed for the different roles: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/hisi_hikey_usb.c#n97 > > You'll see the ordering of powering things on/off and switching the > mux is a bit subtle. > > So while I guess we could call some onboard_usb_hub hook for the hub > power on/off calls (hub_power_ctrl() in the hisi driver), I'm not sure > that really is saving much logic wise, and splits the details across > an additional driver in an already somewhat complicated config. > > Again, apologies if I'm being thick headed. :)
I agree with John here: this driver itself is really simple. It has less than 300 lines of code, as it just turns the regulator on/off, has one GPIO for the reset pin of the HUB, and two other GPIOs to turn on/off Type-C power and to switch OTG phy. The actual OTG implementation is done at the DWC3 driver and the Type-C support is provided by the RT1711H driver.
Splitting its code on two separate drivers, being one just to replace the 20-30 lines of code that control the regulator sounds overkill and will require a glue between those drivers that will likely be bigger than that.
So, I guess the best is to keep this as a separate platform-specific driver.
Thanks, Mauro
| |