Messages in this thread | | | From | Stanley Chang[昌育德] <> | Subject | RE: [PATCH v2 1/2] usb: dwc3: add Realtek DHC RTD SoC dwc3 glue layer driver | Date | Wed, 2 Aug 2023 08:26:50 +0000 |
| |
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@realtek.com>"); > > +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
| |