lkml.org 
[lkml]   [2016]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC][PATCH 1/3 v2] usb: dwc2: Add extcon support to dwc2 driver
On Tue, Dec 6, 2016 at 3:17 PM, John Youn <John.Youn@synopsys.com> 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 <xuwei5@hisilicon.com>
>> Cc: Guodong Xu <guodong.xu@linaro.org>
>> Cc: Amit Pundir <amit.pundir@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: John Youn <johnyoun@synopsys.com>
>> Cc: Douglas Anderson <dianders@chromium.org>
>> Cc: Chen Yu <chenyu56@huawei.com>
>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: linux-usb@vger.kernel.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>> 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 <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.

Will do! Thanks so much for the review!
-john

\
 
 \ /
  Last update: 2016-12-07 01:13    [W:0.075 / U:1.564 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site