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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a2a41f77c854b2399a0d87142c5bfe39.sboyd@kernel.org>
Date: Wed, 10 Jul 2024 12:56:33 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Michael Turquette <mturquette@...libre.com>, Nikita Shubin <nikita.shubin@...uefel.me>, Nikita Shubin via B4 Relay <devnull+nikita.shubin.maquefel.me@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v10 03/38] clk: ep93xx: add DT support for Cirrus EP93xx

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()?