Re: [PATCH 10/17] clk: sunxi=ng: add support for R329 R-CCU

Hi,

On Fri, Aug 20, 2021 at 06:34:38AM +0200, Jernej Škrabec wrote:

quoted

quoted

+static void __init sun50i_r329_r_ccu_setup(struct device_node *node)
+{
+	void __iomem *reg;
+	u32 val;
+	int i;
+
+	reg = of_io_request_and_map(node, 0, of_node_full_name(node));
+	if (IS_ERR(reg)) {
+		pr_err("%pOF: Could not map clock registers\n", node);
+		return;
+	}
+
+	/* Enable the lock bits and the output enable bits on all PLLs */
+	for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
+		val = readl(reg + pll_regs[i]);
+		val |= BIT(29) | BIT(27);
+		writel(val, reg + pll_regs[i]);
+	}
+
+	/*
+	 * Force the I/O dividers of PLL-AUDIO1 to reset default value
+	 *
+	 * See the comment before pll-audio1 definition for the reason.
+	 */
+
+	val = readl(reg + SUN50I_R329_PLL_AUDIO1_REG);
+	val &= ~BIT(1);
+	val |= BIT(0);
+	writel(val, reg + SUN50I_R329_PLL_AUDIO1_REG);
+
+	i = sunxi_ccu_probe(node, reg, &sun50i_r329_r_ccu_desc);
+	if (i)
+		pr_err("%pOF: probing clocks fails: %d\n", node, i);
+}
+
+CLK_OF_DECLARE(sun50i_r329_r_ccu, "allwinner,sun50i-r329-r-ccu",
+	       sun50i_r329_r_ccu_setup);

Please make this a platform driver. There is no particular reason why it
needs to be an early OF clock provider.

Why? It's good to have it as early clock provider. It has no dependencies and 
other drivers that depends on it, like IR, can be deferred, if this is loaded 
later.

No, Samuel is right, we should make them regular drivers as much as we
can.

The reason we had CLK_OF_DECLARE in the first place is that timers
usually have a parent clock, and you need the timers before the device
model is set up.

Fortunately for us, since the A20, the architected timers don't require
a parent clock from us, and we can thus boot up fine.

Since the dependencies are minimal, it should probe fairly early and
with the on-demand probing from the device links you might not even tell
the difference for most consumers.

Maxime