On Tue, 12 Oct 2021 at 17:40, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> On Tue, Oct 12, 2021 at 4:42 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> >
> > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> >
> > Add a driver for the StarFive JH7100 clock generator.
>
> ...
>
> > +config CLK_STARFIVE_JH7100
> > + bool "StarFive JH7100 clock support"
> > + depends on SOC_STARFIVE || COMPILE_TEST
>
> > + depends on OF
>
> Why? I haven't found a compile dependency, so you reduce the test
> scope (when COMPILE_TEST=y).
My thinking was that it can't ever be loaded on a !OF system, but
you're right it'll just restrict compile testing. I'll remove, thanks.
> ...
>
> You are using
> bits.h
> mod_devicetable.h
> which are not here
>
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/overflow.h>
> > +#include <linux/platform_device.h>
>
> ...
>
> > + value |= readl_relaxed(reg) & ~mask;
>
> value is not masked, is it okay?
>
> Usual pattern for this kind of operations is
>
> value = (current & ~mask) | (value & mask);
This function is only ever called with constants, already masked
values or the parent number from the clk framework, so it should be
ok.
> > + writel_relaxed(value, reg);
>
> ...
>
> > + if (div > max)
> > + div = max;
> > +
> > + return div;
>
> return min(div, max); ?
>
> ...
>
> > + rate = parent / div;
> > + if (rate < req->min_rate && div > 1) {
> > + div -= 1;
> > + rate = parent / div;
> > + }
>
> Seems like homegrown DIV_ROUND_UP() or so. Who will guarantee that
> decreasing div by 1 will satisfy the conditional again?
Maths unless I'm mistaken: div = DIV_ROUND_UP(parent, target), so in
rational numbers
div - 1 < parent / target
But the target is clamped by min_rate and max_rate, so
min_rate <= target < parent / (div - 1) = rate
Sorry, re-using the rate varable for both the target and result is
confusing. I'll fix that.
> ...
>
> > +#ifdef CONFIG_DEBUG_FS
>
> Perhaps __maybe_unused?
I can definitely use __maybe_unused for the function declaration, but
then I'll need a conditional every time clk_ops.debug_init needs to be
initialized to either the function or NULL depending on
CONFIG_DEBUG_FS below. Is that better?
> > +#else
> > +#define jh7100_clk_debug_init NULL
> > +#endif
>
> ...
>
> > + if (idx >= JH7100_CLK_END) {
>
> > + dev_err(priv->dev, "%s: invalid index %u\n", __func__, idx);
>
> __func__ means that the message has no value on its own. Make it
> unique without using __func__, or drop completely.
>
> > + return ERR_PTR(-EINVAL);
> > + }
>
> ...
>
> > + for (idx = 0; idx < JH7100_CLK_PLL0_OUT; idx++) {
> > + struct clk_init_data init = {
> > + .name = jh7100_clk_data[idx].name,
> > + .ops = jh7100_clk_data[idx].ops,
>
> > + .num_parents = ((jh7100_clk_data[idx].max & JH7100_CLK_MUX_MASK)
> > + >> JH7100_CLK_MUX_SHIFT) + 1,
>
> With temporary variable this can be better written, or consider
> something like this
>
> .num_parents =
> ((jh7100_clk_data[idx].max &
> JH7100_CLK_MUX_MASK) >> JH7100_CLK_MUX_SHIFT) + 1,
>
> > + .flags = jh7100_clk_data[idx].flags,
> > + };
> > + struct jh7100_clk *clk = &priv->reg[idx];
>
> ...
>
> > + while (idx > 0)
> > + clk_hw_unregister(&priv->reg[--idx].hw);
>
> The
>
> while (idx--)
> clk_hw_unregister(&priv->reg[idx].hw);
>
> is slightly better to read.
It's not something I'll insist hard on, but I must admit I disagree.
To me the above looks like cartoon characters running off a cliff and
back. As a middle ground could we maybe do this?
while (idx)
clk_hw_unregister(&priv->reg[--idx].hw);
> > + return ret;
> > +}
>
> ...
>
> > +static int __init clk_starfive_jh7100_init(void)
> > +{
> > + return platform_driver_probe(&clk_starfive_jh7100_driver,
> > + clk_starfive_jh7100_probe);
> > +}
>
> > +
>
> No need to have this blank line.
> > +subsys_initcall(clk_starfive_jh7100_init);
>
> Any explanation why subsys_initcall() is in use?
TBH I just inherited that from Geert's first mock driver and never
thought to question it. What would be a better alternative to try?
Thanks!
/Emil
