Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To:     rahul.tanwar@linux.intel.com
Cc:     andriy.shevchenko@intel.com, cheol.yong.kim@intel.com,
        linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
        mark.rutland@arm.com, mturquette@baylibre.com,
        qi-ming.wu@intel.com, rahul.tanwar@intel.com, robh+dt@kernel.org,
        robhkernel.org@vger.kernel.org, sboyd@kernel.org,
        yixin.zhu@linux.intel.com
Subject: RE: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC
Date:   Tue,  3 Sep 2019 00:20:15 +0200
Message-Id: <20190902222015.11360-1-martin.blumenstingl@googlemail.com>
In-Reply-To: <6a3c26bc6e25d883686287883528dbde30725922.1566975410.git.rahul.tanwar@linux.intel.com>
References: <6a3c26bc6e25d883686287883528dbde30725922.1566975410.git.rahul.tanwar@linux.intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello,

I only noticed this patchset today and I don't have much time left.
Here's my initial impressions without going through the code in detail.
I'll continue my review in the next days (as time permits).

As with all other Intel LGM patches: I don't have access to the
datasheets, so it's possible that I don't understand <insert topic here>
feel free to correct me in this case (I appreciate an explanation where
I was wrong, so I can learn from it)


[...]
--- /dev/null
+++ b/drivers/clk/intel/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+config INTEL_LGM_CGU_CLK
+	depends on COMMON_CLK
+	select MFD_SYSCON
can you please explain the reason why you need to use syscon?
also please see [0] for a comment from Rob on another LGM dt-binding
regarding syscon

+	select OF_EARLY_FLATTREE
there's not a single other "select OF_EARLY_FLATTREE" in driver/clk
I'm not saying this is wrong but it makes me curious why you need this

[...]
diff --git a/drivers/clk/intel/clk-cgu.h b/drivers/clk/intel/clk-cgu.h
new file mode 100644
index 000000000000..e44396b4aad7
--- /dev/null
+++ b/drivers/clk/intel/clk-cgu.h
@@ -0,0 +1,278 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Copyright(c) 2018 Intel Corporation.
+ *  Zhu YiXin <Yixin.zhu@intel.com>
+ */
+
+#ifndef __INTEL_CLK_H
+#define __INTEL_CLK_H
+
+struct intel_clk_mux {
+	struct clk_hw hw;
+	struct device *dev;
+	struct regmap *map;
+	unsigned int reg;
+	u8 shift;
+	u8 width;
+	unsigned long flags;
+};
+
+struct intel_clk_divider {
+	struct clk_hw hw;
+	struct device *dev;
+	struct regmap *map;
+	unsigned int reg;
+	u8 shift;
+	u8 width;
+	unsigned long flags;
+	const struct clk_div_table *table;
+};
+
+struct intel_clk_ddiv {
+	struct clk_hw hw;
+	struct device *dev;
+	struct regmap *map;
+	unsigned int reg;
+	u8 shift0;
+	u8 width0;
+	u8 shift1;
+	u8 width1;
+	u8 shift2;
+	u8 width2;
+	unsigned int mult;
+	unsigned int div;
+	unsigned long flags;
+};
+
+struct intel_clk_gate {
+	struct clk_hw hw;
+	struct device *dev;
+	struct regmap *map;
+	unsigned int reg;
+	u8 shift;
+	unsigned long flags;
+};
I know at least two existing regmap clock implementations:
- drivers/clk/qcom/clk-regmap*
- drivers/clk/meson/clk-regmap*

it would be great if we could decide to re-use one of those for the
"generic" clock types (mux, divider and gate).
Stephen, do you have any preference here?
personally I like the meson one, but I'm biased because I've used it
a lot in the past and I haven't used the qcom one at all.


Martin


[0] https://lkml.org/lkml/2019/8/27/849