On Tue, Oct 12, 2021 at 4:42 PM Emil Renner Berthing [off-list ref] 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