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

[John Stultz <john.stultz@linaro.org>]

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