Re: [PATCH v4 09/18] usb: dwc3: qcom: Override VBUS when using gpio_usb_connector

From: Jack Pham <jackp@codeaurora.org>
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
	gregkh@linuxfoundation.org, balbi@kernel.org,
	bjorn.andersson@linaro.org, linux-kernel@vger.kernel.org,
	Andy Gross <agross@kernel.org>, Lee Jones <lee.jones@linaro.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Manu Gautam <mgautam@codeaurora.org>
Subject: Re: [PATCH v4 09/18] usb: dwc3: qcom: Override VBUS when using gpio_usb_connector
Date: Fri, 7 Feb 2020 00:07:29 -0800	[thread overview]
Message-ID: <20200207080729.GA30341@jackp-linux.qualcomm.com> (raw)
In-Reply-To: <20200207015907.242991-10-bryan.odonoghue@linaro.org>

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