On 07/02/2020 08:07, Jack Pham wrote:
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.
Hmm, I don't see anything much in the documentation that flags why we
want or need to toggle this.
/* 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?
It is probably necessary eventually, but, per my reading of the
documents and working with the hardware, I couldn't justify the
additional work.
However if you think this patchset needs the toggle, I can look into
getting the indicator to toggle here too.
We'd need to add some sort of linked list of notifiers to the role
switching logic and toggle them in order.
Similar to what is done in extcon now for the various notifer hooks.
---
bod
