El Thu, Dec 08, 2022 at 09:12:08PM +0100, Marek Vasut deia:
On 12/8/22 17:53, Xavier Drudis Ferran wrote:
[...]
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c @@ -7,7 +7,7 @@ */ #include <common.h> -#include <clk.h> +#include <clk-uclass.h> #include <dm.h> #include <asm/global_data.h> #include <dm/device_compat.h> @@ -168,6 +168,9 @@ static struct phy_ops rockchip_usb2phy_ops = { .of_xlate = rockchip_usb2phy_of_xlate, }; +static struct clk_ops rockchip_usb2phy_clk_ops = { +};
Is this empty structure needed ? Why ?
Either it shouldn't be here, or it should implement some callbacks, like clock enable/disable ?
I tried without it but it gave me a runtime error. I think I have the log somewhere if you want to see it. It looked like a null pointer dereference at first sight. I just added it and it got fixed. I didn't research what the failing functions were trying to do.
Or is this a common case and there's some null_clk_ops() function or macro magic or something somewhere ?
The thing is nobody is using this clk in u-boot. It's just its phandle in a clock phandle list that ehci-generic.c happens to use in bulk. So the default implementation seems to be enough to allocate, enable and release it in bulk with its clk set. Or at least to call the functions without error.
As I have left it, it might not work if ever someone wants to use it. But should I try to implement it so that it is usable ? How should I test it ? Shouldn't we wait until someone has some real use for it ? Linux spent sometime without it in the phandle list until William wu discovered it was needed for suspend/resume, so u-boot may never need it.
Or should I add a comment in the code ?
Otherwise looks much better, thanks !
Thanks.