Re: [PATCH v3] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran <xdrudis@tinet.cat>]

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.