Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752510AbcLGAN3 (ORCPT ); Tue, 6 Dec 2016 19:13:29 -0500 Received: from mail-oi0-f52.google.com ([209.85.218.52]:34244 "EHLO mail-oi0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751518AbcLGAN2 (ORCPT ); Tue, 6 Dec 2016 19:13:28 -0500 MIME-Version: 1.0 In-Reply-To: <4be239ba-b2f1-a951-d558-7469bf835556@synopsys.com> References: <1481011582-7162-1-git-send-email-john.stultz@linaro.org> <1481011582-7162-2-git-send-email-john.stultz@linaro.org> <4be239ba-b2f1-a951-d558-7469bf835556@synopsys.com> From: John Stultz Date: Tue, 6 Dec 2016 16:04:54 -0800 Message-ID: Subject: Re: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver To: John Youn Cc: lkml , Wei Xu , Guodong Xu , Amit Pundir , Rob Herring , Douglas Anderson , Chen Yu , Kishon Vijay Abraham I , Felipe Balbi , Greg Kroah-Hartman , "linux-usb@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6437 Lines: 167 On Tue, Dec 6, 2016 at 3:17 PM, John Youn wrote: > 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. Not at the moment. Apologies for my ignorance, I'm not totally familiar with the usage of the vbus vs id pins, so I'm somewhat following what I was seeing from other drivers. I know with a usb OTG to usb A adapter, you get the vbus signal but not the id signal, but I don't quite see what should be done differently in that case (as it seems to work ok). > 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. So it can be made work w/o this, but we needed other hacks because the usb-gadget disconnect logic never triggered when the cable was unplugged. The controller would jump over to host mode, then when we re-plugged in the usb-gadget cable, it would fail often as we never got a disconnect signal. That's why earlier I was using this hack to force gadget disconnect before the reset was called: https://lkml.org/lkml/2016/10/20/26 >> 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. Yes. I've already split it out, and added bindings documentation in my tree. I included this here to make it easier to see what all was involved w/o having to go through a bunch of patches. >> 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. Yes. Apologies. I noticed this once I sent it and fixed it up in my tree (as well as a few other extra whitespace lines elsewhere). > [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. Will do! Thanks so much for the review! -john