Re: [PATCH 07/10] hikey960: Support usb functionality of Hikey960

[Heikki Krogerus]

On Tue, Oct 30, 2018 at 10:50:22AM +0800, Chen Yu wrote:
> > I think you have too many things integrated into this one driver. IMO
> > it would at least be better to just let the Type-C port driver take
> > care of VBUS like I mentioned above. I'm also wondering if it would
> > make sense to handle the role switch and the "hub" in their own
> > drivers, but I don't know enough about your platform at this point to
> > say for sure.
> 
> Thanks for your advice! The HiKey 960 development platform is based around the Huawei Kirin 960.
> The Hikey960 Development Board supports three USB host port via a USB hub (U1803 USB5734).
> The Hikey960 Development Board also implements a USB2.0 typeC OTG port.??
> The Dp and Dm of Soc can be switched between the typeC port and the USB hub.
> If there is no cable on the typeC port, then dwc3 core of Soc will be switch to host mode and the
> driver of this patch will switch Dp and Dp to the hub. The driver also power on the hub in the meantime.

Thank you for the explanation. I got the picture now. I realized that
there is some online information for this board:
https://www.96boards.org/documentation/consumer/hikey/hikey960/hardware-docs/hardware-user-manual.md.html#usb-ports

So that mux is actually _not_ switching between the host and device
modes, but instead, it's switching between Type-C and Type-A
connectors (I'm skipping the hub, as it's irrelevant from the PoW of
the mux), so I've misunderstood. And yes, you did say that it is
switching between connectors in the commit message, but I got confused
because you are registers a role switch.

I don't think you should register a role switch from this driver. This
driver is not really representing USB role switch. The mux on this
board is something else. Instead you should register the role switch
from the dwc3 drd code. That is the part that is representing the role
switch here. I actually think that we should do that in any case. The
dwc3 drd code should always register a role switch.

We can solve the problem of getting the role change events in this
driver by adding notification chain to struct usb_role_switch (check
the attached diff). You would then just need to call
usb_role_switch_get() and usb_role_switch_register_notifier(), and
wait for those notifications. The extcon device does not serve any
purpose anymore.

This driver would continue to take care of the hub powering and the
mux, and also the VBUS like before.


br,

-- 
heikki
diff --git a/drivers/usb/common/roles.c b/drivers/usb/common/roles.c
index bb52e006d203..2881777c16e5 100644
--- a/drivers/usb/common/roles.c
+++ b/drivers/usb/common/roles.c
@@ -20,6 +20,7 @@ struct usb_role_switch {
 	struct device dev;
 	struct mutex lock; /* device lock*/
 	enum usb_role role;
+	struct blocking_notifier_head nh;
 
 	/* From descriptor */
 	struct device *usb2_port;
@@ -49,8 +50,10 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
 	mutex_lock(&sw->lock);
 
 	ret = sw->set(sw->dev.parent, role);
-	if (!ret)
+	if (!ret) {
 		sw->role = role;
+		blocking_notifier_call_chain(&sw->nh, role, NULL);
+	}
 
 	mutex_unlock(&sw->lock);
 
@@ -110,6 +113,20 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
 	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
 
+int usb_role_switch_register_notifier(struct usb_role_switch *sw,
+				      struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&sw->nh, nb);
+}
+EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier);
+
+int usb_role_switch_unregister_notifier(struct usb_role_switch *sw,
+					struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&sw->nh, nb);
+}
+EXPORT_SYMBOL_GPL(usb_role_switch_unregister_notifier);
+
 /**
  * usb_role_switch_get - Find USB role switch linked with the caller
  * @dev: The caller device
@@ -267,6 +284,7 @@ usb_role_switch_register(struct device *parent,
 		return ERR_PTR(-ENOMEM);
 
 	mutex_init(&sw->lock);
+	BLOCKING_INIT_NOTIFIER_HEAD(&sw->nh);
 
 	sw->allow_userspace_control = desc->allow_userspace_control;
 	sw->usb2_port = desc->usb2_port;