Quoting Emil Renner Berthing (2021-10-26 15:35:36)

On Tue, 26 Oct 2021 at 22:20, Stephen Boyd [off-list ref] wrote:

quoted

Quoting Emil Renner Berthing (2021-10-21 10:42:13)

quoted

+};
+
+struct clk_starfive_jh7100_priv {
+       /* protect registers against overlapping read-modify-write */
+       spinlock_t rmw_lock;

Does overlapping mean concurrent?

Yes, sorry.

quoted

Do different clks share the same registers?

No, each clock has their own register, but they use that register both
to gate the clock and other configuration. The Locking chapter of
Documentation/driver-api/clk.rst talks about the prepare lock and the
enable lock and then says:
"However, access to resources that are shared between operations of
the two groups needs to be protected by the drivers. An example of
such a resource would be a register that controls both the clock rate
and the clock enable/disable state."

Alright got it. Maybe say "protect clk enable and set rate from
happening at the same time".

quoted

quoted

+               return ERR_PTR(-EINVAL);
+       }
+
+       if (idx >= JH7100_CLK_PLL0_OUT)
+               return priv->pll[idx - JH7100_CLK_PLL0_OUT];
+
+       return &priv->reg[idx].hw;
+}
+
+static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)

Drop __init as this can be called after kernel init is over.

Oh interesting, I'd like to know when that can happen. The comment for
the builtin_platform_driver macro says it's just a wrapper for

I thought this was using module_platform_driver() macro?

device_initcall.

Won't we then need to remove all the __initconst tags too since the
probe function walks through jh7100_clk_data which eventually
references all __initconst data?

Yes. If it's builtin_platform_driver() it can't be a module/tristate
Kconfig, in which case all the init markings can stay.

quoted

quoted

+
+               clk->hw.init = &init;
+               clk->idx = idx;
+               clk->max = jh7100_clk_data[idx].max;
+
+               ret = clk_hw_register(priv->dev, &clk->hw);

Why not use devm_clk_hw_register()?

I probably could. Just for my understanding that's just to avoid the
loop on error below, because as a builtin driver the device won't
otherwise go away, right?

Yes

quoted

quoted

+               if (ret)
+                       goto err;
+       }
+
+       ret = devm_of_clk_add_hw_provider(priv->dev, clk_starfive_jh7100_get, priv);
+       if (ret)
+               goto err;
+
+       return 0;
+err:
+       while (idx)
+               clk_hw_unregister(&priv->reg[--idx].hw);
+       return ret;
+}
+
+static const struct of_device_id clk_starfive_jh7100_match[] = {
+       { .compatible = "starfive,jh7100-clkgen" },
+       { /* sentinel */ }
+};

Please add MODULE_DEVICE_TABLE()

Will do!

If it's never going to be a module then don't add any module_* things.