[07/10] hikey960: Support usb functionality of Hikey960

[07/10] hikey960: Support usb functionality of Hikey960

[Yu Chen]

This driver handles usb hub power on and typeC port event of HiKey960 board:
1)DP&DM switching between usb hub and typeC port base on typeC port
state
2)Control power of usb hub on Hikey960
3)Control vbus of typeC port
4)Handle typeC port event to switch data role

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Binghui Wang <wangbinghui@hisilicon.com>
Signed-off-by: Yu Chen <chenyu56@huawei.com>
---
 drivers/misc/Kconfig          |   7 +
 drivers/misc/Makefile         |   1 +
 drivers/misc/hisi_hikey_usb.c | 319 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 327 insertions(+)
 create mode 100644 drivers/misc/hisi_hikey_usb.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 3726eacdf65d..8e04fc87b685 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -513,6 +513,13 @@ config MISC_RTSX
 	tristate
 	default MISC_RTSX_PCI || MISC_RTSX_USB
 
+config HISI_HIKEY_USB
+	tristate "USB functionality of HiSilicon Hikey Platform"
+	depends on GPIOLIB
+	default n
+	help
+	  If you say yes here you get support for usb functionality of HiSilicon Hikey Platform.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index af22bbc3d00c..387dd302815c 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -58,3 +58,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)	+= aspeed-lpc-snoop.o
 obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
 obj-$(CONFIG_OCXL)		+= ocxl/
 obj-$(CONFIG_MISC_RTSX)		+= cardreader/
+obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
new file mode 100644
index 000000000000..4965719c99ae
--- /dev/null
+++ b/drivers/misc/hisi_hikey_usb.c
@@ -0,0 +1,319 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * hisi_hikey_usb.c
+ *
+ * Copyright (c) Hisilicon Tech. Co., Ltd. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/extcon-provider.h>
+#include <linux/usb/role.h>
+
+#define DEVICE_DRIVER_NAME "hisi_hikey_usb"
+
+#define HUB_VBUS_POWER_ON 1
+#define HUB_VBUS_POWER_OFF 0
+#define USB_SWITCH_TO_HUB 1
+#define USB_SWITCH_TO_TYPEC 0
+
+#define INVALID_GPIO_VALUE (-1)
+
+struct hisi_hikey_usb {
+	int otg_switch_gpio;
+	int typec_vbus_gpio;
+	int typec_vbus_enable_val;
+	int hub_vbus_gpio;
+
+	struct extcon_dev *edev;
+	struct usb_role_switch *role_sw;
+};
+
+static const unsigned int usb_extcon_cable[] = {
+	EXTCON_USB,
+	EXTCON_USB_HOST,
+	EXTCON_NONE,
+};
+
+static void hub_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb, int value)
+{
+	int gpio = hisi_hikey_usb->hub_vbus_gpio;
+
+	if (gpio_is_valid(gpio))
+		gpio_set_value(gpio, value);
+}
+
+static void usb_switch_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
+		int switch_to)
+{
+	int gpio = hisi_hikey_usb->otg_switch_gpio;
+	const char *switch_to_str = (switch_to == USB_SWITCH_TO_HUB) ?
+		"hub" : "typec";
+
+	if (!gpio_is_valid(gpio)) {
+		pr_err("%s: otg_switch_gpio is err\n", __func__);
+		return;
+	}
+
+	if (gpio_get_value(gpio) == switch_to) {
+		pr_info("%s: already switch to %s\n", __func__, switch_to_str);
+		return;
+	}
+
+	gpio_direction_output(gpio, switch_to);
+	pr_info("%s: switch to %s\n", __func__, switch_to_str);
+}
+
+static void usb_typec_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
+		int value)
+{
+	int gpio = hisi_hikey_usb->typec_vbus_gpio;
+
+	if (!gpio_is_valid(gpio)) {
+		pr_err("%s: typec power gpio is err\n", __func__);
+		return;
+	}
+
+	if (gpio_get_value(gpio) == value) {
+		pr_info("%s: typec power no change\n", __func__);
+		return;
+	}
+
+	gpio_direction_output(gpio, value);
+	pr_info("%s: set typec vbus gpio to %d\n", __func__, value);
+}
+
+static int extcon_hisi_pd_set_role(struct device *dev, enum usb_role role)
+{
+	struct hisi_hikey_usb *hisi_hikey_usb = dev_get_drvdata(dev);
+
+	dev_info(dev, "%s:set usb role to %d\n", __func__, role);
+	switch (role) {
+	case USB_ROLE_NONE:
+		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_HUB);
+		usb_typec_power_ctrl(hisi_hikey_usb,
+				!hisi_hikey_usb->typec_vbus_enable_val);
+		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_ON);
+		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB, false);
+		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB_HOST,
+				true);
+		break;
+	case USB_ROLE_HOST:
+		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
+		usb_typec_power_ctrl(hisi_hikey_usb,
+				hisi_hikey_usb->typec_vbus_enable_val);
+		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB, false);
+		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB_HOST,
+				true);
+		break;
+	case USB_ROLE_DEVICE:
+		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_OFF);
+		usb_typec_power_ctrl(hisi_hikey_usb,
+				hisi_hikey_usb->typec_vbus_enable_val);
+		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
+		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB_HOST,
+				false);
+		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB, true);
+		break;
+	}
+
+	return 0;
+}
+
+static enum usb_role extcon_hisi_pd_get_role(struct device *dev)
+{
+	struct hisi_hikey_usb *hisi_hikey_usb = dev_get_drvdata(dev);
+
+	return usb_role_switch_get_role(hisi_hikey_usb->role_sw);
+}
+
+static const struct usb_role_switch_desc sw_desc = {
+	.set = extcon_hisi_pd_set_role,
+	.get = extcon_hisi_pd_get_role,
+	.allow_userspace_control = true,
+};
+
+static int hisi_hikey_usb_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *root = dev->of_node;
+	struct hisi_hikey_usb *hisi_hikey_usb;
+	int ret;
+
+	hisi_hikey_usb = devm_kzalloc(dev, sizeof(*hisi_hikey_usb), GFP_KERNEL);
+	if (!hisi_hikey_usb)
+		return -ENOMEM;
+
+	dev_set_name(dev, "hisi_hikey_usb");
+
+	hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
+	hisi_hikey_usb->otg_switch_gpio = INVALID_GPIO_VALUE;
+	hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
+
+	hisi_hikey_usb->hub_vbus_gpio = of_get_named_gpio(root,
+			"hub_vdd33_en_gpio", 0);
+	if (!gpio_is_valid(hisi_hikey_usb->hub_vbus_gpio)) {
+		pr_err("%s: hub_vbus_gpio is err\n", __func__);
+		return hisi_hikey_usb->hub_vbus_gpio;
+	}
+
+	ret = gpio_request(hisi_hikey_usb->hub_vbus_gpio, "hub_vbus_int_gpio");
+	if (ret) {
+		pr_err("%s: request hub_vbus_gpio err\n", __func__);
+		hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
+		return ret;
+	}
+
+	ret = gpio_direction_output(hisi_hikey_usb->hub_vbus_gpio,
+			HUB_VBUS_POWER_ON);
+	if (ret) {
+		pr_err("%s: power on hub vbus err\n", __func__);
+		goto free_gpio1;
+	}
+
+	hisi_hikey_usb->typec_vbus_gpio = of_get_named_gpio(root,
+		"typc_vbus_int_gpio,typec-gpios", 0);
+	if (!gpio_is_valid(hisi_hikey_usb->typec_vbus_gpio)) {
+		pr_err("%s: typec_vbus_gpio is err\n", __func__);
+		ret = hisi_hikey_usb->typec_vbus_gpio;
+		goto free_gpio1;
+	}
+
+	ret = gpio_request(hisi_hikey_usb->typec_vbus_gpio,
+			"typc_vbus_int_gpio");
+	if (ret) {
+		pr_err("%s: request typec_vbus_gpio err\n", __func__);
+		hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
+		goto free_gpio1;
+	}
+
+	ret = of_property_read_u32(root, "typc_vbus_enable_val",
+				   &hisi_hikey_usb->typec_vbus_enable_val);
+	if (ret) {
+		pr_err("%s: typc_vbus_enable_val can't get\n", __func__);
+		goto free_gpio2;
+	}
+
+	hisi_hikey_usb->typec_vbus_enable_val =
+		!!hisi_hikey_usb->typec_vbus_enable_val;
+
+	ret = gpio_direction_output(hisi_hikey_usb->typec_vbus_gpio,
+				    hisi_hikey_usb->typec_vbus_enable_val);
+	if (ret) {
+		pr_err("%s: power on typec vbus err", __func__);
+		goto free_gpio2;
+	}
+
+	if (of_device_is_compatible(root, "hisilicon,hikey960_usb")) {
+		hisi_hikey_usb->otg_switch_gpio = of_get_named_gpio(root,
+				"otg_gpio", 0);
+		if (!gpio_is_valid(hisi_hikey_usb->otg_switch_gpio)) {
+			pr_info("%s: otg_switch_gpio is err\n", __func__);
+			goto free_gpio2;
+		}
+
+		ret = gpio_request(hisi_hikey_usb->otg_switch_gpio,
+				"otg_switch_gpio");
+		if (ret) {
+			hisi_hikey_usb->otg_switch_gpio = INVALID_GPIO_VALUE;
+			pr_err("%s: request typec_vbus_gpio err\n", __func__);
+			goto free_gpio2;
+		}
+	}
+
+	hisi_hikey_usb->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable);
+	if (IS_ERR(hisi_hikey_usb->edev)) {
+		dev_err(dev, "failed to allocate extcon device\n");
+		goto free_gpio2;
+	}
+
+	ret = devm_extcon_dev_register(dev, hisi_hikey_usb->edev);
+	if (ret < 0) {
+		dev_err(dev, "failed to register extcon device\n");
+		goto free_gpio2;
+	}
+	extcon_set_state(hisi_hikey_usb->edev, EXTCON_USB_HOST, true);
+
+	hisi_hikey_usb->role_sw = usb_role_switch_register(dev, &sw_desc);
+	if (IS_ERR(hisi_hikey_usb->role_sw))
+		goto free_gpio2;
+
+	platform_set_drvdata(pdev, hisi_hikey_usb);
+
+	return 0;
+
+free_gpio2:
+	if (gpio_is_valid(hisi_hikey_usb->typec_vbus_gpio)) {
+		gpio_free(hisi_hikey_usb->typec_vbus_gpio);
+		hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
+	}
+
+free_gpio1:
+	if (gpio_is_valid(hisi_hikey_usb->hub_vbus_gpio)) {
+		gpio_free(hisi_hikey_usb->hub_vbus_gpio);
+		hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
+	}
+
+	return ret;
+}
+
+static int  hisi_hikey_usb_remove(struct platform_device *pdev)
+{
+	struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
+
+	if (gpio_is_valid(hisi_hikey_usb->otg_switch_gpio)) {
+		gpio_free(hisi_hikey_usb->otg_switch_gpio);
+		hisi_hikey_usb->otg_switch_gpio = INVALID_GPIO_VALUE;
+	}
+
+	if (gpio_is_valid(hisi_hikey_usb->typec_vbus_gpio)) {
+		gpio_free(hisi_hikey_usb->typec_vbus_gpio);
+		hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
+	}
+
+	if (gpio_is_valid(hisi_hikey_usb->hub_vbus_gpio)) {
+		gpio_free(hisi_hikey_usb->hub_vbus_gpio);
+		hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
+	}
+
+	usb_role_switch_unregister(hisi_hikey_usb->role_sw);
+
+	return 0;
+}
+
+static const struct of_device_id id_table_hisi_hikey_usb[] = {
+	{.compatible = "hisilicon,gpio_hubv1"},
+	{.compatible = "hisilicon,hikey960_usb"},
+	{}
+};
+
+static struct platform_driver  hisi_hikey_usb_driver = {
+	.probe = hisi_hikey_usb_probe,
+	.remove = hisi_hikey_usb_remove,
+	.driver = {
+		.name = DEVICE_DRIVER_NAME,
+		.of_match_table = of_match_ptr(id_table_hisi_hikey_usb),
+
+	},
+};
+
+module_platform_driver(hisi_hikey_usb_driver);
+
+MODULE_AUTHOR("Yu Chen <chenyu56@huawei.com>");
+MODULE_DESCRIPTION("Driver Support for USB functionality of Hikey");
+MODULE_LICENSE("GPL v2");

[07/10] hikey960: Support usb functionality of Hikey960

[Heikki Krogerus]

Hi,

On Sat, Oct 27, 2018 at 05:58:17PM +0800, Yu Chen wrote:
> This driver handles usb hub power on and typeC port event of HiKey960 board:
> 1)DP&DM switching between usb hub and typeC port base on typeC port
> state

By "hub" do you mean you have some kind of an integrated USB hub on
your SoC?

> 2)Control power of usb hub on Hikey960
> 3)Control vbus of typeC port
> 4)Handle typeC port event to switch data role

Is your role switch a discrete component, or something part of the SoC?

> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Binghui Wang <wangbinghui@hisilicon.com>
> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> ---
>  drivers/misc/Kconfig          |   7 +
>  drivers/misc/Makefile         |   1 +
>  drivers/misc/hisi_hikey_usb.c | 319 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 327 insertions(+)
>  create mode 100644 drivers/misc/hisi_hikey_usb.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 3726eacdf65d..8e04fc87b685 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -513,6 +513,13 @@ config MISC_RTSX
>  	tristate
>  	default MISC_RTSX_PCI || MISC_RTSX_USB
>  
> +config HISI_HIKEY_USB
> +	tristate "USB functionality of HiSilicon Hikey Platform"
> +	depends on GPIOLIB
> +	default n
> +	help
> +	  If you say yes here you get support for usb functionality of HiSilicon Hikey Platform.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index af22bbc3d00c..387dd302815c 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -58,3 +58,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)	+= aspeed-lpc-snoop.o
>  obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
>  obj-$(CONFIG_OCXL)		+= ocxl/
>  obj-$(CONFIG_MISC_RTSX)		+= cardreader/
> +obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
> diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
> new file mode 100644
> index 000000000000..4965719c99ae
> --- /dev/null
> +++ b/drivers/misc/hisi_hikey_usb.c
> @@ -0,0 +1,319 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * hisi_hikey_usb.c
> + *
> + * Copyright (c) Hisilicon Tech. Co., Ltd. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/extcon-provider.h>
> +#include <linux/usb/role.h>
> +
> +#define DEVICE_DRIVER_NAME "hisi_hikey_usb"
> +
> +#define HUB_VBUS_POWER_ON 1
> +#define HUB_VBUS_POWER_OFF 0
> +#define USB_SWITCH_TO_HUB 1
> +#define USB_SWITCH_TO_TYPEC 0
> +
> +#define INVALID_GPIO_VALUE (-1)
> +
> +struct hisi_hikey_usb {
> +	int otg_switch_gpio;
> +	int typec_vbus_gpio;
> +	int typec_vbus_enable_val;
> +	int hub_vbus_gpio;

I think you should use struct gpio_desc and gpiod_*() API with the
gpios.

> +	struct extcon_dev *edev;
> +	struct usb_role_switch *role_sw;
> +};
> +
> +static const unsigned int usb_extcon_cable[] = {
> +	EXTCON_USB,
> +	EXTCON_USB_HOST,
> +	EXTCON_NONE,
> +};
> +
> +static void hub_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb, int value)
> +{
> +	int gpio = hisi_hikey_usb->hub_vbus_gpio;
> +
> +	if (gpio_is_valid(gpio))
> +		gpio_set_value(gpio, value);
> +}
> +
> +static void usb_switch_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
> +		int switch_to)
> +{
> +	int gpio = hisi_hikey_usb->otg_switch_gpio;
> +	const char *switch_to_str = (switch_to == USB_SWITCH_TO_HUB) ?
> +		"hub" : "typec";
> +
> +	if (!gpio_is_valid(gpio)) {
> +		pr_err("%s: otg_switch_gpio is err\n", __func__);
> +		return;
> +	}
> +
> +	if (gpio_get_value(gpio) == switch_to) {
> +		pr_info("%s: already switch to %s\n", __func__, switch_to_str);

That kind of prints are really just noise.

> +		return;
> +	}
> +
> +	gpio_direction_output(gpio, switch_to);
> +	pr_info("%s: switch to %s\n", __func__, switch_to_str);

That is also just noise.

> +}
> +
> +static void usb_typec_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
> +		int value)
> +{
> +	int gpio = hisi_hikey_usb->typec_vbus_gpio;
> +
> +	if (!gpio_is_valid(gpio)) {
> +		pr_err("%s: typec power gpio is err\n", __func__);
> +		return;
> +	}
> +
> +	if (gpio_get_value(gpio) == value) {
> +		pr_info("%s: typec power no change\n", __func__);

Ditto.

> +		return;
> +	}
> +
> +	gpio_direction_output(gpio, value);
> +	pr_info("%s: set typec vbus gpio to %d\n", __func__, value);

Ditto.

> +}
> +
> +static int extcon_hisi_pd_set_role(struct device *dev, enum usb_role role)
> +{
> +	struct hisi_hikey_usb *hisi_hikey_usb = dev_get_drvdata(dev);
> +
> +	dev_info(dev, "%s:set usb role to %d\n", __func__, role);
> +	switch (role) {
> +	case USB_ROLE_NONE:
> +		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_HUB);
> +		usb_typec_power_ctrl(hisi_hikey_usb,
> +				!hisi_hikey_usb->typec_vbus_enable_val);
> +		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_ON);
> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB, false);
> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB_HOST,
> +				true);
> +		break;
> +	case USB_ROLE_HOST:
> +		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
> +		usb_typec_power_ctrl(hisi_hikey_usb,
> +				hisi_hikey_usb->typec_vbus_enable_val);
> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB, false);
> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB_HOST,
> +				true);
> +		break;
> +	case USB_ROLE_DEVICE:
> +		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_OFF);
> +		usb_typec_power_ctrl(hisi_hikey_usb,
> +				hisi_hikey_usb->typec_vbus_enable_val);
> +		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB_HOST,
> +				false);
> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB, true);
> +		break;
> +	}

If I understood the above correctly, you are controlling the VBUS
based on the data role, right?

With USB Type-C connectors the power and data roles are separate, so
you should not be doing that.

But since you are controlling the VBUS with a single GPIO, wouldn't it
be easier to just give the Type-C port controller device that GPIO
resources and let the Type-C drivers take care of it? You could add a
"set_vbus" callback to struct tcpci_data and take care of the GPIO in
tcpci_rt1711h.c, or alternatively, just handle it in tcpci.c.

> +	return 0;
> +}
> +
> +static enum usb_role extcon_hisi_pd_get_role(struct device *dev)
> +{
> +	struct hisi_hikey_usb *hisi_hikey_usb = dev_get_drvdata(dev);
> +
> +	return usb_role_switch_get_role(hisi_hikey_usb->role_sw);
> +}
> +
> +static const struct usb_role_switch_desc sw_desc = {
> +	.set = extcon_hisi_pd_set_role,
> +	.get = extcon_hisi_pd_get_role,
> +	.allow_userspace_control = true,
> +};
> +
> +static int hisi_hikey_usb_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *root = dev->of_node;
> +	struct hisi_hikey_usb *hisi_hikey_usb;
> +	int ret;
> +
> +	hisi_hikey_usb = devm_kzalloc(dev, sizeof(*hisi_hikey_usb), GFP_KERNEL);
> +	if (!hisi_hikey_usb)
> +		return -ENOMEM;
> +
> +	dev_set_name(dev, "hisi_hikey_usb");
> +
> +	hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
> +	hisi_hikey_usb->otg_switch_gpio = INVALID_GPIO_VALUE;
> +	hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
> +
> +	hisi_hikey_usb->hub_vbus_gpio = of_get_named_gpio(root,
> +			"hub_vdd33_en_gpio", 0);
> +	if (!gpio_is_valid(hisi_hikey_usb->hub_vbus_gpio)) {
> +		pr_err("%s: hub_vbus_gpio is err\n", __func__);
> +		return hisi_hikey_usb->hub_vbus_gpio;
> +	}
> +
> +	ret = gpio_request(hisi_hikey_usb->hub_vbus_gpio, "hub_vbus_int_gpio");
> +	if (ret) {
> +		pr_err("%s: request hub_vbus_gpio err\n", __func__);
> +		hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
> +		return ret;
> +	}
> +
> +	ret = gpio_direction_output(hisi_hikey_usb->hub_vbus_gpio,
> +			HUB_VBUS_POWER_ON);
> +	if (ret) {
> +		pr_err("%s: power on hub vbus err\n", __func__);
> +		goto free_gpio1;
> +	}
> +
> +	hisi_hikey_usb->typec_vbus_gpio = of_get_named_gpio(root,
> +		"typc_vbus_int_gpio,typec-gpios", 0);
> +	if (!gpio_is_valid(hisi_hikey_usb->typec_vbus_gpio)) {
> +		pr_err("%s: typec_vbus_gpio is err\n", __func__);
> +		ret = hisi_hikey_usb->typec_vbus_gpio;
> +		goto free_gpio1;
> +	}
> +
> +	ret = gpio_request(hisi_hikey_usb->typec_vbus_gpio,
> +			"typc_vbus_int_gpio");
> +	if (ret) {
> +		pr_err("%s: request typec_vbus_gpio err\n", __func__);
> +		hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
> +		goto free_gpio1;
> +	}
> +
> +	ret = of_property_read_u32(root, "typc_vbus_enable_val",
> +				   &hisi_hikey_usb->typec_vbus_enable_val);
> +	if (ret) {
> +		pr_err("%s: typc_vbus_enable_val can't get\n", __func__);
> +		goto free_gpio2;
> +	}
> +
> +	hisi_hikey_usb->typec_vbus_enable_val =
> +		!!hisi_hikey_usb->typec_vbus_enable_val;
> +
> +	ret = gpio_direction_output(hisi_hikey_usb->typec_vbus_gpio,
> +				    hisi_hikey_usb->typec_vbus_enable_val);
> +	if (ret) {
> +		pr_err("%s: power on typec vbus err", __func__);
> +		goto free_gpio2;
> +	}
> +
> +	if (of_device_is_compatible(root, "hisilicon,hikey960_usb")) {

Instead of that kind of checks, isn't it enough to just use optional
gpios?

> +		hisi_hikey_usb->otg_switch_gpio = of_get_named_gpio(root,
> +				"otg_gpio", 0);
> +		if (!gpio_is_valid(hisi_hikey_usb->otg_switch_gpio)) {
> +			pr_info("%s: otg_switch_gpio is err\n", __func__);
> +			goto free_gpio2;
> +		}
> +
> +		ret = gpio_request(hisi_hikey_usb->otg_switch_gpio,
> +				"otg_switch_gpio");
> +		if (ret) {
> +			hisi_hikey_usb->otg_switch_gpio = INVALID_GPIO_VALUE;
> +			pr_err("%s: request typec_vbus_gpio err\n", __func__);
> +			goto free_gpio2;
> +		}
> +	}
> +
> +	hisi_hikey_usb->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable);
> +	if (IS_ERR(hisi_hikey_usb->edev)) {
> +		dev_err(dev, "failed to allocate extcon device\n");
> +		goto free_gpio2;
> +	}
> +
> +	ret = devm_extcon_dev_register(dev, hisi_hikey_usb->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register extcon device\n");
> +		goto free_gpio2;
> +	}
> +	extcon_set_state(hisi_hikey_usb->edev, EXTCON_USB_HOST, true);

Is the primary purpose for this extcon device to satisfy the DRD code
in dwc3 driver?

> +	hisi_hikey_usb->role_sw = usb_role_switch_register(dev, &sw_desc);
> +	if (IS_ERR(hisi_hikey_usb->role_sw))
> +		goto free_gpio2;

It looks a bit clumsy to me to register both the extcon device and the
mux device, but I'm guessing you need to get a notification in dwc3
driver when the role changes, right? Perhaps we should simply add
notification chain to the role mux structure. That could potentially
allow this kind of code to be organized a bit better.

> +	platform_set_drvdata(pdev, hisi_hikey_usb);
> +
> +	return 0;
> +
> +free_gpio2:
> +	if (gpio_is_valid(hisi_hikey_usb->typec_vbus_gpio)) {
> +		gpio_free(hisi_hikey_usb->typec_vbus_gpio);
> +		hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
> +	}
> +
> +free_gpio1:
> +	if (gpio_is_valid(hisi_hikey_usb->hub_vbus_gpio)) {
> +		gpio_free(hisi_hikey_usb->hub_vbus_gpio);
> +		hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
> +	}
> +
> +	return ret;
> +}
> +
> +static int  hisi_hikey_usb_remove(struct platform_device *pdev)
> +{
> +	struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
> +
> +	if (gpio_is_valid(hisi_hikey_usb->otg_switch_gpio)) {
> +		gpio_free(hisi_hikey_usb->otg_switch_gpio);
> +		hisi_hikey_usb->otg_switch_gpio = INVALID_GPIO_VALUE;
> +	}
> +
> +	if (gpio_is_valid(hisi_hikey_usb->typec_vbus_gpio)) {
> +		gpio_free(hisi_hikey_usb->typec_vbus_gpio);
> +		hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
> +	}
> +
> +	if (gpio_is_valid(hisi_hikey_usb->hub_vbus_gpio)) {
> +		gpio_free(hisi_hikey_usb->hub_vbus_gpio);
> +		hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
> +	}
> +
> +	usb_role_switch_unregister(hisi_hikey_usb->role_sw);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id id_table_hisi_hikey_usb[] = {
> +	{.compatible = "hisilicon,gpio_hubv1"},
> +	{.compatible = "hisilicon,hikey960_usb"},
> +	{}
> +};
> +
> +static struct platform_driver  hisi_hikey_usb_driver = {
> +	.probe = hisi_hikey_usb_probe,
> +	.remove = hisi_hikey_usb_remove,
> +	.driver = {
> +		.name = DEVICE_DRIVER_NAME,
> +		.of_match_table = of_match_ptr(id_table_hisi_hikey_usb),
> +
> +	},
> +};
> +
> +module_platform_driver(hisi_hikey_usb_driver);
> +
> +MODULE_AUTHOR("Yu Chen <chenyu56@huawei.com>");
> +MODULE_DESCRIPTION("Driver Support for USB functionality of Hikey");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.15.0-rc2

I think you have too many things integrated into this one driver. IMO
it would at least be better to just let the Type-C port driver take
care of VBUS like I mentioned above. I'm also wondering if it would
make sense to handle the role switch and the "hub" in their own
drivers, but I don't know enough about your platform at this point to
say for sure.


br,

[07/10] hikey960: Support usb functionality of Hikey960

[Yu Chen]

Hi


On 2018/10/29 22:30, Heikki Krogerus wrote:
> Hi,
>
> On Sat, Oct 27, 2018 at 05:58:17PM +0800, Yu Chen wrote:
>> This driver handles usb hub power on and typeC port event of HiKey960 board:
>> 1)DP&DM switching between usb hub and typeC port base on typeC port
>> state
> By "hub" do you mean you have some kind of an integrated USB hub on
> your SoC?
No. The "hub" is on the board of HiKey960 development platform.

>> 2)Control power of usb hub on Hikey960
>> 3)Control vbus of typeC port
>> 4)Handle typeC port event to switch data role
> Is your role switch a discrete component, or something part of the SoC?
Yes, the dwc3 usb controller of the SoC needs switch between device and host
mode based on the typeC port event.

>
>> +#define USB_SWITCH_TO_HUB 1
>> +#define USB_SWITCH_TO_TYPEC 0
>> +
>> +#define INVALID_GPIO_VALUE (-1)
>> +
>> +struct hisi_hikey_usb {
>> +	int otg_switch_gpio;
>> +	int typec_vbus_gpio;
>> +	int typec_vbus_enable_val;
>> +	int hub_vbus_gpio;
> I think you should use struct gpio_desc and gpiod_*() API with the
> gpios.

You are right, I will fix that. Thanks!
>> +	struct extcon_dev *edev;
>> +	struct usb_role_switch *role_sw;
>> +};
>> +
>> +static const unsigned int usb_extcon_cable[] = {
>> +	EXTCON_USB,
>> +	EXTCON_USB_HOST,
>> +	EXTCON_NONE,
>> +};
>> +
>> +static void hub_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb, int value)
>> +{
>> +	int gpio = hisi_hikey_usb->hub_vbus_gpio;
>> +
>> +	if (gpio_is_valid(gpio))
>> +		gpio_set_value(gpio, value);
>> +}
>> +
>> +static void usb_switch_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
>> +		int switch_to)
>> +{
>> +	int gpio = hisi_hikey_usb->otg_switch_gpio;
>> +	const char *switch_to_str = (switch_to == USB_SWITCH_TO_HUB) ?
>> +		"hub" : "typec";
>> +
>> +	if (!gpio_is_valid(gpio)) {
>> +		pr_err("%s: otg_switch_gpio is err\n", __func__);
>> +		return;
>> +	}
>> +
>> +	if (gpio_get_value(gpio) == switch_to) {
>> +		pr_info("%s: already switch to %s\n", __func__, switch_to_str);
> That kind of prints are really just noise.
>
>> +		return;
>> +	}
>> +
>> +	gpio_direction_output(gpio, switch_to);
>> +	pr_info("%s: switch to %s\n", __func__, switch_to_str);
> That is also just noise.
>
>> +}
>> +
>> +static void usb_typec_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
>> +		int value)
>> +{
>> +	int gpio = hisi_hikey_usb->typec_vbus_gpio;
>> +
>> +	if (!gpio_is_valid(gpio)) {
>> +		pr_err("%s: typec power gpio is err\n", __func__);
>> +		return;
>> +	}
>> +
>> +	if (gpio_get_value(gpio) == value) {
>> +		pr_info("%s: typec power no change\n", __func__);
> Ditto.
>
>> +		return;
>> +	}
>> +
>> +	gpio_direction_output(gpio, value);
>> +	pr_info("%s: set typec vbus gpio to %d\n", __func__, value);
> Ditto.
>
>> +}
>> +
>> +static int extcon_hisi_pd_set_role(struct device *dev, enum usb_role role)
>> +{
>> +	struct hisi_hikey_usb *hisi_hikey_usb = dev_get_drvdata(dev);
>> +
>> +	dev_info(dev, "%s:set usb role to %d\n", __func__, role);
>> +	switch (role) {
>> +	case USB_ROLE_NONE:
>> +		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_HUB);
>> +		usb_typec_power_ctrl(hisi_hikey_usb,
>> +				!hisi_hikey_usb->typec_vbus_enable_val);
>> +		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_ON);
>> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB, false);
>> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB_HOST,
>> +				true);
>> +		break;
>> +	case USB_ROLE_HOST:
>> +		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
>> +		usb_typec_power_ctrl(hisi_hikey_usb,
>> +				hisi_hikey_usb->typec_vbus_enable_val);
>> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB, false);
>> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB_HOST,
>> +				true);
>> +		break;
>> +	case USB_ROLE_DEVICE:
>> +		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_OFF);
>> +		usb_typec_power_ctrl(hisi_hikey_usb,
>> +				hisi_hikey_usb->typec_vbus_enable_val);
>> +		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
>> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB_HOST,
>> +				false);
>> +		extcon_set_state_sync(hisi_hikey_usb->edev, EXTCON_USB, true);
>> +		break;
>> +	}
> If I understood the above correctly, you are controlling the VBUS
> based on the data role, right?
Yes, you are right.
> With USB Type-C connectors the power and data roles are separate, so
> you should not be doing that.
>
> But since you are controlling the VBUS with a single GPIO, wouldn't it
> be easier to just give the Type-C port controller device that GPIO
> resources and let the Type-C drivers take care of it? You could add a
> "set_vbus" callback to struct tcpci_data and take care of the GPIO in
> tcpci_rt1711h.c, or alternatively, just handle it in tcpci.c.
I will try to implement the "set_vbus" callback.

>> +	return 0;
>> +}
>> +
>> +static enum usb_role extcon_hisi_pd_get_role(struct device *dev)
>> +{
>> +	struct hisi_hikey_usb *hisi_hikey_usb = dev_get_drvdata(dev);
>> +
>> +	return usb_role_switch_get_role(hisi_hikey_usb->role_sw);
>> +}
>> +
>> +static const struct usb_role_switch_desc sw_desc = {
>> +	.set = extcon_hisi_pd_set_role,
>> +	.get = extcon_hisi_pd_get_role,
>> +	.allow_userspace_control = true,
>> +};
>> +
>> +static int hisi_hikey_usb_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *root = dev->of_node;
>> +	struct hisi_hikey_usb *hisi_hikey_usb;
>> +	int ret;
>> +
>> +	hisi_hikey_usb = devm_kzalloc(dev, sizeof(*hisi_hikey_usb), GFP_KERNEL);
>> +	if (!hisi_hikey_usb)
>> +		return -ENOMEM;
>> +
>> +	dev_set_name(dev, "hisi_hikey_usb");
>> +
>> +	hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
>> +	hisi_hikey_usb->otg_switch_gpio = INVALID_GPIO_VALUE;
>> +	hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
>> +
>> +	hisi_hikey_usb->hub_vbus_gpio = of_get_named_gpio(root,
>> +			"hub_vdd33_en_gpio", 0);
>> +	if (!gpio_is_valid(hisi_hikey_usb->hub_vbus_gpio)) {
>> +		pr_err("%s: hub_vbus_gpio is err\n", __func__);
>> +		return hisi_hikey_usb->hub_vbus_gpio;
>> +	}
>> +
>> +	ret = gpio_request(hisi_hikey_usb->hub_vbus_gpio, "hub_vbus_int_gpio");
>> +	if (ret) {
>> +		pr_err("%s: request hub_vbus_gpio err\n", __func__);
>> +		hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
>> +		return ret;
>> +	}
>> +
>> +	ret = gpio_direction_output(hisi_hikey_usb->hub_vbus_gpio,
>> +			HUB_VBUS_POWER_ON);
>> +	if (ret) {
>> +		pr_err("%s: power on hub vbus err\n", __func__);
>> +		goto free_gpio1;
>> +	}
>> +
>> +	hisi_hikey_usb->typec_vbus_gpio = of_get_named_gpio(root,
>> +		"typc_vbus_int_gpio,typec-gpios", 0);
>> +	if (!gpio_is_valid(hisi_hikey_usb->typec_vbus_gpio)) {
>> +		pr_err("%s: typec_vbus_gpio is err\n", __func__);
>> +		ret = hisi_hikey_usb->typec_vbus_gpio;
>> +		goto free_gpio1;
>> +	}
>> +
>> +	ret = gpio_request(hisi_hikey_usb->typec_vbus_gpio,
>> +			"typc_vbus_int_gpio");
>> +	if (ret) {
>> +		pr_err("%s: request typec_vbus_gpio err\n", __func__);
>> +		hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
>> +		goto free_gpio1;
>> +	}
>> +
>> +	ret = of_property_read_u32(root, "typc_vbus_enable_val",
>> +				   &hisi_hikey_usb->typec_vbus_enable_val);
>> +	if (ret) {
>> +		pr_err("%s: typc_vbus_enable_val can't get\n", __func__);
>> +		goto free_gpio2;
>> +	}
>> +
>> +	hisi_hikey_usb->typec_vbus_enable_val =
>> +		!!hisi_hikey_usb->typec_vbus_enable_val;
>> +
>> +	ret = gpio_direction_output(hisi_hikey_usb->typec_vbus_gpio,
>> +				    hisi_hikey_usb->typec_vbus_enable_val);
>> +	if (ret) {
>> +		pr_err("%s: power on typec vbus err", __func__);
>> +		goto free_gpio2;
>> +	}
>> +
>> +	if (of_device_is_compatible(root, "hisilicon,hikey960_usb")) {
> Instead of that kind of checks, isn't it enough to just use optional
> gpios?
>
>> +		hisi_hikey_usb->otg_switch_gpio = of_get_named_gpio(root,
>> +				"otg_gpio", 0);
>> +		if (!gpio_is_valid(hisi_hikey_usb->otg_switch_gpio)) {
>> +			pr_info("%s: otg_switch_gpio is err\n", __func__);
>> +			goto free_gpio2;
>> +		}
>> +
>> +		ret = gpio_request(hisi_hikey_usb->otg_switch_gpio,
>> +				"otg_switch_gpio");
>> +		if (ret) {
>> +			hisi_hikey_usb->otg_switch_gpio = INVALID_GPIO_VALUE;
>> +			pr_err("%s: request typec_vbus_gpio err\n", __func__);
>> +			goto free_gpio2;
>> +		}
>> +	}
>> +
>> +	hisi_hikey_usb->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable);
>> +	if (IS_ERR(hisi_hikey_usb->edev)) {
>> +		dev_err(dev, "failed to allocate extcon device\n");
>> +		goto free_gpio2;
>> +	}
>> +
>> +	ret = devm_extcon_dev_register(dev, hisi_hikey_usb->edev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to register extcon device\n");
>> +		goto free_gpio2;
>> +	}
>> +	extcon_set_state(hisi_hikey_usb->edev, EXTCON_USB_HOST, true);
> Is the primary purpose for this extcon device to satisfy the DRD code
> in dwc3 driver?
Yes. I need it to switch mode of dwc3.
>> +	hisi_hikey_usb->role_sw = usb_role_switch_register(dev, &sw_desc);
>> +	if (IS_ERR(hisi_hikey_usb->role_sw))
>> +		goto free_gpio2;
> It looks a bit clumsy to me to register both the extcon device and the
> mux device, but I'm guessing you need to get a notification in dwc3
> driver when the role changes, right? Perhaps we should simply add
> notification chain to the role mux structure. That could potentially
> allow this kind of code to be organized a bit better.
>
>> +	platform_set_drvdata(pdev, hisi_hikey_usb);
>> +
>> +	return 0;
>> +
>> +free_gpio2:
>> +	if (gpio_is_valid(hisi_hikey_usb->typec_vbus_gpio)) {
>> +		gpio_free(hisi_hikey_usb->typec_vbus_gpio);
>> +		hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
>> +	}
>> +
>> +free_gpio1:
>> +	if (gpio_is_valid(hisi_hikey_usb->hub_vbus_gpio)) {
>> +		gpio_free(hisi_hikey_usb->hub_vbus_gpio);
>> +		hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int  hisi_hikey_usb_remove(struct platform_device *pdev)
>> +{
>> +	struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
>> +
>> +	if (gpio_is_valid(hisi_hikey_usb->otg_switch_gpio)) {
>> +		gpio_free(hisi_hikey_usb->otg_switch_gpio);
>> +		hisi_hikey_usb->otg_switch_gpio = INVALID_GPIO_VALUE;
>> +	}
>> +
>> +	if (gpio_is_valid(hisi_hikey_usb->typec_vbus_gpio)) {
>> +		gpio_free(hisi_hikey_usb->typec_vbus_gpio);
>> +		hisi_hikey_usb->typec_vbus_gpio = INVALID_GPIO_VALUE;
>> +	}
>> +
>> +	if (gpio_is_valid(hisi_hikey_usb->hub_vbus_gpio)) {
>> +		gpio_free(hisi_hikey_usb->hub_vbus_gpio);
>> +		hisi_hikey_usb->hub_vbus_gpio = INVALID_GPIO_VALUE;
>> +	}
>> +
>> +	usb_role_switch_unregister(hisi_hikey_usb->role_sw);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id id_table_hisi_hikey_usb[] = {
>> +	{.compatible = "hisilicon,gpio_hubv1"},
>> +	{.compatible = "hisilicon,hikey960_usb"},
>> +	{}
>> +};
>> +
>> +static struct platform_driver  hisi_hikey_usb_driver = {
>> +	.probe = hisi_hikey_usb_probe,
>> +	.remove = hisi_hikey_usb_remove,
>> +	.driver = {
>> +		.name = DEVICE_DRIVER_NAME,
>> +		.of_match_table = of_match_ptr(id_table_hisi_hikey_usb),
>> +
>> +	},
>> +};
>> +
>> +module_platform_driver(hisi_hikey_usb_driver);
>> +
>> +MODULE_AUTHOR("Yu Chen <chenyu56@huawei.com>");
>> +MODULE_DESCRIPTION("Driver Support for USB functionality of Hikey");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.15.0-rc2
> I think you have too many things integrated into this one driver. IMO
> it would at least be better to just let the Type-C port driver take
> care of VBUS like I mentioned above. I'm also wondering if it would
> make sense to handle the role switch and the "hub" in their own
> drivers, but I don't know enough about your platform at this point to
> say for sure.

Thanks for your advice! The HiKey 960 development platform is based around the Huawei Kirin 960.
The Hikey960 Development Board supports three USB host port via a USB hub (U1803 USB5734).
The Hikey960 Development Board also implements a USB2.0 typeC OTG port. 
The Dp and Dm of Soc can be switched between the typeC port and the USB hub.
If there is no cable on the typeC port, then dwc3 core of Soc will be switch to host mode and the
driver of this patch will switch Dp and Dp to the hub. The driver also power on the hub in the meantime.
>
> br,
>

[07/10] hikey960: Support usb functionality of Hikey960

[Heikki Krogerus]

On Tue, Oct 30, 2018 at 10:50:22AM +0800, Chen Yu wrote:
> > I think you have too many things integrated into this one driver. IMO
> > it would at least be better to just let the Type-C port driver take
> > care of VBUS like I mentioned above. I'm also wondering if it would
> > make sense to handle the role switch and the "hub" in their own
> > drivers, but I don't know enough about your platform at this point to
> > say for sure.
> 
> Thanks for your advice! The HiKey 960 development platform is based around the Huawei Kirin 960.
> The Hikey960 Development Board supports three USB host port via a USB hub (U1803 USB5734).
> The Hikey960 Development Board also implements a USB2.0 typeC OTG port.??
> The Dp and Dm of Soc can be switched between the typeC port and the USB hub.
> If there is no cable on the typeC port, then dwc3 core of Soc will be switch to host mode and the
> driver of this patch will switch Dp and Dp to the hub. The driver also power on the hub in the meantime.

Thank you for the explanation. I got the picture now. I realized that
there is some online information for this board:
https://www.96boards.org/documentation/consumer/hikey/hikey960/hardware-docs/hardware-user-manual.md.html#usb-ports

So that mux is actually _not_ switching between the host and device
modes, but instead, it's switching between Type-C and Type-A
connectors (I'm skipping the hub, as it's irrelevant from the PoW of
the mux), so I've misunderstood. And yes, you did say that it is
switching between connectors in the commit message, but I got confused
because you are registers a role switch.

I don't think you should register a role switch from this driver. This
driver is not really representing USB role switch. The mux on this
board is something else. Instead you should register the role switch
from the dwc3 drd code. That is the part that is representing the role
switch here. I actually think that we should do that in any case. The
dwc3 drd code should always register a role switch.

We can solve the problem of getting the role change events in this
driver by adding notification chain to struct usb_role_switch (check
the attached diff). You would then just need to call
usb_role_switch_get() and usb_role_switch_register_notifier(), and
wait for those notifications. The extcon device does not serve any
purpose anymore.

This driver would continue to take care of the hub powering and the
mux, and also the VBUS like before.


br,

diff --git a/drivers/usb/common/roles.c b/drivers/usb/common/roles.c
index bb52e006d203..2881777c16e5 100644
--- a/drivers/usb/common/roles.c
+++ b/drivers/usb/common/roles.c
@@ -20,6 +20,7 @@ struct usb_role_switch {
 	struct device dev;
 	struct mutex lock; /* device lock*/
 	enum usb_role role;
+	struct blocking_notifier_head nh;
 
 	/* From descriptor */
 	struct device *usb2_port;
@@ -49,8 +50,10 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
 	mutex_lock(&sw->lock);
 
 	ret = sw->set(sw->dev.parent, role);
-	if (!ret)
+	if (!ret) {
 		sw->role = role;
+		blocking_notifier_call_chain(&sw->nh, role, NULL);
+	}
 
 	mutex_unlock(&sw->lock);
 
@@ -110,6 +113,20 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
 	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
 
+int usb_role_switch_register_notifier(struct usb_role_switch *sw,
+				      struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&sw->nh, nb);
+}
+EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier);
+
+int usb_role_switch_unregister_notifier(struct usb_role_switch *sw,
+					struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&sw->nh, nb);
+}
+EXPORT_SYMBOL_GPL(usb_role_switch_unregister_notifier);
+
 /**
  * usb_role_switch_get - Find USB role switch linked with the caller
  * @dev: The caller device
@@ -267,6 +284,7 @@ usb_role_switch_register(struct device *parent,
 		return ERR_PTR(-ENOMEM);
 
 	mutex_init(&sw->lock);
+	BLOCKING_INIT_NOTIFIER_HEAD(&sw->nh);
 
 	sw->allow_userspace_control = desc->allow_userspace_control;
 	sw->usb2_port = desc->usb2_port;