On 2020/1/15 16:01, Neil Armstrong wrote: > 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 > Ok, I will do it in v6 patch. Thanks, Hanjie >> >> >> Martin >> > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-amlogic > > . >
