Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema

[John Stultz]

On Thu, Sep 2, 2021 at 1:03 PM Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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. :)

thanks
-john