| Message ID | 20220131192102.4115473-1-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: Prioritize extcon over USB role switching API | expand |
Andrey Smirnov wrote: > It is necessary that: > > ROLE_SWITCH && device_property_read_bool(dwc->dev, "usb-role-switch") > > is true in order for dwc3_get_dr_mode() to _not_ force us from OTG to > PERIPHERAL mode here: > > if (mode == USB_DR_MODE_OTG && > (!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) || > !device_property_read_bool(dwc->dev, "usb-role-switch")) && > !DWC3_VER_IS_PRIOR(DWC3, 330A)) > mode = USB_DR_MODE_PERIPHERAL; > > and dwc3_drd_init() to be called later in dwc3_core_init_mode(). So, > to avoid always ignoring extcon device returned by dwc3_get_extcon() > change dwc3_drd_init() to check and use it first, before checking if > dwc3_setup_role_switch() should be called. > > Cc: Felipe Balbi <balbi@kernel.org> > Cc: Thinh Nguyen <thinhn@synopsys.com> > Cc: linux-usb@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > > Hopefully I didn't miss something important making this patch > unnecessary. Don't know if this is a good solution or not, part of me > thinks than maybe changing the aforementioned code in > dwc3_get_dr_mode() to account for extcon wopuld be > simpler/better. Happy to rework this. The driver either use extcon or usb-role-switch. It doesn't make sense to enable usb-role-switch for extcon just so the driver doesn't default the dr_mode to peripheral. Perhaps, this is what you're looking for? (code is not tested) diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index bffd719b8b52..a52331ea7a0d 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -86,7 +86,7 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc) * mode. If the controller supports DRD but the dr_mode is not * specified or set to OTG, then set the mode to peripheral. */ - if (mode == USB_DR_MODE_OTG && + if (mode == USB_DR_MODE_OTG && !dwc->edev && (!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) || !device_property_read_bool(dwc->dev, "usb-role-switch")) && !DWC3_VER_IS_PRIOR(DWC3, 330A)) @@ -1715,6 +1715,13 @@ static int dwc3_probe(struct platform_device *pdev) goto err2; } + dwc->edev = dwc3_get_extcon(dwc); + if (IS_ERR(dwc->edev)) { + ret = PTR_ERR(dwc->edev); + dev_err(dwc->dev, "failed to get extcon %d\n", ret); + goto err3; + } + ret = dwc3_get_dr_mode(dwc); if (ret) goto err3; diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c index 63089c7fb530..d02fcfdc74db 100644 --- a/drivers/usb/dwc3/drd.c +++ b/drivers/usb/dwc3/drd.c @@ -585,16 +585,7 @@ int dwc3_drd_init(struct dwc3 *dwc) { int ret, irq; - dwc->edev = dwc3_get_extcon(dwc); - if (IS_ERR(dwc->edev)) - return PTR_ERR(dwc->edev); - - if (ROLE_SWITCH && - device_property_read_bool(dwc->dev, "usb-role-switch")) { - ret = dwc3_setup_role_switch(dwc); - if (ret < 0) - return ret; - } else if (dwc->edev) { + if (dwc->edev) { dwc->edev_nb.notifier_call = dwc3_drd_notifier; ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST, &dwc->edev_nb); @@ -604,6 +595,11 @@ int dwc3_drd_init(struct dwc3 *dwc) } dwc3_drd_update(dwc); + } else if (ROLE_SWITCH && + device_property_read_bool(dwc->dev, "usb-role-switch")) { + ret = dwc3_setup_role_switch(dwc); + if (ret < 0) + return ret; } else { dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_OTG);
On Wed, Feb 2, 2022 at 6:54 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > Andrey Smirnov wrote: > > It is necessary that: > > > > ROLE_SWITCH && device_property_read_bool(dwc->dev, "usb-role-switch") > > > > is true in order for dwc3_get_dr_mode() to _not_ force us from OTG to > > PERIPHERAL mode here: > > > > if (mode == USB_DR_MODE_OTG && > > (!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) || > > !device_property_read_bool(dwc->dev, "usb-role-switch")) && > > !DWC3_VER_IS_PRIOR(DWC3, 330A)) > > mode = USB_DR_MODE_PERIPHERAL; > > > > and dwc3_drd_init() to be called later in dwc3_core_init_mode(). So, > > to avoid always ignoring extcon device returned by dwc3_get_extcon() > > change dwc3_drd_init() to check and use it first, before checking if > > dwc3_setup_role_switch() should be called. > > > > Cc: Felipe Balbi <balbi@kernel.org> > > Cc: Thinh Nguyen <thinhn@synopsys.com> > > Cc: linux-usb@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > --- > > > > Hopefully I didn't miss something important making this patch > > unnecessary. Don't know if this is a good solution or not, part of me > > thinks than maybe changing the aforementioned code in > > dwc3_get_dr_mode() to account for extcon wopuld be > > simpler/better. Happy to rework this. > > The driver either use extcon or usb-role-switch. It doesn't make sense to > enable usb-role-switch for extcon just so the driver doesn't default > the dr_mode to peripheral. > > Perhaps, this is what you're looking for? (code is not tested) > Yeah that kind of what I meant by "maybe changing the aforementioned code in dwc3_get_dr_mode() to account for extcon wopuld be simpler/better." OK let me rework the patch to do that. > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index bffd719b8b52..a52331ea7a0d 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -86,7 +86,7 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc) > * mode. If the controller supports DRD but the dr_mode is not > * specified or set to OTG, then set the mode to peripheral. > */ > - if (mode == USB_DR_MODE_OTG && > + if (mode == USB_DR_MODE_OTG && !dwc->edev && > (!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) || > !device_property_read_bool(dwc->dev, "usb-role-switch")) && > !DWC3_VER_IS_PRIOR(DWC3, 330A)) > @@ -1715,6 +1715,13 @@ static int dwc3_probe(struct platform_device *pdev) > goto err2; > } > > + dwc->edev = dwc3_get_extcon(dwc); > + if (IS_ERR(dwc->edev)) { > + ret = PTR_ERR(dwc->edev); > + dev_err(dwc->dev, "failed to get extcon %d\n", ret); > + goto err3; > + } > + > ret = dwc3_get_dr_mode(dwc); > if (ret) > goto err3; > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > index 63089c7fb530..d02fcfdc74db 100644 > --- a/drivers/usb/dwc3/drd.c > +++ b/drivers/usb/dwc3/drd.c > @@ -585,16 +585,7 @@ int dwc3_drd_init(struct dwc3 *dwc) > { > int ret, irq; > > - dwc->edev = dwc3_get_extcon(dwc); > - if (IS_ERR(dwc->edev)) > - return PTR_ERR(dwc->edev); > - > - if (ROLE_SWITCH && > - device_property_read_bool(dwc->dev, "usb-role-switch")) { > - ret = dwc3_setup_role_switch(dwc); > - if (ret < 0) > - return ret; > - } else if (dwc->edev) { We probably don't even need to move this around. If "usb-role-switch" isn't going to be set then that "if" could go first and not interfere. > + if (dwc->edev) { > dwc->edev_nb.notifier_call = dwc3_drd_notifier; > ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST, > &dwc->edev_nb); > @@ -604,6 +595,11 @@ int dwc3_drd_init(struct dwc3 *dwc) > } > > dwc3_drd_update(dwc); > + } else if (ROLE_SWITCH && > + device_property_read_bool(dwc->dev, "usb-role-switch")) { > + ret = dwc3_setup_role_switch(dwc); > + if (ret < 0) > + return ret; > } else { > dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_OTG); > >
Andrey Smirnov wrote: > On Wed, Feb 2, 2022 at 6:54 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: >> >> Andrey Smirnov wrote: >>> It is necessary that: >>> >>> ROLE_SWITCH && device_property_read_bool(dwc->dev, "usb-role-switch") >>> >>> is true in order for dwc3_get_dr_mode() to _not_ force us from OTG to >>> PERIPHERAL mode here: >>> >>> if (mode == USB_DR_MODE_OTG && >>> (!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) || >>> !device_property_read_bool(dwc->dev, "usb-role-switch")) && >>> !DWC3_VER_IS_PRIOR(DWC3, 330A)) >>> mode = USB_DR_MODE_PERIPHERAL; >>> >>> and dwc3_drd_init() to be called later in dwc3_core_init_mode(). So, >>> to avoid always ignoring extcon device returned by dwc3_get_extcon() >>> change dwc3_drd_init() to check and use it first, before checking if >>> dwc3_setup_role_switch() should be called. >>> >>> Cc: Felipe Balbi <balbi@kernel.org> >>> Cc: Thinh Nguyen <thinhn@synopsys.com> >>> Cc: linux-usb@vger.kernel.org >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >>> --- >>> >>> Hopefully I didn't miss something important making this patch >>> unnecessary. Don't know if this is a good solution or not, part of me >>> thinks than maybe changing the aforementioned code in >>> dwc3_get_dr_mode() to account for extcon wopuld be >>> simpler/better. Happy to rework this. >> >> The driver either use extcon or usb-role-switch. It doesn't make sense to >> enable usb-role-switch for extcon just so the driver doesn't default >> the dr_mode to peripheral. >> >> Perhaps, this is what you're looking for? (code is not tested) >> > > Yeah that kind of what I meant by "maybe changing the aforementioned code in > dwc3_get_dr_mode() to account for extcon wopuld be > simpler/better." OK let me rework the patch to do that. > >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index bffd719b8b52..a52331ea7a0d 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -86,7 +86,7 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc) >> * mode. If the controller supports DRD but the dr_mode is not >> * specified or set to OTG, then set the mode to peripheral. >> */ >> - if (mode == USB_DR_MODE_OTG && >> + if (mode == USB_DR_MODE_OTG && !dwc->edev && >> (!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) || >> !device_property_read_bool(dwc->dev, "usb-role-switch")) && >> !DWC3_VER_IS_PRIOR(DWC3, 330A)) >> @@ -1715,6 +1715,13 @@ static int dwc3_probe(struct platform_device *pdev) >> goto err2; >> } >> >> + dwc->edev = dwc3_get_extcon(dwc); >> + if (IS_ERR(dwc->edev)) { >> + ret = PTR_ERR(dwc->edev); >> + dev_err(dwc->dev, "failed to get extcon %d\n", ret); >> + goto err3; >> + } >> + >> ret = dwc3_get_dr_mode(dwc); >> if (ret) >> goto err3; >> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c >> index 63089c7fb530..d02fcfdc74db 100644 >> --- a/drivers/usb/dwc3/drd.c >> +++ b/drivers/usb/dwc3/drd.c >> @@ -585,16 +585,7 @@ int dwc3_drd_init(struct dwc3 *dwc) >> { >> int ret, irq; >> >> - dwc->edev = dwc3_get_extcon(dwc); >> - if (IS_ERR(dwc->edev)) >> - return PTR_ERR(dwc->edev); >> - >> - if (ROLE_SWITCH && >> - device_property_read_bool(dwc->dev, "usb-role-switch")) { >> - ret = dwc3_setup_role_switch(dwc); >> - if (ret < 0) >> - return ret; >> - } else if (dwc->edev) { > > We probably don't even need to move this around. If "usb-role-switch" > isn't going to be set then that "if" could go first and not interfere. > Yeah, this is unnecessary. >> + if (dwc->edev) { >> dwc->edev_nb.notifier_call = dwc3_drd_notifier; >> ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST, >> &dwc->edev_nb); >> @@ -604,6 +595,11 @@ int dwc3_drd_init(struct dwc3 *dwc) >> } >> >> dwc3_drd_update(dwc); >> + } else if (ROLE_SWITCH && >> + device_property_read_bool(dwc->dev, "usb-role-switch")) { >> + ret = dwc3_setup_role_switch(dwc); >> + if (ret < 0) >> + return ret; >> } else { >> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_OTG); >> >> Thanks, Thinh
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c index e2b68bb770d1..835bd0be87d5 100644 --- a/drivers/usb/dwc3/drd.c +++ b/drivers/usb/dwc3/drd.c @@ -579,12 +579,7 @@ int dwc3_drd_init(struct dwc3 *dwc) if (IS_ERR(dwc->edev)) return PTR_ERR(dwc->edev); - if (ROLE_SWITCH && - device_property_read_bool(dwc->dev, "usb-role-switch")) { - ret = dwc3_setup_role_switch(dwc); - if (ret < 0) - return ret; - } else if (dwc->edev) { + if (dwc->edev) { dwc->edev_nb.notifier_call = dwc3_drd_notifier; ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST, &dwc->edev_nb); @@ -594,6 +589,11 @@ int dwc3_drd_init(struct dwc3 *dwc) } dwc3_drd_update(dwc); + } else if (ROLE_SWITCH && + device_property_read_bool(dwc->dev, "usb-role-switch")) { + ret = dwc3_setup_role_switch(dwc); + if (ret < 0) + return ret; } else { dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_OTG); dwc->current_dr_role = DWC3_GCTL_PRTCAP_OTG;
It is necessary that: ROLE_SWITCH && device_property_read_bool(dwc->dev, "usb-role-switch") is true in order for dwc3_get_dr_mode() to _not_ force us from OTG to PERIPHERAL mode here: if (mode == USB_DR_MODE_OTG && (!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) || !device_property_read_bool(dwc->dev, "usb-role-switch")) && !DWC3_VER_IS_PRIOR(DWC3, 330A)) mode = USB_DR_MODE_PERIPHERAL; and dwc3_drd_init() to be called later in dwc3_core_init_mode(). So, to avoid always ignoring extcon device returned by dwc3_get_extcon() change dwc3_drd_init() to check and use it first, before checking if dwc3_setup_role_switch() should be called. Cc: Felipe Balbi <balbi@kernel.org> Cc: Thinh Nguyen <thinhn@synopsys.com> Cc: linux-usb@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- Hopefully I didn't miss something important making this patch unnecessary. Don't know if this is a good solution or not, part of me thinks than maybe changing the aforementioned code in dwc3_get_dr_mode() to account for extcon wopuld be simpler/better. Happy to rework this. drivers/usb/dwc3/drd.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) -- 2.25.1