usb: add support for Intel dual role port mux

Documentation/ABI/testing/sysfs-bus-platform Description: · line 22

Quoted 10 lines · Lu Baolu

Documentation/ABI/testing/sysfs-bus-platform +2 -0

index 5172a61..a2261cb 100644
@@ -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"?

Quoted 14 lines · Oliver Neukum

Documentation/ABI/testing/sysfs-bus-platform Description: · line 22

Quoted 10 lines · Lu Baolu

Documentation/ABI/testing/sysfs-bus-platform +2 -0

index 5172a61..a2261cb 100644
@@ -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,

Quoted 2 lines

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.

Quoted 14 lines · Oliver Neukum

Documentation/ABI/testing/sysfs-bus-platform Description: · line 22

Quoted 10 lines · Lu Baolu

Documentation/ABI/testing/sysfs-bus-platform +2 -0

index 5172a61..a2261cb 100644
@@ -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,

Quoted 2 lines

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.

Hi,

Quoted 15 lines · Greg Kroah-Hartman

Quoted 14 lines · Oliver Neukum

Documentation/ABI/testing/sysfs-bus-platform Description: · line 22

Quoted 10 lines · Lu Baolu

Documentation/ABI/testing/sysfs-bus-platform +2 -0

index 5172a61..a2261cb 100644
@@ -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,

Quoted 4 lines

Quoted 2 lines

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?

Quoted 16 lines · Greg Kroah-Hartman

Quoted 15 lines · Oliver Neukum

Quoted 14 lines · Lu Baolu

Documentation/ABI/testing/sysfs-bus-platform Description: · line 22

Quoted 11 lines

Documentation/ABI/testing/sysfs-bus-platform +2 -0

index 5172a61..a2261cb 100644
@@ -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,

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

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.

Quoted 1 line

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 :)

Quoted 2 lines

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 ?

Quoted 6 lines · Felipe Balbi

Quoted 2 lines

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...

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 <[email protected]>

Reviewed-by Heikki Krogerus <[email protected]>

Reviewed-by Felipe Balbi <[email protected]>

---
 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

Documentation/ABI/testing/sysfs-bus-platform +15 -0

index 5172a61..a2261cb 100644
@@ -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 <[email protected]>
+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.

MAINTAINERS +7 -0

index c55b37e..a93d26a 100644
@@ -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 <[email protected]>
+L:	[email protected]
+S:	Supported
+F:	drivers/usb/mux/intel-mux.c
+F:	include/linux/usb/intel-mux.h
+
 USB PRINTER DRIVER (usblp)
 M:	Pete Zaitcev <[email protected]>
 L:	[email protected]

drivers/usb/Kconfig +2 -0

index 8ed451d..dbd6620 100644
@@ -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

drivers/usb/Makefile +1 -0

index dca7856..9a92338 100644
@@ -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/

drivers/usb/mux/Kconfig new file +0 -0

new file mode 100644
index 0000000..8fedc4f

drivers/usb/mux/Kconfig new file +2 -0

+
+endmenu

drivers/usb/mux/Makefile new file +4 -0

new file mode 100644
index 0000000..84f0ae8
@@ -0,0 +1,4 @@
+#
+# Makefile for USB port mux drivers
+#
+obj-$(CONFIG_INTEL_USB_MUX)		+= intel-mux.o

drivers/usb/mux/intel-mux.c new file +166 -0

new file mode 100644
index 0000000..b243758
@@ -0,0 +1,166 @@
+/**
+ * mux.c - USB Port Mux support
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/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

include/linux/usb/intel-mux.h new file +14 -0

new file mode 100644
index 0000000..6990174
@@ -0,0 +1,47 @@
+/**
+ * mux.h - USB Port Mux defines
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LINUX_USB_INTEL_MUX_H
+#define __LINUX_USB_INTEL_MUX_H

Quoted 103 lines · 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 <[email protected]>

Reviewed-by Heikki Krogerus <[email protected]>

Reviewed-by Felipe Balbi <[email protected]>

---
 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

Documentation/ABI/testing/sysfs-bus-platform +15 -0

index 5172a61..a2261cb 100644
@@ -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 <[email protected]>
+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.

MAINTAINERS +7 -0

index c55b37e..a93d26a 100644
@@ -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 <[email protected]>
+L:	[email protected]
+S:	Supported
+F:	drivers/usb/mux/intel-mux.c
+F:	include/linux/usb/intel-mux.h
+
 USB PRINTER DRIVER (usblp)
 M:	Pete Zaitcev <[email protected]>
 L:	[email protected]

drivers/usb/Kconfig +2 -0

index 8ed451d..dbd6620 100644
@@ -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

drivers/usb/Makefile +1 -0

index dca7856..9a92338 100644
@@ -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/

drivers/usb/mux/Kconfig new file +0 -0

new file mode 100644
index 0000000..8fedc4f

drivers/usb/mux/Kconfig line 7

Quoted 7 lines

drivers/usb/mux/Kconfig new file +7 -0

@@ -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?

Quoted 208 lines

drivers/usb/mux/Kconfig new file +2 -0

+
+endmenu

drivers/usb/mux/Makefile new file +4 -0

new file mode 100644
index 0000000..84f0ae8
@@ -0,0 +1,4 @@
+#
+# Makefile for USB port mux drivers
+#
+obj-$(CONFIG_INTEL_USB_MUX)		+= intel-mux.o

drivers/usb/mux/intel-mux.c new file +166 -0

new file mode 100644
index 0000000..b243758
@@ -0,0 +1,166 @@
+/**
+ * mux.c - USB Port Mux support
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/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

include/linux/usb/intel-mux.h new file +18 -0

new file mode 100644
index 0000000..6990174
@@ -0,0 +1,47 @@
+/**
+ * mux.h - USB Port Mux defines
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LINUX_USB_INTEL_MUX_H
+#define __LINUX_USB_INTEL_MUX_H
+
+#include <linux/usb.h>
+
+struct intel_mux_dev {

include/linux/usb/intel-mux.h line 24

Quoted 6 lines

include/linux/usb/intel-mux.h new file +6 -0

+	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?

include/linux/usb/intel-mux.h line 28

Quoted 4 lines

include/linux/usb/intel-mux.h new file +3 -0

+#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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Greg Kroah-Hartman

include/linux/usb/intel-mux.h line 24

Quoted 7 lines · Lu Baolu

include/linux/usb/intel-mux.h new file +7 -0

+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().

Quoted 4 lines

include/linux/usb/intel-mux.h line 28

Quoted 3 lines

include/linux/usb/intel-mux.h new file +3 -0

+#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.

Quoted 3 lines

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()?

Quoted 2 lines

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?

Quoted 11 lines · Lu Baolu

Quoted 10 lines · Greg Kroah-Hartman

include/linux/usb/intel-mux.h line 24

Quoted 7 lines · Lu Baolu

include/linux/usb/intel-mux.h new file +7 -0

+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?

Quoted 2 lines

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...

Quoted 6 lines

Quoted 5 lines

include/linux/usb/intel-mux.h line 28

Quoted 4 lines

include/linux/usb/intel-mux.h new file +3 -0

+#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.

Quoted 6 lines

Quoted 4 lines

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 :)

Quoted 4 lines

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_" :(

Quoted 2 lines

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...

Quoted 4 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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

How about making it a real device? :)

thanks,

greg k-h

Quoted 11 lines · Lu Baolu

Quoted 10 lines · Greg Kroah-Hartman

include/linux/usb/intel-mux.h line 24

Quoted 7 lines · Lu Baolu

include/linux/usb/intel-mux.h new file +7 -0

+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?

Quoted 2 lines

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...

Quoted 5 lines

Quoted 3 lines

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 :)

Quoted 3 lines

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_" :(

Quoted 2 lines

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...

Quoted 4 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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

How about making it a real device? :)

thanks,

greg k-h

Hi,

Quoted 26 lines · Greg Kroah-Hartman

Quoted 11 lines · Lu Baolu

Quoted 10 lines · Greg Kroah-Hartman

include/linux/usb/intel-mux.h line 24

Quoted 7 lines · Lu Baolu

include/linux/usb/intel-mux.h new file +7 -0

+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?

Quoted 2 lines

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...

Quoted 5 lines

Quoted 3 lines

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 :)

Quoted 5 lines

Quoted 3 lines

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?

Quoted 15 lines

Quoted 2 lines

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...

Quoted 4 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 1 line

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?

Quoted 32 lines · Lu Baolu

Hi,

Quoted 29 lines · Greg Kroah-Hartman

Quoted 11 lines · Lu Baolu

Quoted 10 lines · Greg Kroah-Hartman

include/linux/usb/intel-mux.h line 24

Quoted 7 lines · Lu Baolu

include/linux/usb/intel-mux.h new file +7 -0

+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?

Quoted 2 lines

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...

Quoted 5 lines

Quoted 3 lines

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 :)

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

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? :)

Quoted 4 lines

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.

Quoted 23 lines

Quoted 11 lines

Quoted 9 lines

include/linux/usb/intel-mux.h line 24

Quoted 7 lines

include/linux/usb/intel-mux.h new file +7 -0

+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...

Quoted 5 lines

Quoted 3 lines

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 :)

Quoted 5 lines

Quoted 3 lines

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?

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

Quoted 26 lines · 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.

Quoted 8 lines

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

Quoted 35 lines · Greg Kroah-Hartman

Quoted 34 lines · 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

Quoted 3 lines

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.