Re: [PATCH v14 06/15] clk: Add Lynx 10G SerDes PLL driver

[Vinod Koul <vkoul@kernel.org>]

On 16-05-23, 11:11, Sean Anderson wrote:
> On 5/16/23 09:22, Vinod Koul wrote:
> > On 09-05-23, 11:26, Sean Anderson wrote:
> >> On 5/9/23 09:00, Vinod Koul wrote:
> >> > On 08-05-23, 11:31, Sean Anderson wrote:
> >> >> On 5/8/23 05:15, Vinod Koul wrote:
> >> > 
> >> >> >> +int lynx_clks_init(struct device *dev, struct regmap *regmap,
> >> >> >> +		   struct clk *plls[2], struct clk *ex_dlys[2], bool compat);
> >> >> > 
> >> >> > so you have an exported symbol for clk driver init in phy driver header?
> >> >> > can you please explain why..?
> >> >> 
> >> >> So that it can be called at the appropriate time during the phy's probe function.
> >> >> 
> >> >> This is really an integral part of the phy driver, but I was directed to split it
> >> >> off and put it in another subsystem's directory.
> >> > 
> >> > That is right clock should be belong to clk driver. IIUC the hardware is
> >> > phy along with clocks and you are doing the clk init. I think that may
> >> > not be correct model, you should really have a device tree node to
> >> > represent the clock and the phy node
> >> > 
> >> > 
> >> > What stops this from being modelled as it is in the hardware?
> >> 
> >> It *is* modeled as it is in hardware. With just the serdes compatible,
> >> we have all the information we need to create the clocks. So we do so.
> >> There's no need for a separate device just to create four clocks.
> >> 
> >> These clocks cannot be used by any other device (except possibly by
> >> putting a lane into test mode). So there is no benefit from making them
> >> a separate device, except an increase in complexity due to ordering and
> >> dynamic lookup. By doing things this way we know that either there was
> >> an error or the clocks all exist, and the lifetime of the clocks matches
> >> the serdes.
> > 
> > Okay that does makes sense.
> > 
> > In that case why should this not be in the phy driver itself. There are
> > already few examples og phy drivers having inbuild clk where is makes
> > sense. You register the clk_ops as part of phy driver
> 
> You told me to do it this way

I agree that was not the right recommendation back then and your view
was right

-- 
~Vinod