Hi Bryan, On Fri, Feb 07, 2020 at 01:58:58AM +0000, Bryan O'Donoghue wrote: > Using the gpio_usb_connector driver also means that we are not supplying > VBUS via the SoC but by an external PMIC directly. > > This patch searches for a gpio_usb_connector as a child node of the core > DWC3 block and if found switches on the VBUS over-ride, leaving it up to > the role-switching code in gpio-usb-connector to switch off and on VBUS. <snip> > static int dwc3_qcom_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > @@ -557,7 +572,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > struct dwc3_qcom *qcom; > struct resource *res, *parent_res = NULL; > int ret, i; > - bool ignore_pipe_clk; > + bool ignore_pipe_clk, gpio_usb_conn; > > qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); > if (!qcom) > @@ -649,9 +664,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > } > > qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev); > + gpio_usb_conn = dwc3_qcom_find_gpio_usb_connector(qcom->dwc3); > > - /* enable vbus override for device mode */ > - if (qcom->mode == USB_DR_MODE_PERIPHERAL) > + /* enable vbus override for device mode or GPIO USB connector mode */ > + if (qcom->mode == USB_DR_MODE_PERIPHERAL || gpio_usb_conn) > dwc3_qcom_vbus_overrride_enable(qcom, true); This doesn't seem right. It looks like you are doing the vbus_override only once on probe() and keeping it that way regardless of the dynamic state of the connector, i.e. even after VBUS is physically removed and/or ID pin is low. > /* register extcon to override sw_vbus on Vbus change later */ As suggested by this comment, if you look at the extcon handling, it intercepts the VBUS state toggling in dwc3_qcom_vbus_notifier() and calls vbus_override() accordingly. That way it should only be true when the role==USB_ROLE_DEVICE and disabled otherwise (USB_ROLE_HOST/NONE). To me the gpio-b connector + usb-role-switch is attempting to be an alternative to extcon. But to correctly mimic the vbus_override() behavior I think we need a way to intercept when the connector child driver calls usb_role_switch_set_role() to the dwc3 device, but somehow be able to do it from up here in the parent/glue layer. Unfortunately I don't have a good idea of how to do that, short of shoehorning an "upcall" notification from drd.c to the glue, something I don't think Felipe would be a fan of. Could the usb_role_switch class somehow be enhanced to support chaining multiple "consumers" to support this case? Such that when the gpio-b driver calls set_role() it could get handled both by drd.c and dwc3-qcom.c? Jack -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project