On 2020/1/15 16:01, Neil Armstrong wrote:

On 11/01/2020 21:45, Martin Blumenstingl wrote:

quoted

Hi Hanjie,

On Fri, Jan 10, 2020 at 6:43 AM Hanjie Lin [off-list ref] wrote:
[...]

quoted

-       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

quoted

+
+       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 <redacted>

Neil

Ok, 

I will do it in v6 patch.

Thanks,
Hanjie

quoted

Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel