RE: [PATCH v2 1/2] usb: dwc3: add Realtek DHC RTD SoC dwc3 glue layer driver

[Stanley Chang[昌育德]]

Hi Thinh,

> > +struct dwc3_rtk {
> > +     struct device *dev;
> > +     void __iomem *regs;
> > +     size_t regs_size;
> > +
> > +     struct dwc3 *dwc;
> > +
> > +     int cur_dr_mode; /* current dr mode */
> > +     bool support_drd_mode; /* if support Host/device switch */
> 
> I think we may not need this and do away the boolean support_drd_mode.

Yes, the initial flow should be simplified as
@@ -346,12 +342,7 @@ static int dwc3_rtk_probe_dwc3_core(struct dwc3_rtk *rtk)

        rtk->cur_dr_mode = dr_mode;

-       if (device_property_read_bool(dwc3_dev, "usb-role-switch"))
-               rtk->support_drd_mode = true;
-       else
-               rtk->support_drd_mode = false;
-
-       if (rtk->support_drd_mode) {
+       if (device_property_read_bool(dwc3_dev, "usb-role-switch")) {
                dwc3_rtk_setup_role_switch(rtk);
                rtk->cur_dr_mode = dwc3_rtk_get_dr_mode(rtk);
        }

> > +static int dwc3_rtk_set_dr_mode(struct dwc3_rtk *rtk, int dr_mode)
> 
> Why return the mode rather than status if the setting? You're not checking the
> return of this function in the caller anyway.

You are right, this return value is unnecessary.
I will remove it.

> > +{
> > +     if (!rtk->support_drd_mode)
> > +             return rtk->cur_dr_mode;
> > +
> > +     rtk->cur_dr_mode = dr_mode;
> > +
> > +     switch_dwc3_dr_mode(rtk, dr_mode);
> > +     mdelay(10);
> > +     switch_usb2_dr_mode(rtk, dr_mode);
> > +
> > +     return rtk->cur_dr_mode;
> > +}
> > +

> > +static int dwc3_rtk_setup_role_switch(struct dwc3_rtk *rtk)
> 
> Any reason why we're doing the role switch here and not what's implemented
> from the core?
> 
Because we have to set the usb 2.0 phy mode through switch_usb2_dr_mode in the function dwc3_rtk_set_dr_mode.
In fact, switch_dwc3_dr_mode will use the role switching implemented by core.

> > +
> > +module_platform_driver(dwc3_rtk_driver);
> > +
> > +MODULE_AUTHOR("Stanley Chang <stanley_chang@xxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("DesignWare USB3 Realtek Glue Layer");
> > +MODULE_ALIAS("platform:rtk-dwc3");
> > +MODULE_LICENSE("GPL");
> 
> I'm not familiar with licensing much, but can the SPDX header indicates
> different version than the module license?
> 
Thanks Greg for your comment.
Either GPL or GPL v2 are suitable for our source code.

Thanks,
Stanley