Re: [PATCH v10 03/38] clk: ep93xx: add DT support for Cirrus EP93xx

[Stephen Boyd]

Quoting Nikita Shubin (2024-07-10 03:34:12)
> Hello Stephen!
> 
> On Mon, 2024-07-08 at 15:18 -0700, Stephen Boyd wrote:
> > Quoting Nikita Shubin via B4 Relay (2024-06-17 02:36:37)
> > > diff --git a/drivers/clk/clk-ep93xx.c b/drivers/clk/clk-ep93xx.c
> > > new file mode 100644
> > > index 000000000000..a0430a5ae4da
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-ep93xx.c
> > > @@ -0,0 +1,834 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > [...]
> > > +
> > > +static int ep93xx_clk_enable(struct clk_hw *hw)
> > > +{
> > > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > > +       u32 val;
> > > +
> > > +       guard(spinlock_irqsave)(&priv->lock);
> > 
> > I thought guard() was most important when there were multiple exit
> > paths
> > from a function, but OK.
> > 
> > > +
> > [...]
> > > +
> > > +static int ep93xx_plls_init(struct ep93xx_clk_priv *priv)
> > > +{
> > > +       const char fclk_divisors[] = { 1, 2, 4, 8, 16, 1, 1, 1 };
> > > +       const char hclk_divisors[] = { 1, 2, 4, 5, 6, 8, 16, 32 };
> > [...]
> > > +       if (!(value & EP93XX_SYSCON_CLKSET2_NBYP2))
> > > +               clk_pll2_rate = EP93XX_EXT_CLK_RATE;
> > > +       else if (value & EP93XX_SYSCON_CLKSET2_PLL2_EN)
> > > +               clk_pll2_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE,
> > > value);
> > > +       else
> > > +               clk_pll2_rate = 0;
> > > +
> > > +       hw = devm_clk_hw_register_fixed_rate(dev, "pll2", "xtali",
> > > 0, clk_pll2_rate);
> > 
> > Please use clk_parent_data for topology descriptions.
> 
> It's look like clk_hw_register_fixed_rate_parent_hw() is missing devm_
> version of this function. Is it ok to provide wrapper for
> __clk_hw_register_fixed_rate() in drivers/clk/clk-ep93xx.c ?
> 

It's not very hard to put it in the header file though, so why not
implement the wrapper in include/linux/clk-provider.h like
devm_clk_hw_register_fixed_rate()?