| Message ID | 20240730222439.3469-8-quic_eserrao@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable EUD on Qualcomm sm8450 SoC | expand |
On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote: > Since EUD is physically present between the USB connector and > the USB controller, it should relay the usb role notifications > from the connector. Hence register a role switch handler to > process and relay these roles to the USB controller. This results > in a common framework to send both connector related events > and eud attach/detach events to the USB controller. > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> > --- > drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++--------- > 1 file changed, 69 insertions(+), 22 deletions(-) > > diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c > index 3de7d465912c..9a49c934e8cf 100644 > --- a/drivers/usb/misc/qcom_eud.c > +++ b/drivers/usb/misc/qcom_eud.c > @@ -10,6 +10,7 @@ > #include <linux/iopoll.h> > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/of.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > @@ -35,12 +36,16 @@ struct eud_chip { > struct device *dev; > struct usb_role_switch *role_sw; > struct phy *usb2_phy; > + > + /* mode lock */ > + struct mutex mutex; > void __iomem *base; > void __iomem *mode_mgr; > unsigned int int_status; > int irq; > bool enabled; > bool usb_attached; > + enum usb_role current_role; > }; > > static int eud_phy_enable(struct eud_chip *chip) > @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip) > phy_exit(chip->usb2_phy); > } > > +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role) > +{ > + struct usb_role_switch *sw; > + int ret = 0; > + > + mutex_lock(&chip->mutex); > + > + /* Avoid duplicate role handling */ > + if (role == chip->current_role) > + goto err; > + > + sw = usb_role_switch_get(chip->dev); Why isn't chip->role_sw good enough? Why do you need to get it each time? > + if (IS_ERR_OR_NULL(sw)) { > + dev_err(chip->dev, "failed to get usb switch\n"); > + ret = -EINVAL; > + goto err; > + } > + > + ret = usb_role_switch_set_role(sw, role); > + usb_role_switch_put(sw); > + > + if (ret) { > + dev_err(chip->dev, "failed to set role\n"); > + goto err; > + } > + chip->current_role = role; > +err: > + mutex_unlock(&chip->mutex); > + > + return ret; > +} > + > static int enable_eud(struct eud_chip *priv) > { > int ret; > @@ -77,7 +114,7 @@ static int enable_eud(struct eud_chip *priv) > priv->base + EUD_REG_INT1_EN_MASK); > writel(1, priv->mode_mgr + EUD_REG_EUD_EN2); > > - return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE); > + return ret; > } > > static void disable_eud(struct eud_chip *priv) > @@ -106,15 +143,20 @@ static ssize_t enable_store(struct device *dev, > if (kstrtobool(buf, &enable)) > return -EINVAL; > > + /* EUD enable is applicable only in DEVICE mode */ > + if (enable && chip->current_role != USB_ROLE_DEVICE) > + return -EINVAL; > + > if (enable) { > ret = enable_eud(chip); > - if (!ret) > - chip->enabled = enable; > - else > - disable_eud(chip); > + if (ret) { > + dev_err(chip->dev, "failed to enable eud\n"); > + return count; > + } > } else { > disable_eud(chip); > } > + chip->enabled = enable; > > return count; > } > @@ -185,11 +227,9 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data) > int ret; > > if (chip->usb_attached) > - ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE); > + ret = eud_usb_role_set(chip, USB_ROLE_DEVICE); > else > - ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST); > - if (ret) > - dev_err(chip->dev, "failed to set role switch\n"); > + ret = eud_usb_role_set(chip, USB_ROLE_HOST); > > /* set and clear vbus_int_clr[0] to clear interrupt */ > writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR); > @@ -198,16 +238,18 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data) > return IRQ_HANDLED; > } > > -static void eud_role_switch_release(void *data) > +static int eud_usb_role_switch_set(struct usb_role_switch *sw, > + enum usb_role role) > { > - struct eud_chip *chip = data; > + struct eud_chip *chip = usb_role_switch_get_drvdata(sw); > > - usb_role_switch_put(chip->role_sw); > + return eud_usb_role_set(chip, role); > } > > static int eud_probe(struct platform_device *pdev) > { > struct eud_chip *chip; > + struct usb_role_switch_desc eud_role_switch = {NULL}; > int ret; > > chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > @@ -221,16 +263,6 @@ static int eud_probe(struct platform_device *pdev) > return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy), > "no usb2 phy configured\n"); > > - chip->role_sw = usb_role_switch_get(&pdev->dev); > - if (IS_ERR(chip->role_sw)) > - return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw), > - "failed to get role switch\n"); > - > - ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip); > - if (ret) > - return dev_err_probe(chip->dev, ret, > - "failed to add role switch release action\n"); > - > chip->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(chip->base)) > return PTR_ERR(chip->base); > @@ -248,6 +280,18 @@ static int eud_probe(struct platform_device *pdev) > if (ret) > return dev_err_probe(chip->dev, ret, "failed to allocate irq\n"); > > + eud_role_switch.fwnode = dev_fwnode(chip->dev); > + eud_role_switch.set = eud_usb_role_switch_set; > + eud_role_switch.get = NULL; > + eud_role_switch.driver_data = chip; > + chip->role_sw = usb_role_switch_register(chip->dev, &eud_role_switch); > + > + if (IS_ERR(chip->role_sw)) > + return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw), > + "failed to register role switch\n"); > + > + mutex_init(&chip->mutex); please move mutex_init earlier. > + > enable_irq_wake(chip->irq); > > platform_set_drvdata(pdev, chip); > @@ -262,6 +306,9 @@ static void eud_remove(struct platform_device *pdev) > if (chip->enabled) > disable_eud(chip); > > + if (chip->role_sw) > + usb_role_switch_unregister(chip->role_sw); > + > device_init_wakeup(&pdev->dev, false); > disable_irq_wake(chip->irq); > } > -- > 2.17.1 >
On 7/31/2024 6:06 AM, Dmitry Baryshkov wrote: > On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote: >> Since EUD is physically present between the USB connector and >> the USB controller, it should relay the usb role notifications >> from the connector. Hence register a role switch handler to >> process and relay these roles to the USB controller. This results >> in a common framework to send both connector related events >> and eud attach/detach events to the USB controller. >> >> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> >> --- >> drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++--------- >> 1 file changed, 69 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c >> index 3de7d465912c..9a49c934e8cf 100644 >> --- a/drivers/usb/misc/qcom_eud.c >> +++ b/drivers/usb/misc/qcom_eud.c >> @@ -10,6 +10,7 @@ >> #include <linux/iopoll.h> >> #include <linux/kernel.h> >> #include <linux/module.h> >> +#include <linux/mutex.h> >> #include <linux/of.h> >> #include <linux/phy/phy.h> >> #include <linux/platform_device.h> >> @@ -35,12 +36,16 @@ struct eud_chip { >> struct device *dev; >> struct usb_role_switch *role_sw; >> struct phy *usb2_phy; >> + >> + /* mode lock */ >> + struct mutex mutex; >> void __iomem *base; >> void __iomem *mode_mgr; >> unsigned int int_status; >> int irq; >> bool enabled; >> bool usb_attached; >> + enum usb_role current_role; >> }; >> >> static int eud_phy_enable(struct eud_chip *chip) >> @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip) >> phy_exit(chip->usb2_phy); >> } >> >> +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role) >> +{ >> + struct usb_role_switch *sw; >> + int ret = 0; >> + >> + mutex_lock(&chip->mutex); >> + >> + /* Avoid duplicate role handling */ >> + if (role == chip->current_role) >> + goto err; >> + >> + sw = usb_role_switch_get(chip->dev); > > Why isn't chip->role_sw good enough? Why do you need to get it each > time? > Hi Dmitry chip->role_sw is the eud role switch handler to receive role switch notifications from the USB connector. The 'sw' I am getting above is the role switch handler of the USB controller. As per this design, EUD receives role switch notification from the connector (via chip->role_sw) and then relays it to the 'sw' switch handler of the USB controller. Thanks Elson >> + if (IS_ERR_OR_NULL(sw)) { >> + dev_err(chip->dev, "failed to get usb switch\n"); >> + ret = -EINVAL; >> + goto err; >> + } >> + >> + ret = usb_role_switch_set_role(sw, role); >> + usb_role_switch_put(sw); >> + >> + if (ret) { >> + dev_err(chip->dev, "failed to set role\n"); >> + goto err; >> + } >> + chip->current_role = role; >> +err: >> + mutex_unlock(&chip->mutex); >> + >> + return ret; >> +} >> + >> static int enable_eud(struct eud_chip *priv) >> { >> int ret; >> @@ -77,7 +114,7 @@ static int enable_eud(struct eud_chip *priv) >> priv->base + EUD_REG_INT1_EN_MASK); >> writel(1, priv->mode_mgr + EUD_REG_EUD_EN2); >> >> - return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE); >> + return ret; >> } >> >> static void disable_eud(struct eud_chip *priv) >> @@ -106,15 +143,20 @@ static ssize_t enable_store(struct device *dev, >> if (kstrtobool(buf, &enable)) >> return -EINVAL; >> >> + /* EUD enable is applicable only in DEVICE mode */ >> + if (enable && chip->current_role != USB_ROLE_DEVICE) >> + return -EINVAL; >> + >> if (enable) { >> ret = enable_eud(chip); >> - if (!ret) >> - chip->enabled = enable; >> - else >> - disable_eud(chip); >> + if (ret) { >> + dev_err(chip->dev, "failed to enable eud\n"); >> + return count; >> + } >> } else { >> disable_eud(chip); >> } >> + chip->enabled = enable; >> >> return count; >> } >> @@ -185,11 +227,9 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data) >> int ret; >> >> if (chip->usb_attached) >> - ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE); >> + ret = eud_usb_role_set(chip, USB_ROLE_DEVICE); >> else >> - ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST); >> - if (ret) >> - dev_err(chip->dev, "failed to set role switch\n"); >> + ret = eud_usb_role_set(chip, USB_ROLE_HOST); >> >> /* set and clear vbus_int_clr[0] to clear interrupt */ >> writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR); >> @@ -198,16 +238,18 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data) >> return IRQ_HANDLED; >> } >> >> -static void eud_role_switch_release(void *data) >> +static int eud_usb_role_switch_set(struct usb_role_switch *sw, >> + enum usb_role role) >> { >> - struct eud_chip *chip = data; >> + struct eud_chip *chip = usb_role_switch_get_drvdata(sw); >> >> - usb_role_switch_put(chip->role_sw); >> + return eud_usb_role_set(chip, role); >> } >> >> static int eud_probe(struct platform_device *pdev) >> { >> struct eud_chip *chip; >> + struct usb_role_switch_desc eud_role_switch = {NULL}; >> int ret; >> >> chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); >> @@ -221,16 +263,6 @@ static int eud_probe(struct platform_device *pdev) >> return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy), >> "no usb2 phy configured\n"); >> >> - chip->role_sw = usb_role_switch_get(&pdev->dev); >> - if (IS_ERR(chip->role_sw)) >> - return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw), >> - "failed to get role switch\n"); >> - >> - ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip); >> - if (ret) >> - return dev_err_probe(chip->dev, ret, >> - "failed to add role switch release action\n"); >> - >> chip->base = devm_platform_ioremap_resource(pdev, 0); >> if (IS_ERR(chip->base)) >> return PTR_ERR(chip->base); >> @@ -248,6 +280,18 @@ static int eud_probe(struct platform_device *pdev) >> if (ret) >> return dev_err_probe(chip->dev, ret, "failed to allocate irq\n"); >> >> + eud_role_switch.fwnode = dev_fwnode(chip->dev); >> + eud_role_switch.set = eud_usb_role_switch_set; >> + eud_role_switch.get = NULL; >> + eud_role_switch.driver_data = chip; >> + chip->role_sw = usb_role_switch_register(chip->dev, &eud_role_switch); >> + >> + if (IS_ERR(chip->role_sw)) >> + return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw), >> + "failed to register role switch\n"); >> + >> + mutex_init(&chip->mutex); > > please move mutex_init earlier. > Ack >> + >> enable_irq_wake(chip->irq); >> >> platform_set_drvdata(pdev, chip); >> @@ -262,6 +306,9 @@ static void eud_remove(struct platform_device *pdev) >> if (chip->enabled) >> disable_eud(chip); >> >> + if (chip->role_sw) >> + usb_role_switch_unregister(chip->role_sw); >> + >> device_init_wakeup(&pdev->dev, false); >> disable_irq_wake(chip->irq); >> } >> -- >> 2.17.1 >> >
On Wed, Jul 31, 2024 at 05:51:17PM GMT, Elson Serrao wrote: > > > On 7/31/2024 6:06 AM, Dmitry Baryshkov wrote: > > On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote: > >> Since EUD is physically present between the USB connector and > >> the USB controller, it should relay the usb role notifications > >> from the connector. Hence register a role switch handler to > >> process and relay these roles to the USB controller. This results > >> in a common framework to send both connector related events > >> and eud attach/detach events to the USB controller. > >> > >> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> > >> --- > >> drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++--------- > >> 1 file changed, 69 insertions(+), 22 deletions(-) > >> > >> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c > >> index 3de7d465912c..9a49c934e8cf 100644 > >> --- a/drivers/usb/misc/qcom_eud.c > >> +++ b/drivers/usb/misc/qcom_eud.c > >> @@ -10,6 +10,7 @@ > >> #include <linux/iopoll.h> > >> #include <linux/kernel.h> > >> #include <linux/module.h> > >> +#include <linux/mutex.h> > >> #include <linux/of.h> > >> #include <linux/phy/phy.h> > >> #include <linux/platform_device.h> > >> @@ -35,12 +36,16 @@ struct eud_chip { > >> struct device *dev; > >> struct usb_role_switch *role_sw; > >> struct phy *usb2_phy; > >> + > >> + /* mode lock */ > >> + struct mutex mutex; > >> void __iomem *base; > >> void __iomem *mode_mgr; > >> unsigned int int_status; > >> int irq; > >> bool enabled; > >> bool usb_attached; > >> + enum usb_role current_role; > >> }; > >> > >> static int eud_phy_enable(struct eud_chip *chip) > >> @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip) > >> phy_exit(chip->usb2_phy); > >> } > >> > >> +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role) > >> +{ > >> + struct usb_role_switch *sw; > >> + int ret = 0; > >> + > >> + mutex_lock(&chip->mutex); > >> + > >> + /* Avoid duplicate role handling */ > >> + if (role == chip->current_role) > >> + goto err; > >> + > >> + sw = usb_role_switch_get(chip->dev); > > > > Why isn't chip->role_sw good enough? Why do you need to get it each > > time? > > > > Hi Dmitry > > chip->role_sw is the eud role switch handler to receive role switch notifications from the > USB connector. The 'sw' I am getting above is the role switch handler of the USB controller. > As per this design, EUD receives role switch notification from the connector > (via chip->role_sw) and then relays it to the 'sw' switch handler of the USB controller. The fact that you have repurposed existing structure field is not a waiver for the inefficiency. So what about keeping chip->role_sw as is and adding _new_ field for the self-provided role switch?
Hi Elson,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.11-rc1 next-20240801]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Elson-Roy-Serrao/dt-bindings-soc-qcom-eud-Add-phy-related-bindings/20240801-210521
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240730222439.3469-8-quic_eserrao%40quicinc.com
patch subject: [PATCH 7/8] usb: misc: eud: Handle usb role switch notifications
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240802/202408020600.vU0uKLa7-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240802/202408020600.vU0uKLa7-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408020600.vU0uKLa7-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/usb/misc/qcom_eud.c: In function 'handle_eud_irq_thread':
>> drivers/usb/misc/qcom_eud.c:227:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
227 | int ret;
| ^~~
vim +/ret +227 drivers/usb/misc/qcom_eud.c
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 223
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 224 static irqreturn_t handle_eud_irq_thread(int irq, void *data)
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 225 {
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 226 struct eud_chip *chip = data;
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 @227 int ret;
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 228
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 229 if (chip->usb_attached)
c007e96bfd0471 Elson Roy Serrao 2024-07-30 230 ret = eud_usb_role_set(chip, USB_ROLE_DEVICE);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 231 else
c007e96bfd0471 Elson Roy Serrao 2024-07-30 232 ret = eud_usb_role_set(chip, USB_ROLE_HOST);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 233
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 234 /* set and clear vbus_int_clr[0] to clear interrupt */
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 235 writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 236 writel(0, chip->base + EUD_REG_VBUS_INT_CLR);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 237
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 238 return IRQ_HANDLED;
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 239 }
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 240
diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c index 3de7d465912c..9a49c934e8cf 100644 --- a/drivers/usb/misc/qcom_eud.c +++ b/drivers/usb/misc/qcom_eud.c @@ -10,6 +10,7 @@ #include <linux/iopoll.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/of.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> @@ -35,12 +36,16 @@ struct eud_chip { struct device *dev; struct usb_role_switch *role_sw; struct phy *usb2_phy; + + /* mode lock */ + struct mutex mutex; void __iomem *base; void __iomem *mode_mgr; unsigned int int_status; int irq; bool enabled; bool usb_attached; + enum usb_role current_role; }; static int eud_phy_enable(struct eud_chip *chip) @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip) phy_exit(chip->usb2_phy); } +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role) +{ + struct usb_role_switch *sw; + int ret = 0; + + mutex_lock(&chip->mutex); + + /* Avoid duplicate role handling */ + if (role == chip->current_role) + goto err; + + sw = usb_role_switch_get(chip->dev); + if (IS_ERR_OR_NULL(sw)) { + dev_err(chip->dev, "failed to get usb switch\n"); + ret = -EINVAL; + goto err; + } + + ret = usb_role_switch_set_role(sw, role); + usb_role_switch_put(sw); + + if (ret) { + dev_err(chip->dev, "failed to set role\n"); + goto err; + } + chip->current_role = role; +err: + mutex_unlock(&chip->mutex); + + return ret; +} + static int enable_eud(struct eud_chip *priv) { int ret; @@ -77,7 +114,7 @@ static int enable_eud(struct eud_chip *priv) priv->base + EUD_REG_INT1_EN_MASK); writel(1, priv->mode_mgr + EUD_REG_EUD_EN2); - return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE); + return ret; } static void disable_eud(struct eud_chip *priv) @@ -106,15 +143,20 @@ static ssize_t enable_store(struct device *dev, if (kstrtobool(buf, &enable)) return -EINVAL; + /* EUD enable is applicable only in DEVICE mode */ + if (enable && chip->current_role != USB_ROLE_DEVICE) + return -EINVAL; + if (enable) { ret = enable_eud(chip); - if (!ret) - chip->enabled = enable; - else - disable_eud(chip); + if (ret) { + dev_err(chip->dev, "failed to enable eud\n"); + return count; + } } else { disable_eud(chip); } + chip->enabled = enable; return count; } @@ -185,11 +227,9 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data) int ret; if (chip->usb_attached) - ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE); + ret = eud_usb_role_set(chip, USB_ROLE_DEVICE); else - ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST); - if (ret) - dev_err(chip->dev, "failed to set role switch\n"); + ret = eud_usb_role_set(chip, USB_ROLE_HOST); /* set and clear vbus_int_clr[0] to clear interrupt */ writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR); @@ -198,16 +238,18 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data) return IRQ_HANDLED; } -static void eud_role_switch_release(void *data) +static int eud_usb_role_switch_set(struct usb_role_switch *sw, + enum usb_role role) { - struct eud_chip *chip = data; + struct eud_chip *chip = usb_role_switch_get_drvdata(sw); - usb_role_switch_put(chip->role_sw); + return eud_usb_role_set(chip, role); } static int eud_probe(struct platform_device *pdev) { struct eud_chip *chip; + struct usb_role_switch_desc eud_role_switch = {NULL}; int ret; chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); @@ -221,16 +263,6 @@ static int eud_probe(struct platform_device *pdev) return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy), "no usb2 phy configured\n"); - chip->role_sw = usb_role_switch_get(&pdev->dev); - if (IS_ERR(chip->role_sw)) - return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw), - "failed to get role switch\n"); - - ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip); - if (ret) - return dev_err_probe(chip->dev, ret, - "failed to add role switch release action\n"); - chip->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(chip->base)) return PTR_ERR(chip->base); @@ -248,6 +280,18 @@ static int eud_probe(struct platform_device *pdev) if (ret) return dev_err_probe(chip->dev, ret, "failed to allocate irq\n"); + eud_role_switch.fwnode = dev_fwnode(chip->dev); + eud_role_switch.set = eud_usb_role_switch_set; + eud_role_switch.get = NULL; + eud_role_switch.driver_data = chip; + chip->role_sw = usb_role_switch_register(chip->dev, &eud_role_switch); + + if (IS_ERR(chip->role_sw)) + return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw), + "failed to register role switch\n"); + + mutex_init(&chip->mutex); + enable_irq_wake(chip->irq); platform_set_drvdata(pdev, chip); @@ -262,6 +306,9 @@ static void eud_remove(struct platform_device *pdev) if (chip->enabled) disable_eud(chip); + if (chip->role_sw) + usb_role_switch_unregister(chip->role_sw); + device_init_wakeup(&pdev->dev, false); disable_irq_wake(chip->irq); }
Since EUD is physically present between the USB connector and the USB controller, it should relay the usb role notifications from the connector. Hence register a role switch handler to process and relay these roles to the USB controller. This results in a common framework to send both connector related events and eud attach/detach events to the USB controller. Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> --- drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++--------- 1 file changed, 69 insertions(+), 22 deletions(-)