Hi Stephen,

I am back after long silence.

2016-02-26 9:07 GMT+09:00 Stephen Boyd [off-list ref]:

On 12/28, Masahiro Yamada wrote:

quoted

This is the initial commit for the UniPhier clock drivers, including
support for PH1-sLD3, PH1-LD4, PH1-Pro4, PH1-sLD8, PH1-Pro5, and
ProXstream2/PH1-LD6b.

To improve the code maintainability, the driver consists of common
functions (clk-uniphier-core.c) and clock data arrays needed to
support each SoC.

Signed-off-by: Masahiro Yamada <redacted>
---

Where's the DT binding? Also, can these clks be registered from a
platform device driver instead of from

quoted

diff --git a/drivers/clk/uniphier/Kconfig b/drivers/clk/uniphier/Kconfig
new file mode 100644
index 0000000..7606f27
--- /dev/null
+++ b/drivers/clk/uniphier/Kconfig

@@ -0,0 +1,35 @@
+menuconfig CLK_UNIPHIER
+     bool "Clock drivers for UniPhier SoCs"
+     depends on ARCH_UNIPHIER

Can we have COMPILE_TEST here?

OK.
But if you recommend to drop "depends on ARCH_UNIPHIER",
COMPILE_TEST is not necessary.

quoted

+     depends on OF
+     default y

And then default ARCH_UNIPHIER instead.

The problem is that CLK_UNIPHIER is not enabled along with
ARCH_UNIPHIER in "make menuconfig".

If we use "depends on ARCH_UNIPHIER"
and we enable ARCH_UNIPHIER from "make menuconfig",
CLK_UNIPHIER is automatically enabled as well.

Is the latter more like what we expect?

quoted

+endif

diff --git a/drivers/clk/uniphier/clk-uniphier-core.c b/drivers/clk/uniphier/clk-uniphier-core.c

Maybe this patch can be split between core code and SoC specific
data? That way we can focus on the core code without wading
through all the random clk data arrays.

OK.  will do so.

quoted

+static void __init uniphier_clk_update_parent_name(struct device_node *np,
+                                                const char **parent_name)
+{
+     const char *new_name;
+     int index;
+
+     if (!parent_name || !*parent_name)
+             return;
+
+     if (strncmp(*parent_name, UNIPHIER_CLK_EXT, strlen(UNIPHIER_CLK_EXT)))
+             return;
+
+     index = of_property_match_string(np, "clock-names",
+                             *parent_name + strlen(UNIPHIER_CLK_EXT));

If possible please don't use clock-names to set up clock
hierarchy.

So, what shall I do instead?

I am not comfortable with hard-coding the parents' names
for clock-cascading.

Maybe, can we support clk registration
with clk_hw of parents, not names of parents?

For example, by adding  "struct clk_hw **parents"
to struct clk_init_data.

quoted

+int __init uniphier_clk_init(struct device_node *np,
+                          struct uniphier_clk_init_data *idata)
+{
+     struct clk_onecell_data *clk_data;
+     struct uniphier_clk_init_data *p;
+     void __iomem *regbase;
+     int max_index = 0;
+     int ret;
+
+     regbase = of_iomap(np, 0);

If not a platform device, use of_io_request_and_map()?

I am planning to change all of them into platform drivers in the next version.

quoted

+#ifndef __CLK_UNIPHIER_H__
+#define __CLK_UNIPHIER_H__
+
+#include <linux/kernel.h>

What's this include for?

Probably my mistake.  Will remove.

quoted

+
+#define UNIPHIER_CLK_EXT     "[EXT]"
+
+enum uniphier_clk_type {
+     UNIPHIER_CLK_TYPE_FIXED_FACTOR,
+     UNIPHIER_CLK_TYPE_FIXED_RATE,
+     UNIPHIER_CLK_TYPE_GATE,
+     UNIPHIER_CLK_TYPE_MUX,
+};
+
+struct uniphier_clk_fixed_factor_data {
+     const char *parent_name;
+     unsigned int mult;
+     unsigned int div;
+};
+
+struct uniphier_clk_fixed_rate_data {
+     unsigned long fixed_rate;
+};
+
+struct uniphier_clk_gate_data {
+     const char *parent_name;
+     unsigned int reg;
+     u8 bit_idx;
+};
+
+struct uniphier_clk_mux_data {
+     const char *parent_names[4];
+     u8 num_parents;
+     unsigned int reg;
+     u8 shift;
+};
+
+struct uniphier_clk_init_data {
+     const char *name;
+     enum uniphier_clk_type type;
+     int output_index;
+     union {
+             struct uniphier_clk_fixed_factor_data factor;
+             struct uniphier_clk_fixed_rate_data rate;
+             struct uniphier_clk_gate_data gate;
+             struct uniphier_clk_mux_data mux;
+     } data;
+     struct clk *clk;

Probably need to forward declare this struct.

OK, thanks.

quoted

+};
+
+int uniphier_clk_init(struct device_node *np,

Same for device_node.

-- 
Best Regards
Masahiro Yamada