Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 12 Oct 2021 21:40:05 +0300 | Subject | Re: [PATCH v1 06/16] clk: starfive: Add JH7100 clock generator driver |
| |
On Tue, Oct 12, 2021 at 4:42 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > 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).
...
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);
> + 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?
...
> +#ifdef CONFIG_DEBUG_FS
Perhaps __maybe_unused?
> +#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.
> + 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?
-- With Best Regards, Andy Shevchenko
| |