Re: [PATCH v5 4/6] usb: dwc3: Add Amlogic A1 DWC3 glue

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux