[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200224131442.GA5365@b29397-desktop>
Date: Mon, 24 Feb 2020 13:14:40 +0000
From: Peter Chen <peter.chen@....com>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Benson Leung <bleung@...omium.org>,
Prashant Malani <pmalani@...omium.org>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
Felipe Balbi <balbi@...nel.org>,
Chunfeng Yun <chunfeng.yun@...iatek.com>,
Bin Liu <b-liu@...com>
Subject: Re: [PATCH v2 5/9] usb: roles: Provide the switch drivers handle to
the switch in the API
On 20-02-24 15:14:02, 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 <Peter.Chen@....com>
> Cc: Felipe Balbi <balbi@...nel.org>
> Cc: Chunfeng Yun <chunfeng.yun@...iatek.com>
> Cc: Bin Liu <b-liu@...com>
For chipidea and cdns3 changes:
Acked-by: Peter Chen <peter.chen@....com>
Peter
> Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> ---
> drivers/usb/cdns3/core.c | 22 ++++++++--------
> 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 | 16 +++++++-----
> drivers/usb/roles/class.c | 4 +--
> .../usb/roles/intel-xhci-usb-role-switch.c | 26 +++++++++++--------
> include/linux/usb/role.h | 5 ++--
> 10 files changed, 77 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index c2123ef8d8a3..82af9d2b33b9 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);
> @@ -423,12 +423,6 @@ static int cdns3_role_set(struct device *dev, enum usb_role role)
> return ret;
> }
>
> -static const struct usb_role_switch_desc cdns3_switch_desc = {
> - .set = cdns3_role_set,
> - .get = cdns3_role_get,
> - .allow_userspace_control = true,
> -};
> -
> /**
> * cdns3_probe - probe for cdns3 core device
> * @pdev: Pointer to cdns3 core platform device
> @@ -437,6 +431,7 @@ static const struct usb_role_switch_desc cdns3_switch_desc = {
> */
> static int cdns3_probe(struct platform_device *pdev)
> {
> + struct usb_role_switch_desc sw_desc = { };
> struct device *dev = &pdev->dev;
> struct resource *res;
> struct cdns3 *cdns;
> @@ -529,7 +524,12 @@ static int cdns3_probe(struct platform_device *pdev)
> if (ret)
> goto err3;
>
> - cdns->role_sw = usb_role_switch_register(dev, &cdns3_switch_desc);
> + sw_desc.set = cdns3_role_set,
> + sw_desc.get = cdns3_role_get,
> + sw_desc.allow_userspace_control = true,
> + sw_desc.driver_data = cdns;
> +
> + cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
> if (IS_ERR(cdns->role_sw)) {
> ret = PTR_ERR(cdns->role_sw);
> dev_warn(dev, "Unable to register Role Switch\n");
> 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..a627f4133d6b 100644
> --- a/drivers/usb/musb/mediatek.c
> +++ b/drivers/usb/musb/mediatek.c
> @@ -115,9 +115,8 @@ 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 mtk_otg_switch_set(struct mtk_glue *glue, enum usb_role role)
> {
> - struct mtk_glue *glue = dev_get_drvdata(dev);
> struct musb *musb = glue->musb;
> u8 devctl = readb(musb->mregs + MUSB_DEVCTL);
> enum usb_role new_role;
> @@ -168,9 +167,14 @@ 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 int musb_usb_role_sx_set(struct usb_role_switch *sw, enum usb_role role)
> {
> - struct mtk_glue *glue = dev_get_drvdata(dev);
> + return mtk_otg_switch_set(usb_role_switch_get_drvdata(sw), role);
> +}
> +
> +static enum usb_role musb_usb_role_sx_get(struct usb_role_switch *sw)
> +{
> + struct mtk_glue *glue = usb_role_switch_get_drvdata(sw);
>
> return glue->role;
> }
> @@ -182,6 +186,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);
> @@ -288,8 +293,7 @@ static int mtk_musb_set_mode(struct musb *musb, u8 mode)
> return -EINVAL;
> }
>
> - glue->role = new_role;
> - musb_usb_role_sx_set(dev, glue->role);
> + mtk_otg_switch_set(glue, new_role);
> return 0;
> }
>
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index 49511d446410..11e49213176f 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,
Peter Chen
Powered by blists - more mailing lists