On 11/01/2020 21:45, Martin Blumenstingl wrote: > Hi Hanjie, > > On Fri, Jan 10, 2020 at 6:43 AM Hanjie Lin <hanjie.lin@xxxxxxxxxxx> wrote: > [...] >> - devm_add_action_or_reset(dev, >> - (void(*)(void *))clk_disable_unprepare, >> - priv->clk); >> + ret = clk_bulk_prepare_enable(priv->drvdata->num_clks, >> + priv->drvdata->clks); > I don't see clk_bulk_disable_unprepare in dwc3_meson_g12a_remove() > please test that the clocks are all disabled (see > /sys/kernel/debug/clk/clk_summary for example) when unloading all USB > related kernel modules > >> + >> + if (!priv->drvdata->otg_switch_supported) >> + goto setup_pm_runtime; > my brain doesn't like the goto in this place because this is not an > error condition. I was about to write that > usb_role_switch_unregister() is now skipped even though we're calling > usb_role_switch_register(). > > I want to hear Neil's opinion on this because I got confused while > reading the code again. > my proposal is to move all of this OTG related code from > dwc3_meson_g12a_probe() into a new function, for example called > dwc3_meson_g12a_otg_init() > then only call that function when OTG switching is supported Indeed it's not cleanest way to do that, if you respin a v6, put all the OTG setup and role switch register in a separate function. with that and :clk_bulk_disable_unprepare() in remove: Reviewed-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx> Neil > > > Martin >
