Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753734AbcLFXR3 (ORCPT ); Tue, 6 Dec 2016 18:17:29 -0500 Received: from smtprelay4.synopsys.com ([198.182.47.9]:55678 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751726AbcLFXR1 (ORCPT ); Tue, 6 Dec 2016 18:17:27 -0500 Subject: Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver To: John Stultz , lkml References: <1481011582-7162-1-git-send-email-john.stultz@linaro.org> <1481011582-7162-2-git-send-email-john.stultz@linaro.org> From: John Youn CC: Wei Xu , Guodong Xu , "Amit Pundir" , Rob Herring , John Youn , Douglas Anderson , Chen Yu , Kishon Vijay Abraham I , Felipe Balbi , Greg Kroah-Hartman , "linux-usb@vger.kernel.org" Message-ID: <4be239ba-b2f1-a951-d558-7469bf835556@synopsys.com> Date: Tue, 6 Dec 2016 15:17:24 -0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.0 MIME-Version: 1.0 In-Reply-To: <1481011582-7162-2-git-send-email-john.stultz@linaro.org> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.10.186.99] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4573 Lines: 141 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 > Cc: Guodong Xu > Cc: Amit Pundir > Cc: Rob Herring > Cc: John Youn > Cc: Douglas Anderson > Cc: Chen Yu > Cc: Kishon Vijay Abraham I > Cc: Felipe Balbi > Cc: Greg Kroah-Hartman > Cc: linux-usb@vger.kernel.org > Signed-off-by: John Stultz > --- > 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 > #include > #include > +#include > > #include > > @@ -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