[PATCH v3 0/7] usb: add support for Intel dual role port mux

[PATCH v3 0/7] usb: add support for Intel dual role port mux

[Lu Baolu]

Intel SOC chips are featured with USB dual role. The host role is
provided by Intel xHCI IP, and the gadget role is provided by IP
from designware. Tablet platform designs always share a single
port for both host and gadget controllers. There is a mux to
switch the port to the right controller according to the cable
type. OS needs to provide the callback to control the mux when
a plug-in event raises. The method to control the mux is platform
dependent. At least three types of implementation can be found
across current devices. 1) GPIO pins; 2) a unit which can be
controlled by memory mapped registers; 3) ACPI ASL code.

This patch series adds supports for Intel dual role port mux.
It includes:
(1) A helper layer on top of extcon for individual mux driver.
    It listens to the USB-HOST extcon cable and call the switch
    call-back when the cable state changes.
(2) Drivers for GPIO controlled port mux which could be found on
    Baytrail devices. A mfd driver is used to split the GPIOs into
    USB gpio extcon device and a USB mux device. Driver for USB
    gpio extcon device is already in upstream Linux. This patch
    series includes a driver for GPIO USB mux.
(3) Drivers for USB port mux controlled through memory mapped
    registers and the logic to create the mux device. This type
    of dual role port mux could be found in Cherry Trail and
    Broxton devices.

Lu Baolu (7):
  extcon: usb-gpio: add device binding for platform device
  extcon: usb-gpio: add support for ACPI gpio interface
  usb: mux: add common code for Intel dual role port mux
  usb: mux: add driver for Intel gpio controlled port mux
  usb: mux: add driver for Intel drcfg controlled port mux
  usb: pci-quirks: add Intel USB drcfg mux device
  mfd: intel_vuport: Add Intel virtual USB port MFD Driver

 Documentation/ABI/testing/sysfs-bus-platform |  15 +++
 MAINTAINERS                                  |  15 +++
 drivers/extcon/extcon-usb-gpio.c             |  10 +-
 drivers/mfd/Kconfig                          |   8 ++
 drivers/mfd/Makefile                         |   1 +
 drivers/mfd/intel-vuport.c                   |  74 ++++++++++++
 drivers/usb/Kconfig                          |   2 +
 drivers/usb/Makefile                         |   1 +
 drivers/usb/host/pci-quirks.c                |  47 +++++++-
 drivers/usb/host/xhci-ext-caps.h             |   2 +
 drivers/usb/mux/Kconfig                      |  25 ++++
 drivers/usb/mux/Makefile                     |   6 +
 drivers/usb/mux/intel-mux-drcfg.c            | 161 ++++++++++++++++++++++++++
 drivers/usb/mux/intel-mux-gpio.c             | 125 ++++++++++++++++++++
 drivers/usb/mux/intel-mux.c                  | 166 +++++++++++++++++++++++++++
 include/linux/usb/intel-mux.h                |  47 ++++++++
 16 files changed, 702 insertions(+), 3 deletions(-)
 create mode 100644 drivers/mfd/intel-vuport.c
 create mode 100644 drivers/usb/mux/Kconfig
 create mode 100644 drivers/usb/mux/Makefile
 create mode 100644 drivers/usb/mux/intel-mux-drcfg.c
 create mode 100644 drivers/usb/mux/intel-mux-gpio.c
 create mode 100644 drivers/usb/mux/intel-mux.c
 create mode 100644 include/linux/usb/intel-mux.h

Change log:
v2->v3:
 - uvport mfd driver got reviewed by Lee Jones, the following
   changes were made accordingly.
 - seperate uvport driver from the mux drivers in MAINTAINERS file
 - refine the description in Kconfig
 - refine the mfd_cell structure data

v1->v2:
 - move mux driver from drivers/usb/misc to drivers/usb/mux;
 - replace debugfs with sysfs for user level mux control;
 - remove unnecessary register restore if mux registeration failed;
 - Add "Acked-by: Chanwoo Choi <cw00.choi@samsung.com>" to extcon changes;
 - Make the file names and exported function names more specific;
 - Remove the usb_mux_get_dev() interface;
 - Move "struct intel_usb_mux" from .h to .c file;
 - Fix various kbuild robot warnings.
-- 
2.1.4

[PATCH v3 1/7] extcon: usb-gpio: add device binding for platform device

[Lu Baolu]

This is needed to handle the GPIO connected USB ID pin found on
Intel Baytrail devices.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Felipe Balbi <balbi@kernel.org>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/extcon-usb-gpio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index 2b2fecf..af9c8b0 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -206,6 +206,12 @@ static const struct of_device_id usb_extcon_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, usb_extcon_dt_match);
 
+static const struct platform_device_id usb_extcon_platform_ids[] = {
+	{ .name = "extcon-usb-gpio", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, usb_extcon_platform_ids);
+
 static struct platform_driver usb_extcon_driver = {
 	.probe		= usb_extcon_probe,
 	.remove		= usb_extcon_remove,
@@ -214,6 +220,7 @@ static struct platform_driver usb_extcon_driver = {
 		.pm	= &usb_extcon_pm_ops,
 		.of_match_table = usb_extcon_dt_match,
 	},
+	.id_table = usb_extcon_platform_ids,
 };
 
 module_platform_driver(usb_extcon_driver);
-- 
2.1.4

[PATCH v3 2/7] extcon: usb-gpio: add support for ACPI gpio interface

[Lu Baolu]

GPIO resource could be retrieved through APCI as well.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Felipe Balbi <balbi@kernel.org>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/extcon-usb-gpio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index af9c8b0..472c431 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -26,6 +26,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
+#include <linux/acpi.h>
 
 #define USB_GPIO_DEBOUNCE_MS	20	/* ms */
 
@@ -91,7 +92,7 @@ static int usb_extcon_probe(struct platform_device *pdev)
 	struct usb_extcon_info *info;
 	int ret;
 
-	if (!np)
+	if (!np && !ACPI_HANDLE(dev))
 		return -EINVAL;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
-- 
2.1.4

[PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Lu Baolu]

Several Intel PCHs and SOCs have an internal mux that is used to
share one USB port between device controller and host controller.

A usb port mux could be abstracted as the following elements:
1) mux state: HOST or PERIPHERAL;
2) an extcon cable which triggers the change of mux state between
   HOST and PERIPHERAL;
3) The required action to do the real port switch.

This patch adds the common code to handle usb port mux. With this
common code, the individual mux driver, which always is platform
dependent, could focus on the real operation of mux switch.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Felipe Balbi <balbi@kernel.org>
---
 Documentation/ABI/testing/sysfs-bus-platform |  15 +++
 MAINTAINERS                                  |   7 ++
 drivers/usb/Kconfig                          |   2 +
 drivers/usb/Makefile                         |   1 +
 drivers/usb/mux/Kconfig                      |   9 ++
 drivers/usb/mux/Makefile                     |   4 +
 drivers/usb/mux/intel-mux.c                  | 166 +++++++++++++++++++++++++++
 include/linux/usb/intel-mux.h                |  47 ++++++++
 8 files changed, 251 insertions(+)
 create mode 100644 drivers/usb/mux/Kconfig
 create mode 100644 drivers/usb/mux/Makefile
 create mode 100644 drivers/usb/mux/intel-mux.c
 create mode 100644 include/linux/usb/intel-mux.h

diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
index 5172a61..a2261cb 100644
--- a/Documentation/ABI/testing/sysfs-bus-platform
+++ b/Documentation/ABI/testing/sysfs-bus-platform
@@ -18,3 +18,18 @@ Description:
 		devices to opt-out of driver binding using a driver_override
 		name such as "none".  Only a single driver may be specified in
 		the override, there is no support for parsing delimiters.
+
+What:		/sys/bus/platform/devices/.../intel_mux
+Date:		Febuary 2016
+Contact:	Lu Baolu <baolu.lu@linux.intel.com>
+Description:
+		In some platforms, a single USB port is shared between a USB host
+		controller and a device controller. A USB mux driver is needed to
+		handle the port mux. intel_mux attribute shows and stores the mux
+		state.
+		For read:
+		'peripheral' - mux switched to PERIPHERAL controller;
+		'host'       - mux switched to HOST controller.
+		For write:
+		'peripheral' - mux will be switched to PERIPHERAL controller;
+		'host'       - mux will be switched to HOST controller.
diff --git a/MAINTAINERS b/MAINTAINERS
index c55b37e..a93d26a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11400,6 +11400,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
 S:	Maintained
 F:	drivers/usb/phy/
 
+USB PORT MUX DRIVER
+M:	Lu Baolu <baolu.lu@linux.intel.com>
+L:	linux-usb@vger.kernel.org
+S:	Supported
+F:	drivers/usb/mux/intel-mux.c
+F:	include/linux/usb/intel-mux.h
+
 USB PRINTER DRIVER (usblp)
 M:	Pete Zaitcev <zaitcev@redhat.com>
 L:	linux-usb@vger.kernel.org
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8ed451d..dbd6620 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -149,6 +149,8 @@ endif # USB
 
 source "drivers/usb/phy/Kconfig"
 
+source "drivers/usb/mux/Kconfig"
+
 source "drivers/usb/gadget/Kconfig"
 
 config USB_LED_TRIG
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index dca7856..9a92338 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -6,6 +6,7 @@
 
 obj-$(CONFIG_USB)		+= core/
 obj-$(CONFIG_USB_SUPPORT)	+= phy/
+obj-$(CONFIG_USB_SUPPORT)	+= mux/
 
 obj-$(CONFIG_USB_DWC3)		+= dwc3/
 obj-$(CONFIG_USB_DWC2)		+= dwc2/
diff --git a/drivers/usb/mux/Kconfig b/drivers/usb/mux/Kconfig
new file mode 100644
index 0000000..8fedc4f
--- /dev/null
+++ b/drivers/usb/mux/Kconfig
@@ -0,0 +1,9 @@
+#
+# USB port mux driver configuration
+#
+menu "USB Port MUX drivers"
+config INTEL_USB_MUX
+	select EXTCON
+	def_bool n
+
+endmenu
diff --git a/drivers/usb/mux/Makefile b/drivers/usb/mux/Makefile
new file mode 100644
index 0000000..84f0ae8
--- /dev/null
+++ b/drivers/usb/mux/Makefile
@@ -0,0 +1,4 @@
+#
+# Makefile for USB port mux drivers
+#
+obj-$(CONFIG_INTEL_USB_MUX)		+= intel-mux.o
diff --git a/drivers/usb/mux/intel-mux.c b/drivers/usb/mux/intel-mux.c
new file mode 100644
index 0000000..b243758
--- /dev/null
+++ b/drivers/usb/mux/intel-mux.c
@@ -0,0 +1,166 @@
+/**
+ * mux.c - USB Port Mux support
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ *
+ * 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/notifier.h>
+#include <linux/extcon.h>
+#include <linux/usb/intel-mux.h>
+#include <linux/err.h>
+
+struct intel_usb_mux {
+	struct intel_mux_dev		*umdev;
+	struct notifier_block		nb;
+	struct extcon_specific_cable_nb	obj;
+
+	/*
+	 * The state of the mux.
+	 * 0, 1 - mux switch state
+	 * -1   - uninitialized state
+	 *
+	 * mux_mutex is lock to protect mux_state
+	 */
+	int				mux_state;
+	struct mutex			mux_mutex;
+};
+
+static int usb_mux_change_state(struct intel_usb_mux *mux, int state)
+{
+	int ret;
+	struct intel_mux_dev *umdev = mux->umdev;
+
+	dev_WARN_ONCE(umdev->dev, !mutex_is_locked(&mux->mux_mutex),
+			"mutex is unlocked\n");
+
+	mux->mux_state = state;
+
+	if (mux->mux_state)
+		ret = umdev->cable_set_cb(umdev);
+	else
+		ret = umdev->cable_unset_cb(umdev);
+
+	return ret;
+}
+
+static int usb_mux_notifier(struct notifier_block *nb,
+		unsigned long event, void *ptr)
+{
+	struct intel_usb_mux *mux;
+	int state;
+	int ret = NOTIFY_DONE;
+
+	mux = container_of(nb, struct intel_usb_mux, nb);
+
+	state = extcon_get_cable_state(mux->obj.edev,
+			mux->umdev->cable_name);
+
+	if (mux->mux_state == -1 || mux->mux_state != state) {
+		mutex_lock(&mux->mux_mutex);
+		ret = usb_mux_change_state(mux, state);
+		mutex_unlock(&mux->mux_mutex);
+	}
+
+	return ret;
+}
+
+static ssize_t intel_mux_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_usb_mux *mux = dev_get_drvdata(dev);
+
+	if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
+		return 0;
+
+	return sprintf(buf, "%s\n", mux->mux_state ? "host" : "peripheral");
+}
+
+static ssize_t intel_mux_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct intel_usb_mux *mux = dev_get_drvdata(dev);
+	int state;
+
+	if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
+		return -EINVAL;
+
+	if (sysfs_streq(buf, "peripheral"))
+		state = 0;
+	else if (sysfs_streq(buf, "host"))
+		state = 1;
+	else
+		return -EINVAL;
+
+	mutex_lock(&mux->mux_mutex);
+	usb_mux_change_state(mux, state);
+	mutex_unlock(&mux->mux_mutex);
+
+	return count;
+}
+static DEVICE_ATTR_RW(intel_mux);
+
+int intel_usb_mux_register(struct intel_mux_dev *umdev)
+{
+	int ret;
+	struct device *dev = umdev->dev;
+	struct intel_usb_mux *mux;
+
+	if (!umdev->cable_name)
+		return -ENODEV;
+
+	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return -ENOMEM;
+
+	mux->umdev = umdev;
+	mux->nb.notifier_call = usb_mux_notifier;
+	mutex_init(&mux->mux_mutex);
+	mux->mux_state = -1;
+	dev_set_drvdata(dev, mux);
+	ret = extcon_register_interest(&mux->obj, umdev->extcon_name,
+			umdev->cable_name, &mux->nb);
+	if (ret) {
+		dev_err(dev, "failed to register extcon notifier\n");
+		return -ENODEV;
+	}
+
+	usb_mux_notifier(&mux->nb, 0, NULL);
+
+	/* register the sysfs interface */
+	ret = device_create_file(dev, &dev_attr_intel_mux);
+	if (ret) {
+		dev_err(dev, "failed to create sysfs attribute\n");
+		if (umdev->cable_name)
+			extcon_unregister_interest(&mux->obj);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_usb_mux_register);
+
+int intel_usb_mux_unregister(struct device *dev)
+{
+	struct intel_usb_mux *mux = dev_get_drvdata(dev);
+
+	device_remove_file(dev, &dev_attr_intel_mux);
+	extcon_unregister_interest(&mux->obj);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
+
+#ifdef CONFIG_PM_SLEEP
+void intel_usb_mux_complete(struct device *dev)
+{
+	struct intel_usb_mux *mux = dev_get_drvdata(dev);
+
+	usb_mux_notifier(&mux->nb, 0, NULL);
+}
+EXPORT_SYMBOL_GPL(intel_usb_mux_complete);
+#endif
diff --git a/include/linux/usb/intel-mux.h b/include/linux/usb/intel-mux.h
new file mode 100644
index 0000000..6990174
--- /dev/null
+++ b/include/linux/usb/intel-mux.h
@@ -0,0 +1,47 @@
+/**
+ * mux.h - USB Port Mux defines
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_USB_INTEL_MUX_H
+#define __LINUX_USB_INTEL_MUX_H
+
+#include <linux/usb.h>
+
+struct intel_mux_dev {
+	struct device	*dev;
+	char		*extcon_name;
+	char		*cable_name;
+	int		(*cable_set_cb)(struct intel_mux_dev *mux);
+	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
+};
+
+#if IS_ENABLED(CONFIG_INTEL_USB_MUX)
+extern int intel_usb_mux_register(struct intel_mux_dev *mux);
+extern int intel_usb_mux_unregister(struct device *dev);
+
+#ifdef CONFIG_PM_SLEEP
+extern void intel_usb_mux_complete(struct device *dev);
+#endif
+
+#else
+static inline int intel_usb_mux_register(struct intel_mux_dev *mux)
+{
+	return -ENODEV;
+}
+
+static inline int intel_usb_mux_unregister(struct device *dev)
+{
+	return 0;
+}
+
+#endif /* CONFIG_INTEL_USB_MUX */
+
+#endif /* __LINUX_USB_INTEL_MUX_H */
-- 
2.1.4

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Oliver Neukum]

On Tue, 2016-03-08 at 15:53 +0800, Lu Baolu wrote:

> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
> index 5172a61..a2261cb 100644
> --- a/Documentation/ABI/testing/sysfs-bus-platform
> +++ b/Documentation/ABI/testing/sysfs-bus-platform
> @@ -18,3 +18,18 @@ Description:
>  		devices to opt-out of driver binding using a driver_override
>  		name such as "none".  Only a single driver may be specified in
>  		the override, there is no support for parsing delimiters.
> +
> +What:		/sys/bus/platform/devices/.../intel_mux

Hi,

is there any reason to call this "intel_mux"? We want a common interface
for such things. So how about "role_mux" or "master_slave_mux"?

	Regards
		Oliver

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Thu, Mar 10, 2016 at 01:39:43PM +0100, Oliver Neukum wrote:
> On Tue, 2016-03-08 at 15:53 +0800, Lu Baolu wrote:
> 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
> > index 5172a61..a2261cb 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-platform
> > +++ b/Documentation/ABI/testing/sysfs-bus-platform
> > @@ -18,3 +18,18 @@ Description:
> >  		devices to opt-out of driver binding using a driver_override
> >  		name such as "none".  Only a single driver may be specified in
> >  		the override, there is no support for parsing delimiters.
> > +
> > +What:		/sys/bus/platform/devices/.../intel_mux
> 
> Hi,
> 
> is there any reason to call this "intel_mux"? We want a common interface
> for such things. So how about "role_mux" or "master_slave_mux"?

I agree, don't make this intel specific, as it shouldn't be.

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Lu Baolu]

On 03/11/2016 07:57 AM, Greg Kroah-Hartman wrote:
> On Thu, Mar 10, 2016 at 01:39:43PM +0100, Oliver Neukum wrote:
>> On Tue, 2016-03-08 at 15:53 +0800, Lu Baolu wrote:
>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
>>> index 5172a61..a2261cb 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-platform
>>> +++ b/Documentation/ABI/testing/sysfs-bus-platform
>>> @@ -18,3 +18,18 @@ Description:
>>>  		devices to opt-out of driver binding using a driver_override
>>>  		name such as "none".  Only a single driver may be specified in
>>>  		the override, there is no support for parsing delimiters.
>>> +
>>> +What:		/sys/bus/platform/devices/.../intel_mux
>> Hi,
>>
>> is there any reason to call this "intel_mux"? We want a common interface
>> for such things. So how about "role_mux" or "master_slave_mux"?
> I agree, don't make this intel specific, as it shouldn't be.

Sure. I will change it to "port_mux".

>
>

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Lu Baolu]

Hi,

On 03/11/2016 07:57 AM, Greg Kroah-Hartman wrote:
> On Thu, Mar 10, 2016 at 01:39:43PM +0100, Oliver Neukum wrote:
>> On Tue, 2016-03-08 at 15:53 +0800, Lu Baolu wrote:
>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
>>> index 5172a61..a2261cb 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-platform
>>> +++ b/Documentation/ABI/testing/sysfs-bus-platform
>>> @@ -18,3 +18,18 @@ Description:
>>>  		devices to opt-out of driver binding using a driver_override
>>>  		name such as "none".  Only a single driver may be specified in
>>>  		the override, there is no support for parsing delimiters.
>>> +
>>> +What:		/sys/bus/platform/devices/.../intel_mux
>> Hi,
>>
>> is there any reason to call this "intel_mux"? We want a common interface
>> for such things. So how about "role_mux" or "master_slave_mux"?
> I agree, don't make this intel specific, as it shouldn't be.
>

By the way, do you expect a class for port mux in sysfs?

Best regards,
Baolu

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Wed, Apr 06, 2016 at 01:58:52PM +0800, Lu Baolu wrote:
> Hi,
> 
> On 03/11/2016 07:57 AM, Greg Kroah-Hartman wrote:
> > On Thu, Mar 10, 2016 at 01:39:43PM +0100, Oliver Neukum wrote:
> >> On Tue, 2016-03-08 at 15:53 +0800, Lu Baolu wrote:
> >>
> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
> >>> index 5172a61..a2261cb 100644
> >>> --- a/Documentation/ABI/testing/sysfs-bus-platform
> >>> +++ b/Documentation/ABI/testing/sysfs-bus-platform
> >>> @@ -18,3 +18,18 @@ Description:
> >>>  		devices to opt-out of driver binding using a driver_override
> >>>  		name such as "none".  Only a single driver may be specified in
> >>>  		the override, there is no support for parsing delimiters.
> >>> +
> >>> +What:		/sys/bus/platform/devices/.../intel_mux
> >> Hi,
> >>
> >> is there any reason to call this "intel_mux"? We want a common interface
> >> for such things. So how about "role_mux" or "master_slave_mux"?
> > I agree, don't make this intel specific, as it shouldn't be.
> >
> 
> By the way, do you expect a class for port mux in sysfs?

Why would you create a class?  What is that going to do here?

What happened to getting internal help in designing this api?  This
shouldn't be my job :)

thanks,

greg k-h

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]


Hi,

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> On 03/11/2016 07:57 AM, Greg Kroah-Hartman wrote:
>> > On Thu, Mar 10, 2016 at 01:39:43PM +0100, Oliver Neukum wrote:
>> >> On Tue, 2016-03-08 at 15:53 +0800, Lu Baolu wrote:
>> >>
>> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
>> >>> index 5172a61..a2261cb 100644
>> >>> --- a/Documentation/ABI/testing/sysfs-bus-platform
>> >>> +++ b/Documentation/ABI/testing/sysfs-bus-platform
>> >>> @@ -18,3 +18,18 @@ Description:
>> >>>  		devices to opt-out of driver binding using a driver_override
>> >>>  		name such as "none".  Only a single driver may be specified in
>> >>>  		the override, there is no support for parsing delimiters.
>> >>> +
>> >>> +What:		/sys/bus/platform/devices/.../intel_mux
>> >> Hi,
>> >>
>> >> is there any reason to call this "intel_mux"? We want a common interface
>> >> for such things. So how about "role_mux" or "master_slave_mux"?
>> > I agree, don't make this intel specific, as it shouldn't be.

right, we can easily change the name. What happens when
e.g. AMD/NVidia/TI/MediaTek/Allwinner/etc come up with a similar setup
which can't be supported by Intel's mux driver ? (see more below,
agreeing on a name here doesn't seem to be enough)

>> By the way, do you expect a class for port mux in sysfs?
>
> Why would you create a class?  What is that going to do here?

well, you said you didn't want this to be Intel-specific. We could just
change the name and remove "intel_" from it, but then we would fall into
this situation where possibly different mux drivers might be
needed. That almost requires some small abstraction between sysfs and
the actual implementation, does it not ?

A bus_type doesn't fit here and a simple 'agreement' that mux drivers
will create a sysfs file named 'mux' is too fragile. Thus...

> What happened to getting internal help in designing this api?  This
> shouldn't be my job :)

I looked at this Baolu but, at least to me, it's unclear what you're
hinting here. Sure, the API is a bit odd in that we register a mux and
unregister a device (that has been fixed), but is there anything else ?

We're not asking that you design the API for us, rather just make your
comments a little clearer.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Wed, Apr 06, 2016 at 01:19:04PM +0300, Felipe Balbi wrote:
> > What happened to getting internal help in designing this api?  This
> > shouldn't be my job :)
> 
> I looked at this Baolu but, at least to me, it's unclear what you're
> hinting here. Sure, the API is a bit odd in that we register a mux and
> unregister a device (that has been fixed), but is there anything else ?

I don't remember anymore, that was a few thousand emails ago...

But the suggestion about using devm_* shows that the original author has
very little understanding about how the driver model works at all, which
doesn't give me much hope at the moment...

greg k-h

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]


Hi,

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Wed, Apr 06, 2016 at 01:19:04PM +0300, Felipe Balbi wrote:
>> > What happened to getting internal help in designing this api?  This
>> > shouldn't be my job :)
>> 
>> I looked at this Baolu but, at least to me, it's unclear what you're
>> hinting here. Sure, the API is a bit odd in that we register a mux and
>> unregister a device (that has been fixed), but is there anything else ?
>
> I don't remember anymore, that was a few thousand emails ago...
>
> But the suggestion about using devm_* shows that the original author
> has very little understanding about how the driver model works at all,
> which doesn't give me much hope at the moment...

fair enough, that can also be 'fixed' with experience, right ? Just
takes time ;-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
> Several Intel PCHs and SOCs have an internal mux that is used to
> share one USB port between device controller and host controller.
> 
> A usb port mux could be abstracted as the following elements:
> 1) mux state: HOST or PERIPHERAL;
> 2) an extcon cable which triggers the change of mux state between
>    HOST and PERIPHERAL;
> 3) The required action to do the real port switch.
> 
> This patch adds the common code to handle usb port mux. With this
> common code, the individual mux driver, which always is platform
> dependent, could focus on the real operation of mux switch.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Reviewed-by: Felipe Balbi <balbi@kernel.org>
> ---
>  Documentation/ABI/testing/sysfs-bus-platform |  15 +++
>  MAINTAINERS                                  |   7 ++
>  drivers/usb/Kconfig                          |   2 +
>  drivers/usb/Makefile                         |   1 +
>  drivers/usb/mux/Kconfig                      |   9 ++
>  drivers/usb/mux/Makefile                     |   4 +
>  drivers/usb/mux/intel-mux.c                  | 166 +++++++++++++++++++++++++++
>  include/linux/usb/intel-mux.h                |  47 ++++++++
>  8 files changed, 251 insertions(+)
>  create mode 100644 drivers/usb/mux/Kconfig
>  create mode 100644 drivers/usb/mux/Makefile
>  create mode 100644 drivers/usb/mux/intel-mux.c
>  create mode 100644 include/linux/usb/intel-mux.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
> index 5172a61..a2261cb 100644
> --- a/Documentation/ABI/testing/sysfs-bus-platform
> +++ b/Documentation/ABI/testing/sysfs-bus-platform
> @@ -18,3 +18,18 @@ Description:
>  		devices to opt-out of driver binding using a driver_override
>  		name such as "none".  Only a single driver may be specified in
>  		the override, there is no support for parsing delimiters.
> +
> +What:		/sys/bus/platform/devices/.../intel_mux
> +Date:		Febuary 2016
> +Contact:	Lu Baolu <baolu.lu@linux.intel.com>
> +Description:
> +		In some platforms, a single USB port is shared between a USB host
> +		controller and a device controller. A USB mux driver is needed to
> +		handle the port mux. intel_mux attribute shows and stores the mux
> +		state.
> +		For read:
> +		'peripheral' - mux switched to PERIPHERAL controller;
> +		'host'       - mux switched to HOST controller.
> +		For write:
> +		'peripheral' - mux will be switched to PERIPHERAL controller;
> +		'host'       - mux will be switched to HOST controller.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c55b37e..a93d26a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11400,6 +11400,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
>  S:	Maintained
>  F:	drivers/usb/phy/
>  
> +USB PORT MUX DRIVER
> +M:	Lu Baolu <baolu.lu@linux.intel.com>
> +L:	linux-usb@vger.kernel.org
> +S:	Supported
> +F:	drivers/usb/mux/intel-mux.c
> +F:	include/linux/usb/intel-mux.h
> +
>  USB PRINTER DRIVER (usblp)
>  M:	Pete Zaitcev <zaitcev@redhat.com>
>  L:	linux-usb@vger.kernel.org
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index 8ed451d..dbd6620 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -149,6 +149,8 @@ endif # USB
>  
>  source "drivers/usb/phy/Kconfig"
>  
> +source "drivers/usb/mux/Kconfig"
> +
>  source "drivers/usb/gadget/Kconfig"
>  
>  config USB_LED_TRIG
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index dca7856..9a92338 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -6,6 +6,7 @@
>  
>  obj-$(CONFIG_USB)		+= core/
>  obj-$(CONFIG_USB_SUPPORT)	+= phy/
> +obj-$(CONFIG_USB_SUPPORT)	+= mux/
>  
>  obj-$(CONFIG_USB_DWC3)		+= dwc3/
>  obj-$(CONFIG_USB_DWC2)		+= dwc2/
> diff --git a/drivers/usb/mux/Kconfig b/drivers/usb/mux/Kconfig
> new file mode 100644
> index 0000000..8fedc4f
> --- /dev/null
> +++ b/drivers/usb/mux/Kconfig
> @@ -0,0 +1,9 @@
> +#
> +# USB port mux driver configuration
> +#
> +menu "USB Port MUX drivers"
> +config INTEL_USB_MUX
> +	select EXTCON
> +	def_bool n

No help text?

> +
> +endmenu
> diff --git a/drivers/usb/mux/Makefile b/drivers/usb/mux/Makefile
> new file mode 100644
> index 0000000..84f0ae8
> --- /dev/null
> +++ b/drivers/usb/mux/Makefile
> @@ -0,0 +1,4 @@
> +#
> +# Makefile for USB port mux drivers
> +#
> +obj-$(CONFIG_INTEL_USB_MUX)		+= intel-mux.o
> diff --git a/drivers/usb/mux/intel-mux.c b/drivers/usb/mux/intel-mux.c
> new file mode 100644
> index 0000000..b243758
> --- /dev/null
> +++ b/drivers/usb/mux/intel-mux.c
> @@ -0,0 +1,166 @@
> +/**
> + * mux.c - USB Port Mux support
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * Author: Lu Baolu <baolu.lu@linux.intel.com>
> + *
> + * 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/notifier.h>
> +#include <linux/extcon.h>
> +#include <linux/usb/intel-mux.h>
> +#include <linux/err.h>
> +
> +struct intel_usb_mux {
> +	struct intel_mux_dev		*umdev;
> +	struct notifier_block		nb;
> +	struct extcon_specific_cable_nb	obj;
> +
> +	/*
> +	 * The state of the mux.
> +	 * 0, 1 - mux switch state
> +	 * -1   - uninitialized state
> +	 *
> +	 * mux_mutex is lock to protect mux_state
> +	 */
> +	int				mux_state;
> +	struct mutex			mux_mutex;
> +};
> +
> +static int usb_mux_change_state(struct intel_usb_mux *mux, int state)
> +{
> +	int ret;
> +	struct intel_mux_dev *umdev = mux->umdev;
> +
> +	dev_WARN_ONCE(umdev->dev, !mutex_is_locked(&mux->mux_mutex),
> +			"mutex is unlocked\n");
> +
> +	mux->mux_state = state;
> +
> +	if (mux->mux_state)
> +		ret = umdev->cable_set_cb(umdev);
> +	else
> +		ret = umdev->cable_unset_cb(umdev);
> +
> +	return ret;
> +}
> +
> +static int usb_mux_notifier(struct notifier_block *nb,
> +		unsigned long event, void *ptr)
> +{
> +	struct intel_usb_mux *mux;
> +	int state;
> +	int ret = NOTIFY_DONE;
> +
> +	mux = container_of(nb, struct intel_usb_mux, nb);
> +
> +	state = extcon_get_cable_state(mux->obj.edev,
> +			mux->umdev->cable_name);
> +
> +	if (mux->mux_state == -1 || mux->mux_state != state) {
> +		mutex_lock(&mux->mux_mutex);
> +		ret = usb_mux_change_state(mux, state);
> +		mutex_unlock(&mux->mux_mutex);
> +	}
> +
> +	return ret;
> +}
> +
> +static ssize_t intel_mux_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
> +
> +	if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
> +		return 0;
> +
> +	return sprintf(buf, "%s\n", mux->mux_state ? "host" : "peripheral");
> +}
> +
> +static ssize_t intel_mux_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
> +	int state;
> +
> +	if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
> +		return -EINVAL;
> +
> +	if (sysfs_streq(buf, "peripheral"))
> +		state = 0;
> +	else if (sysfs_streq(buf, "host"))
> +		state = 1;
> +	else
> +		return -EINVAL;
> +
> +	mutex_lock(&mux->mux_mutex);
> +	usb_mux_change_state(mux, state);
> +	mutex_unlock(&mux->mux_mutex);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(intel_mux);
> +
> +int intel_usb_mux_register(struct intel_mux_dev *umdev)
> +{
> +	int ret;
> +	struct device *dev = umdev->dev;
> +	struct intel_usb_mux *mux;
> +
> +	if (!umdev->cable_name)
> +		return -ENODEV;
> +
> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> +	if (!mux)
> +		return -ENOMEM;
> +
> +	mux->umdev = umdev;
> +	mux->nb.notifier_call = usb_mux_notifier;
> +	mutex_init(&mux->mux_mutex);
> +	mux->mux_state = -1;
> +	dev_set_drvdata(dev, mux);
> +	ret = extcon_register_interest(&mux->obj, umdev->extcon_name,
> +			umdev->cable_name, &mux->nb);
> +	if (ret) {
> +		dev_err(dev, "failed to register extcon notifier\n");
> +		return -ENODEV;
> +	}
> +
> +	usb_mux_notifier(&mux->nb, 0, NULL);
> +
> +	/* register the sysfs interface */
> +	ret = device_create_file(dev, &dev_attr_intel_mux);
> +	if (ret) {
> +		dev_err(dev, "failed to create sysfs attribute\n");
> +		if (umdev->cable_name)
> +			extcon_unregister_interest(&mux->obj);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> +
> +int intel_usb_mux_unregister(struct device *dev)
> +{
> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
> +
> +	device_remove_file(dev, &dev_attr_intel_mux);
> +	extcon_unregister_interest(&mux->obj);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
> +
> +#ifdef CONFIG_PM_SLEEP
> +void intel_usb_mux_complete(struct device *dev)
> +{
> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
> +
> +	usb_mux_notifier(&mux->nb, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_complete);
> +#endif
> diff --git a/include/linux/usb/intel-mux.h b/include/linux/usb/intel-mux.h
> new file mode 100644
> index 0000000..6990174
> --- /dev/null
> +++ b/include/linux/usb/intel-mux.h
> @@ -0,0 +1,47 @@
> +/**
> + * mux.h - USB Port Mux defines
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * Author: Lu Baolu <baolu.lu@linux.intel.com>
> + *
> + * 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.
> + */
> +
> +#ifndef __LINUX_USB_INTEL_MUX_H
> +#define __LINUX_USB_INTEL_MUX_H
> +
> +#include <linux/usb.h>
> +
> +struct intel_mux_dev {
> +	struct device	*dev;
> +	char		*extcon_name;
> +	char		*cable_name;
> +	int		(*cable_set_cb)(struct intel_mux_dev *mux);
> +	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
> +};

This is a device, why not make it one?  Don't just hold a reference.
And do you really even hold that reference?

> +#if IS_ENABLED(CONFIG_INTEL_USB_MUX)
> +extern int intel_usb_mux_register(struct intel_mux_dev *mux);
> +extern int intel_usb_mux_unregister(struct device *dev);

It's obvious you didn't run this through checkpatch.pl, please do so...

And your api is horrid, think about what you want the "core" to do here,
it should be the one creating the device and returning it to the caller,
not forcing the caller to somehow create it first and then pass it in.

And why is it not symmetrical, you are passing one thing into register
and another into unregister.

Come on, I expect better work from Intel kernel developers, I shouldn't
have to be complaining about basic crap like this...

greg k-h

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Lu Baolu]

Hi,

On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote:
> On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
>> Several Intel PCHs and SOCs have an internal mux that is used to
>> share one USB port between device controller and host controller.
>>
>> A usb port mux could be abstracted as the following elements:
>> 1) mux state: HOST or PERIPHERAL;
>> 2) an extcon cable which triggers the change of mux state between
>>    HOST and PERIPHERAL;
>> 3) The required action to do the real port switch.
>>
>> This patch adds the common code to handle usb port mux. With this
>> common code, the individual mux driver, which always is platform
>> dependent, could focus on the real operation of mux switch.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Reviewed-by: Felipe Balbi <balbi@kernel.org>
>> ---
>>  Documentation/ABI/testing/sysfs-bus-platform |  15 +++
>>  MAINTAINERS                                  |   7 ++
>>  drivers/usb/Kconfig                          |   2 +
>>  drivers/usb/Makefile                         |   1 +
>>  drivers/usb/mux/Kconfig                      |   9 ++
>>  drivers/usb/mux/Makefile                     |   4 +
>>  drivers/usb/mux/intel-mux.c                  | 166 +++++++++++++++++++++++++++
>>  include/linux/usb/intel-mux.h                |  47 ++++++++
>>  8 files changed, 251 insertions(+)
>>  create mode 100644 drivers/usb/mux/Kconfig
>>  create mode 100644 drivers/usb/mux/Makefile
>>  create mode 100644 drivers/usb/mux/intel-mux.c
>>  create mode 100644 include/linux/usb/intel-mux.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
>> index 5172a61..a2261cb 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-platform
>> +++ b/Documentation/ABI/testing/sysfs-bus-platform
>> @@ -18,3 +18,18 @@ Description:
>>  		devices to opt-out of driver binding using a driver_override
>>  		name such as "none".  Only a single driver may be specified in
>>  		the override, there is no support for parsing delimiters.
>> +
>> +What:		/sys/bus/platform/devices/.../intel_mux
>> +Date:		Febuary 2016
>> +Contact:	Lu Baolu <baolu.lu@linux.intel.com>
>> +Description:
>> +		In some platforms, a single USB port is shared between a USB host
>> +		controller and a device controller. A USB mux driver is needed to
>> +		handle the port mux. intel_mux attribute shows and stores the mux
>> +		state.
>> +		For read:
>> +		'peripheral' - mux switched to PERIPHERAL controller;
>> +		'host'       - mux switched to HOST controller.
>> +		For write:
>> +		'peripheral' - mux will be switched to PERIPHERAL controller;
>> +		'host'       - mux will be switched to HOST controller.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c55b37e..a93d26a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11400,6 +11400,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
>>  S:	Maintained
>>  F:	drivers/usb/phy/
>>  
>> +USB PORT MUX DRIVER
>> +M:	Lu Baolu <baolu.lu@linux.intel.com>
>> +L:	linux-usb@vger.kernel.org
>> +S:	Supported
>> +F:	drivers/usb/mux/intel-mux.c
>> +F:	include/linux/usb/intel-mux.h
>> +
>>  USB PRINTER DRIVER (usblp)
>>  M:	Pete Zaitcev <zaitcev@redhat.com>
>>  L:	linux-usb@vger.kernel.org
>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
>> index 8ed451d..dbd6620 100644
>> --- a/drivers/usb/Kconfig
>> +++ b/drivers/usb/Kconfig
>> @@ -149,6 +149,8 @@ endif # USB
>>  
>>  source "drivers/usb/phy/Kconfig"
>>  
>> +source "drivers/usb/mux/Kconfig"
>> +
>>  source "drivers/usb/gadget/Kconfig"
>>  
>>  config USB_LED_TRIG
>> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
>> index dca7856..9a92338 100644
>> --- a/drivers/usb/Makefile
>> +++ b/drivers/usb/Makefile
>> @@ -6,6 +6,7 @@
>>  
>>  obj-$(CONFIG_USB)		+= core/
>>  obj-$(CONFIG_USB_SUPPORT)	+= phy/
>> +obj-$(CONFIG_USB_SUPPORT)	+= mux/
>>  
>>  obj-$(CONFIG_USB_DWC3)		+= dwc3/
>>  obj-$(CONFIG_USB_DWC2)		+= dwc2/
>> diff --git a/drivers/usb/mux/Kconfig b/drivers/usb/mux/Kconfig
>> new file mode 100644
>> index 0000000..8fedc4f
>> --- /dev/null
>> +++ b/drivers/usb/mux/Kconfig
>> @@ -0,0 +1,9 @@
>> +#
>> +# USB port mux driver configuration
>> +#
>> +menu "USB Port MUX drivers"
>> +config INTEL_USB_MUX
>> +	select EXTCON
>> +	def_bool n
> No help text?
>
>> +
>> +endmenu
>> diff --git a/drivers/usb/mux/Makefile b/drivers/usb/mux/Makefile
>> new file mode 100644
>> index 0000000..84f0ae8
>> --- /dev/null
>> +++ b/drivers/usb/mux/Makefile
>> @@ -0,0 +1,4 @@
>> +#
>> +# Makefile for USB port mux drivers
>> +#
>> +obj-$(CONFIG_INTEL_USB_MUX)		+= intel-mux.o
>> diff --git a/drivers/usb/mux/intel-mux.c b/drivers/usb/mux/intel-mux.c
>> new file mode 100644
>> index 0000000..b243758
>> --- /dev/null
>> +++ b/drivers/usb/mux/intel-mux.c
>> @@ -0,0 +1,166 @@
>> +/**
>> + * mux.c - USB Port Mux support
>> + *
>> + * Copyright (C) 2016 Intel Corporation
>> + *
>> + * Author: Lu Baolu <baolu.lu@linux.intel.com>
>> + *
>> + * 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/notifier.h>
>> +#include <linux/extcon.h>
>> +#include <linux/usb/intel-mux.h>
>> +#include <linux/err.h>
>> +
>> +struct intel_usb_mux {
>> +	struct intel_mux_dev		*umdev;
>> +	struct notifier_block		nb;
>> +	struct extcon_specific_cable_nb	obj;
>> +
>> +	/*
>> +	 * The state of the mux.
>> +	 * 0, 1 - mux switch state
>> +	 * -1   - uninitialized state
>> +	 *
>> +	 * mux_mutex is lock to protect mux_state
>> +	 */
>> +	int				mux_state;
>> +	struct mutex			mux_mutex;
>> +};
>> +
>> +static int usb_mux_change_state(struct intel_usb_mux *mux, int state)
>> +{
>> +	int ret;
>> +	struct intel_mux_dev *umdev = mux->umdev;
>> +
>> +	dev_WARN_ONCE(umdev->dev, !mutex_is_locked(&mux->mux_mutex),
>> +			"mutex is unlocked\n");
>> +
>> +	mux->mux_state = state;
>> +
>> +	if (mux->mux_state)
>> +		ret = umdev->cable_set_cb(umdev);
>> +	else
>> +		ret = umdev->cable_unset_cb(umdev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int usb_mux_notifier(struct notifier_block *nb,
>> +		unsigned long event, void *ptr)
>> +{
>> +	struct intel_usb_mux *mux;
>> +	int state;
>> +	int ret = NOTIFY_DONE;
>> +
>> +	mux = container_of(nb, struct intel_usb_mux, nb);
>> +
>> +	state = extcon_get_cable_state(mux->obj.edev,
>> +			mux->umdev->cable_name);
>> +
>> +	if (mux->mux_state == -1 || mux->mux_state != state) {
>> +		mutex_lock(&mux->mux_mutex);
>> +		ret = usb_mux_change_state(mux, state);
>> +		mutex_unlock(&mux->mux_mutex);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static ssize_t intel_mux_show(struct device *dev,
>> +		struct device_attribute *attr, char *buf)
>> +{
>> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
>> +
>> +	if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
>> +		return 0;
>> +
>> +	return sprintf(buf, "%s\n", mux->mux_state ? "host" : "peripheral");
>> +}
>> +
>> +static ssize_t intel_mux_store(struct device *dev,
>> +		struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
>> +	int state;
>> +
>> +	if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
>> +		return -EINVAL;
>> +
>> +	if (sysfs_streq(buf, "peripheral"))
>> +		state = 0;
>> +	else if (sysfs_streq(buf, "host"))
>> +		state = 1;
>> +	else
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&mux->mux_mutex);
>> +	usb_mux_change_state(mux, state);
>> +	mutex_unlock(&mux->mux_mutex);
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(intel_mux);
>> +
>> +int intel_usb_mux_register(struct intel_mux_dev *umdev)
>> +{
>> +	int ret;
>> +	struct device *dev = umdev->dev;
>> +	struct intel_usb_mux *mux;
>> +
>> +	if (!umdev->cable_name)
>> +		return -ENODEV;
>> +
>> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> +	if (!mux)
>> +		return -ENOMEM;
>> +
>> +	mux->umdev = umdev;
>> +	mux->nb.notifier_call = usb_mux_notifier;
>> +	mutex_init(&mux->mux_mutex);
>> +	mux->mux_state = -1;
>> +	dev_set_drvdata(dev, mux);
>> +	ret = extcon_register_interest(&mux->obj, umdev->extcon_name,
>> +			umdev->cable_name, &mux->nb);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register extcon notifier\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	usb_mux_notifier(&mux->nb, 0, NULL);
>> +
>> +	/* register the sysfs interface */
>> +	ret = device_create_file(dev, &dev_attr_intel_mux);
>> +	if (ret) {
>> +		dev_err(dev, "failed to create sysfs attribute\n");
>> +		if (umdev->cable_name)
>> +			extcon_unregister_interest(&mux->obj);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
>> +
>> +int intel_usb_mux_unregister(struct device *dev)
>> +{
>> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
>> +
>> +	device_remove_file(dev, &dev_attr_intel_mux);
>> +	extcon_unregister_interest(&mux->obj);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +void intel_usb_mux_complete(struct device *dev)
>> +{
>> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
>> +
>> +	usb_mux_notifier(&mux->nb, 0, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(intel_usb_mux_complete);
>> +#endif
>> diff --git a/include/linux/usb/intel-mux.h b/include/linux/usb/intel-mux.h
>> new file mode 100644
>> index 0000000..6990174
>> --- /dev/null
>> +++ b/include/linux/usb/intel-mux.h
>> @@ -0,0 +1,47 @@
>> +/**
>> + * mux.h - USB Port Mux defines
>> + *
>> + * Copyright (C) 2016 Intel Corporation
>> + *
>> + * Author: Lu Baolu <baolu.lu@linux.intel.com>
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef __LINUX_USB_INTEL_MUX_H
>> +#define __LINUX_USB_INTEL_MUX_H
>> +
>> +#include <linux/usb.h>
>> +
>> +struct intel_mux_dev {
>> +	struct device	*dev;
>> +	char		*extcon_name;
>> +	char		*cable_name;
>> +	int		(*cable_set_cb)(struct intel_mux_dev *mux);
>> +	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
>> +};
> This is a device, why not make it one?  Don't just hold a reference.
> And do you really even hold that reference?

It's not a device. It's just an encapsulation for parameters passed into
intel_usb_mux_register().

>
>> +#if IS_ENABLED(CONFIG_INTEL_USB_MUX)
>> +extern int intel_usb_mux_register(struct intel_mux_dev *mux);
>> +extern int intel_usb_mux_unregister(struct device *dev);
> It's obvious you didn't run this through checkpatch.pl, please do so...

I did, but didn't hit any errors or warnings.

$ ./scripts/checkpatch.pl v3-0003-usb-mux-add-common-code-for-Intel-dual-role-port-.patch
total: 0 errors, 0 warnings, 272 lines checked

v3-0003-usb-mux-add-common-code-for-Intel-dual-role-port-.patch has no obvious style problems and is ready for submission.

> And your api is horrid, think about what you want the "core" to do here,
> it should be the one creating the device and returning it to the caller,
> not forcing the caller to somehow create it first and then pass it in.

This isn't a layer or core. It doesn't create any new devices. It's actually
some shared code which can be used by all Intel dual role port drivers.

I put it in a separated file because 1) this can avoid duplication; 2) this code
could be used for any architectures as long as a USB port is shared by
two components and it needs an OS response when event triggers.

I guess intel_usb_mux_register/unregister() is a bit misleading. How about
changing them to intel_usb_mux_probe/remove()?

> And why is it not symmetrical, you are passing one thing into register
> and another into unregister.

struct intel_mux_dev is an encapsulation for parameters passed into
intel_usb_mux_register(). It's not a new device structure though the name
is a bit misleading. How about remove this structure and put these in
function parameters?

>
> Come on, I expect better work from Intel kernel developers, I shouldn't
> have to be complaining about basic crap like this...

I am sorry to disappoint you.

Best Regards,
Baolu

>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Mon, Mar 14, 2016 at 09:09:22AM +0800, Lu Baolu wrote:
> On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote:
> > On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
> >> +struct intel_mux_dev {
> >> +	struct device	*dev;
> >> +	char		*extcon_name;
> >> +	char		*cable_name;
> >> +	int		(*cable_set_cb)(struct intel_mux_dev *mux);
> >> +	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
> >> +};
> > This is a device, why not make it one?  Don't just hold a reference.
> > And do you really even hold that reference?
> 
> It's not a device. It's just an encapsulation for parameters passed into
> intel_usb_mux_register().

But you called it a device, so you can understand my confusion.

And why not make it a device?  Why isn't this one?  Hint, I really think
it should be...

> >> +#if IS_ENABLED(CONFIG_INTEL_USB_MUX)
> >> +extern int intel_usb_mux_register(struct intel_mux_dev *mux);
> >> +extern int intel_usb_mux_unregister(struct device *dev);
> > It's obvious you didn't run this through checkpatch.pl, please do so...
> 
> I did, but didn't hit any errors or warnings.

Odd, don't put extern in .h files for functions, I thought checkpatch
catches that...

Try it with --strict, as you should with all new code you submit.

> > And your api is horrid, think about what you want the "core" to do here,
> > it should be the one creating the device and returning it to the caller,
> > not forcing the caller to somehow create it first and then pass it in.
> 
> This isn't a layer or core. It doesn't create any new devices. It's actually
> some shared code which can be used by all Intel dual role port drivers.

It should be a device, as you are treating it like one :)

> I put it in a separated file because 1) this can avoid duplication; 2) this code
> could be used for any architectures as long as a USB port is shared by
> two components and it needs an OS response when event triggers.

It's a bit hard for other arches to be using something called "intel_"
:(

> I guess intel_usb_mux_register/unregister() is a bit misleading. How about
> changing them to intel_usb_mux_probe/remove()?

You are going to probe/remove something that isn't a device?  Come on
now...

> > And why is it not symmetrical, you are passing one thing into register
> > and another into unregister.
> 
> struct intel_mux_dev is an encapsulation for parameters passed into
> intel_usb_mux_register().

Which is a device.

> It's not a new device structure though the name
> is a bit misleading.

Yes it is, hint, you want it to be a device.

> How about remove this structure and put these in function parameters?

How about making it a real device? :)

thanks,

greg k-h

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Lu Baolu]

On 03/14/2016 11:27 AM, Greg Kroah-Hartman wrote:
> On Mon, Mar 14, 2016 at 09:09:22AM +0800, Lu Baolu wrote:
>> On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote:
>>> On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
>>>> +struct intel_mux_dev {
>>>> +	struct device	*dev;
>>>> +	char		*extcon_name;
>>>> +	char		*cable_name;
>>>> +	int		(*cable_set_cb)(struct intel_mux_dev *mux);
>>>> +	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
>>>> +};
>>> This is a device, why not make it one?  Don't just hold a reference.
>>> And do you really even hold that reference?
>> It's not a device. It's just an encapsulation for parameters passed into
>> intel_usb_mux_register().
> But you called it a device, so you can understand my confusion.
>
> And why not make it a device?  Why isn't this one?  Hint, I really think
> it should be...

I am sorry for the confusion.

The mux device has already been created. It could be a child of a mfd device,
or be created explicitly as a platform device. The mux driver (for example,
intel-mux-gpio.ko) will bind to the device. intel-mux-gpio.ko will then call into this
shared code. This shared code is actually part of the mux driver, except that it
could also be used in other mux driver (for example, intel-mux-drcfg.ko).

I can't always expect the mux device to be created with intel_usb_mux_register()
since it could possibly be a cell of a mfd, or just an ACPI device. :-)

>
>>>> +#if IS_ENABLED(CONFIG_INTEL_USB_MUX)
>>>> +extern int intel_usb_mux_register(struct intel_mux_dev *mux);
>>>> +extern int intel_usb_mux_unregister(struct device *dev);
>>> It's obvious you didn't run this through checkpatch.pl, please do so...
>> I did, but didn't hit any errors or warnings.
> Odd, don't put extern in .h files for functions, I thought checkpatch
> catches that...
>
> Try it with --strict, as you should with all new code you submit.

Yes, if I add --strict, I hit some warnings.

I will fix these warnings and always run checkpatch.pl with --strict
before I submit patches.

Thank you!

>
>>> And your api is horrid, think about what you want the "core" to do here,
>>> it should be the one creating the device and returning it to the caller,
>>> not forcing the caller to somehow create it first and then pass it in.
>> This isn't a layer or core. It doesn't create any new devices. It's actually
>> some shared code which can be used by all Intel dual role port drivers.
> It should be a device, as you are treating it like one :)

I have answered above.

>
>> I put it in a separated file because 1) this can avoid duplication; 2) this code
>> could be used for any architectures as long as a USB port is shared by
>> two components and it needs an OS response when event triggers.
> It's a bit hard for other arches to be using something called "intel_"
> :(

Do you mind if I change the symbols to something like "usb_mux_"?
Or, keep it Intel specific now and change it when there is real other
consumers later?

>> I guess intel_usb_mux_register/unregister() is a bit misleading. How about
>> changing them to intel_usb_mux_probe/remove()?
> You are going to probe/remove something that isn't a device?  Come on
> now...
>
>>> And why is it not symmetrical, you are passing one thing into register
>>> and another into unregister.
>> struct intel_mux_dev is an encapsulation for parameters passed into
>> intel_usb_mux_register().
> Which is a device.
>
>> It's not a new device structure though the name
>> is a bit misleading.
> Yes it is, hint, you want it to be a device.
>
>> How about remove this structure and put these in function parameters?
> How about making it a real device? :)

The mux is a real device. :-)   As my understanding (please correct me if I
misunderstood it), the question is that should 1) the device be created before
passing it to intel_usb_mux_register(), or 2) let intel_usb_mux_register() create
the device.

IMHO, option 2) is not possible for Intel platform. The port mux in Intel device
could be an ACPI device or part of an ACPI device. Hence, the mux device could
be created implicitly by ACPI or mfd framework.

ps. this function name is really confusing. It isn't designed to register a new
mux device, but an interface for a shared common code. Sorry about it and
I will look for a meaningful one for the next version.

Best Regards,
Baolu

>
> thanks,
>
> greg k-h
>

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Lu Baolu]

Hi,

On 03/14/2016 11:27 AM, Greg Kroah-Hartman wrote:
> On Mon, Mar 14, 2016 at 09:09:22AM +0800, Lu Baolu wrote:
>> On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote:
>>> On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
>>>> +struct intel_mux_dev {
>>>> +	struct device	*dev;
>>>> +	char		*extcon_name;
>>>> +	char		*cable_name;
>>>> +	int		(*cable_set_cb)(struct intel_mux_dev *mux);
>>>> +	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
>>>> +};
>>> This is a device, why not make it one?  Don't just hold a reference.
>>> And do you really even hold that reference?
>> It's not a device. It's just an encapsulation for parameters passed into
>> intel_usb_mux_register().
> But you called it a device, so you can understand my confusion.
>
> And why not make it a device?  Why isn't this one?  Hint, I really think
> it should be...
>
>>> And your api is horrid, think about what you want the "core" to do here,
>>> it should be the one creating the device and returning it to the caller,
>>> not forcing the caller to somehow create it first and then pass it in.
>> This isn't a layer or core. It doesn't create any new devices. It's actually
>> some shared code which can be used by all Intel dual role port drivers.
> It should be a device, as you are treating it like one :)
>
>> I put it in a separated file because 1) this can avoid duplication; 2) this code
>> could be used for any architectures as long as a USB port is shared by
>> two components and it needs an OS response when event triggers.
> It's a bit hard for other arches to be using something called "intel_"
> :(

Are there any other implementations which need an external mux
to swap the usb roles?

>> I guess intel_usb_mux_register/unregister() is a bit misleading. How about
>> changing them to intel_usb_mux_probe/remove()?
> You are going to probe/remove something that isn't a device?  Come on
> now...
>
>>> And why is it not symmetrical, you are passing one thing into register
>>> and another into unregister.
>> struct intel_mux_dev is an encapsulation for parameters passed into
>> intel_usb_mux_register().
> Which is a device.
>
>> It's not a new device structure though the name
>> is a bit misleading.
> Yes it is, hint, you want it to be a device.
>
>> How about remove this structure and put these in function parameters?
> How about making it a real device? :)

Hi Greg,

Just want to make myself clear about your expectation.
Did you mean to create a port mux device and return it to the caller?

The interfaces look like:

struct portmux_dev *devm_portmux_register(struct device *dev,
                                  const struct portmux_desc *desc);

void devm_portmux_unregister(struct device *dev,
                                   struct portmux_dev *pdev)

Do I get it right?

Best regards,
Baolu

>
> thanks,
>
> greg k-h
>

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Wed, Apr 06, 2016 at 02:44:55PM +0800, Lu Baolu wrote:
> Hi,
> 
> On 03/14/2016 11:27 AM, Greg Kroah-Hartman wrote:
> > On Mon, Mar 14, 2016 at 09:09:22AM +0800, Lu Baolu wrote:
> >> On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote:
> >>> On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
> >>>> +struct intel_mux_dev {
> >>>> +	struct device	*dev;
> >>>> +	char		*extcon_name;
> >>>> +	char		*cable_name;
> >>>> +	int		(*cable_set_cb)(struct intel_mux_dev *mux);
> >>>> +	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
> >>>> +};
> >>> This is a device, why not make it one?  Don't just hold a reference.
> >>> And do you really even hold that reference?
> >> It's not a device. It's just an encapsulation for parameters passed into
> >> intel_usb_mux_register().
> > But you called it a device, so you can understand my confusion.
> >
> > And why not make it a device?  Why isn't this one?  Hint, I really think
> > it should be...
> >
> >>> And your api is horrid, think about what you want the "core" to do here,
> >>> it should be the one creating the device and returning it to the caller,
> >>> not forcing the caller to somehow create it first and then pass it in.
> >> This isn't a layer or core. It doesn't create any new devices. It's actually
> >> some shared code which can be used by all Intel dual role port drivers.
> > It should be a device, as you are treating it like one :)
> >
> >> I put it in a separated file because 1) this can avoid duplication; 2) this code
> >> could be used for any architectures as long as a USB port is shared by
> >> two components and it needs an OS response when event triggers.
> > It's a bit hard for other arches to be using something called "intel_"
> > :(
> 
> Are there any other implementations which need an external mux
> to swap the usb roles?

Why wouldn't there be?  :)

> >> I guess intel_usb_mux_register/unregister() is a bit misleading. How about
> >> changing them to intel_usb_mux_probe/remove()?
> > You are going to probe/remove something that isn't a device?  Come on
> > now...
> >
> >>> And why is it not symmetrical, you are passing one thing into register
> >>> and another into unregister.
> >> struct intel_mux_dev is an encapsulation for parameters passed into
> >> intel_usb_mux_register().
> > Which is a device.
> >
> >> It's not a new device structure though the name
> >> is a bit misleading.
> > Yes it is, hint, you want it to be a device.
> >
> >> How about remove this structure and put these in function parameters?
> > How about making it a real device? :)
> 
> Hi Greg,
> 
> Just want to make myself clear about your expectation.
> Did you mean to create a port mux device and return it to the caller?

Yes, that's the correct way to do this type of thing.

> The interfaces look like:
> 
> struct portmux_dev *devm_portmux_register(struct device *dev,
>                                   const struct portmux_desc *desc);
> 
> void devm_portmux_unregister(struct device *dev,
>                                    struct portmux_dev *pdev)
> 
> Do I get it right?

Why devm?  Your lifetime rules don't allow that at all.

thanks,

greg k-h

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3195 bytes --]


Hi,

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Wed, Apr 06, 2016 at 02:44:55PM +0800, Lu Baolu wrote:
>> Hi,
>> 
>> On 03/14/2016 11:27 AM, Greg Kroah-Hartman wrote:
>> > On Mon, Mar 14, 2016 at 09:09:22AM +0800, Lu Baolu wrote:
>> >> On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote:
>> >>> On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
>> >>>> +struct intel_mux_dev {
>> >>>> +	struct device	*dev;
>> >>>> +	char		*extcon_name;
>> >>>> +	char		*cable_name;
>> >>>> +	int		(*cable_set_cb)(struct intel_mux_dev *mux);
>> >>>> +	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
>> >>>> +};
>> >>> This is a device, why not make it one?  Don't just hold a reference.
>> >>> And do you really even hold that reference?
>> >> It's not a device. It's just an encapsulation for parameters passed into
>> >> intel_usb_mux_register().
>> > But you called it a device, so you can understand my confusion.
>> >
>> > And why not make it a device?  Why isn't this one?  Hint, I really think
>> > it should be...
>> >
>> >>> And your api is horrid, think about what you want the "core" to do here,
>> >>> it should be the one creating the device and returning it to the caller,
>> >>> not forcing the caller to somehow create it first and then pass it in.
>> >> This isn't a layer or core. It doesn't create any new devices. It's actually
>> >> some shared code which can be used by all Intel dual role port drivers.
>> > It should be a device, as you are treating it like one :)
>> >
>> >> I put it in a separated file because 1) this can avoid duplication; 2) this code
>> >> could be used for any architectures as long as a USB port is shared by
>> >> two components and it needs an OS response when event triggers.
>> > It's a bit hard for other arches to be using something called "intel_"
>> > :(
>> 
>> Are there any other implementations which need an external mux
>> to swap the usb roles?
>
> Why wouldn't there be?  :)

Well, e.g. dwc3 can be configured with an optional dual-role  block
which handles role swapping. It's not the same as intel mux. What Intel
did is that we have completely separate xHCI (Intel's own) and dwc3
(licensed from SNPS) IPs and another physical mux (not discrete, though)
which controls where the data lines should go (it also tells dwc3 about
voltage levels, etc).

What I'm trying to say is that there are other means for implementing
this sort of solution. Also, I still subscribe to Dave B's "3-user
requirement": if there are 3 or more users for something, then it really
deserves some deeper thoughts on how it needs to be abstracted.

There is, however, the detail that we're exposing a sysfs ABI, so we
shouldn't carelessly choose a name to the sysfs file; in any case, if
you know of any other similar dual-role implementations, that would help
guiding Baolu's implementation, right ? ;-)

>> void devm_portmux_unregister(struct device *dev,
>>                                    struct portmux_dev *pdev)
>> 
>> Do I get it right?
>
> Why devm?  Your lifetime rules don't allow that at all.

I agree here, devm doesn't make a lot of sense.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Sergei Shtylyov]

Hello.

On 4/6/2016 9:44 AM, Lu Baolu wrote:

>>>>> +struct intel_mux_dev {
>>>>> +	struct device	*dev;
>>>>> +	char		*extcon_name;
>>>>> +	char		*cable_name;
>>>>> +	int		(*cable_set_cb)(struct intel_mux_dev *mux);
>>>>> +	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
>>>>> +};
>>>> This is a device, why not make it one?  Don't just hold a reference.
>>>> And do you really even hold that reference?
>>> It's not a device. It's just an encapsulation for parameters passed into
>>> intel_usb_mux_register().
>> But you called it a device, so you can understand my confusion.
>>
>> And why not make it a device?  Why isn't this one?  Hint, I really think
>> it should be...
>>
>>>> And your api is horrid, think about what you want the "core" to do here,
>>>> it should be the one creating the device and returning it to the caller,
>>>> not forcing the caller to somehow create it first and then pass it in.
>>> This isn't a layer or core. It doesn't create any new devices. It's actually
>>> some shared code which can be used by all Intel dual role port drivers.
>> It should be a device, as you are treating it like one :)
>>
>>> I put it in a separated file because 1) this can avoid duplication; 2) this code
>>> could be used for any architectures as long as a USB port is shared by
>>> two components and it needs an OS response when event triggers.
>> It's a bit hard for other arches to be using something called "intel_"
>> :(
>
> Are there any other implementations which need an external mux
> to swap the usb roles?

    There are, of course, like Renesas R-Car gen2 SoCs. I've implemented the 
USB port multiplexing as a part of the PHY driver (see 
drivers/phy/phy-rcar-gen2.c).

MBR, Sergei

[PATCH v3 4/7] usb: mux: add driver for Intel gpio controlled port mux

[Lu Baolu]

In some Intel platforms, a single usb port is shared between USB host
and device controller. The shared port is under control of GPIO pins.

This patch adds the support for USB GPIO controlled port mux.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 MAINTAINERS                      |   1 +
 drivers/usb/mux/Kconfig          |   9 +++
 drivers/usb/mux/Makefile         |   1 +
 drivers/usb/mux/intel-mux-gpio.c | 125 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+)
 create mode 100644 drivers/usb/mux/intel-mux-gpio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a93d26a..ab876eb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11406,6 +11406,7 @@ L:	linux-usb@vger.kernel.org
 S:	Supported
 F:	drivers/usb/mux/intel-mux.c
 F:	include/linux/usb/intel-mux.h
+F:	drivers/usb/mux/intel-mux-gpio.c
 
 USB PRINTER DRIVER (usblp)
 M:	Pete Zaitcev <zaitcev@redhat.com>
diff --git a/drivers/usb/mux/Kconfig b/drivers/usb/mux/Kconfig
index 8fedc4f..060bf5c 100644
--- a/drivers/usb/mux/Kconfig
+++ b/drivers/usb/mux/Kconfig
@@ -6,4 +6,13 @@ config INTEL_USB_MUX
 	select EXTCON
 	def_bool n
 
+config INTEL_MUX_GPIO
+	tristate "Intel dual role port mux controlled by GPIOs"
+	depends on GPIOLIB
+	depends on X86 && ACPI
+	select INTEL_USB_MUX
+	help
+	  Say Y here to enable support for Intel dual role port mux
+	  controlled by GPIOs.
+
 endmenu
diff --git a/drivers/usb/mux/Makefile b/drivers/usb/mux/Makefile
index 84f0ae8..66f765c 100644
--- a/drivers/usb/mux/Makefile
+++ b/drivers/usb/mux/Makefile
@@ -2,3 +2,4 @@
 # Makefile for USB port mux drivers
 #
 obj-$(CONFIG_INTEL_USB_MUX)		+= intel-mux.o
+obj-$(CONFIG_INTEL_MUX_GPIO)		+= intel-mux-gpio.o
diff --git a/drivers/usb/mux/intel-mux-gpio.c b/drivers/usb/mux/intel-mux-gpio.c
new file mode 100644
index 0000000..adcb496
--- /dev/null
+++ b/drivers/usb/mux/intel-mux-gpio.c
@@ -0,0 +1,125 @@
+/*
+ * USB Dual Role Port Mux driver controlled by gpios
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ * Author: David Cohen <david.a.cohen@linux.intel.com>
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ *
+ * 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/platform_device.h>
+#include <linux/usb/intel-mux.h>
+
+struct vuport {
+	struct intel_mux_dev umdev;
+	struct gpio_desc *gpio_vbus_en;
+	struct gpio_desc *gpio_usb_mux;
+};
+
+/*
+ * id == 0, HOST connected, USB port should be set to peripheral
+ * id == 1, HOST disconnected, USB port should be set to host
+ *
+ * Peripheral: set USB mux to peripheral and disable VBUS
+ * Host: set USB mux to host and enable VBUS
+ */
+static inline int vuport_set_port(struct intel_mux_dev *umdev, int id)
+{
+	struct vuport *vup = container_of(umdev, struct vuport, umdev);
+
+	dev_dbg(umdev->dev, "USB PORT ID: %s\n", id ? "HOST" : "PERIPHERAL");
+
+	gpiod_set_value_cansleep(vup->gpio_usb_mux, !id);
+	gpiod_set_value_cansleep(vup->gpio_vbus_en, id);
+
+	return 0;
+}
+
+static int vuport_cable_set(struct intel_mux_dev *umdev)
+{
+	return vuport_set_port(umdev, 1);
+}
+
+static int vuport_cable_unset(struct intel_mux_dev *umdev)
+{
+	return vuport_set_port(umdev, 0);
+}
+
+static int vuport_probe(struct platform_device *pdev)
+{
+	struct intel_mux_dev *umdev;
+	struct device *dev = &pdev->dev;
+	struct vuport *vup;
+
+	vup = devm_kzalloc(dev, sizeof(*vup), GFP_KERNEL);
+	if (!vup)
+		return -ENOMEM;
+
+	/* retrieve vbus and mux gpios */
+	vup->gpio_vbus_en = devm_gpiod_get_optional(dev,
+				"vbus_en", GPIOD_ASIS);
+	if (IS_ERR(vup->gpio_vbus_en))
+		return PTR_ERR(vup->gpio_vbus_en);
+
+	vup->gpio_usb_mux = devm_gpiod_get_optional(dev,
+				"usb_mux", GPIOD_ASIS);
+	if (IS_ERR(vup->gpio_usb_mux))
+		return PTR_ERR(vup->gpio_usb_mux);
+
+	/* populate the mux generic structure */
+	umdev = &vup->umdev;
+	umdev->dev = dev;
+	umdev->cable_name = "USB-HOST";
+	umdev->cable_set_cb = vuport_cable_set;
+	umdev->cable_unset_cb = vuport_cable_unset;
+
+	return intel_usb_mux_register(umdev);
+}
+
+static int vuport_remove(struct platform_device *pdev)
+{
+	return intel_usb_mux_unregister(&pdev->dev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+/*
+ * In case a micro A cable was plugged in while device was sleeping,
+ * we missed the interrupt. We need to poll usb id gpio when waking the
+ * driver to detect the missed event.
+ * We use 'complete' callback to give time to all extcon listeners to
+ * resume before we send new events.
+ */
+static const struct dev_pm_ops vuport_pm_ops = {
+	.complete = intel_usb_mux_complete,
+};
+#endif
+
+static const struct platform_device_id vuport_platform_ids[] = {
+	{ .name = "intel-mux-gpio", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, vuport_platform_ids);
+
+static struct platform_driver vuport_driver = {
+	.driver = {
+		.name = "intel-mux-gpio",
+#ifdef CONFIG_PM_SLEEP
+		.pm = &vuport_pm_ops,
+#endif
+	},
+	.probe = vuport_probe,
+	.remove = vuport_remove,
+	.id_table = vuport_platform_ids,
+};
+
+module_platform_driver(vuport_driver);
+
+MODULE_AUTHOR("David Cohen <david.a.cohen@linux.intel.com>");
+MODULE_AUTHOR("Lu Baolu <baolu.lu@linux.intel.com>");
+MODULE_DESCRIPTION("Intel USB gpio mux driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

[PATCH v3 5/7] usb: mux: add driver for Intel drcfg controlled port mux

[Lu Baolu]

Several Intel PCHs and SOCs have an internal mux that is used to
share one USB port between device controller and host controller.
The mux is handled through the Dual Role Configuration Register.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Reviewed-by: Felipe Balbi <balbi@kernel.org>
---
 MAINTAINERS                       |   1 +
 drivers/usb/mux/Kconfig           |   7 ++
 drivers/usb/mux/Makefile          |   1 +
 drivers/usb/mux/intel-mux-drcfg.c | 161 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 170 insertions(+)
 create mode 100644 drivers/usb/mux/intel-mux-drcfg.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ab876eb..399cefe 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11407,6 +11407,7 @@ S:	Supported
 F:	drivers/usb/mux/intel-mux.c
 F:	include/linux/usb/intel-mux.h
 F:	drivers/usb/mux/intel-mux-gpio.c
+F:	drivers/usb/mux/intel-mux-drcfg.c
 
 USB PRINTER DRIVER (usblp)
 M:	Pete Zaitcev <zaitcev@redhat.com>
diff --git a/drivers/usb/mux/Kconfig b/drivers/usb/mux/Kconfig
index 060bf5c..775d5da 100644
--- a/drivers/usb/mux/Kconfig
+++ b/drivers/usb/mux/Kconfig
@@ -15,4 +15,11 @@ config INTEL_MUX_GPIO
 	  Say Y here to enable support for Intel dual role port mux
 	  controlled by GPIOs.
 
+config INTEL_MUX_DRCFG
+	tristate "Intel dual role port mux controlled by register"
+	depends on X86
+	select INTEL_USB_MUX
+	help
+	  Say Y here to enable support for Intel dual role port mux
+	  controlled by the Dual Role Configuration Register.
 endmenu
diff --git a/drivers/usb/mux/Makefile b/drivers/usb/mux/Makefile
index 66f765c..3179909 100644
--- a/drivers/usb/mux/Makefile
+++ b/drivers/usb/mux/Makefile
@@ -3,3 +3,4 @@
 #
 obj-$(CONFIG_INTEL_USB_MUX)		+= intel-mux.o
 obj-$(CONFIG_INTEL_MUX_GPIO)		+= intel-mux-gpio.o
+obj-$(CONFIG_INTEL_MUX_DRCFG)		+= intel-mux-drcfg.o
diff --git a/drivers/usb/mux/intel-mux-drcfg.c b/drivers/usb/mux/intel-mux-drcfg.c
new file mode 100644
index 0000000..11db826
--- /dev/null
+++ b/drivers/usb/mux/intel-mux-drcfg.c
@@ -0,0 +1,161 @@
+/**
+ * intel-mux-drcfg.c - Driver for Intel USB mux via register
+ *
+ * Copyright (C) 2016 Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ *
+ * 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/slab.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/usb/intel-mux.h>
+#include <linux/platform_device.h>
+
+#define INTEL_MUX_CFG0		0x00
+#define INTEL_MUX_CFG1		0x04
+#define CFG0_SW_IDPIN		BIT(20)
+#define CFG0_SW_IDPIN_EN	BIT(21)
+#define CFG0_SW_VBUS_VALID	BIT(24)
+#define CFG1_SW_MODE		BIT(29)
+#define CFG1_POLL_TIMEOUT	1000
+
+struct intel_usb_mux {
+	struct intel_mux_dev umdev;
+	void __iomem *regs;
+};
+
+static inline int intel_mux_drcfg_switch(struct intel_mux_dev *umdev, bool host)
+{
+	struct intel_usb_mux *mux;
+	unsigned long timeout;
+	u32 data;
+
+	mux = container_of(umdev, struct intel_usb_mux, umdev);
+
+	/* Check and set mux to SW controlled mode */
+	data = readl(mux->regs + INTEL_MUX_CFG0);
+	if (!(data & CFG0_SW_IDPIN_EN)) {
+		data |= CFG0_SW_IDPIN_EN;
+		writel(data, mux->regs + INTEL_MUX_CFG0);
+	}
+
+	/*
+	 * Configure CFG0 to switch the mux and VBUS_VALID bit is
+	 * required for device mode.
+	 */
+	data = readl(mux->regs + INTEL_MUX_CFG0);
+	if (host)
+		data &= ~(CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID);
+	else
+		data |= (CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID);
+	writel(data, mux->regs + INTEL_MUX_CFG0);
+
+	/*
+	 * Polling CFG1 for safety, most case it takes about 600ms
+	 * to finish mode switching, set TIMEOUT long enough.
+	 */
+	timeout = jiffies + msecs_to_jiffies(CFG1_POLL_TIMEOUT);
+
+	/* Polling on CFG1 register to confirm mode switch. */
+	while (!time_after(jiffies, timeout)) {
+		data = readl(mux->regs + INTEL_MUX_CFG1);
+		if (!(host ^ (data & CFG1_SW_MODE)))
+			return 0;
+		/* interval for polling is set to about 5ms */
+		usleep_range(5000, 5100);
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int intel_mux_drcfg_cable_set(struct intel_mux_dev *umdev)
+{
+	dev_dbg(umdev->dev, "drcfg mux switch to HOST\n");
+
+	return intel_mux_drcfg_switch(umdev, true);
+}
+
+static int intel_mux_drcfg_cable_unset(struct intel_mux_dev *umdev)
+{
+	dev_dbg(umdev->dev, "drcfg mux switch to DEVICE\n");
+
+	return intel_mux_drcfg_switch(umdev, false);
+}
+
+static int intel_mux_drcfg_probe(struct platform_device *pdev)
+{
+	struct intel_usb_mux *mux;
+	struct intel_mux_dev *umdev;
+	struct device *dev = &pdev->dev;
+	u64 start, size;
+	int ret;
+
+	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return -ENOMEM;
+
+	ret = device_property_read_u64(dev, "reg-start", &start);
+	ret |= device_property_read_u64(dev, "reg-size", &size);
+	if (ret)
+		return -ENODEV;
+
+	mux->regs = devm_ioremap_nocache(dev, start, size);
+	if (!mux->regs)
+		return -ENOMEM;
+
+	umdev = &mux->umdev;
+	umdev->dev = dev;
+	umdev->cable_name = "USB-HOST";
+	umdev->cable_set_cb = intel_mux_drcfg_cable_set;
+	umdev->cable_unset_cb = intel_mux_drcfg_cable_unset;
+
+	return intel_usb_mux_register(umdev);
+}
+
+static int intel_mux_drcfg_remove(struct platform_device *pdev)
+{
+	return intel_usb_mux_unregister(&pdev->dev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+/*
+ * In case a micro A cable was plugged in while device was sleeping,
+ * we missed the interrupt. We need to poll usb id state when waking
+ * the driver to detect the missed event.
+ * We use 'complete' callback to give time to all extcon listeners to
+ * resume before we send new events.
+ */
+static const struct dev_pm_ops intel_mux_drcfg_pm_ops = {
+	.complete = intel_usb_mux_complete,
+};
+#endif
+
+static const struct platform_device_id intel_mux_drcfg_platform_ids[] = {
+	{ .name = "intel-mux-drcfg", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, intel_mux_drcfg_platform_ids);
+
+static struct platform_driver intel_mux_drcfg_driver = {
+	.probe		= intel_mux_drcfg_probe,
+	.remove		= intel_mux_drcfg_remove,
+	.driver		= {
+		.name	= "intel-mux-drcfg",
+#ifdef CONFIG_PM_SLEEP
+		.pm	= &intel_mux_drcfg_pm_ops,
+#endif
+	},
+	.id_table = intel_mux_drcfg_platform_ids,
+};
+
+module_platform_driver(intel_mux_drcfg_driver);
+
+MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
+MODULE_AUTHOR("Lu Baolu <baolu.lu@linux.intel.com>");
+MODULE_DESCRIPTION("Intel USB drcfg mux driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

[PATCH v3 6/7] usb: pci-quirks: add Intel USB drcfg mux device

[Lu Baolu]

In some Intel platforms, a single usb port is shared between USB host
and device controllers. The shared port is under control of a switch
which is defined in the Intel vendor defined extended capability for
xHCI.

This patch adds the support to detect and create the platform device
for the port mux switch.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Felipe Balbi <balbi@kernel.org>
---
 drivers/usb/host/pci-quirks.c    | 47 ++++++++++++++++++++++++++++++++++++++--
 drivers/usb/host/xhci-ext-caps.h |  2 ++
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 35af362..6a737cf 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -16,10 +16,11 @@
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/platform_device.h>
+
 #include "pci-quirks.h"
 #include "xhci-ext-caps.h"
 
-
 #define UHCI_USBLEGSUP		0xc0		/* legacy support */
 #define UHCI_USBCMD		0		/* command register */
 #define UHCI_USBINTR		4		/* interrupt register */
@@ -78,6 +79,9 @@
 #define USB_INTEL_USB3_PSSEN   0xD8
 #define USB_INTEL_USB3PRM      0xDC
 
+#define DEVICE_ID_INTEL_CHERRYVIEW_XHCI		0x22b5
+#define DEVICE_ID_INTEL_BROXTON_M_XHCI		0x0aa8
+
 /*
  * amd_chipset_gen values represent AMD different chipset generations
  */
@@ -956,6 +960,41 @@ void usb_disable_xhci_ports(struct pci_dev *xhci_pdev)
 }
 EXPORT_SYMBOL_GPL(usb_disable_xhci_ports);
 
+static void create_intel_usb_mux_device(struct pci_dev *xhci_pdev,
+			void __iomem *base)
+{
+	struct platform_device *plat_dev;
+	struct property_set pset;
+	int ret;
+
+	struct property_entry pentry[] = {
+		PROPERTY_ENTRY_U64("reg-start",
+				pci_resource_start(xhci_pdev, 0) + 0x80d8),
+		PROPERTY_ENTRY_U64("reg-size", 8),
+		{ },
+	};
+
+	if (!xhci_find_next_ext_cap(base, 0, XHCI_EXT_CAPS_INTEL_USB_MUX))
+		return;
+
+	plat_dev = platform_device_alloc("intel-mux-drcfg",
+					PLATFORM_DEVID_AUTO);
+	if (!plat_dev)
+		return;
+
+	plat_dev->dev.parent = &xhci_pdev->dev;
+	pset.properties = pentry;
+	platform_device_add_properties(plat_dev, &pset);
+
+	ret = platform_device_add(plat_dev);
+	if (ret) {
+		dev_warn(&xhci_pdev->dev,
+				"failed to create mux device with error %d",
+				ret);
+		platform_device_put(plat_dev);
+	}
+}
+
 /**
  * PCI Quirks for xHCI.
  *
@@ -1022,8 +1061,12 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	writel(val, base + ext_cap_offset + XHCI_LEGACY_CONTROL_OFFSET);
 
 hc_init:
-	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
 		usb_enable_intel_xhci_ports(pdev);
+		if (pdev->device == DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
+				pdev->device == DEVICE_ID_INTEL_BROXTON_M_XHCI)
+			create_intel_usb_mux_device(pdev, base);
+	}
 
 	op_reg_base = base + XHCI_HC_LENGTH(readl(base));
 
diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci-ext-caps.h
index e0244fb..e368ccb 100644
--- a/drivers/usb/host/xhci-ext-caps.h
+++ b/drivers/usb/host/xhci-ext-caps.h
@@ -51,6 +51,8 @@
 #define XHCI_EXT_CAPS_ROUTE	5
 /* IDs 6-9 reserved */
 #define XHCI_EXT_CAPS_DEBUG	10
+/* Vendor defined 192-255 */
+#define XHCI_EXT_CAPS_INTEL_USB_MUX	192
 /* USB Legacy Support Capability - section 7.1.1 */
 #define XHCI_HC_BIOS_OWNED	(1 << 16)
 #define XHCI_HC_OS_OWNED	(1 << 24)
-- 
2.1.4

[PATCH v3 7/7] mfd: intel_vuport: Add Intel virtual USB port MFD Driver

[Lu Baolu]

Some Intel platforms have an USB port mux controlled by GPIOs.
There's a single ACPI platform device that provides both USB ID
extcon device and a USB port mux device. This MFD driver will
split the 2 devices for their respective drivers.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Suggested-by: David Cohen <david.a.cohen@linux.intel.com>
Reviewed-by: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 MAINTAINERS                |  6 ++++
 drivers/mfd/Kconfig        |  8 +++++
 drivers/mfd/Makefile       |  1 +
 drivers/mfd/intel-vuport.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+)
 create mode 100644 drivers/mfd/intel-vuport.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 399cefe..1486c80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5599,6 +5599,12 @@ L:	linux-pm@vger.kernel.org
 S:	Supported
 F:	drivers/cpufreq/intel_pstate.c
 
+INTEL VIRTUAL USB PORT DRIVER
+M:	Lu Baolu <baolu.lu@linux.intel.com>
+L:	linux-usb@vger.kernel.org
+S:	Supported
+F:	drivers/mfd/intel-vuport.c
+
 INTEL FRAMEBUFFER DRIVER (excluding 810 and 815)
 M:	Maik Broemme <mbroemme@plusserver.de>
 L:	linux-fbdev@vger.kernel.org
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 9ca66de..48933d4 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1534,5 +1534,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
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 0f230a6..0ccd107 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -198,3 +198,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
diff --git a/drivers/mfd/intel-vuport.c b/drivers/mfd/intel-vuport.c
new file mode 100644
index 0000000..a07920f
--- /dev/null
+++ b/drivers/mfd/intel-vuport.c
@@ -0,0 +1,74 @@
+/*
+ * MFD driver for Intel virtual USB port
+ *
+ * Copyright(c) 2016 Intel Corporation.
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ *
+ * 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/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 },
+	{ "vbus_en-gpios", &vbus_gpio, 1 },
+	{ "usb_mux-gpios", &mux_gpio, 1 },
+	{ },
+};
+
+static const struct mfd_cell intel_vuport_mfd_cells[] = {
+	{ .name = "extcon-usb-gpio", },
+	{ .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, 0, 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 <baolu.lu@linux.intel.com>");
+MODULE_DESCRIPTION("Intel virtual USB port");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [PATCH v3 0/7] usb: add support for Intel dual role port mux

[Greg Kroah-Hartman]

On Tue, Mar 08, 2016 at 03:53:41PM +0800, Lu Baolu wrote:
> Intel SOC chips are featured with USB dual role. The host role is
> provided by Intel xHCI IP, and the gadget role is provided by IP
> from designware. Tablet platform designs always share a single
> port for both host and gadget controllers. There is a mux to
> switch the port to the right controller according to the cable
> type. OS needs to provide the callback to control the mux when
> a plug-in event raises. The method to control the mux is platform
> dependent. At least three types of implementation can be found
> across current devices. 1) GPIO pins; 2) a unit which can be
> controlled by memory mapped registers; 3) ACPI ASL code.
> 
> This patch series adds supports for Intel dual role port mux.
> It includes:
> (1) A helper layer on top of extcon for individual mux driver.
>     It listens to the USB-HOST extcon cable and call the switch
>     call-back when the cable state changes.
> (2) Drivers for GPIO controlled port mux which could be found on
>     Baytrail devices. A mfd driver is used to split the GPIOs into
>     USB gpio extcon device and a USB mux device. Driver for USB
>     gpio extcon device is already in upstream Linux. This patch
>     series includes a driver for GPIO USB mux.
> (3) Drivers for USB port mux controlled through memory mapped
>     registers and the logic to create the mux device. This type
>     of dual role port mux could be found in Cherry Trail and
>     Broxton devices.
> 
> Lu Baolu (7):
>   extcon: usb-gpio: add device binding for platform device
>   extcon: usb-gpio: add support for ACPI gpio interface
>   usb: mux: add common code for Intel dual role port mux
>   usb: mux: add driver for Intel gpio controlled port mux
>   usb: mux: add driver for Intel drcfg controlled port mux
>   usb: pci-quirks: add Intel USB drcfg mux device
>   mfd: intel_vuport: Add Intel virtual USB port MFD Driver

You don't describe here what you changed from v2, or even from v1.
Please always be specific, we don't remember what your last submissions
looked like at all...

thanks,

greg k-h

Re: [PATCH v3 0/7] usb: add support for Intel dual role port mux

[Lu Baolu]

On 03/11/2016 08:03 AM, Greg Kroah-Hartman wrote:
> On Tue, Mar 08, 2016 at 03:53:41PM +0800, Lu Baolu wrote:
>> Intel SOC chips are featured with USB dual role. The host role is
>> provided by Intel xHCI IP, and the gadget role is provided by IP
>> from designware. Tablet platform designs always share a single
>> port for both host and gadget controllers. There is a mux to
>> switch the port to the right controller according to the cable
>> type. OS needs to provide the callback to control the mux when
>> a plug-in event raises. The method to control the mux is platform
>> dependent. At least three types of implementation can be found
>> across current devices. 1) GPIO pins; 2) a unit which can be
>> controlled by memory mapped registers; 3) ACPI ASL code.
>>
>> This patch series adds supports for Intel dual role port mux.
>> It includes:
>> (1) A helper layer on top of extcon for individual mux driver.
>>     It listens to the USB-HOST extcon cable and call the switch
>>     call-back when the cable state changes.
>> (2) Drivers for GPIO controlled port mux which could be found on
>>     Baytrail devices. A mfd driver is used to split the GPIOs into
>>     USB gpio extcon device and a USB mux device. Driver for USB
>>     gpio extcon device is already in upstream Linux. This patch
>>     series includes a driver for GPIO USB mux.
>> (3) Drivers for USB port mux controlled through memory mapped
>>     registers and the logic to create the mux device. This type
>>     of dual role port mux could be found in Cherry Trail and
>>     Broxton devices.
>>
>> Lu Baolu (7):
>>   extcon: usb-gpio: add device binding for platform device
>>   extcon: usb-gpio: add support for ACPI gpio interface
>>   usb: mux: add common code for Intel dual role port mux
>>   usb: mux: add driver for Intel gpio controlled port mux
>>   usb: mux: add driver for Intel drcfg controlled port mux
>>   usb: pci-quirks: add Intel USB drcfg mux device
>>   mfd: intel_vuport: Add Intel virtual USB port MFD Driver
> You don't describe here what you changed from v2, or even from v1.
> Please always be specific, we don't remember what your last submissions
> looked like at all...

I am sorry. I put the change log after file change summary.
I will move it up here next time.

Best regards,
Baolu

>
> thanks,
>
> greg k-h
>

Re: [PATCH v3 0/7] usb: add support for Intel dual role port mux

[Greg Kroah-Hartman]

On Fri, Mar 11, 2016 at 08:20:43AM +0800, Lu Baolu wrote:
> 
> 
> On 03/11/2016 08:03 AM, Greg Kroah-Hartman wrote:
> > On Tue, Mar 08, 2016 at 03:53:41PM +0800, Lu Baolu wrote:
> >> Intel SOC chips are featured with USB dual role. The host role is
> >> provided by Intel xHCI IP, and the gadget role is provided by IP
> >> from designware. Tablet platform designs always share a single
> >> port for both host and gadget controllers. There is a mux to
> >> switch the port to the right controller according to the cable
> >> type. OS needs to provide the callback to control the mux when
> >> a plug-in event raises. The method to control the mux is platform
> >> dependent. At least three types of implementation can be found
> >> across current devices. 1) GPIO pins; 2) a unit which can be
> >> controlled by memory mapped registers; 3) ACPI ASL code.
> >>
> >> This patch series adds supports for Intel dual role port mux.
> >> It includes:
> >> (1) A helper layer on top of extcon for individual mux driver.
> >>     It listens to the USB-HOST extcon cable and call the switch
> >>     call-back when the cable state changes.
> >> (2) Drivers for GPIO controlled port mux which could be found on
> >>     Baytrail devices. A mfd driver is used to split the GPIOs into
> >>     USB gpio extcon device and a USB mux device. Driver for USB
> >>     gpio extcon device is already in upstream Linux. This patch
> >>     series includes a driver for GPIO USB mux.
> >> (3) Drivers for USB port mux controlled through memory mapped
> >>     registers and the logic to create the mux device. This type
> >>     of dual role port mux could be found in Cherry Trail and
> >>     Broxton devices.
> >>
> >> Lu Baolu (7):
> >>   extcon: usb-gpio: add device binding for platform device
> >>   extcon: usb-gpio: add support for ACPI gpio interface
> >>   usb: mux: add common code for Intel dual role port mux
> >>   usb: mux: add driver for Intel gpio controlled port mux
> >>   usb: mux: add driver for Intel drcfg controlled port mux
> >>   usb: pci-quirks: add Intel USB drcfg mux device
> >>   mfd: intel_vuport: Add Intel virtual USB port MFD Driver
> > You don't describe here what you changed from v2, or even from v1.
> > Please always be specific, we don't remember what your last submissions
> > looked like at all...
> 
> I am sorry. I put the change log after file change summary.
> I will move it up here next time.

Ah, totally missed it, sorry about that am not expecting that below a
diffstat.

greg k-h