Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver

[John Youn]

On 12/6/2016 12:06 AM, John Stultz wrote:
> This patch wires up extcon support to the dwc2 driver
> so that devices that use a modern generic phy driver
> and don't use the usb-phy infrastructure can still
> signal connect/disconnect events.
> 
> Cc: Wei Xu <xuwei5@xxxxxxxxxxxxx>
> Cc: Guodong Xu <guodong.xu@xxxxxxxxxx>
> Cc: Amit Pundir <amit.pundir@xxxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: John Youn <johnyoun@xxxxxxxxxxxx>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: Chen Yu <chenyu56@xxxxxxxxxx>
> Cc: Kishon Vijay Abraham I <kishon@xxxxxx>
> Cc: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
> v2:
> * Move extcon logic from generic phy to dwc2 driver, as
>   suggested by Kishon.
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 11 +++++++++
>  drivers/usb/dwc2/core.h                   | 16 ++++++++++++
>  drivers/usb/dwc2/core_intr.c              | 25 +++++++++++++++++++
>  drivers/usb/dwc2/hcd.c                    | 23 +++++++++++++++++
>  drivers/usb/dwc2/platform.c               | 41 +++++++++++++++++++++++++++++++
>  5 files changed, 116 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 17839db..8a86a11 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -732,6 +732,16 @@
>  			regulator-always-on;
>  		};
>  
> +		usb_vbus: usb-vbus {
> +			compatible = "linux,extcon-usb-gpio";
> +			id-gpio = <&gpio2 6 1>;
> +		};
> +
> +		usb_id: usb-id {
> +			compatible = "linux,extcon-usb-gpio";
> +			id-gpio = <&gpio2 5 1>;
> +		};
> +

So you are using extcon-usb-gpio driver to detect both the ID pin and
VBUS status correct? Do you need the VBUS one? It doesn't look like
you are using it.

Also, do you really need this at all? Wasn't your system previously
able to detect the ID pin change correctly via the connection id
status change interrupt? This would only be needed if that were not
the case.

>  		usb_phy: usbphy {
>  			compatible = "hisilicon,hi6220-usb-phy";
>  			#phy-cells = <0>;
> @@ -745,6 +755,7 @@
>  			phys = <&usb_phy>;
>  			phy-names = "usb2-phy";
>  			clocks = <&sys_ctrl HI6220_USBOTG_HCLK>;
> +			extcon = <&usb_vbus>, <&usb_id>;

You should document what should be set for "extcon" in
/Documentation/devicetree/bindings/usb/dwc2.txt. And make that a
separate commit before using the binding.

>  			clock-names = "otg";
>  			dr_mode = "otg";
>  			g-use-dma;
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 2a21a04..4cfce62 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -623,6 +623,13 @@ struct dwc2_hregs_backup {
>  	bool valid;
>  };
>  
> +struct dwc2_extcon {
> +	struct notifier_block	nb;
> +	struct extcon_dev	*extcon;
> +	int			state;
> +};
> +
> +

Don't use multiple blank lines. Please run "checkpatch.pl --strict"
and fix other issues it reports, if possible.

[snip]

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 8e1728b..2e4545f 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -46,6 +46,7 @@
>  #include <linux/phy/phy.h>
>  #include <linux/platform_data/s3c-hsotg.h>
>  #include <linux/reset.h>
> +#include <linux/extcon.h>
>  
>  #include <linux/usb/of.h>
>  
> @@ -543,6 +544,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  	struct dwc2_core_params defparams;
>  	struct dwc2_hsotg *hsotg;
>  	struct resource *res;
> +	struct extcon_dev *ext_id, *ext_vbus;
>  	int retval;
>  
>  	match = of_match_device(dwc2_of_match_table, &dev->dev);
> @@ -620,6 +622,45 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  	if (retval)
>  		goto error;
>  
> +	ext_id = ERR_PTR(-ENODEV);
> +	ext_vbus = ERR_PTR(-ENODEV);
> +	if (of_property_read_bool(dev->dev.of_node, "extcon")) {

You can omit this check since it's done in the api function.

> +		/* Each one of them is not mandatory */
> +		ext_vbus = extcon_get_edev_by_phandle(&dev->dev, 0);
> +		if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
> +			return PTR_ERR(ext_vbus);
> +
> +		ext_id = extcon_get_edev_by_phandle(&dev->dev, 1);
> +		if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
> +			return PTR_ERR(ext_id);
> +	}

Ensure when you initialize hsotg->extcon_vbus/id that they are either
valid and set or NULL so that you don't have to do IS_ERR() all the
time.

Regards,
John