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
