usb: add support for Intel dual role port mux

drivers/regulator/fixed.c of_get_fixed_voltage_config(struct device *dev, · line 143

Quoted 7 lines · Lu Baolu

drivers/regulator/fixed.c +7 -0

+	gpiod = gpiod_get(dev, "gpio", GPIOD_ASIS);
+	if (IS_ERR(gpiod))
+		return ERR_PTR(-ENODEV);
+	config->gpio = desc_to_gpio(gpiod);
+	config->enable_high = device_property_read_bool(dev,
+							"enable-active-high");
+	gpiod_put(gpiod);

This isn't going to work at all if the GPIO is shared between multiple regulators but I can't immediately see a sensible way to fix that without some surgery on the GPIO APIs so let's leave it for now.

drivers/usb/mux/portmux-core.c new file +38 -0

+struct portmux_dev *portmux_register(struct portmux_desc *desc)
+{
+	static atomic_t portmux_no = ATOMIC_INIT(-1);
+	struct portmux_dev *pdev;
+	struct extcon_dev *edev = NULL;
+	struct device *dev;
+	int ret;
+
+	/* parameter sanity check */
+	if (!desc || !desc->name || !desc->ops || !desc->dev ||
+	    !desc->ops->cable_set_cb || !desc->ops->cable_unset_cb)
+		return ERR_PTR(-EINVAL);
+
+	dev = desc->dev;
+
+	if (desc->extcon_name) {
+		edev = extcon_get_extcon_dev(desc->extcon_name);
+		if (IS_ERR_OR_NULL(edev))
+			return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
+	if (!pdev)
+		return ERR_PTR(-ENOMEM);
+
+	pdev->desc = desc;
+	pdev->edev = edev;
+	pdev->nb.notifier_call = usb_mux_notifier;
+	mutex_init(&pdev->mux_mutex);
+
+	pdev->dev.parent = dev;
+	dev_set_name(&pdev->dev, "portmux.%lu",
+		     (unsigned long)atomic_inc_return(&portmux_no));
+	pdev->dev.groups = portmux_group;
+	ret = device_register(&pdev->dev);
+	if (ret)
+		goto cleanup_mem;
+

Hi Baolu,

Sorry to comment this so late, but we got hardware that needs to configure the mux in OS, and I noticed some problem.

We are missing
means to bind a port to the correct mux on multiport systems. That we need to solve later in any case, but there is an other issue related to the fact that the notifiers now have to be extcon devices. It's related, as extcon offers no means to solve the multiport issue, but in any case..

Quoted 45 lines

drivers/usb/mux/portmux-core.c new file +45 -0

+struct portmux_dev *portmux_register(struct portmux_desc *desc)
+{
+	static atomic_t portmux_no = ATOMIC_INIT(-1);
+	struct portmux_dev *pdev;
+	struct extcon_dev *edev = NULL;
+	struct device *dev;
+	int ret;
+
+	/* parameter sanity check */
+	if (!desc || !desc->name || !desc->ops || !desc->dev ||
+	    !desc->ops->cable_set_cb || !desc->ops->cable_unset_cb)
+		return ERR_PTR(-EINVAL);
+
+	dev = desc->dev;
+
+	if (desc->extcon_name) {
+		edev = extcon_get_extcon_dev(desc->extcon_name);
+		if (IS_ERR_OR_NULL(edev))
+			return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
+	if (!pdev)
+		return ERR_PTR(-ENOMEM);
+
+	pdev->desc = desc;
+	pdev->edev = edev;
+	pdev->nb.notifier_call = usb_mux_notifier;
+	mutex_init(&pdev->mux_mutex);
+
+	pdev->dev.parent = dev;
+	dev_set_name(&pdev->dev, "portmux.%lu",
+		     (unsigned long)atomic_inc_return(&portmux_no));
+	pdev->dev.groups = portmux_group;
+	ret = device_register(&pdev->dev);
+	if (ret)
+		goto cleanup_mem;
+
+	dev_set_drvdata(&pdev->dev, pdev);
+
+	if (edev) {
+		ret = extcon_register_notifier(edev, EXTCON_USB_HOST,
+					       &pdev->nb);
+		if (ret < 0) {
+			dev_err(dev, "failed to register extcon notifier\n");

drivers/usb/mux/portmux-core.c line 169

Quoted 3 lines

drivers/usb/mux/portmux-core.c new file +3 -0

+			goto cleanup_dev;
+		}
+	}

So I don't actually think this is correct approach. We are forcing the notifying drivers, on top of depending on this framework, depend on extcon too, and that simply is too much. I don't think a USB PHY or charger detection driver should be forced to generate an extcon device just to satisfy the mux in general.

Instead IMO, this framework should provide an API also for the notifiers. The drivers that do the notification should not need to depend on extcon at all. Instead they should be able to just request an optional handle to a portmux, and use it with the function that you already provide (usb_mux_change_state(), which of course needs to be exported). That would make it much easier for us to make problems like figuring out the correct mux for the correct port a problem for the framework and not the drivers.

extcon does not really add any value here, but it does complicate things a lot. We are even exposing new sysfs attributes to control the mux, complete separate from extcon.

Hi Heikki,

Quoted 4 lines · Heikki Krogerus

Hi Baolu,

Sorry to comment this so late, but we got hardware that needs to configure the mux in OS, and I noticed some problem.

Comments are always welcome. :-)

Quoted 52 lines

We are missing
means to bind a port to the correct mux on multiport systems. That we need to solve later in any case, but there is an other issue related to the fact that the notifiers now have to be extcon devices. It's related, as extcon offers no means to solve the multiport issue, but in any case..

Quoted 45 lines

drivers/usb/mux/portmux-core.c new file +45 -0

+struct portmux_dev *portmux_register(struct portmux_desc *desc)
+{
+	static atomic_t portmux_no = ATOMIC_INIT(-1);
+	struct portmux_dev *pdev;
+	struct extcon_dev *edev = NULL;
+	struct device *dev;
+	int ret;
+
+	/* parameter sanity check */
+	if (!desc || !desc->name || !desc->ops || !desc->dev ||
+	    !desc->ops->cable_set_cb || !desc->ops->cable_unset_cb)
+		return ERR_PTR(-EINVAL);
+
+	dev = desc->dev;
+
+	if (desc->extcon_name) {
+		edev = extcon_get_extcon_dev(desc->extcon_name);
+		if (IS_ERR_OR_NULL(edev))
+			return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
+	if (!pdev)
+		return ERR_PTR(-ENOMEM);
+
+	pdev->desc = desc;
+	pdev->edev = edev;
+	pdev->nb.notifier_call = usb_mux_notifier;
+	mutex_init(&pdev->mux_mutex);
+
+	pdev->dev.parent = dev;
+	dev_set_name(&pdev->dev, "portmux.%lu",
+		     (unsigned long)atomic_inc_return(&portmux_no));
+	pdev->dev.groups = portmux_group;
+	ret = device_register(&pdev->dev);
+	if (ret)
+		goto cleanup_mem;
+
+	dev_set_drvdata(&pdev->dev, pdev);
+
+	if (edev) {
+		ret = extcon_register_notifier(edev, EXTCON_USB_HOST,
+					       &pdev->nb);
+		if (ret < 0) {
+			dev_err(dev, "failed to register extcon notifier\n");

Quoted 8 lines

drivers/usb/mux/portmux-core.c line 169

Quoted 3 lines

drivers/usb/mux/portmux-core.c new file +3 -0

+			goto cleanup_dev;
+		}
+	}

So I don't actually think this is correct approach. We are forcing the notifying drivers, on top of depending on this framework, depend on extcon too, and that simply is too much. I don't think a USB PHY or charger detection driver should be forced to generate an extcon device just to satisfy the mux in general.

Fair enough.

Quoted 10 lines

Instead IMO, this framework should provide an API also for the notifiers. The drivers that do the notification should not need to depend on extcon at all. Instead they should be able to just request an optional handle to a portmux, and use it with the function that you already provide (usb_mux_change_state(), which of course needs to be exported). That would make it much easier for us to make problems like figuring out the correct mux for the correct port a problem for the framework and not the drivers.

Quoted 3 lines

extcon does not really add any value here, but it does complicate things a lot. We are even exposing new sysfs attributes to control the mux, complete separate from extcon.

I agree with you that we should move extcon out of the framework.

In order to support multiport systems, I have below proposal.

Currently, we have below interfaces.

struct portmux_dev *portmux_register(struct portmux_desc *desc); void portmux_unregister(struct portmux_dev *pdev);

I will add below ones.

struct portmux_dev *portmux_lookup_by_name(char *name); int portmux_switch(struct portmux_dev *pdev, enum port_role);

The normal usage mode is
1) the mux device is registered as soon as a mux is detected with

    portmux_register();

2) In components like USB PHY or changer drivers, the mux could be retrieved with portmux_lookup_by_name() and controlled via portmux_switch().

Is this helpful?

Quoted 21 lines

I agree with you that we should move extcon out of the framework.

Quoted 20 lines

In order to support multiport systems, I have below proposal.

Currently, we have below interfaces.

struct portmux_dev *portmux_register(struct portmux_desc *desc); void portmux_unregister(struct portmux_dev *pdev);

I will add below ones.

struct portmux_dev *portmux_lookup_by_name(char *name); int portmux_switch(struct portmux_dev *pdev, enum port_role);

The normal usage mode is
1) the mux device is registered as soon as a mux is detected with

    portmux_register();

2) In components like USB PHY or changer drivers, the mux could be retrieved with portmux_lookup_by_name() and controlled via portmux_switch().

Quoted 1 line

Quoted 1 line

Is this helpful?

It works for me, and we can improve it later if needed.

Some Intel platforms have an USB port mux controlled by GPIOs. There's a single ACPI platform device that provides 1) USB ID extcon device; 2) USB vbus regulator device; and 3) USB port switch device. This MFD driver will split these 3 devices for their respective drivers.

[baolu: removed .owner per platform_no_drv_owner.cocci]

Suggested-by David Cohen <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

Reviewed-by Felipe Balbi <[email protected]>

---
 drivers/mfd/Kconfig        |  8 +++++
 drivers/mfd/Makefile       |  1 +
 drivers/mfd/intel-vuport.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)
 create mode 100644 drivers/mfd/intel-vuport.c

drivers/mfd/Kconfig +8 -0

index eea61e3..7e115ab 100644
@@ -1578,5 +1578,13 @@ config MFD_VEXPRESS_SYSREG
 	  System Registers are the platform configuration block
 	  on the ARM Ltd. Versatile Express board.
 
+config MFD_INTEL_VUPORT
+	tristate "Intel virtual USB port controller"
+	select MFD_CORE
+	depends on X86 && ACPI
+	help
+	  Say Y here to enable support for Intel's dual role port mux
+	  controlled by GPIOs.
+
 endmenu
 endif

drivers/mfd/Makefile +1 -0

index 5eaa6465d..65b0518 100644
@@ -203,3 +203,4 @@ intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
 intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)	+= intel_soc_pmic_bxtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
 obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
+obj-$(CONFIG_MFD_INTEL_VUPORT)	+= intel-vuport.o

drivers/mfd/intel-vuport.c new file +89 -0

new file mode 100644
index 0000000..fa84ed7
@@ -0,0 +1,89 @@
+/*
+ * MFD driver for Intel virtual USB port
+ *
+ * Copyright(c) 2016 Intel Corporation.
+ * Author: Lu Baolu <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/gpio.h>
+#include <linux/mfd/core.h>
+#include <linux/property.h>
+#include <linux/platform_device.h>
+
+/* ACPI GPIO Mappings */
+static const struct acpi_gpio_params id_gpio = { 0, 0, false };
+static const struct acpi_gpio_params vbus_gpio = { 1, 0, false };
+static const struct acpi_gpio_params mux_gpio = { 2, 0, false };
+static const struct acpi_gpio_mapping acpi_usb_gpios[] = {
+	{ "id-gpios", &id_gpio, 1 },
+	{ "gpio-gpios", &vbus_gpio, 1 },
+	{ "usb_mux-gpios", &mux_gpio, 1 },
+	{ },
+};
+
+static struct property_entry reg_properties[] = {
+	PROPERTY_ENTRY_STRING("supply-name", "regulator-usb-gpio"),
+	{ },
+};
+
+static const struct property_set reg_properties_pset = {
+	.properties = reg_properties,
+};
+
+static const struct mfd_cell intel_vuport_mfd_cells[] = {
+	{ .name = "extcon-usb-gpio", },
+	{
+		.name = "reg-fixed-voltage",
+		.pset = &reg_properties_pset,
+	},
+	{ .name = "intel-mux-gpio", },
+};
+
+static int vuport_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(dev), acpi_usb_gpios);
+	if (ret)
+		return ret;
+
+	return mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
+				intel_vuport_mfd_cells,
+				ARRAY_SIZE(intel_vuport_mfd_cells), NULL, 0,
+				NULL);
+}
+
+static int vuport_remove(struct platform_device *pdev)
+{
+	mfd_remove_devices(&pdev->dev);
+	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
+
+	return 0;
+}
+
+static struct acpi_device_id vuport_acpi_match[] = {
+	{ "INT3496" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, vuport_acpi_match);
+
+static struct platform_driver vuport_driver = {
+	.driver = {
+		.name = "intel-vuport",
+		.acpi_match_table = ACPI_PTR(vuport_acpi_match),
+	},
+	.probe = vuport_probe,
+	.remove = vuport_remove,
+};
+
+module_platform_driver(vuport_driver);
+
+MODULE_AUTHOR("Lu Baolu <[email protected]>");
+MODULE_DESCRIPTION("Intel virtual USB port");
+MODULE_LICENSE("GPL v2");