Re: [RFC][PATCH 1/3] phy: phy-hi6220-usb: Wire up extconn support to hikey's phy driver

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi,

On Wednesday 23 November 2016 09:16 AM, John Stultz wrote:
> This wires extconn support to hikey's phy driver, and
> connects it to the usb UDC layer via a usb_phy structure.
> 
> Not sure if this is the right way to connect phy -> UDC,
> but I'm lacking a clear example.
> 
> 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>
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  11 +++
>  drivers/phy/Kconfig                       |   2 +
>  drivers/phy/phy-hi6220-usb.c              | 139 ++++++++++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 17839db..171fbb2 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -732,10 +732,21 @@
>  			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>;
> +		};
> +
>  		usb_phy: usbphy {
>  			compatible = "hisilicon,hi6220-usb-phy";
>  			#phy-cells = <0>;
>  			phy-supply = <&fixed_5v_hub>;
> +			extcon = <&usb_vbus>, <&usb_id>;
>  			hisilicon,peripheral-syscon = <&sys_ctrl>;
>  		};
>  
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index fe00f91..76f4f17 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -254,8 +254,10 @@ config PHY_MT65XX_USB3
>  config PHY_HI6220_USB
>  	tristate "hi6220 USB PHY support"
>  	depends on (ARCH_HISI && ARM64) || COMPILE_TEST
> +	depends on EXTCON
>  	select GENERIC_PHY
>  	select MFD_SYSCON
> +	select USB_PHY
>  	help
>  	  Enable this to support the HISILICON HI6220 USB PHY.
>  
> diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
> index b2141cb..89d8475 100644
> --- a/drivers/phy/phy-hi6220-usb.c
> +++ b/drivers/phy/phy-hi6220-usb.c
> @@ -12,7 +12,12 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/phy/phy.h>
> +#include <linux/usb/phy_companion.h>
> +#include <linux/usb/otg.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/phy.h>
>  #include <linux/regmap.h>
> +#include <linux/extcon.h>
>  
>  #define SC_PERIPH_CTRL4			0x00c
>  
> @@ -44,9 +49,21 @@
>  
>  #define EYE_PATTERN_PARA		0x7053348c
>  
> +
> +struct hi6220_usb_cable {
> +	struct notifier_block		nb;
> +	struct extcon_dev		*extcon;
> +	int state;
> +};
> +
>  struct hi6220_priv {
>  	struct regmap *reg;
>  	struct device *dev;
> +	struct usb_phy phy;
> +
> +	struct delayed_work work;
> +	struct hi6220_usb_cable vbus;
> +	struct hi6220_usb_cable id;
>  };
>  
>  static void hi6220_phy_init(struct hi6220_priv *priv)
> @@ -112,23 +129,85 @@ static int hi6220_phy_exit(struct phy *phy)
>  	return hi6220_phy_setup(priv, false);
>  }
>  
> +
>  static struct phy_ops hi6220_phy_ops = {
>  	.init		= hi6220_phy_start,
>  	.exit		= hi6220_phy_exit,
>  	.owner		= THIS_MODULE,
>  };
>  
> +static void hi6220_detect_work(struct work_struct *work)
> +{
> +	struct hi6220_priv *priv =
> +		container_of(to_delayed_work(work), struct hi6220_priv, work);
> +	struct usb_otg *otg = priv->phy.otg;
> +
> +	if (!IS_ERR(priv->vbus.extcon))
> +		priv->vbus.state = extcon_get_cable_state_(priv->vbus.extcon,
> +								 EXTCON_USB);
> +	if (!IS_ERR(priv->id.extcon))
> +		priv->id.state = extcon_get_cable_state_(priv->id.extcon,
> +							 EXTCON_USB_HOST);
> +	if (otg->gadget) {
> +		if (priv->id.state)
> +			usb_gadget_vbus_connect(otg->gadget);
> +		else
> +			usb_gadget_vbus_disconnect(otg->gadget);
> +	}
> +}
> +
> +static int hi6220_otg_vbus_notifier(struct notifier_block *nb,
> +				    unsigned long event, void *ptr)
> +{
> +	struct hi6220_usb_cable *vbus = container_of(nb,
> +						struct hi6220_usb_cable, nb);
> +	struct hi6220_priv *priv = container_of(vbus,
> +						struct hi6220_priv, vbus);
> +
> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
> +	return NOTIFY_DONE;
> +}
> +
> +static int hi6220_otg_id_notifier(struct notifier_block *nb,
> +				  unsigned long event, void *ptr)
> +{
> +	struct hi6220_usb_cable *id = container_of(nb,
> +						struct hi6220_usb_cable, nb);
> +	struct hi6220_priv *priv = container_of(id, struct hi6220_priv, id);
> +
> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
> +	return NOTIFY_DONE;
> +}
> +
> +static int hi6220_otg_set_host(struct usb_otg *otg, struct usb_bus *host)
> +{
> +	otg->host = host;
> +	return 0;
> +}
> +
> +static int hi6220_otg_set_peripheral(struct usb_otg *otg,
> +					struct usb_gadget *gadget)
> +{
> +	otg->gadget = gadget;
> +	return 0;
> +}
> +
>  static int hi6220_phy_probe(struct platform_device *pdev)
>  {
>  	struct phy_provider *phy_provider;
>  	struct device *dev = &pdev->dev;
>  	struct phy *phy;
> +	struct usb_otg *otg;
>  	struct hi6220_priv *priv;
> +	struct extcon_dev *ext_id, *ext_vbus;
> +	int ret;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	INIT_DELAYED_WORK(&priv->work, hi6220_detect_work);
> +
>  	priv->dev = dev;
>  	priv->reg = syscon_regmap_lookup_by_phandle(dev->of_node,
>  					"hisilicon,peripheral-syscon");
> @@ -137,13 +216,73 @@ static int hi6220_phy_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->reg);
>  	}
>  
> +
> +	ext_id = ERR_PTR(-ENODEV);
> +	ext_vbus = ERR_PTR(-ENODEV);
> +	if (of_property_read_bool(dev->of_node, "extcon")) {
> +		/* Each one of them is not mandatory */
> +		ext_vbus = extcon_get_edev_by_phandle(&pdev->dev, 0);
> +		if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
> +			return PTR_ERR(ext_vbus);
> +
> +		ext_id = extcon_get_edev_by_phandle(&pdev->dev, 1);
> +		if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
> +			return PTR_ERR(ext_id);
> +	}
> +
> +	priv->vbus.extcon = ext_vbus;
> +	if (!IS_ERR(ext_vbus)) {
> +		priv->vbus.nb.notifier_call = hi6220_otg_vbus_notifier;
> +		ret = extcon_register_notifier(ext_vbus, EXTCON_USB,
> +						&priv->vbus.nb);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "register VBUS notifier failed\n");
> +			return ret;
> +		}
> +
> +		priv->vbus.state = extcon_get_cable_state_(ext_vbus,
> +								EXTCON_USB);
> +	}
> +
> +	priv->id.extcon = ext_id;
> +	if (!IS_ERR(ext_id)) {
> +		priv->id.nb.notifier_call = hi6220_otg_id_notifier;
> +		ret = extcon_register_notifier(ext_id, EXTCON_USB_HOST,
> +						&priv->id.nb);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "register ID notifier failed\n");
> +			return ret;
> +		}
> +
> +		priv->id.state = extcon_get_cable_state_(ext_id,
> +							 EXTCON_USB_HOST);
> +	}
> +
>  	hi6220_phy_init(priv);
>  
>  	phy = devm_phy_create(dev, NULL, &hi6220_phy_ops);
>  	if (IS_ERR(phy))
>  		return PTR_ERR(phy);
>  
> +	otg = devm_kzalloc(&pdev->dev, sizeof(*otg), GFP_KERNEL);
> +	if (!otg)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;
> +	priv->phy.dev = priv->dev;
> +	priv->phy.label = "hi6220_usb_phy";
> +	priv->phy.otg = otg;
> +	priv->phy.type = USB_PHY_TYPE_USB2;
> +	otg->set_host = hi6220_otg_set_host;
> +	otg->set_peripheral = hi6220_otg_set_peripheral;
> +	otg->usb_phy = &priv->phy;
> +
> +	platform_set_drvdata(pdev, priv);
> +
>  	phy_set_drvdata(phy, priv);
> +
> +	usb_add_phy_dev(&priv->phy);

This would be like using two independent phy infrastructure :-( Should we just
handle the extcon events in USB driver?

Thanks
Kishon