| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 6 Dec 2016 16:04:54 -0800
From: John Stultz <john.stultz@...aro.org>
To: John Youn <John.Youn@...opsys.com>
Cc: lkml <linux-kernel@...r.kernel.org>, Wei Xu <xuwei5@...ilicon.com>,
Guodong Xu <guodong.xu@...aro.org>,
Amit Pundir <amit.pundir@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Douglas Anderson <dianders@...omium.org>,
Chen Yu <chenyu56@...wei.com>,
Kishon Vijay Abraham I <kishon@...com>,
Felipe Balbi <felipe.balbi@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [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@...opsys.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@...ilicon.com>
>> Cc: Guodong Xu <guodong.xu@...aro.org>
>> Cc: Amit Pundir <amit.pundir@...aro.org>
>> Cc: Rob Herring <robh+dt@...nel.org>
>> Cc: John Youn <johnyoun@...opsys.com>
>> Cc: Douglas Anderson <dianders@...omium.org>
>> Cc: Chen Yu <chenyu56@...wei.com>
>> Cc: Kishon Vijay Abraham I <kishon@...com>
>> Cc: Felipe Balbi <felipe.balbi@...ux.intel.com>
>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> Cc: linux-usb@...r.kernel.org
>> Signed-off-by: John Stultz <john.stultz@...aro.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
Powered by blists - more mailing lists