Hi,
The Intel PMC (Power Management Controller) microcontroller, which is
available on most SOCs from Intel, has a function called mux-agent.
The mux-agent, when visible to the operating system, makes it possible
to control the various USB muxes on the system.
In practice the mux-agent is a device that controls multiple muxes.
Unfortunately both the USB Type-C Class and the USB Role Class don't
have proper support for that kind of devices that handle multiple
muxes, which is why I had to tweak the APIs a bit.
On top of the API changes, and the driver of course, I'm adding a
header for the Thunderbolt 3 alt mode since the "mux-agent" supports
it.
thanks,
Heikki Krogerus (9):
usb: typec: mux: Allow the muxes to be named
usb: typec: mux: Add helpers for setting the mux state
usb: typec: mux: Allow the mux handles to be requested with fwnode
usb: roles: Leave the private driver data pointer to the drivers
usb: roles: Provide the switch drivers handle to the switch in the API
usb: roles: Allow the role switches to be named
device property: Export fwnode_get_name()
usb: typec: Add definitions for Thunderbolt 3 Alternate Mode
usb: typec: driver for Intel PMC mux control
drivers/base/property.c | 1 +
drivers/usb/cdns3/core.c | 10 +-
drivers/usb/chipidea/core.c | 10 +-
drivers/usb/dwc3/dwc3-meson-g12a.c | 10 +-
drivers/usb/gadget/udc/renesas_usb3.c | 26 +-
drivers/usb/gadget/udc/tegra-xudc.c | 8 +-
drivers/usb/mtu3/mtu3_dr.c | 9 +-
drivers/usb/musb/mediatek.c | 9 +-
drivers/usb/roles/class.c | 29 +-
.../usb/roles/intel-xhci-usb-role-switch.c | 26 +-
drivers/usb/typec/class.c | 10 +-
drivers/usb/typec/mux.c | 47 +-
drivers/usb/typec/mux/Kconfig | 9 +
drivers/usb/typec/mux/Makefile | 1 +
drivers/usb/typec/mux/intel_pmc_mux.c | 434 ++++++++++++++++++
include/linux/usb/role.h | 23 +-
include/linux/usb/typec_mux.h | 25 +-
include/linux/usb/typec_tbt.h | 53 +++
18 files changed, 667 insertions(+), 73 deletions(-)
create mode 100644 drivers/usb/typec/mux/intel_pmc_mux.c
create mode 100644 include/linux/usb/typec_tbt.h
--
2.25.0
The Intel PMC microcontroller on the latest Intel platforms
has a new function that allows configuration of the USB
Multiplexer/DeMultiplexer switches that are under the
control of the PMC.
The Intel PMC mux control (aka. mux-agent) can be used for
swapping the USB data role and for entering alternate modes,
DisplayPort or Thunderbolt3.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/mux/Kconfig | 9 +
drivers/usb/typec/mux/Makefile | 1 +
drivers/usb/typec/mux/intel_pmc_mux.c | 434 ++++++++++++++++++++++++++
3 files changed, 444 insertions(+)
create mode 100644 drivers/usb/typec/mux/intel_pmc_mux.c
diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index 01ed0d5e10e8..77eb97b2aa86 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -9,4 +9,13 @@ config TYPEC_MUX_PI3USB30532
Say Y or M if your system has a Pericom PI3USB30532 Type-C cross
switch / mux chip found on some devices with a Type-C port.
+config TYPEC_MUX_INTEL_PMC
+ tristate "Intel PMC mux control"
+ depends on INTEL_PMC_IPC
+ select USB_ROLE_SWITCH
+ help
+ Driver for USB muxes controlled by Intel PMC FW. Intel PMC FW can
+ control the USB role switch and also the multiplexer/demultiplexer
+ switches used with USB Type-C Alternate Modes.
+
endmenu
diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
index 1332e469b8a0..280a6f553115 100644
--- a/drivers/usb/typec/mux/Makefile
+++ b/drivers/usb/typec/mux/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_TYPEC_MUX_PI3USB30532) += pi3usb30532.o
+obj-$(CONFIG_TYPEC_MUX_INTEL_PMC) += intel_pmc_mux.o
diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
new file mode 100644
index 000000000000..f5c5e0aef66f
--- /dev/null
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -0,0 +1,434 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Intel PMC USB mux control
+ *
+ * Copyright (C) 2020 Intel Corporation
+ * Author: Heikki Krogerus <[email protected]>
+ */
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/usb/role.h>
+#include <linux/usb/typec_mux.h>
+#include <linux/usb/typec_dp.h>
+#include <linux/usb/typec_tbt.h>
+
+#include <asm/intel_pmc_ipc.h>
+
+#define PMC_USBC_CMD 0xa7
+
+/* "Usage" OOB Message field values */
+enum {
+ PMC_USB_CONNECT,
+ PMC_USB_DISCONNECT,
+ PMC_USB_SAFE_MODE,
+ PMC_USB_ALT_MODE,
+ PMC_USB_DP_HPD,
+};
+
+#define PMC_USB_MSG_USB2_PORT_SHIFT 0
+#define PMC_USB_MSG_USB3_PORT_SHIFT 4
+#define PMC_USB_MSG_UFP_SHIFT 4
+#define PMC_USB_MSG_ORI_HSL_SHIFT 5
+#define PMC_USB_MSG_ORI_AUX_SHIFT 6
+
+/* Alt Mode Request */
+struct altmode_req {
+ u8 usage;
+ u8 mode_type;
+ u8 mode_id;
+ u8 reserved;
+ u32 mode_data;
+} __packed;
+
+#define PMC_USB_MODE_TYPE_SHIFT 4
+
+enum {
+ PMC_USB_MODE_TYPE_USB,
+ PMC_USB_MODE_TYPE_DP,
+ PMC_USB_MODE_TYPE_TBT,
+};
+
+/* Common Mode Data bits */
+#define PMC_USB_ALTMODE_ACTIVE_CABLE BIT(2)
+
+#define PMC_USB_ALTMODE_ORI_SHIFT 1
+#define PMC_USB_ALTMODE_UFP_SHIFT 3
+#define PMC_USB_ALTMODE_ORI_AUX_SHIFT 4
+#define PMC_USB_ALTMODE_ORI_HSL_SHIFT 5
+
+/* DP specific Mode Data bits */
+#define PMC_USB_ALTMODE_DP_MODE_SHIFT 8
+
+/* TBT specific Mode Data bits */
+#define PMC_USB_ALTMODE_TBT_TYPE BIT(17)
+#define PMC_USB_ALTMODE_CABLE_TYPE BIT(18)
+#define PMC_USB_ALTMODE_ACTIVE_LINK BIT(20)
+#define PMC_USB_ALTMODE_FORCE_LSR BIT(23)
+#define PMC_USB_ALTMODE_CABLE_SPD(_s_) (((_s_) & GENMASK(2, 0)) << 25)
+#define PMC_USB_ALTMODE_CABLE_USB31 1
+#define PMC_USB_ALTMODE_CABLE_10GPS 2
+#define PMC_USB_ALTMODE_CABLE_20GPS 3
+#define PMC_USB_ALTMODE_TBT_GEN(_g_) (((_g_) & GENMASK(1, 0)) << 28)
+
+/* Display HPD Request bits */
+#define PMC_USB_DP_HPD_IRQ BIT(5)
+#define PMC_USB_DP_HPD_LVL BIT(6)
+
+struct pmc_usb;
+
+struct pmc_usb_port {
+ int num;
+ struct pmc_usb *pmc;
+ struct typec_mux *typec_mux;
+ struct typec_switch *typec_sw;
+ struct usb_role_switch *usb_sw;
+
+ enum typec_orientation orientation;
+ enum usb_role role;
+
+ u8 usb2_port;
+ u8 usb3_port;
+};
+
+struct pmc_usb {
+ u8 num_ports;
+ struct device *dev;
+ struct pmc_usb_port *port;
+};
+
+static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
+{
+ u8 response[4];
+
+ /*
+ * Error bit will always be 0 with the USBC command.
+ * Status can be checked from the response message.
+ */
+ intel_pmc_ipc_command(PMC_USBC_CMD, 0, msg, len,
+ (void *)response, 1);
+
+ if (response[2]) {
+ if (response[2] & BIT(1))
+ return -EIO;
+ return -EBUSY;
+ }
+
+ return 0;
+}
+
+static int
+pmc_usb_mux_dp_hpd(struct pmc_usb_port *port, struct typec_mux_state *state)
+{
+ struct typec_displayport_data *data = state->data;
+ u8 msg[2] = { };
+
+ msg[0] = PMC_USB_DP_HPD;
+ msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
+
+ msg[1] = PMC_USB_DP_HPD_IRQ;
+
+ if (data->status & DP_STATUS_HPD_STATE)
+ msg[1] |= PMC_USB_DP_HPD_LVL;
+
+ return pmc_usb_command(port, msg, sizeof(msg));
+}
+
+static int
+pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
+{
+ struct typec_displayport_data *data = state->data;
+ struct altmode_req req = { };
+
+ if (data->status & DP_STATUS_IRQ_HPD)
+ return pmc_usb_mux_dp_hpd(port, state);
+
+ req.usage = PMC_USB_ALT_MODE;
+ req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
+ req.mode_type = PMC_USB_MODE_TYPE_DP << PMC_USB_MODE_TYPE_SHIFT;
+
+ req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
+ req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
+ req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
+ req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
+
+ req.mode_data |= (state->mode - TYPEC_STATE_MODAL) <<
+ PMC_USB_ALTMODE_DP_MODE_SHIFT;
+
+ return pmc_usb_command(port, (void *)&req, sizeof(req));
+}
+
+static int
+pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
+{
+ struct typec_thunderbolt_data *data = state->data;
+ u8 cable_speed = TBT_CABLE_SPEED(data->cable_mode);
+ struct altmode_req req = { };
+
+ req.usage = PMC_USB_ALT_MODE;
+ req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
+ req.mode_type = PMC_USB_MODE_TYPE_TBT << PMC_USB_MODE_TYPE_SHIFT;
+
+ req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
+ req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
+ req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
+ req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
+
+ if (TBT_ADAPTER(data->device_mode) == TBT_ADAPTER_TBT3)
+ req.mode_data |= PMC_USB_ALTMODE_TBT_TYPE;
+
+ if (data->cable_mode & TBT_CABLE_OPTICAL)
+ req.mode_data |= PMC_USB_ALTMODE_CABLE_TYPE;
+
+ if (data->cable_mode & TBT_CABLE_LINK_TRAINING)
+ req.mode_data |= PMC_USB_ALTMODE_ACTIVE_LINK;
+
+ if (data->enter_vdo & TBT_ENTER_MODE_ACTIVE_CABLE)
+ req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
+
+ req.mode_data |= PMC_USB_ALTMODE_CABLE_SPD(cable_speed);
+
+ return pmc_usb_command(port, (void *)&req, sizeof(req));
+}
+
+static int pmc_usb_mux_safe_state(struct pmc_usb_port *port)
+{
+ u8 msg;
+
+ msg = PMC_USB_SAFE_MODE;
+ msg |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
+
+ return pmc_usb_command(port, &msg, sizeof(msg));
+}
+
+static int pmc_usb_connect(struct pmc_usb_port *port)
+{
+ u8 msg[2];
+
+ msg[0] = PMC_USB_CONNECT;
+ msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
+
+ msg[1] = port->usb2_port << PMC_USB_MSG_USB2_PORT_SHIFT;
+ msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_HSL_SHIFT;
+ msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_AUX_SHIFT;
+
+ return pmc_usb_command(port, msg, sizeof(msg));
+}
+
+static int pmc_usb_disconnect(struct pmc_usb_port *port)
+{
+ u8 msg[2];
+
+ msg[0] = PMC_USB_DISCONNECT;
+ msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
+
+ msg[1] = port->usb2_port << PMC_USB_MSG_USB2_PORT_SHIFT;
+
+ return pmc_usb_command(port, msg, sizeof(msg));
+}
+
+static int
+pmc_usb_mux_set(struct typec_mux *mux, struct typec_mux_state *state)
+{
+ struct pmc_usb_port *port = typec_mux_get_drvdata(mux);
+
+ if (!state->alt)
+ return 0;
+
+ if (state->mode == TYPEC_STATE_SAFE)
+ return pmc_usb_mux_safe_state(port);
+
+ switch (state->alt->svid) {
+ case USB_TYPEC_TBT_SID:
+ return pmc_usb_mux_tbt(port, state);
+ case USB_TYPEC_DP_SID:
+ return pmc_usb_mux_dp(port, state);
+ }
+
+ return -EOPNOTSUPP;
+}
+
+static int pmc_usb_set_orientation(struct typec_switch *sw,
+ enum typec_orientation orientation)
+{
+ struct pmc_usb_port *port = typec_switch_get_drvdata(sw);
+
+ if (port->orientation == orientation)
+ return 0;
+
+ port->orientation = orientation;
+
+ if (port->role) {
+ if (orientation == TYPEC_ORIENTATION_NONE)
+ return pmc_usb_disconnect(port);
+ else
+ return pmc_usb_connect(port);
+ }
+
+ return 0;
+}
+
+static int pmc_usb_set_role(struct usb_role_switch *sw, enum usb_role role)
+{
+ struct pmc_usb_port *port = usb_role_switch_get_drvdata(sw);
+
+ if (port->role == role)
+ return 0;
+
+ port->role = role;
+
+ if (port->orientation) {
+ if (role == USB_ROLE_NONE)
+ return pmc_usb_disconnect(port);
+ else
+ return pmc_usb_connect(port);
+ }
+
+ return 0;
+}
+
+static int pmc_usb_register_port(struct pmc_usb *pmc, int index,
+ struct fwnode_handle *fwnode)
+{
+ struct pmc_usb_port *port = &pmc->port[index];
+ struct usb_role_switch_desc desc = { };
+ struct typec_switch_desc sw_desc = { };
+ struct typec_mux_desc mux_desc = { };
+ int ret;
+
+ ret = fwnode_property_read_u8(fwnode, "usb2-port", &port->usb2_port);
+ if (ret)
+ return ret;
+
+ ret = fwnode_property_read_u8(fwnode, "usb3-port", &port->usb3_port);
+ if (ret)
+ return ret;
+
+ port->num = index;
+ port->pmc = pmc;
+
+ sw_desc.fwnode = fwnode;
+ sw_desc.drvdata = port;
+ sw_desc.name = fwnode_get_name(fwnode);
+ sw_desc.set = pmc_usb_set_orientation;
+
+ port->typec_sw = typec_switch_register(pmc->dev, &sw_desc);
+ if (IS_ERR(port->typec_sw))
+ return PTR_ERR(port->typec_sw);
+
+ mux_desc.fwnode = fwnode;
+ mux_desc.drvdata = port;
+ mux_desc.name = fwnode_get_name(fwnode);
+ mux_desc.set = pmc_usb_mux_set;
+
+ port->typec_mux = typec_mux_register(pmc->dev, &mux_desc);
+ if (IS_ERR(port->typec_mux)) {
+ ret = PTR_ERR(port->typec_mux);
+ goto err_unregister_switch;
+ }
+
+ desc.fwnode = fwnode;
+ desc.driver_data = port;
+ desc.name = fwnode_get_name(fwnode);
+ desc.set = pmc_usb_set_role;
+
+ port->usb_sw = usb_role_switch_register(pmc->dev, &desc);
+ if (IS_ERR(port->usb_sw)) {
+ ret = PTR_ERR(port->usb_sw);
+ goto err_unregister_mux;
+ }
+
+ return 0;
+
+err_unregister_mux:
+ typec_mux_unregister(port->typec_mux);
+
+err_unregister_switch:
+ typec_switch_unregister(port->typec_sw);
+
+ return ret;
+}
+
+static int pmc_usb_probe(struct platform_device *pdev)
+{
+ struct fwnode_handle *fwnode = NULL;
+ struct pmc_usb *pmc;
+ int i = 0;
+ int ret;
+
+ pmc = devm_kzalloc(&pdev->dev, sizeof(*pmc), GFP_KERNEL);
+ if (!pmc)
+ return -ENOMEM;
+
+ device_for_each_child_node(&pdev->dev, fwnode)
+ pmc->num_ports++;
+
+ pmc->port = devm_kcalloc(&pdev->dev, pmc->num_ports,
+ sizeof(struct pmc_usb_port), GFP_KERNEL);
+ if (!pmc->port)
+ return -ENOMEM;
+
+ pmc->dev = &pdev->dev;
+
+ /*
+ * For every physical USB connector (USB2 and USB3 combo) there is a
+ * child ACPI device node under the PMC mux ACPI device object.
+ */
+ for (i = 0; i < pmc->num_ports; i++) {
+ fwnode = device_get_next_child_node(pmc->dev, fwnode);
+ if (!fwnode)
+ break;
+
+ ret = pmc_usb_register_port(pmc, i, fwnode);
+ if (ret)
+ goto err_remove_ports;
+ }
+
+ platform_set_drvdata(pdev, pmc);
+
+ return 0;
+
+err_remove_ports:
+ for (i = 0; i < pmc->num_ports; i++) {
+ typec_switch_unregister(pmc->port[i].typec_sw);
+ typec_mux_unregister(pmc->port[i].typec_mux);
+ }
+
+ return ret;
+}
+
+static int pmc_usb_remove(struct platform_device *pdev)
+{
+ struct pmc_usb *pmc = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < pmc->num_ports; i++) {
+ typec_switch_unregister(pmc->port[i].typec_sw);
+ typec_mux_unregister(pmc->port[i].typec_mux);
+ }
+
+ return 0;
+}
+
+static const struct acpi_device_id pmc_usb_acpi_ids[] = {
+ { "INTC105C", },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, pmc_usb_acpi_ids);
+
+static struct platform_driver pmc_usb_driver = {
+ .driver = {
+ .name = "intel_pmc_usb",
+ .acpi_match_table = ACPI_PTR(pmc_usb_acpi_ids),
+ },
+ .probe = pmc_usb_probe,
+ .remove = pmc_usb_remove,
+};
+
+module_platform_driver(pmc_usb_driver);
+
+MODULE_AUTHOR("Heikki Krogerus <[email protected]>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel PMC USB mux control");
--
2.25.0
Adding usb_role_switch_get/set_drvdata() functions that the
switch drivers can use for setting and getting private data
pointer that is associated with the switch.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/roles/class.c | 22 ++++++++++++++++++++++
include/linux/usb/role.h | 16 ++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index 63a00ff26655..f3132d231599 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -329,6 +329,7 @@ usb_role_switch_register(struct device *parent,
sw->dev.fwnode = desc->fwnode;
sw->dev.class = role_class;
sw->dev.type = &usb_role_dev_type;
+ sw->dev.driver_data = desc->driver_data;
dev_set_name(&sw->dev, "%s-role-switch", dev_name(parent));
ret = device_register(&sw->dev);
@@ -356,6 +357,27 @@ void usb_role_switch_unregister(struct usb_role_switch *sw)
}
EXPORT_SYMBOL_GPL(usb_role_switch_unregister);
+/**
+ * usb_role_switch_set_drvdata - Assign private data pointer to a switch
+ * @sw: USB Role Switch
+ * @data: Private data pointer
+ */
+void usb_role_switch_set_drvdata(struct usb_role_switch *sw, void *data)
+{
+ dev_set_drvdata(&sw->dev, data);
+}
+EXPORT_SYMBOL_GPL(usb_role_switch_set_drvdata);
+
+/**
+ * usb_role_switch_get_drvdata - Get the private data pointer of a switch
+ * @sw: USB Role Switch
+ */
+void *usb_role_switch_get_drvdata(struct usb_role_switch *sw)
+{
+ return dev_get_drvdata(&sw->dev);
+}
+EXPORT_SYMBOL_GPL(usb_role_switch_get_drvdata);
+
static int __init usb_roles_init(void)
{
role_class = class_create(THIS_MODULE, "usb_role");
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index efac3af83d6b..02dae936cebd 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -25,6 +25,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct device *dev);
* @set: Callback for setting the role
* @get: Callback for getting the role (optional)
* @allow_userspace_control: If true userspace may change the role through sysfs
+ * @driver_data: Private data pointer
*
* @usb2_port and @usb3_port will point to the USB host port and @udc to the USB
* device controller behind the USB connector with the role switch. If
@@ -40,6 +41,7 @@ struct usb_role_switch_desc {
usb_role_switch_set_t set;
usb_role_switch_get_t get;
bool allow_userspace_control;
+ void *driver_data;
};
@@ -57,6 +59,9 @@ struct usb_role_switch *
usb_role_switch_register(struct device *parent,
const struct usb_role_switch_desc *desc);
void usb_role_switch_unregister(struct usb_role_switch *sw);
+
+void usb_role_switch_set_drvdata(struct usb_role_switch *sw, void *data);
+void *usb_role_switch_get_drvdata(struct usb_role_switch *sw);
#else
static inline int usb_role_switch_set_role(struct usb_role_switch *sw,
enum usb_role role)
@@ -90,6 +95,17 @@ usb_role_switch_register(struct device *parent,
}
static inline void usb_role_switch_unregister(struct usb_role_switch *sw) { }
+
+static inline void
+usb_role_switch_set_drvdata(struct usb_role_switch *sw, void *data)
+{
+}
+
+static inline void *usb_role_switch_get_drvdata(struct usb_role_switch *sw)
+{
+ return NULL;
+}
+
#endif
#endif /* __LINUX_USB_ROLE_H */
--
2.25.0
The USB role callback functions had a parameter pointing to
the parent device (struct device) of the switch. The
assumption was that the switch parent is always the
controller. Firstly, that may not be true in every case, and
secondly, it prevents us from supporting devices that supply
multiple muxes.
Changing the first parameter of usb_role_switch_set_t and
usb_role_switch_get_t from struct device to struct
usb_role_switch.
Cc: Peter Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Bin Liu <[email protected]>
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/cdns3/core.c | 10 ++++---
drivers/usb/chipidea/core.c | 10 ++++---
drivers/usb/dwc3/dwc3-meson-g12a.c | 10 ++++---
drivers/usb/gadget/udc/renesas_usb3.c | 26 ++++++++++---------
drivers/usb/gadget/udc/tegra-xudc.c | 8 +++---
drivers/usb/mtu3/mtu3_dr.c | 9 ++++---
drivers/usb/musb/mediatek.c | 9 ++++---
drivers/usb/roles/class.c | 4 +--
.../usb/roles/intel-xhci-usb-role-switch.c | 26 +++++++++++--------
include/linux/usb/role.h | 5 ++--
10 files changed, 67 insertions(+), 50 deletions(-)
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index c2123ef8d8a3..a7a7cb9a2a48 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -330,9 +330,9 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
*
* Returns role
*/
-static enum usb_role cdns3_role_get(struct device *dev)
+static enum usb_role cdns3_role_get(struct usb_role_switch *sw)
{
- struct cdns3 *cdns = dev_get_drvdata(dev);
+ struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
return cdns->role;
}
@@ -346,9 +346,9 @@ static enum usb_role cdns3_role_get(struct device *dev)
* - Role switch for dual-role devices
* - USB_ROLE_GADGET <--> USB_ROLE_NONE for peripheral-only devices
*/
-static int cdns3_role_set(struct device *dev, enum usb_role role)
+static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
{
- struct cdns3 *cdns = dev_get_drvdata(dev);
+ struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
int ret = 0;
pm_runtime_get_sync(cdns->dev);
@@ -536,6 +536,8 @@ static int cdns3_probe(struct platform_device *pdev)
goto err4;
}
+ usb_role_switch_set_drvdata(cdns->role_sw, cdns);
+
ret = cdns3_drd_init(cdns);
if (ret)
goto err5;
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 52139c2a9924..ae0bdc036464 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -600,9 +600,9 @@ static int ci_cable_notifier(struct notifier_block *nb, unsigned long event,
return NOTIFY_DONE;
}
-static enum usb_role ci_usb_role_switch_get(struct device *dev)
+static enum usb_role ci_usb_role_switch_get(struct usb_role_switch *sw)
{
- struct ci_hdrc *ci = dev_get_drvdata(dev);
+ struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
enum usb_role role;
unsigned long flags;
@@ -613,9 +613,10 @@ static enum usb_role ci_usb_role_switch_get(struct device *dev)
return role;
}
-static int ci_usb_role_switch_set(struct device *dev, enum usb_role role)
+static int ci_usb_role_switch_set(struct usb_role_switch *sw,
+ enum usb_role role)
{
- struct ci_hdrc *ci = dev_get_drvdata(dev);
+ struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
struct ci_hdrc_cable *cable = NULL;
enum usb_role current_role = ci_role_to_usb_role(ci);
enum ci_role ci_role = usb_role_to_ci_role(role);
@@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
}
if (ci_role_switch.fwnode) {
+ ci_role_switch.driver_data = ci;
ci->role_switch = usb_role_switch_register(dev,
&ci_role_switch);
if (IS_ERR(ci->role_switch)) {
diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
index 8a3ec1a951fe..3309ce90ca14 100644
--- a/drivers/usb/dwc3/dwc3-meson-g12a.c
+++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
@@ -321,9 +321,10 @@ static int dwc3_meson_g12a_otg_mode_set(struct dwc3_meson_g12a *priv,
return 0;
}
-static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role)
+static int dwc3_meson_g12a_role_set(struct usb_role_switch *sw,
+ enum usb_role role)
{
- struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
+ struct dwc3_meson_g12a *priv = usb_role_switch_get_drvdata(sw);
enum phy_mode mode;
if (role == USB_ROLE_NONE)
@@ -338,9 +339,9 @@ static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role)
return dwc3_meson_g12a_otg_mode_set(priv, mode);
}
-static enum usb_role dwc3_meson_g12a_role_get(struct device *dev)
+static enum usb_role dwc3_meson_g12a_role_get(struct usb_role_switch *sw)
{
- struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
+ struct dwc3_meson_g12a *priv = usb_role_switch_get_drvdata(sw);
return priv->otg_phy_mode == PHY_MODE_USB_HOST ?
USB_ROLE_HOST : USB_ROLE_DEVICE;
@@ -499,6 +500,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
priv->switch_desc.allow_userspace_control = true;
priv->switch_desc.set = dwc3_meson_g12a_role_set;
priv->switch_desc.get = dwc3_meson_g12a_role_get;
+ priv->switch_desc.driver_data = priv;
priv->role_switch = usb_role_switch_register(dev, &priv->switch_desc);
if (IS_ERR(priv->role_switch))
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index c5c3c14df67a..4da90160b400 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -2355,14 +2355,14 @@ static const struct usb_gadget_ops renesas_usb3_gadget_ops = {
.set_selfpowered = renesas_usb3_set_selfpowered,
};
-static enum usb_role renesas_usb3_role_switch_get(struct device *dev)
+static enum usb_role renesas_usb3_role_switch_get(struct usb_role_switch *sw)
{
- struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
+ struct renesas_usb3 *usb3 = usb_role_switch_get_drvdata(sw);
enum usb_role cur_role;
- pm_runtime_get_sync(dev);
+ pm_runtime_get_sync(usb3_to_dev(usb3));
cur_role = usb3_is_host(usb3) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
- pm_runtime_put(dev);
+ pm_runtime_put(usb3_to_dev(usb3));
return cur_role;
}
@@ -2372,7 +2372,7 @@ static void handle_ext_role_switch_states(struct device *dev,
{
struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
struct device *host = usb3->host_dev;
- enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
+ enum usb_role cur_role = renesas_usb3_role_switch_get(usb3->role_sw);
switch (role) {
case USB_ROLE_NONE:
@@ -2424,7 +2424,7 @@ static void handle_role_switch_states(struct device *dev,
{
struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
struct device *host = usb3->host_dev;
- enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
+ enum usb_role cur_role = renesas_usb3_role_switch_get(usb3->role_sw);
if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) {
device_release_driver(host);
@@ -2438,19 +2438,19 @@ static void handle_role_switch_states(struct device *dev,
}
}
-static int renesas_usb3_role_switch_set(struct device *dev,
+static int renesas_usb3_role_switch_set(struct usb_role_switch *sw,
enum usb_role role)
{
- struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
+ struct renesas_usb3 *usb3 = usb_role_switch_get_drvdata(sw);
- pm_runtime_get_sync(dev);
+ pm_runtime_get_sync(usb3_to_dev(usb3));
if (usb3->role_sw_by_connector)
- handle_ext_role_switch_states(dev, role);
+ handle_ext_role_switch_states(usb3_to_dev(usb3), role);
else
- handle_role_switch_states(dev, role);
+ handle_role_switch_states(usb3_to_dev(usb3), role);
- pm_runtime_put(dev);
+ pm_runtime_put(usb3_to_dev(usb3));
return 0;
}
@@ -2831,6 +2831,8 @@ static int renesas_usb3_probe(struct platform_device *pdev)
renesas_usb3_role_switch_desc.fwnode = dev_fwnode(&pdev->dev);
}
+ renesas_usb3_role_switch_desc.driver_data = usb3;
+
INIT_WORK(&usb3->role_work, renesas_usb3_role_work);
usb3->role_sw = usb_role_switch_register(&pdev->dev,
&renesas_usb3_role_switch_desc);
diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
index 634c2c19a176..b9df6369d56d 100644
--- a/drivers/usb/gadget/udc/tegra-xudc.c
+++ b/drivers/usb/gadget/udc/tegra-xudc.c
@@ -676,12 +676,13 @@ static void tegra_xudc_usb_role_sw_work(struct work_struct *work)
}
-static int tegra_xudc_usb_role_sw_set(struct device *dev, enum usb_role role)
+static int tegra_xudc_usb_role_sw_set(struct usb_role_switch *sw,
+ enum usb_role role)
{
- struct tegra_xudc *xudc = dev_get_drvdata(dev);
+ struct tegra_xudc *xudc = usb_role_switch_get_drvdata(sw);
unsigned long flags;
- dev_dbg(dev, "%s role is %d\n", __func__, role);
+ dev_dbg(xudc->dev, "%s role is %d\n", __func__, role);
spin_lock_irqsave(&xudc->lock, flags);
@@ -3590,6 +3591,7 @@ static int tegra_xudc_probe(struct platform_device *pdev)
if (of_property_read_bool(xudc->dev->of_node, "usb-role-switch")) {
role_sx_desc.set = tegra_xudc_usb_role_sw_set;
role_sx_desc.fwnode = dev_fwnode(xudc->dev);
+ role_sx_desc.driver_data = xudc;
xudc->usb_role_sw = usb_role_switch_register(xudc->dev,
&role_sx_desc);
diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
index 08e18448e8b8..04f666e85731 100644
--- a/drivers/usb/mtu3/mtu3_dr.c
+++ b/drivers/usb/mtu3/mtu3_dr.c
@@ -320,9 +320,9 @@ void ssusb_set_force_mode(struct ssusb_mtk *ssusb,
mtu3_writel(ssusb->ippc_base, SSUSB_U2_CTRL(0), value);
}
-static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
+static int ssusb_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
{
- struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
+ struct ssusb_mtk *ssusb = usb_role_switch_get_drvdata(sw);
bool to_host = false;
if (role == USB_ROLE_HOST)
@@ -334,9 +334,9 @@ static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
return 0;
}
-static enum usb_role ssusb_role_sw_get(struct device *dev)
+static enum usb_role ssusb_role_sw_get(struct usb_role_switch *sw)
{
- struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
+ struct ssusb_mtk *ssusb = usb_role_switch_get_drvdata(sw);
enum usb_role role;
role = ssusb->is_host ? USB_ROLE_HOST : USB_ROLE_DEVICE;
@@ -356,6 +356,7 @@ static int ssusb_role_sw_register(struct otg_switch_mtk *otg_sx)
role_sx_desc.set = ssusb_role_sw_set;
role_sx_desc.get = ssusb_role_sw_get;
role_sx_desc.fwnode = dev_fwnode(ssusb->dev);
+ role_sx_desc.driver_data = ssusb;
otg_sx->role_sw = usb_role_switch_register(ssusb->dev, &role_sx_desc);
return PTR_ERR_OR_ZERO(otg_sx->role_sw);
diff --git a/drivers/usb/musb/mediatek.c b/drivers/usb/musb/mediatek.c
index 6b88c2f5d970..703b98d71180 100644
--- a/drivers/usb/musb/mediatek.c
+++ b/drivers/usb/musb/mediatek.c
@@ -115,9 +115,9 @@ static void mtk_musb_clks_disable(struct mtk_glue *glue)
clk_disable_unprepare(glue->main);
}
-static int musb_usb_role_sx_set(struct device *dev, enum usb_role role)
+static int musb_usb_role_sx_set(struct usb_role_switch *sw, enum usb_role role)
{
- struct mtk_glue *glue = dev_get_drvdata(dev);
+ struct mtk_glue *glue = usb_role_switch_get_drvdata(sw);
struct musb *musb = glue->musb;
u8 devctl = readb(musb->mregs + MUSB_DEVCTL);
enum usb_role new_role;
@@ -168,9 +168,9 @@ static int musb_usb_role_sx_set(struct device *dev, enum usb_role role)
return 0;
}
-static enum usb_role musb_usb_role_sx_get(struct device *dev)
+static enum usb_role musb_usb_role_sx_get(struct usb_role_switch *sw)
{
- struct mtk_glue *glue = dev_get_drvdata(dev);
+ struct mtk_glue *glue = usb_role_switch_get_drvdata(sw);
return glue->role;
}
@@ -182,6 +182,7 @@ static int mtk_otg_switch_init(struct mtk_glue *glue)
role_sx_desc.set = musb_usb_role_sx_set;
role_sx_desc.get = musb_usb_role_sx_get;
role_sx_desc.fwnode = dev_fwnode(glue->dev);
+ role_sx_desc.driver_data = glue;
glue->role_sw = usb_role_switch_register(glue->dev, &role_sx_desc);
return PTR_ERR_OR_ZERO(glue->role_sw);
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index f3132d231599..d5e57d26c31f 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -48,7 +48,7 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
mutex_lock(&sw->lock);
- ret = sw->set(sw->dev.parent, role);
+ ret = sw->set(sw, role);
if (!ret)
sw->role = role;
@@ -75,7 +75,7 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
mutex_lock(&sw->lock);
if (sw->get)
- role = sw->get(sw->dev.parent);
+ role = sw->get(sw);
else
role = sw->role;
diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index 80d6559bbcb2..5c96e929acea 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -42,6 +42,7 @@
#define DRV_NAME "intel_xhci_usb_sw"
struct intel_xhci_usb_data {
+ struct device *dev;
struct usb_role_switch *role_sw;
void __iomem *base;
bool enable_sw_switch;
@@ -51,9 +52,10 @@ static const struct software_node intel_xhci_usb_node = {
"intel-xhci-usb-sw",
};
-static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
+static int intel_xhci_usb_set_role(struct usb_role_switch *sw,
+ enum usb_role role)
{
- struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
+ struct intel_xhci_usb_data *data = usb_role_switch_get_drvdata(sw);
unsigned long timeout;
acpi_status status;
u32 glk, val;
@@ -66,11 +68,11 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
*/
status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
- dev_err(dev, "Error could not acquire lock\n");
+ dev_err(data->dev, "Error could not acquire lock\n");
return -EIO;
}
- pm_runtime_get_sync(dev);
+ pm_runtime_get_sync(data->dev);
/*
* Set idpin value as requested.
@@ -112,7 +114,7 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
do {
val = readl(data->base + DUAL_ROLE_CFG1);
if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) {
- pm_runtime_put(dev);
+ pm_runtime_put(data->dev);
return 0;
}
@@ -120,21 +122,21 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
usleep_range(5000, 10000);
} while (time_before(jiffies, timeout));
- pm_runtime_put(dev);
+ pm_runtime_put(data->dev);
- dev_warn(dev, "Timeout waiting for role-switch\n");
+ dev_warn(data->dev, "Timeout waiting for role-switch\n");
return -ETIMEDOUT;
}
-static enum usb_role intel_xhci_usb_get_role(struct device *dev)
+static enum usb_role intel_xhci_usb_get_role(struct usb_role_switch *sw)
{
- struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
+ struct intel_xhci_usb_data *data = usb_role_switch_get_drvdata(sw);
enum usb_role role;
u32 val;
- pm_runtime_get_sync(dev);
+ pm_runtime_get_sync(data->dev);
val = readl(data->base + DUAL_ROLE_CFG0);
- pm_runtime_put(dev);
+ pm_runtime_put(data->dev);
if (!(val & SW_IDPIN))
role = USB_ROLE_HOST;
@@ -175,7 +177,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
sw_desc.get = intel_xhci_usb_get_role,
sw_desc.allow_userspace_control = true,
sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
+ sw_desc.driver_data = data;
+ data->dev = dev;
data->enable_sw_switch = !device_property_read_bool(dev,
"sw_switch_disable");
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index 02dae936cebd..c028ba8029ad 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -13,8 +13,9 @@ enum usb_role {
USB_ROLE_DEVICE,
};
-typedef int (*usb_role_switch_set_t)(struct device *dev, enum usb_role role);
-typedef enum usb_role (*usb_role_switch_get_t)(struct device *dev);
+typedef int (*usb_role_switch_set_t)(struct usb_role_switch *sw,
+ enum usb_role role);
+typedef enum usb_role (*usb_role_switch_get_t)(struct usb_role_switch *sw);
/**
* struct usb_role_switch_desc - USB Role Switch Descriptor
--
2.25.0
The switch devices have been named by using the name of the
parent device as base for now, but if for example the
parent device controls multiple muxes, that will not work.
Adding an optional member "name" to the switch descriptor
that can be used for naming the switch during registration.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/roles/class.c | 3 ++-
include/linux/usb/role.h | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index d5e57d26c31f..e17df54f2852 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -330,7 +330,8 @@ usb_role_switch_register(struct device *parent,
sw->dev.class = role_class;
sw->dev.type = &usb_role_dev_type;
sw->dev.driver_data = desc->driver_data;
- dev_set_name(&sw->dev, "%s-role-switch", dev_name(parent));
+ dev_set_name(&sw->dev, "%s-role-switch",
+ desc->name ? desc->name : dev_name(parent));
ret = device_register(&sw->dev);
if (ret) {
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index c028ba8029ad..0164fed31b06 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -27,6 +27,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct usb_role_switch *sw);
* @get: Callback for getting the role (optional)
* @allow_userspace_control: If true userspace may change the role through sysfs
* @driver_data: Private data pointer
+ * @name: Name for the switch (optional)
*
* @usb2_port and @usb3_port will point to the USB host port and @udc to the USB
* device controller behind the USB connector with the role switch. If
@@ -43,6 +44,7 @@ struct usb_role_switch_desc {
usb_role_switch_get_t get;
bool allow_userspace_control;
void *driver_data;
+ const char *name;
};
--
2.25.0
Hi guys,
On Thu, Feb 13, 2020 at 04:24:24PM +0300, Heikki Krogerus wrote:
> The USB role callback functions had a parameter pointing to
> the parent device (struct device) of the switch. The
> assumption was that the switch parent is always the
> controller. Firstly, that may not be true in every case, and
> secondly, it prevents us from supporting devices that supply
> multiple muxes.
>
> Changing the first parameter of usb_role_switch_set_t and
> usb_role_switch_get_t from struct device to struct
> usb_role_switch.
>
> Cc: Peter Chen <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Chunfeng Yun <[email protected]>
> Cc: Bin Liu <[email protected]>
This was meant for you guys, but I suppressed CC for everybody. Sorry
about that.
> Signed-off-by: Heikki Krogerus <[email protected]>
> ---
> drivers/usb/cdns3/core.c | 10 ++++---
> drivers/usb/chipidea/core.c | 10 ++++---
> drivers/usb/dwc3/dwc3-meson-g12a.c | 10 ++++---
> drivers/usb/gadget/udc/renesas_usb3.c | 26 ++++++++++---------
> drivers/usb/gadget/udc/tegra-xudc.c | 8 +++---
> drivers/usb/mtu3/mtu3_dr.c | 9 ++++---
> drivers/usb/musb/mediatek.c | 9 ++++---
> drivers/usb/roles/class.c | 4 +--
> .../usb/roles/intel-xhci-usb-role-switch.c | 26 +++++++++++--------
> include/linux/usb/role.h | 5 ++--
> 10 files changed, 67 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index c2123ef8d8a3..a7a7cb9a2a48 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -330,9 +330,9 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> *
> * Returns role
> */
> -static enum usb_role cdns3_role_get(struct device *dev)
> +static enum usb_role cdns3_role_get(struct usb_role_switch *sw)
> {
> - struct cdns3 *cdns = dev_get_drvdata(dev);
> + struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
>
> return cdns->role;
> }
> @@ -346,9 +346,9 @@ static enum usb_role cdns3_role_get(struct device *dev)
> * - Role switch for dual-role devices
> * - USB_ROLE_GADGET <--> USB_ROLE_NONE for peripheral-only devices
> */
> -static int cdns3_role_set(struct device *dev, enum usb_role role)
> +static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
> {
> - struct cdns3 *cdns = dev_get_drvdata(dev);
> + struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
> int ret = 0;
>
> pm_runtime_get_sync(cdns->dev);
> @@ -536,6 +536,8 @@ static int cdns3_probe(struct platform_device *pdev)
> goto err4;
> }
>
> + usb_role_switch_set_drvdata(cdns->role_sw, cdns);
> +
> ret = cdns3_drd_init(cdns);
> if (ret)
> goto err5;
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 52139c2a9924..ae0bdc036464 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -600,9 +600,9 @@ static int ci_cable_notifier(struct notifier_block *nb, unsigned long event,
> return NOTIFY_DONE;
> }
>
> -static enum usb_role ci_usb_role_switch_get(struct device *dev)
> +static enum usb_role ci_usb_role_switch_get(struct usb_role_switch *sw)
> {
> - struct ci_hdrc *ci = dev_get_drvdata(dev);
> + struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> enum usb_role role;
> unsigned long flags;
>
> @@ -613,9 +613,10 @@ static enum usb_role ci_usb_role_switch_get(struct device *dev)
> return role;
> }
>
> -static int ci_usb_role_switch_set(struct device *dev, enum usb_role role)
> +static int ci_usb_role_switch_set(struct usb_role_switch *sw,
> + enum usb_role role)
> {
> - struct ci_hdrc *ci = dev_get_drvdata(dev);
> + struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> struct ci_hdrc_cable *cable = NULL;
> enum usb_role current_role = ci_role_to_usb_role(ci);
> enum ci_role ci_role = usb_role_to_ci_role(role);
> @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> }
>
> if (ci_role_switch.fwnode) {
> + ci_role_switch.driver_data = ci;
> ci->role_switch = usb_role_switch_register(dev,
> &ci_role_switch);
> if (IS_ERR(ci->role_switch)) {
> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> index 8a3ec1a951fe..3309ce90ca14 100644
> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> @@ -321,9 +321,10 @@ static int dwc3_meson_g12a_otg_mode_set(struct dwc3_meson_g12a *priv,
> return 0;
> }
>
> -static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role)
> +static int dwc3_meson_g12a_role_set(struct usb_role_switch *sw,
> + enum usb_role role)
> {
> - struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
> + struct dwc3_meson_g12a *priv = usb_role_switch_get_drvdata(sw);
> enum phy_mode mode;
>
> if (role == USB_ROLE_NONE)
> @@ -338,9 +339,9 @@ static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role)
> return dwc3_meson_g12a_otg_mode_set(priv, mode);
> }
>
> -static enum usb_role dwc3_meson_g12a_role_get(struct device *dev)
> +static enum usb_role dwc3_meson_g12a_role_get(struct usb_role_switch *sw)
> {
> - struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
> + struct dwc3_meson_g12a *priv = usb_role_switch_get_drvdata(sw);
>
> return priv->otg_phy_mode == PHY_MODE_USB_HOST ?
> USB_ROLE_HOST : USB_ROLE_DEVICE;
> @@ -499,6 +500,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
> priv->switch_desc.allow_userspace_control = true;
> priv->switch_desc.set = dwc3_meson_g12a_role_set;
> priv->switch_desc.get = dwc3_meson_g12a_role_get;
> + priv->switch_desc.driver_data = priv;
>
> priv->role_switch = usb_role_switch_register(dev, &priv->switch_desc);
> if (IS_ERR(priv->role_switch))
> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
> index c5c3c14df67a..4da90160b400 100644
> --- a/drivers/usb/gadget/udc/renesas_usb3.c
> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> @@ -2355,14 +2355,14 @@ static const struct usb_gadget_ops renesas_usb3_gadget_ops = {
> .set_selfpowered = renesas_usb3_set_selfpowered,
> };
>
> -static enum usb_role renesas_usb3_role_switch_get(struct device *dev)
> +static enum usb_role renesas_usb3_role_switch_get(struct usb_role_switch *sw)
> {
> - struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> + struct renesas_usb3 *usb3 = usb_role_switch_get_drvdata(sw);
> enum usb_role cur_role;
>
> - pm_runtime_get_sync(dev);
> + pm_runtime_get_sync(usb3_to_dev(usb3));
> cur_role = usb3_is_host(usb3) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
> - pm_runtime_put(dev);
> + pm_runtime_put(usb3_to_dev(usb3));
>
> return cur_role;
> }
> @@ -2372,7 +2372,7 @@ static void handle_ext_role_switch_states(struct device *dev,
> {
> struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> struct device *host = usb3->host_dev;
> - enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
> + enum usb_role cur_role = renesas_usb3_role_switch_get(usb3->role_sw);
>
> switch (role) {
> case USB_ROLE_NONE:
> @@ -2424,7 +2424,7 @@ static void handle_role_switch_states(struct device *dev,
> {
> struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> struct device *host = usb3->host_dev;
> - enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
> + enum usb_role cur_role = renesas_usb3_role_switch_get(usb3->role_sw);
>
> if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) {
> device_release_driver(host);
> @@ -2438,19 +2438,19 @@ static void handle_role_switch_states(struct device *dev,
> }
> }
>
> -static int renesas_usb3_role_switch_set(struct device *dev,
> +static int renesas_usb3_role_switch_set(struct usb_role_switch *sw,
> enum usb_role role)
> {
> - struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> + struct renesas_usb3 *usb3 = usb_role_switch_get_drvdata(sw);
>
> - pm_runtime_get_sync(dev);
> + pm_runtime_get_sync(usb3_to_dev(usb3));
>
> if (usb3->role_sw_by_connector)
> - handle_ext_role_switch_states(dev, role);
> + handle_ext_role_switch_states(usb3_to_dev(usb3), role);
> else
> - handle_role_switch_states(dev, role);
> + handle_role_switch_states(usb3_to_dev(usb3), role);
>
> - pm_runtime_put(dev);
> + pm_runtime_put(usb3_to_dev(usb3));
>
> return 0;
> }
> @@ -2831,6 +2831,8 @@ static int renesas_usb3_probe(struct platform_device *pdev)
> renesas_usb3_role_switch_desc.fwnode = dev_fwnode(&pdev->dev);
> }
>
> + renesas_usb3_role_switch_desc.driver_data = usb3;
> +
> INIT_WORK(&usb3->role_work, renesas_usb3_role_work);
> usb3->role_sw = usb_role_switch_register(&pdev->dev,
> &renesas_usb3_role_switch_desc);
> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
> index 634c2c19a176..b9df6369d56d 100644
> --- a/drivers/usb/gadget/udc/tegra-xudc.c
> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
> @@ -676,12 +676,13 @@ static void tegra_xudc_usb_role_sw_work(struct work_struct *work)
>
> }
>
> -static int tegra_xudc_usb_role_sw_set(struct device *dev, enum usb_role role)
> +static int tegra_xudc_usb_role_sw_set(struct usb_role_switch *sw,
> + enum usb_role role)
> {
> - struct tegra_xudc *xudc = dev_get_drvdata(dev);
> + struct tegra_xudc *xudc = usb_role_switch_get_drvdata(sw);
> unsigned long flags;
>
> - dev_dbg(dev, "%s role is %d\n", __func__, role);
> + dev_dbg(xudc->dev, "%s role is %d\n", __func__, role);
>
> spin_lock_irqsave(&xudc->lock, flags);
>
> @@ -3590,6 +3591,7 @@ static int tegra_xudc_probe(struct platform_device *pdev)
> if (of_property_read_bool(xudc->dev->of_node, "usb-role-switch")) {
> role_sx_desc.set = tegra_xudc_usb_role_sw_set;
> role_sx_desc.fwnode = dev_fwnode(xudc->dev);
> + role_sx_desc.driver_data = xudc;
>
> xudc->usb_role_sw = usb_role_switch_register(xudc->dev,
> &role_sx_desc);
> diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
> index 08e18448e8b8..04f666e85731 100644
> --- a/drivers/usb/mtu3/mtu3_dr.c
> +++ b/drivers/usb/mtu3/mtu3_dr.c
> @@ -320,9 +320,9 @@ void ssusb_set_force_mode(struct ssusb_mtk *ssusb,
> mtu3_writel(ssusb->ippc_base, SSUSB_U2_CTRL(0), value);
> }
>
> -static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
> +static int ssusb_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
> {
> - struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
> + struct ssusb_mtk *ssusb = usb_role_switch_get_drvdata(sw);
> bool to_host = false;
>
> if (role == USB_ROLE_HOST)
> @@ -334,9 +334,9 @@ static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
> return 0;
> }
>
> -static enum usb_role ssusb_role_sw_get(struct device *dev)
> +static enum usb_role ssusb_role_sw_get(struct usb_role_switch *sw)
> {
> - struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
> + struct ssusb_mtk *ssusb = usb_role_switch_get_drvdata(sw);
> enum usb_role role;
>
> role = ssusb->is_host ? USB_ROLE_HOST : USB_ROLE_DEVICE;
> @@ -356,6 +356,7 @@ static int ssusb_role_sw_register(struct otg_switch_mtk *otg_sx)
> role_sx_desc.set = ssusb_role_sw_set;
> role_sx_desc.get = ssusb_role_sw_get;
> role_sx_desc.fwnode = dev_fwnode(ssusb->dev);
> + role_sx_desc.driver_data = ssusb;
> otg_sx->role_sw = usb_role_switch_register(ssusb->dev, &role_sx_desc);
>
> return PTR_ERR_OR_ZERO(otg_sx->role_sw);
> diff --git a/drivers/usb/musb/mediatek.c b/drivers/usb/musb/mediatek.c
> index 6b88c2f5d970..703b98d71180 100644
> --- a/drivers/usb/musb/mediatek.c
> +++ b/drivers/usb/musb/mediatek.c
> @@ -115,9 +115,9 @@ static void mtk_musb_clks_disable(struct mtk_glue *glue)
> clk_disable_unprepare(glue->main);
> }
>
> -static int musb_usb_role_sx_set(struct device *dev, enum usb_role role)
> +static int musb_usb_role_sx_set(struct usb_role_switch *sw, enum usb_role role)
> {
> - struct mtk_glue *glue = dev_get_drvdata(dev);
> + struct mtk_glue *glue = usb_role_switch_get_drvdata(sw);
> struct musb *musb = glue->musb;
> u8 devctl = readb(musb->mregs + MUSB_DEVCTL);
> enum usb_role new_role;
> @@ -168,9 +168,9 @@ static int musb_usb_role_sx_set(struct device *dev, enum usb_role role)
> return 0;
> }
>
> -static enum usb_role musb_usb_role_sx_get(struct device *dev)
> +static enum usb_role musb_usb_role_sx_get(struct usb_role_switch *sw)
> {
> - struct mtk_glue *glue = dev_get_drvdata(dev);
> + struct mtk_glue *glue = usb_role_switch_get_drvdata(sw);
>
> return glue->role;
> }
> @@ -182,6 +182,7 @@ static int mtk_otg_switch_init(struct mtk_glue *glue)
> role_sx_desc.set = musb_usb_role_sx_set;
> role_sx_desc.get = musb_usb_role_sx_get;
> role_sx_desc.fwnode = dev_fwnode(glue->dev);
> + role_sx_desc.driver_data = glue;
> glue->role_sw = usb_role_switch_register(glue->dev, &role_sx_desc);
>
> return PTR_ERR_OR_ZERO(glue->role_sw);
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index f3132d231599..d5e57d26c31f 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -48,7 +48,7 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
>
> mutex_lock(&sw->lock);
>
> - ret = sw->set(sw->dev.parent, role);
> + ret = sw->set(sw, role);
> if (!ret)
> sw->role = role;
>
> @@ -75,7 +75,7 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
> mutex_lock(&sw->lock);
>
> if (sw->get)
> - role = sw->get(sw->dev.parent);
> + role = sw->get(sw);
> else
> role = sw->role;
>
> diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> index 80d6559bbcb2..5c96e929acea 100644
> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> @@ -42,6 +42,7 @@
> #define DRV_NAME "intel_xhci_usb_sw"
>
> struct intel_xhci_usb_data {
> + struct device *dev;
> struct usb_role_switch *role_sw;
> void __iomem *base;
> bool enable_sw_switch;
> @@ -51,9 +52,10 @@ static const struct software_node intel_xhci_usb_node = {
> "intel-xhci-usb-sw",
> };
>
> -static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> +static int intel_xhci_usb_set_role(struct usb_role_switch *sw,
> + enum usb_role role)
> {
> - struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
> + struct intel_xhci_usb_data *data = usb_role_switch_get_drvdata(sw);
> unsigned long timeout;
> acpi_status status;
> u32 glk, val;
> @@ -66,11 +68,11 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> */
> status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
> if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
> - dev_err(dev, "Error could not acquire lock\n");
> + dev_err(data->dev, "Error could not acquire lock\n");
> return -EIO;
> }
>
> - pm_runtime_get_sync(dev);
> + pm_runtime_get_sync(data->dev);
>
> /*
> * Set idpin value as requested.
> @@ -112,7 +114,7 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> do {
> val = readl(data->base + DUAL_ROLE_CFG1);
> if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) {
> - pm_runtime_put(dev);
> + pm_runtime_put(data->dev);
> return 0;
> }
>
> @@ -120,21 +122,21 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> usleep_range(5000, 10000);
> } while (time_before(jiffies, timeout));
>
> - pm_runtime_put(dev);
> + pm_runtime_put(data->dev);
>
> - dev_warn(dev, "Timeout waiting for role-switch\n");
> + dev_warn(data->dev, "Timeout waiting for role-switch\n");
> return -ETIMEDOUT;
> }
>
> -static enum usb_role intel_xhci_usb_get_role(struct device *dev)
> +static enum usb_role intel_xhci_usb_get_role(struct usb_role_switch *sw)
> {
> - struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
> + struct intel_xhci_usb_data *data = usb_role_switch_get_drvdata(sw);
> enum usb_role role;
> u32 val;
>
> - pm_runtime_get_sync(dev);
> + pm_runtime_get_sync(data->dev);
> val = readl(data->base + DUAL_ROLE_CFG0);
> - pm_runtime_put(dev);
> + pm_runtime_put(data->dev);
>
> if (!(val & SW_IDPIN))
> role = USB_ROLE_HOST;
> @@ -175,7 +177,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
> sw_desc.get = intel_xhci_usb_get_role,
> sw_desc.allow_userspace_control = true,
> sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
> + sw_desc.driver_data = data;
>
> + data->dev = dev;
> data->enable_sw_switch = !device_property_read_bool(dev,
> "sw_switch_disable");
>
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index 02dae936cebd..c028ba8029ad 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -13,8 +13,9 @@ enum usb_role {
> USB_ROLE_DEVICE,
> };
>
> -typedef int (*usb_role_switch_set_t)(struct device *dev, enum usb_role role);
> -typedef enum usb_role (*usb_role_switch_get_t)(struct device *dev);
> +typedef int (*usb_role_switch_set_t)(struct usb_role_switch *sw,
> + enum usb_role role);
> +typedef enum usb_role (*usb_role_switch_get_t)(struct usb_role_switch *sw);
>
> /**
> * struct usb_role_switch_desc - USB Role Switch Descriptor
> --
> 2.25.0
thanks,
--
heikki
On Thu, 2020-02-13 at 16:24 +0300, Heikki Krogerus wrote:
> Adding usb_role_switch_get/set_drvdata() functions that the
> switch drivers can use for setting and getting private data
> pointer that is associated with the switch.
>
> Signed-off-by: Heikki Krogerus <[email protected]>
> ---
> drivers/usb/roles/class.c | 22 ++++++++++++++++++++++
> include/linux/usb/role.h | 16 ++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index 63a00ff26655..f3132d231599 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -329,6 +329,7 @@ usb_role_switch_register(struct device *parent,
> sw->dev.fwnode = desc->fwnode;
> sw->dev.class = role_class;
> sw->dev.type = &usb_role_dev_type;
> + sw->dev.driver_data = desc->driver_data;
How about use dev_set_drvdata()? will keep align with
usb_role_switch_set/get_drvdata(),
> dev_set_name(&sw->dev, "%s-role-switch", dev_name(parent));
>
> ret = device_register(&sw->dev);
> @@ -356,6 +357,27 @@ void usb_role_switch_unregister(struct usb_role_switch *sw)
> }
> EXPORT_SYMBOL_GPL(usb_role_switch_unregister);
>
> +/**
> + * usb_role_switch_set_drvdata - Assign private data pointer to a switch
> + * @sw: USB Role Switch
> + * @data: Private data pointer
> + */
> +void usb_role_switch_set_drvdata(struct usb_role_switch *sw, void *data)
> +{
> + dev_set_drvdata(&sw->dev, data);
> +}
> +EXPORT_SYMBOL_GPL(usb_role_switch_set_drvdata);
> +
> +/**
> + * usb_role_switch_get_drvdata - Get the private data pointer of a switch
> + * @sw: USB Role Switch
> + */
> +void *usb_role_switch_get_drvdata(struct usb_role_switch *sw)
> +{
> + return dev_get_drvdata(&sw->dev);
> +}
> +EXPORT_SYMBOL_GPL(usb_role_switch_get_drvdata);
> +
> static int __init usb_roles_init(void)
> {
> role_class = class_create(THIS_MODULE, "usb_role");
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index efac3af83d6b..02dae936cebd 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -25,6 +25,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct device *dev);
> * @set: Callback for setting the role
> * @get: Callback for getting the role (optional)
> * @allow_userspace_control: If true userspace may change the role through sysfs
> + * @driver_data: Private data pointer
> *
> * @usb2_port and @usb3_port will point to the USB host port and @udc to the USB
> * device controller behind the USB connector with the role switch. If
> @@ -40,6 +41,7 @@ struct usb_role_switch_desc {
> usb_role_switch_set_t set;
> usb_role_switch_get_t get;
> bool allow_userspace_control;
> + void *driver_data;
> };
>
>
> @@ -57,6 +59,9 @@ struct usb_role_switch *
> usb_role_switch_register(struct device *parent,
> const struct usb_role_switch_desc *desc);
> void usb_role_switch_unregister(struct usb_role_switch *sw);
> +
> +void usb_role_switch_set_drvdata(struct usb_role_switch *sw, void *data);
> +void *usb_role_switch_get_drvdata(struct usb_role_switch *sw);
> #else
> static inline int usb_role_switch_set_role(struct usb_role_switch *sw,
> enum usb_role role)
> @@ -90,6 +95,17 @@ usb_role_switch_register(struct device *parent,
> }
>
> static inline void usb_role_switch_unregister(struct usb_role_switch *sw) { }
> +
> +static inline void
> +usb_role_switch_set_drvdata(struct usb_role_switch *sw, void *data)
> +{
> +}
> +
> +static inline void *usb_role_switch_get_drvdata(struct usb_role_switch *sw)
> +{
> + return NULL;
> +}
> +
> #endif
>
> #endif /* __LINUX_USB_ROLE_H */
On Thu, 2020-02-13 at 16:24 +0300, Heikki Krogerus wrote:
> The USB role callback functions had a parameter pointing to
> the parent device (struct device) of the switch. The
> assumption was that the switch parent is always the
> controller. Firstly, that may not be true in every case, and
> secondly, it prevents us from supporting devices that supply
> multiple muxes.
>
> Changing the first parameter of usb_role_switch_set_t and
> usb_role_switch_get_t from struct device to struct
> usb_role_switch.
>
> Cc: Peter Chen <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Chunfeng Yun <[email protected]>
> Cc: Bin Liu <[email protected]>
> Signed-off-by: Heikki Krogerus <[email protected]>
> ---
> drivers/usb/cdns3/core.c | 10 ++++---
> drivers/usb/chipidea/core.c | 10 ++++---
> drivers/usb/dwc3/dwc3-meson-g12a.c | 10 ++++---
> drivers/usb/gadget/udc/renesas_usb3.c | 26 ++++++++++---------
> drivers/usb/gadget/udc/tegra-xudc.c | 8 +++---
> drivers/usb/mtu3/mtu3_dr.c | 9 ++++---
> drivers/usb/musb/mediatek.c | 9 ++++---
> drivers/usb/roles/class.c | 4 +--
> .../usb/roles/intel-xhci-usb-role-switch.c | 26 +++++++++++--------
> include/linux/usb/role.h | 5 ++--
> 10 files changed, 67 insertions(+), 50 deletions(-)
Looks good to me for mtu3/mtu3_dr.c and musb/mediatek.c
Only build pass, but have no platforms to test it due to n-COV and work
at home now, so
Reviewed-by: Chunfeng Yun <[email protected]>
>
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index c2123ef8d8a3..a7a7cb9a2a48 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -330,9 +330,9 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> *
> * Returns role
> */
> -static enum usb_role cdns3_role_get(struct device *dev)
> +static enum usb_role cdns3_role_get(struct usb_role_switch *sw)
> {
> - struct cdns3 *cdns = dev_get_drvdata(dev);
> + struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
>
> return cdns->role;
> }
> @@ -346,9 +346,9 @@ static enum usb_role cdns3_role_get(struct device *dev)
> * - Role switch for dual-role devices
> * - USB_ROLE_GADGET <--> USB_ROLE_NONE for peripheral-only devices
> */
> -static int cdns3_role_set(struct device *dev, enum usb_role role)
> +static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
> {
> - struct cdns3 *cdns = dev_get_drvdata(dev);
> + struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
> int ret = 0;
>
> pm_runtime_get_sync(cdns->dev);
> @@ -536,6 +536,8 @@ static int cdns3_probe(struct platform_device *pdev)
> goto err4;
> }
>
> + usb_role_switch_set_drvdata(cdns->role_sw, cdns);
> +
> ret = cdns3_drd_init(cdns);
> if (ret)
> goto err5;
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 52139c2a9924..ae0bdc036464 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -600,9 +600,9 @@ static int ci_cable_notifier(struct notifier_block *nb, unsigned long event,
> return NOTIFY_DONE;
> }
>
> -static enum usb_role ci_usb_role_switch_get(struct device *dev)
> +static enum usb_role ci_usb_role_switch_get(struct usb_role_switch *sw)
> {
> - struct ci_hdrc *ci = dev_get_drvdata(dev);
> + struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> enum usb_role role;
> unsigned long flags;
>
> @@ -613,9 +613,10 @@ static enum usb_role ci_usb_role_switch_get(struct device *dev)
> return role;
> }
>
> -static int ci_usb_role_switch_set(struct device *dev, enum usb_role role)
> +static int ci_usb_role_switch_set(struct usb_role_switch *sw,
> + enum usb_role role)
> {
> - struct ci_hdrc *ci = dev_get_drvdata(dev);
> + struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> struct ci_hdrc_cable *cable = NULL;
> enum usb_role current_role = ci_role_to_usb_role(ci);
> enum ci_role ci_role = usb_role_to_ci_role(role);
> @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> }
>
> if (ci_role_switch.fwnode) {
> + ci_role_switch.driver_data = ci;
> ci->role_switch = usb_role_switch_register(dev,
> &ci_role_switch);
> if (IS_ERR(ci->role_switch)) {
> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> index 8a3ec1a951fe..3309ce90ca14 100644
> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> @@ -321,9 +321,10 @@ static int dwc3_meson_g12a_otg_mode_set(struct dwc3_meson_g12a *priv,
> return 0;
> }
>
> -static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role)
> +static int dwc3_meson_g12a_role_set(struct usb_role_switch *sw,
> + enum usb_role role)
> {
> - struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
> + struct dwc3_meson_g12a *priv = usb_role_switch_get_drvdata(sw);
> enum phy_mode mode;
>
> if (role == USB_ROLE_NONE)
> @@ -338,9 +339,9 @@ static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role)
> return dwc3_meson_g12a_otg_mode_set(priv, mode);
> }
>
> -static enum usb_role dwc3_meson_g12a_role_get(struct device *dev)
> +static enum usb_role dwc3_meson_g12a_role_get(struct usb_role_switch *sw)
> {
> - struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
> + struct dwc3_meson_g12a *priv = usb_role_switch_get_drvdata(sw);
>
> return priv->otg_phy_mode == PHY_MODE_USB_HOST ?
> USB_ROLE_HOST : USB_ROLE_DEVICE;
> @@ -499,6 +500,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
> priv->switch_desc.allow_userspace_control = true;
> priv->switch_desc.set = dwc3_meson_g12a_role_set;
> priv->switch_desc.get = dwc3_meson_g12a_role_get;
> + priv->switch_desc.driver_data = priv;
>
> priv->role_switch = usb_role_switch_register(dev, &priv->switch_desc);
> if (IS_ERR(priv->role_switch))
> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
> index c5c3c14df67a..4da90160b400 100644
> --- a/drivers/usb/gadget/udc/renesas_usb3.c
> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> @@ -2355,14 +2355,14 @@ static const struct usb_gadget_ops renesas_usb3_gadget_ops = {
> .set_selfpowered = renesas_usb3_set_selfpowered,
> };
>
> -static enum usb_role renesas_usb3_role_switch_get(struct device *dev)
> +static enum usb_role renesas_usb3_role_switch_get(struct usb_role_switch *sw)
> {
> - struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> + struct renesas_usb3 *usb3 = usb_role_switch_get_drvdata(sw);
> enum usb_role cur_role;
>
> - pm_runtime_get_sync(dev);
> + pm_runtime_get_sync(usb3_to_dev(usb3));
> cur_role = usb3_is_host(usb3) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
> - pm_runtime_put(dev);
> + pm_runtime_put(usb3_to_dev(usb3));
>
> return cur_role;
> }
> @@ -2372,7 +2372,7 @@ static void handle_ext_role_switch_states(struct device *dev,
> {
> struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> struct device *host = usb3->host_dev;
> - enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
> + enum usb_role cur_role = renesas_usb3_role_switch_get(usb3->role_sw);
>
> switch (role) {
> case USB_ROLE_NONE:
> @@ -2424,7 +2424,7 @@ static void handle_role_switch_states(struct device *dev,
> {
> struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> struct device *host = usb3->host_dev;
> - enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
> + enum usb_role cur_role = renesas_usb3_role_switch_get(usb3->role_sw);
>
> if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) {
> device_release_driver(host);
> @@ -2438,19 +2438,19 @@ static void handle_role_switch_states(struct device *dev,
> }
> }
>
> -static int renesas_usb3_role_switch_set(struct device *dev,
> +static int renesas_usb3_role_switch_set(struct usb_role_switch *sw,
> enum usb_role role)
> {
> - struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> + struct renesas_usb3 *usb3 = usb_role_switch_get_drvdata(sw);
>
> - pm_runtime_get_sync(dev);
> + pm_runtime_get_sync(usb3_to_dev(usb3));
>
> if (usb3->role_sw_by_connector)
> - handle_ext_role_switch_states(dev, role);
> + handle_ext_role_switch_states(usb3_to_dev(usb3), role);
> else
> - handle_role_switch_states(dev, role);
> + handle_role_switch_states(usb3_to_dev(usb3), role);
>
> - pm_runtime_put(dev);
> + pm_runtime_put(usb3_to_dev(usb3));
>
> return 0;
> }
> @@ -2831,6 +2831,8 @@ static int renesas_usb3_probe(struct platform_device *pdev)
> renesas_usb3_role_switch_desc.fwnode = dev_fwnode(&pdev->dev);
> }
>
> + renesas_usb3_role_switch_desc.driver_data = usb3;
> +
> INIT_WORK(&usb3->role_work, renesas_usb3_role_work);
> usb3->role_sw = usb_role_switch_register(&pdev->dev,
> &renesas_usb3_role_switch_desc);
> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
> index 634c2c19a176..b9df6369d56d 100644
> --- a/drivers/usb/gadget/udc/tegra-xudc.c
> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
> @@ -676,12 +676,13 @@ static void tegra_xudc_usb_role_sw_work(struct work_struct *work)
>
> }
>
> -static int tegra_xudc_usb_role_sw_set(struct device *dev, enum usb_role role)
> +static int tegra_xudc_usb_role_sw_set(struct usb_role_switch *sw,
> + enum usb_role role)
> {
> - struct tegra_xudc *xudc = dev_get_drvdata(dev);
> + struct tegra_xudc *xudc = usb_role_switch_get_drvdata(sw);
> unsigned long flags;
>
> - dev_dbg(dev, "%s role is %d\n", __func__, role);
> + dev_dbg(xudc->dev, "%s role is %d\n", __func__, role);
>
> spin_lock_irqsave(&xudc->lock, flags);
>
> @@ -3590,6 +3591,7 @@ static int tegra_xudc_probe(struct platform_device *pdev)
> if (of_property_read_bool(xudc->dev->of_node, "usb-role-switch")) {
> role_sx_desc.set = tegra_xudc_usb_role_sw_set;
> role_sx_desc.fwnode = dev_fwnode(xudc->dev);
> + role_sx_desc.driver_data = xudc;
>
> xudc->usb_role_sw = usb_role_switch_register(xudc->dev,
> &role_sx_desc);
> diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
> index 08e18448e8b8..04f666e85731 100644
> --- a/drivers/usb/mtu3/mtu3_dr.c
> +++ b/drivers/usb/mtu3/mtu3_dr.c
> @@ -320,9 +320,9 @@ void ssusb_set_force_mode(struct ssusb_mtk *ssusb,
> mtu3_writel(ssusb->ippc_base, SSUSB_U2_CTRL(0), value);
> }
>
> -static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
> +static int ssusb_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
> {
> - struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
> + struct ssusb_mtk *ssusb = usb_role_switch_get_drvdata(sw);
> bool to_host = false;
>
> if (role == USB_ROLE_HOST)
> @@ -334,9 +334,9 @@ static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
> return 0;
> }
>
> -static enum usb_role ssusb_role_sw_get(struct device *dev)
> +static enum usb_role ssusb_role_sw_get(struct usb_role_switch *sw)
> {
> - struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
> + struct ssusb_mtk *ssusb = usb_role_switch_get_drvdata(sw);
> enum usb_role role;
>
> role = ssusb->is_host ? USB_ROLE_HOST : USB_ROLE_DEVICE;
> @@ -356,6 +356,7 @@ static int ssusb_role_sw_register(struct otg_switch_mtk *otg_sx)
> role_sx_desc.set = ssusb_role_sw_set;
> role_sx_desc.get = ssusb_role_sw_get;
> role_sx_desc.fwnode = dev_fwnode(ssusb->dev);
> + role_sx_desc.driver_data = ssusb;
> otg_sx->role_sw = usb_role_switch_register(ssusb->dev, &role_sx_desc);
>
> return PTR_ERR_OR_ZERO(otg_sx->role_sw);
> diff --git a/drivers/usb/musb/mediatek.c b/drivers/usb/musb/mediatek.c
> index 6b88c2f5d970..703b98d71180 100644
> --- a/drivers/usb/musb/mediatek.c
> +++ b/drivers/usb/musb/mediatek.c
> @@ -115,9 +115,9 @@ static void mtk_musb_clks_disable(struct mtk_glue *glue)
> clk_disable_unprepare(glue->main);
> }
>
> -static int musb_usb_role_sx_set(struct device *dev, enum usb_role role)
> +static int musb_usb_role_sx_set(struct usb_role_switch *sw, enum usb_role role)
> {
> - struct mtk_glue *glue = dev_get_drvdata(dev);
> + struct mtk_glue *glue = usb_role_switch_get_drvdata(sw);
> struct musb *musb = glue->musb;
> u8 devctl = readb(musb->mregs + MUSB_DEVCTL);
> enum usb_role new_role;
> @@ -168,9 +168,9 @@ static int musb_usb_role_sx_set(struct device *dev, enum usb_role role)
> return 0;
> }
>
> -static enum usb_role musb_usb_role_sx_get(struct device *dev)
> +static enum usb_role musb_usb_role_sx_get(struct usb_role_switch *sw)
> {
> - struct mtk_glue *glue = dev_get_drvdata(dev);
> + struct mtk_glue *glue = usb_role_switch_get_drvdata(sw);
>
> return glue->role;
> }
> @@ -182,6 +182,7 @@ static int mtk_otg_switch_init(struct mtk_glue *glue)
> role_sx_desc.set = musb_usb_role_sx_set;
> role_sx_desc.get = musb_usb_role_sx_get;
> role_sx_desc.fwnode = dev_fwnode(glue->dev);
> + role_sx_desc.driver_data = glue;
> glue->role_sw = usb_role_switch_register(glue->dev, &role_sx_desc);
>
> return PTR_ERR_OR_ZERO(glue->role_sw);
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index f3132d231599..d5e57d26c31f 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -48,7 +48,7 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
>
> mutex_lock(&sw->lock);
>
> - ret = sw->set(sw->dev.parent, role);
> + ret = sw->set(sw, role);
> if (!ret)
> sw->role = role;
>
> @@ -75,7 +75,7 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
> mutex_lock(&sw->lock);
>
> if (sw->get)
> - role = sw->get(sw->dev.parent);
> + role = sw->get(sw);
> else
> role = sw->role;
>
> diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> index 80d6559bbcb2..5c96e929acea 100644
> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> @@ -42,6 +42,7 @@
> #define DRV_NAME "intel_xhci_usb_sw"
>
> struct intel_xhci_usb_data {
> + struct device *dev;
> struct usb_role_switch *role_sw;
> void __iomem *base;
> bool enable_sw_switch;
> @@ -51,9 +52,10 @@ static const struct software_node intel_xhci_usb_node = {
> "intel-xhci-usb-sw",
> };
>
> -static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> +static int intel_xhci_usb_set_role(struct usb_role_switch *sw,
> + enum usb_role role)
> {
> - struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
> + struct intel_xhci_usb_data *data = usb_role_switch_get_drvdata(sw);
> unsigned long timeout;
> acpi_status status;
> u32 glk, val;
> @@ -66,11 +68,11 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> */
> status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
> if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
> - dev_err(dev, "Error could not acquire lock\n");
> + dev_err(data->dev, "Error could not acquire lock\n");
> return -EIO;
> }
>
> - pm_runtime_get_sync(dev);
> + pm_runtime_get_sync(data->dev);
>
> /*
> * Set idpin value as requested.
> @@ -112,7 +114,7 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> do {
> val = readl(data->base + DUAL_ROLE_CFG1);
> if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) {
> - pm_runtime_put(dev);
> + pm_runtime_put(data->dev);
> return 0;
> }
>
> @@ -120,21 +122,21 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> usleep_range(5000, 10000);
> } while (time_before(jiffies, timeout));
>
> - pm_runtime_put(dev);
> + pm_runtime_put(data->dev);
>
> - dev_warn(dev, "Timeout waiting for role-switch\n");
> + dev_warn(data->dev, "Timeout waiting for role-switch\n");
> return -ETIMEDOUT;
> }
>
> -static enum usb_role intel_xhci_usb_get_role(struct device *dev)
> +static enum usb_role intel_xhci_usb_get_role(struct usb_role_switch *sw)
> {
> - struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
> + struct intel_xhci_usb_data *data = usb_role_switch_get_drvdata(sw);
> enum usb_role role;
> u32 val;
>
> - pm_runtime_get_sync(dev);
> + pm_runtime_get_sync(data->dev);
> val = readl(data->base + DUAL_ROLE_CFG0);
> - pm_runtime_put(dev);
> + pm_runtime_put(data->dev);
>
> if (!(val & SW_IDPIN))
> role = USB_ROLE_HOST;
> @@ -175,7 +177,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
> sw_desc.get = intel_xhci_usb_get_role,
> sw_desc.allow_userspace_control = true,
> sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
> + sw_desc.driver_data = data;
>
> + data->dev = dev;
> data->enable_sw_switch = !device_property_read_bool(dev,
> "sw_switch_disable");
>
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index 02dae936cebd..c028ba8029ad 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -13,8 +13,9 @@ enum usb_role {
> USB_ROLE_DEVICE,
> };
>
> -typedef int (*usb_role_switch_set_t)(struct device *dev, enum usb_role role);
> -typedef enum usb_role (*usb_role_switch_get_t)(struct device *dev);
> +typedef int (*usb_role_switch_set_t)(struct usb_role_switch *sw,
> + enum usb_role role);
> +typedef enum usb_role (*usb_role_switch_get_t)(struct usb_role_switch *sw);
>
> /**
> * struct usb_role_switch_desc - USB Role Switch Descriptor
On Sat, Feb 15, 2020 at 05:19:58PM +0800, Chunfeng Yun wrote:
> On Thu, 2020-02-13 at 16:24 +0300, Heikki Krogerus wrote:
> > Adding usb_role_switch_get/set_drvdata() functions that the
> > switch drivers can use for setting and getting private data
> > pointer that is associated with the switch.
> >
> > Signed-off-by: Heikki Krogerus <[email protected]>
> > ---
> > drivers/usb/roles/class.c | 22 ++++++++++++++++++++++
> > include/linux/usb/role.h | 16 ++++++++++++++++
> > 2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index 63a00ff26655..f3132d231599 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -329,6 +329,7 @@ usb_role_switch_register(struct device *parent,
> > sw->dev.fwnode = desc->fwnode;
> > sw->dev.class = role_class;
> > sw->dev.type = &usb_role_dev_type;
> > + sw->dev.driver_data = desc->driver_data;
> How about use dev_set_drvdata()? will keep align with
> usb_role_switch_set/get_drvdata(),
Yes, makes sense. I'll change that.
Thanks,
--
heikki
On 20-02-13 15:32:39, Heikki Krogerus wrote:
> Hi guys,
>
> On Thu, Feb 13, 2020 at 04:24:24PM +0300, Heikki Krogerus wrote:
> > The USB role callback functions had a parameter pointing to
> > the parent device (struct device) of the switch. The
> > assumption was that the switch parent is always the
> > controller. Firstly, that may not be true in every case, and
> > secondly, it prevents us from supporting devices that supply
> > multiple muxes.
> >
> > Changing the first parameter of usb_role_switch_set_t and
> > usb_role_switch_get_t from struct device to struct
> > usb_role_switch.
> >
> > Cc: Peter Chen <[email protected]>
> > Cc: Felipe Balbi <[email protected]>
> > Cc: Chunfeng Yun <[email protected]>
> > Cc: Bin Liu <[email protected]>
>
> This was meant for you guys, but I suppressed CC for everybody. Sorry
> about that.
>
> > Signed-off-by: Heikki Krogerus <[email protected]>
> > ---
> > drivers/usb/cdns3/core.c | 10 ++++---
> > drivers/usb/chipidea/core.c | 10 ++++---
> > drivers/usb/dwc3/dwc3-meson-g12a.c | 10 ++++---
> > drivers/usb/gadget/udc/renesas_usb3.c | 26 ++++++++++---------
> > drivers/usb/gadget/udc/tegra-xudc.c | 8 +++---
> > drivers/usb/mtu3/mtu3_dr.c | 9 ++++---
> > drivers/usb/musb/mediatek.c | 9 ++++---
> > drivers/usb/roles/class.c | 4 +--
> > .../usb/roles/intel-xhci-usb-role-switch.c | 26 +++++++++++--------
> > include/linux/usb/role.h | 5 ++--
> > 10 files changed, 67 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index c2123ef8d8a3..a7a7cb9a2a48 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -330,9 +330,9 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> > *
> > * Returns role
> > */
> > -static enum usb_role cdns3_role_get(struct device *dev)
> > +static enum usb_role cdns3_role_get(struct usb_role_switch *sw)
> > {
> > - struct cdns3 *cdns = dev_get_drvdata(dev);
> > + struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
> >
> > return cdns->role;
> > }
> > @@ -346,9 +346,9 @@ static enum usb_role cdns3_role_get(struct device *dev)
> > * - Role switch for dual-role devices
> > * - USB_ROLE_GADGET <--> USB_ROLE_NONE for peripheral-only devices
> > */
> > -static int cdns3_role_set(struct device *dev, enum usb_role role)
> > +static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
> > {
> > - struct cdns3 *cdns = dev_get_drvdata(dev);
> > + struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
> > int ret = 0;
> >
> > pm_runtime_get_sync(cdns->dev);
> > @@ -536,6 +536,8 @@ static int cdns3_probe(struct platform_device *pdev)
> > goto err4;
> > }
> >
> > + usb_role_switch_set_drvdata(cdns->role_sw, cdns);
> > +
> > ret = cdns3_drd_init(cdns);
> > if (ret)
> > goto err5;
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index 52139c2a9924..ae0bdc036464 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -600,9 +600,9 @@ static int ci_cable_notifier(struct notifier_block *nb, unsigned long event,
> > return NOTIFY_DONE;
> > }
> >
> > -static enum usb_role ci_usb_role_switch_get(struct device *dev)
> > +static enum usb_role ci_usb_role_switch_get(struct usb_role_switch *sw)
> > {
> > - struct ci_hdrc *ci = dev_get_drvdata(dev);
> > + struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> > enum usb_role role;
> > unsigned long flags;
> >
> > @@ -613,9 +613,10 @@ static enum usb_role ci_usb_role_switch_get(struct device *dev)
> > return role;
> > }
> >
> > -static int ci_usb_role_switch_set(struct device *dev, enum usb_role role)
> > +static int ci_usb_role_switch_set(struct usb_role_switch *sw,
> > + enum usb_role role)
> > {
> > - struct ci_hdrc *ci = dev_get_drvdata(dev);
> > + struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> > struct ci_hdrc_cable *cable = NULL;
> > enum usb_role current_role = ci_role_to_usb_role(ci);
> > enum ci_role ci_role = usb_role_to_ci_role(role);
> > @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > }
> >
> > if (ci_role_switch.fwnode) {
> > + ci_role_switch.driver_data = ci;
> > ci->role_switch = usb_role_switch_register(dev,
> > &ci_role_switch);
Why the struct usb_role_switch_desc needs drvdata, the struct
usb_role_switch has already one?
Peter
> > if (IS_ERR(ci->role_switch)) {
> > diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> > index 8a3ec1a951fe..3309ce90ca14 100644
> > --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> > +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> > @@ -321,9 +321,10 @@ static int dwc3_meson_g12a_otg_mode_set(struct dwc3_meson_g12a *priv,
> > return 0;
> > }
> >
> > -static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role)
> > +static int dwc3_meson_g12a_role_set(struct usb_role_switch *sw,
> > + enum usb_role role)
> > {
> > - struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
> > + struct dwc3_meson_g12a *priv = usb_role_switch_get_drvdata(sw);
> > enum phy_mode mode;
> >
> > if (role == USB_ROLE_NONE)
> > @@ -338,9 +339,9 @@ static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role)
> > return dwc3_meson_g12a_otg_mode_set(priv, mode);
> > }
> >
> > -static enum usb_role dwc3_meson_g12a_role_get(struct device *dev)
> > +static enum usb_role dwc3_meson_g12a_role_get(struct usb_role_switch *sw)
> > {
> > - struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
> > + struct dwc3_meson_g12a *priv = usb_role_switch_get_drvdata(sw);
> >
> > return priv->otg_phy_mode == PHY_MODE_USB_HOST ?
> > USB_ROLE_HOST : USB_ROLE_DEVICE;
> > @@ -499,6 +500,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
> > priv->switch_desc.allow_userspace_control = true;
> > priv->switch_desc.set = dwc3_meson_g12a_role_set;
> > priv->switch_desc.get = dwc3_meson_g12a_role_get;
> > + priv->switch_desc.driver_data = priv;
> >
> > priv->role_switch = usb_role_switch_register(dev, &priv->switch_desc);
> > if (IS_ERR(priv->role_switch))
> > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
> > index c5c3c14df67a..4da90160b400 100644
> > --- a/drivers/usb/gadget/udc/renesas_usb3.c
> > +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> > @@ -2355,14 +2355,14 @@ static const struct usb_gadget_ops renesas_usb3_gadget_ops = {
> > .set_selfpowered = renesas_usb3_set_selfpowered,
> > };
> >
> > -static enum usb_role renesas_usb3_role_switch_get(struct device *dev)
> > +static enum usb_role renesas_usb3_role_switch_get(struct usb_role_switch *sw)
> > {
> > - struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> > + struct renesas_usb3 *usb3 = usb_role_switch_get_drvdata(sw);
> > enum usb_role cur_role;
> >
> > - pm_runtime_get_sync(dev);
> > + pm_runtime_get_sync(usb3_to_dev(usb3));
> > cur_role = usb3_is_host(usb3) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
> > - pm_runtime_put(dev);
> > + pm_runtime_put(usb3_to_dev(usb3));
> >
> > return cur_role;
> > }
> > @@ -2372,7 +2372,7 @@ static void handle_ext_role_switch_states(struct device *dev,
> > {
> > struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> > struct device *host = usb3->host_dev;
> > - enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
> > + enum usb_role cur_role = renesas_usb3_role_switch_get(usb3->role_sw);
> >
> > switch (role) {
> > case USB_ROLE_NONE:
> > @@ -2424,7 +2424,7 @@ static void handle_role_switch_states(struct device *dev,
> > {
> > struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> > struct device *host = usb3->host_dev;
> > - enum usb_role cur_role = renesas_usb3_role_switch_get(dev);
> > + enum usb_role cur_role = renesas_usb3_role_switch_get(usb3->role_sw);
> >
> > if (cur_role == USB_ROLE_HOST && role == USB_ROLE_DEVICE) {
> > device_release_driver(host);
> > @@ -2438,19 +2438,19 @@ static void handle_role_switch_states(struct device *dev,
> > }
> > }
> >
> > -static int renesas_usb3_role_switch_set(struct device *dev,
> > +static int renesas_usb3_role_switch_set(struct usb_role_switch *sw,
> > enum usb_role role)
> > {
> > - struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> > + struct renesas_usb3 *usb3 = usb_role_switch_get_drvdata(sw);
> >
> > - pm_runtime_get_sync(dev);
> > + pm_runtime_get_sync(usb3_to_dev(usb3));
> >
> > if (usb3->role_sw_by_connector)
> > - handle_ext_role_switch_states(dev, role);
> > + handle_ext_role_switch_states(usb3_to_dev(usb3), role);
> > else
> > - handle_role_switch_states(dev, role);
> > + handle_role_switch_states(usb3_to_dev(usb3), role);
> >
> > - pm_runtime_put(dev);
> > + pm_runtime_put(usb3_to_dev(usb3));
> >
> > return 0;
> > }
> > @@ -2831,6 +2831,8 @@ static int renesas_usb3_probe(struct platform_device *pdev)
> > renesas_usb3_role_switch_desc.fwnode = dev_fwnode(&pdev->dev);
> > }
> >
> > + renesas_usb3_role_switch_desc.driver_data = usb3;
> > +
> > INIT_WORK(&usb3->role_work, renesas_usb3_role_work);
> > usb3->role_sw = usb_role_switch_register(&pdev->dev,
> > &renesas_usb3_role_switch_desc);
> > diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
> > index 634c2c19a176..b9df6369d56d 100644
> > --- a/drivers/usb/gadget/udc/tegra-xudc.c
> > +++ b/drivers/usb/gadget/udc/tegra-xudc.c
> > @@ -676,12 +676,13 @@ static void tegra_xudc_usb_role_sw_work(struct work_struct *work)
> >
> > }
> >
> > -static int tegra_xudc_usb_role_sw_set(struct device *dev, enum usb_role role)
> > +static int tegra_xudc_usb_role_sw_set(struct usb_role_switch *sw,
> > + enum usb_role role)
> > {
> > - struct tegra_xudc *xudc = dev_get_drvdata(dev);
> > + struct tegra_xudc *xudc = usb_role_switch_get_drvdata(sw);
> > unsigned long flags;
> >
> > - dev_dbg(dev, "%s role is %d\n", __func__, role);
> > + dev_dbg(xudc->dev, "%s role is %d\n", __func__, role);
> >
> > spin_lock_irqsave(&xudc->lock, flags);
> >
> > @@ -3590,6 +3591,7 @@ static int tegra_xudc_probe(struct platform_device *pdev)
> > if (of_property_read_bool(xudc->dev->of_node, "usb-role-switch")) {
> > role_sx_desc.set = tegra_xudc_usb_role_sw_set;
> > role_sx_desc.fwnode = dev_fwnode(xudc->dev);
> > + role_sx_desc.driver_data = xudc;
> >
> > xudc->usb_role_sw = usb_role_switch_register(xudc->dev,
> > &role_sx_desc);
> > diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
> > index 08e18448e8b8..04f666e85731 100644
> > --- a/drivers/usb/mtu3/mtu3_dr.c
> > +++ b/drivers/usb/mtu3/mtu3_dr.c
> > @@ -320,9 +320,9 @@ void ssusb_set_force_mode(struct ssusb_mtk *ssusb,
> > mtu3_writel(ssusb->ippc_base, SSUSB_U2_CTRL(0), value);
> > }
> >
> > -static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
> > +static int ssusb_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
> > {
> > - struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
> > + struct ssusb_mtk *ssusb = usb_role_switch_get_drvdata(sw);
> > bool to_host = false;
> >
> > if (role == USB_ROLE_HOST)
> > @@ -334,9 +334,9 @@ static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
> > return 0;
> > }
> >
> > -static enum usb_role ssusb_role_sw_get(struct device *dev)
> > +static enum usb_role ssusb_role_sw_get(struct usb_role_switch *sw)
> > {
> > - struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
> > + struct ssusb_mtk *ssusb = usb_role_switch_get_drvdata(sw);
> > enum usb_role role;
> >
> > role = ssusb->is_host ? USB_ROLE_HOST : USB_ROLE_DEVICE;
> > @@ -356,6 +356,7 @@ static int ssusb_role_sw_register(struct otg_switch_mtk *otg_sx)
> > role_sx_desc.set = ssusb_role_sw_set;
> > role_sx_desc.get = ssusb_role_sw_get;
> > role_sx_desc.fwnode = dev_fwnode(ssusb->dev);
> > + role_sx_desc.driver_data = ssusb;
> > otg_sx->role_sw = usb_role_switch_register(ssusb->dev, &role_sx_desc);
> >
> > return PTR_ERR_OR_ZERO(otg_sx->role_sw);
> > diff --git a/drivers/usb/musb/mediatek.c b/drivers/usb/musb/mediatek.c
> > index 6b88c2f5d970..703b98d71180 100644
> > --- a/drivers/usb/musb/mediatek.c
> > +++ b/drivers/usb/musb/mediatek.c
> > @@ -115,9 +115,9 @@ static void mtk_musb_clks_disable(struct mtk_glue *glue)
> > clk_disable_unprepare(glue->main);
> > }
> >
> > -static int musb_usb_role_sx_set(struct device *dev, enum usb_role role)
> > +static int musb_usb_role_sx_set(struct usb_role_switch *sw, enum usb_role role)
> > {
> > - struct mtk_glue *glue = dev_get_drvdata(dev);
> > + struct mtk_glue *glue = usb_role_switch_get_drvdata(sw);
> > struct musb *musb = glue->musb;
> > u8 devctl = readb(musb->mregs + MUSB_DEVCTL);
> > enum usb_role new_role;
> > @@ -168,9 +168,9 @@ static int musb_usb_role_sx_set(struct device *dev, enum usb_role role)
> > return 0;
> > }
> >
> > -static enum usb_role musb_usb_role_sx_get(struct device *dev)
> > +static enum usb_role musb_usb_role_sx_get(struct usb_role_switch *sw)
> > {
> > - struct mtk_glue *glue = dev_get_drvdata(dev);
> > + struct mtk_glue *glue = usb_role_switch_get_drvdata(sw);
> >
> > return glue->role;
> > }
> > @@ -182,6 +182,7 @@ static int mtk_otg_switch_init(struct mtk_glue *glue)
> > role_sx_desc.set = musb_usb_role_sx_set;
> > role_sx_desc.get = musb_usb_role_sx_get;
> > role_sx_desc.fwnode = dev_fwnode(glue->dev);
> > + role_sx_desc.driver_data = glue;
> > glue->role_sw = usb_role_switch_register(glue->dev, &role_sx_desc);
> >
> > return PTR_ERR_OR_ZERO(glue->role_sw);
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index f3132d231599..d5e57d26c31f 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -48,7 +48,7 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
> >
> > mutex_lock(&sw->lock);
> >
> > - ret = sw->set(sw->dev.parent, role);
> > + ret = sw->set(sw, role);
> > if (!ret)
> > sw->role = role;
> >
> > @@ -75,7 +75,7 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
> > mutex_lock(&sw->lock);
> >
> > if (sw->get)
> > - role = sw->get(sw->dev.parent);
> > + role = sw->get(sw);
> > else
> > role = sw->role;
> >
> > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > index 80d6559bbcb2..5c96e929acea 100644
> > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > @@ -42,6 +42,7 @@
> > #define DRV_NAME "intel_xhci_usb_sw"
> >
> > struct intel_xhci_usb_data {
> > + struct device *dev;
> > struct usb_role_switch *role_sw;
> > void __iomem *base;
> > bool enable_sw_switch;
> > @@ -51,9 +52,10 @@ static const struct software_node intel_xhci_usb_node = {
> > "intel-xhci-usb-sw",
> > };
> >
> > -static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> > +static int intel_xhci_usb_set_role(struct usb_role_switch *sw,
> > + enum usb_role role)
> > {
> > - struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
> > + struct intel_xhci_usb_data *data = usb_role_switch_get_drvdata(sw);
> > unsigned long timeout;
> > acpi_status status;
> > u32 glk, val;
> > @@ -66,11 +68,11 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> > */
> > status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
> > if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
> > - dev_err(dev, "Error could not acquire lock\n");
> > + dev_err(data->dev, "Error could not acquire lock\n");
> > return -EIO;
> > }
> >
> > - pm_runtime_get_sync(dev);
> > + pm_runtime_get_sync(data->dev);
> >
> > /*
> > * Set idpin value as requested.
> > @@ -112,7 +114,7 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> > do {
> > val = readl(data->base + DUAL_ROLE_CFG1);
> > if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) {
> > - pm_runtime_put(dev);
> > + pm_runtime_put(data->dev);
> > return 0;
> > }
> >
> > @@ -120,21 +122,21 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> > usleep_range(5000, 10000);
> > } while (time_before(jiffies, timeout));
> >
> > - pm_runtime_put(dev);
> > + pm_runtime_put(data->dev);
> >
> > - dev_warn(dev, "Timeout waiting for role-switch\n");
> > + dev_warn(data->dev, "Timeout waiting for role-switch\n");
> > return -ETIMEDOUT;
> > }
> >
> > -static enum usb_role intel_xhci_usb_get_role(struct device *dev)
> > +static enum usb_role intel_xhci_usb_get_role(struct usb_role_switch *sw)
> > {
> > - struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
> > + struct intel_xhci_usb_data *data = usb_role_switch_get_drvdata(sw);
> > enum usb_role role;
> > u32 val;
> >
> > - pm_runtime_get_sync(dev);
> > + pm_runtime_get_sync(data->dev);
> > val = readl(data->base + DUAL_ROLE_CFG0);
> > - pm_runtime_put(dev);
> > + pm_runtime_put(data->dev);
> >
> > if (!(val & SW_IDPIN))
> > role = USB_ROLE_HOST;
> > @@ -175,7 +177,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
> > sw_desc.get = intel_xhci_usb_get_role,
> > sw_desc.allow_userspace_control = true,
> > sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
> > + sw_desc.driver_data = data;
> >
> > + data->dev = dev;
> > data->enable_sw_switch = !device_property_read_bool(dev,
> > "sw_switch_disable");
> >
> > diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> > index 02dae936cebd..c028ba8029ad 100644
> > --- a/include/linux/usb/role.h
> > +++ b/include/linux/usb/role.h
> > @@ -13,8 +13,9 @@ enum usb_role {
> > USB_ROLE_DEVICE,
> > };
> >
> > -typedef int (*usb_role_switch_set_t)(struct device *dev, enum usb_role role);
> > -typedef enum usb_role (*usb_role_switch_get_t)(struct device *dev);
> > +typedef int (*usb_role_switch_set_t)(struct usb_role_switch *sw,
> > + enum usb_role role);
> > +typedef enum usb_role (*usb_role_switch_get_t)(struct usb_role_switch *sw);
> >
> > /**
> > * struct usb_role_switch_desc - USB Role Switch Descriptor
> > --
> > 2.25.0
>
> thanks,
>
> --
> heikki
--
Thanks,
Peter Chen
Hi,
On Tue, Feb 18, 2020 at 07:23:41AM +0000, Peter Chen wrote:
> > > @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > > }
> > >
> > > if (ci_role_switch.fwnode) {
> > > + ci_role_switch.driver_data = ci;
> > > ci->role_switch = usb_role_switch_register(dev,
> > > &ci_role_switch);
>
> Why the struct usb_role_switch_desc needs drvdata, the struct
> usb_role_switch has already one?
I'm assuming that you are asking why not just register the switch,
and then call usb_role_switch_set_drvdata(), right?
That may create a race condition where the switch is accessed before
the driver data is available. That can happen for example if the
switch is exposed to the user space.
To play it safe, supplying the driver data as part of the descriptor.
That way we can be sure that the driver data is always available
the moment the switch is registered.
thanks,
--
heikki
On 20-02-18 14:25:45, Heikki Krogerus wrote:
> Hi,
>
> On Tue, Feb 18, 2020 at 07:23:41AM +0000, Peter Chen wrote:
> > > > @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > > > }
> > > >
> > > > if (ci_role_switch.fwnode) {
> > > > + ci_role_switch.driver_data = ci;
> > > > ci->role_switch = usb_role_switch_register(dev,
> > > > &ci_role_switch);
> >
> > Why the struct usb_role_switch_desc needs drvdata, the struct
> > usb_role_switch has already one?
>
> I'm assuming that you are asking why not just register the switch,
> and then call usb_role_switch_set_drvdata(), right?
Yes.
>
> That may create a race condition where the switch is accessed before
> the driver data is available. That can happen for example if the
> switch is exposed to the user space.
>
> To play it safe, supplying the driver data as part of the descriptor.
> That way we can be sure that the driver data is always available
> the moment the switch is registered.
>
Then, you may use the uniform way for the driver. Some may have
race condition like you said.
--
Thanks,
Peter Chen
On Wed, Feb 19, 2020 at 01:58:38AM +0000, Peter Chen wrote:
> On 20-02-18 14:25:45, Heikki Krogerus wrote:
> > Hi,
> >
> > On Tue, Feb 18, 2020 at 07:23:41AM +0000, Peter Chen wrote:
> > > > > @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > > > > }
> > > > >
> > > > > if (ci_role_switch.fwnode) {
> > > > > + ci_role_switch.driver_data = ci;
> > > > > ci->role_switch = usb_role_switch_register(dev,
> > > > > &ci_role_switch);
> > >
> > > Why the struct usb_role_switch_desc needs drvdata, the struct
> > > usb_role_switch has already one?
> >
> > I'm assuming that you are asking why not just register the switch,
> > and then call usb_role_switch_set_drvdata(), right?
>
> Yes.
>
> >
> > That may create a race condition where the switch is accessed before
> > the driver data is available. That can happen for example if the
> > switch is exposed to the user space.
> >
> > To play it safe, supplying the driver data as part of the descriptor.
> > That way we can be sure that the driver data is always available
> > the moment the switch is registered.
> >
>
> Then, you may use the uniform way for the driver. Some may have
> race condition like you said.
Uniform way for the driver?
--
heikki
On 20-02-19 15:38:15, Heikki Krogerus wrote:
> On Wed, Feb 19, 2020 at 01:58:38AM +0000, Peter Chen wrote:
> > On 20-02-18 14:25:45, Heikki Krogerus wrote:
> > > Hi,
> > >
> > > On Tue, Feb 18, 2020 at 07:23:41AM +0000, Peter Chen wrote:
> > > > > > @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > > > > > }
> > > > > >
> > > > > > if (ci_role_switch.fwnode) {
> > > > > > + ci_role_switch.driver_data = ci;
> > > > > > ci->role_switch = usb_role_switch_register(dev,
> > > > > > &ci_role_switch);
> > > >
> > > > Why the struct usb_role_switch_desc needs drvdata, the struct
> > > > usb_role_switch has already one?
> > >
> > > I'm assuming that you are asking why not just register the switch,
> > > and then call usb_role_switch_set_drvdata(), right?
> >
> > Yes.
> >
> > >
> > > That may create a race condition where the switch is accessed before
> > > the driver data is available. That can happen for example if the
> > > switch is exposed to the user space.
> > >
> > > To play it safe, supplying the driver data as part of the descriptor.
> > > That way we can be sure that the driver data is always available
> > > the moment the switch is registered.
> > >
> >
> > Then, you may use the uniform way for the driver. Some may have
> > race condition like you said.
>
> Uniform way for the driver?
>
Sorry, unify way. Take chipidea and cdns3 as an example, at chipidea you
use struct usb_role_switch_desc
+ ci_role_switch.driver_data = ci;
ci->role_switch = usb_role_switch_register(dev,
&ci_role_switch);
But at cdns3 side, you set usb_role_switch drvdata directly.
+ usb_role_switch_set_drvdata(cdns->role_sw, cdns);
But according to your comments, all the driver needs to use chipidea's
way to avoid race condition.
--
Thanks,
Peter Chen
On Wed, Feb 19, 2020 at 02:09:54PM +0000, Peter Chen wrote:
> On 20-02-19 15:38:15, Heikki Krogerus wrote:
> > On Wed, Feb 19, 2020 at 01:58:38AM +0000, Peter Chen wrote:
> > > On 20-02-18 14:25:45, Heikki Krogerus wrote:
> > > > Hi,
> > > >
> > > > On Tue, Feb 18, 2020 at 07:23:41AM +0000, Peter Chen wrote:
> > > > > > > @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > > > > > > }
> > > > > > >
> > > > > > > if (ci_role_switch.fwnode) {
> > > > > > > + ci_role_switch.driver_data = ci;
> > > > > > > ci->role_switch = usb_role_switch_register(dev,
> > > > > > > &ci_role_switch);
> > > > >
> > > > > Why the struct usb_role_switch_desc needs drvdata, the struct
> > > > > usb_role_switch has already one?
> > > >
> > > > I'm assuming that you are asking why not just register the switch,
> > > > and then call usb_role_switch_set_drvdata(), right?
> > >
> > > Yes.
> > >
> > > >
> > > > That may create a race condition where the switch is accessed before
> > > > the driver data is available. That can happen for example if the
> > > > switch is exposed to the user space.
> > > >
> > > > To play it safe, supplying the driver data as part of the descriptor.
> > > > That way we can be sure that the driver data is always available
> > > > the moment the switch is registered.
> > > >
> > >
> > > Then, you may use the uniform way for the driver. Some may have
> > > race condition like you said.
> >
> > Uniform way for the driver?
> >
>
> Sorry, unify way. Take chipidea and cdns3 as an example, at chipidea you
> use struct usb_role_switch_desc
>
> + ci_role_switch.driver_data = ci;
> ci->role_switch = usb_role_switch_register(dev,
> &ci_role_switch);
>
> But at cdns3 side, you set usb_role_switch drvdata directly.
> + usb_role_switch_set_drvdata(cdns->role_sw, cdns);
>
> But according to your comments, all the driver needs to use chipidea's
> way to avoid race condition.
OK, I'll fix that.
thanks,
--
heikki