[PATCH v5 0/7] clk: meson-axg: Add AO Cloclk and Reset driver

[PATCH v5 0/7] clk: meson-axg: Add AO Cloclk and Reset driver

[Yixun Lan]

  This patch try to add AO clock and Reset driver for Amlogic's
Meson-AXG SoC.
  Please note that patch 7 need to wait for the DTS changes[3] merged
into mainline first, otherwise it will break the serial console.

 patch 2: factor the common code into a dedicated file
 patch 3-5: add the aoclk driver for AXG SoC
 patch 6-7: drop unnecessary clock flags

changes since v4 at [5]: 
 - fix return err
 - introduce CONFIG_COMMON_CLK_MESON_AO
 - format/style minor fix

changes since v3 at [4]: 
 - add 'const' contraint to the read-only data
 - switch to devm_of_clk_add_hw_provider API
 - check return value of devm_reset_controller_register

changes since v2 at [2]: 
 - rework meson_aoclkc_probe() which leverage the of_match_data
 - merge patch 5-6 into this series
 - seperate DTS patch, will send to Kevin Hilman independently
 
changes since v1 at [0]: 
 - rebase to clk-meson's branch 'next/drivers' [1]
 - fix license, update to BSD-3-Clause
 - drop un-used include header file

[0] https://lkml.kernel.org/r/20180209070026.193879-1-yixun.lan@amlogic.com
[1] git://github.com/BayLibre/clk-meson.git branch: next-drivers
[2] https://lkml.kernel.org/r/20180323143816.200573-1-yixun.lan@amlogic.com
[3] https://lkml.kernel.org/r/20180326081809.49493-4-yixun.lan@amlogic.com
[4] https://lkml.kernel.org/r/20180328025050.221585-1-yixun.lan@amlogic.com
[5] https://lkml.kernel.org/r/20180408031938.153474-1-yixun.lan@amlogic.com

Qiufang Dai (1):
  clk: meson-axg: Add AO Clock and Reset controller driver

Yixun Lan (6):
  clk: meson: migrate to devm_of_clk_add_hw_provider API
  clk: meson: aoclk: refactor common code into dedicated file
  dt-bindings: clock: axg-aoclkc: New binding for Meson-AXG SoC
  dt-bindings: clock: reset: Add AXG AO Clock and Reset Bindings
  clk: meson: drop CLK_SET_RATE_PARENT flag
  clk: meson: drop CLK_IGNORE_UNUSED flag

 .../bindings/clock/amlogic,gxbb-aoclkc.txt         |   1 +
 drivers/clk/meson/Kconfig                          |   8 +
 drivers/clk/meson/Makefile                         |   3 +-
 drivers/clk/meson/axg-aoclk.c                      | 163 +++++++++++++++++++++
 drivers/clk/meson/axg-aoclk.h                      |  31 ++++
 drivers/clk/meson/gxbb-aoclk.c                     |  92 ++++--------
 drivers/clk/meson/gxbb-aoclk.h                     |   7 +
 drivers/clk/meson/meson-aoclk.c                    |  82 +++++++++++
 drivers/clk/meson/meson-aoclk.h                    |  35 +++++
 include/dt-bindings/clock/axg-aoclkc.h             |  26 ++++
 include/dt-bindings/reset/axg-aoclkc.h             |  20 +++
 11 files changed, 406 insertions(+), 62 deletions(-)
 create mode 100644 drivers/clk/meson/axg-aoclk.c
 create mode 100644 drivers/clk/meson/axg-aoclk.h
 create mode 100644 drivers/clk/meson/meson-aoclk.c
 create mode 100644 drivers/clk/meson/meson-aoclk.h
 create mode 100644 include/dt-bindings/clock/axg-aoclkc.h
 create mode 100644 include/dt-bindings/reset/axg-aoclkc.h

-- 
2.15.1

[PATCH v5 1/7] clk: meson: migrate to devm_of_clk_add_hw_provider API

[Yixun Lan]

There is a protential memory leak, as of_clk_del_provider is
never called if of_clk_add_hw_provider has been executed.
Fix this by using devm variant API.

Fixes: f8c11f79912d ("clk: meson: Add GXBB AO Clock and Reset controller driver")
Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/clk/meson/gxbb-aoclk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
index 9ec23ae9a219..eebb580b9e0f 100644
--- a/drivers/clk/meson/gxbb-aoclk.c
+++ b/drivers/clk/meson/gxbb-aoclk.c
@@ -191,7 +191,7 @@ static int gxbb_aoclkc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
 			&gxbb_aoclk_onecell_data);
 }
 
-- 
2.15.1

[PATCH v5 2/7] clk: meson: aoclk: refactor common code into dedicated file

[Yixun Lan]

We try to refactor the common code into one dedicated file,
while preparing to add new Meson-AXG aoclk driver, this would
help us to better share the code by all aoclk drivers.

Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/clk/meson/Kconfig       |  7 ++++
 drivers/clk/meson/Makefile      |  1 +
 drivers/clk/meson/gxbb-aoclk.c  | 91 ++++++++++++++---------------------------
 drivers/clk/meson/gxbb-aoclk.h  |  7 ++++
 drivers/clk/meson/meson-aoclk.c | 82 +++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/meson-aoclk.h | 35 ++++++++++++++++
 6 files changed, 163 insertions(+), 60 deletions(-)
 create mode 100644 drivers/clk/meson/meson-aoclk.c
 create mode 100644 drivers/clk/meson/meson-aoclk.h

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index d5cbec522aec..fddc7ec7b820 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -3,6 +3,12 @@ config COMMON_CLK_AMLOGIC
 	depends on OF
 	depends on ARCH_MESON || COMPILE_TEST
 
+config COMMON_CLK_MESON_AO
+	bool
+	depends on OF
+	depends on ARCH_MESON || COMPILE_TEST
+	select COMMON_CLK_REGMAP_MESON
+
 config COMMON_CLK_REGMAP_MESON
 	bool
 	select REGMAP
@@ -21,6 +27,7 @@ config COMMON_CLK_GXBB
 	bool
 	depends on COMMON_CLK_AMLOGIC
 	select RESET_CONTROLLER
+	select COMMON_CLK_MESON_AO
 	select COMMON_CLK_REGMAP_MESON
 	select MFD_SYSCON
 	help
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index ffee82e60b7a..0a8df284f4e7 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
+obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
 obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
 obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
 obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o
diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
index eebb580b9e0f..f2e0640a7f74 100644
--- a/drivers/clk/meson/gxbb-aoclk.c
+++ b/drivers/clk/meson/gxbb-aoclk.c
@@ -52,39 +52,13 @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
-#include <linux/clk-provider.h>
-#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
 #include <linux/mfd/syscon.h>
-#include <linux/regmap.h>
 #include <linux/init.h>
-#include <linux/delay.h>
-#include <dt-bindings/clock/gxbb-aoclkc.h>
-#include <dt-bindings/reset/gxbb-aoclkc.h>
 #include "clk-regmap.h"
 #include "gxbb-aoclk.h"
 
-struct gxbb_aoclk_reset_controller {
-	struct reset_controller_dev reset;
-	unsigned int *data;
-	struct regmap *regmap;
-};
-
-static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
-			       unsigned long id)
-{
-	struct gxbb_aoclk_reset_controller *reset =
-		container_of(rcdev, struct gxbb_aoclk_reset_controller, reset);
-
-	return regmap_write(reset->regmap, AO_RTI_GEN_CNTL_REG0,
-			    BIT(reset->data[id]));
-}
-
-static const struct reset_control_ops gxbb_aoclk_reset_ops = {
-	.reset = gxbb_aoclk_do_reset,
-};
-
 #define GXBB_AO_GATE(_name, _bit)					\
 static struct clk_regmap _name##_ao = {					\
 	.data = &(struct clk_regmap_gate_data) {			\
@@ -117,7 +91,7 @@ static struct aoclk_cec_32k cec_32k_ao = {
 	},
 };
 
-static unsigned int gxbb_aoclk_reset[] = {
+static const unsigned int gxbb_aoclk_reset[] = {
 	[RESET_AO_REMOTE] = 16,
 	[RESET_AO_I2C_MASTER] = 18,
 	[RESET_AO_I2C_SLAVE] = 19,
@@ -135,7 +109,7 @@ static struct clk_regmap *gxbb_aoclk_gate[] = {
 	[CLKID_AO_IR_BLASTER] = &ir_blaster_ao,
 };
 
-static struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
+static const struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
 	.hws = {
 		[CLKID_AO_REMOTE] = &remote_ao.hw,
 		[CLKID_AO_I2C_MASTER] = &i2c_master_ao.hw,
@@ -145,58 +119,55 @@ static struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
 		[CLKID_AO_IR_BLASTER] = &ir_blaster_ao.hw,
 		[CLKID_AO_CEC_32K] = &cec_32k_ao.hw,
 	},
-	.num = 7,
+	.num = NR_CLKS,
 };
 
-static int gxbb_aoclkc_probe(struct platform_device *pdev)
+static int gxbb_register_cec_ao_32k(struct platform_device *pdev)
 {
-	struct gxbb_aoclk_reset_controller *rstc;
 	struct device *dev = &pdev->dev;
 	struct regmap *regmap;
-	int ret, clkid;
-
-	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
-	if (!rstc)
-		return -ENOMEM;
+	int ret;
 
 	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
 	if (IS_ERR(regmap)) {
 		dev_err(dev, "failed to get regmap\n");
-		return -ENODEV;
-	}
-
-	/* Reset Controller */
-	rstc->regmap = regmap;
-	rstc->data = gxbb_aoclk_reset;
-	rstc->reset.ops = &gxbb_aoclk_reset_ops;
-	rstc->reset.nr_resets = ARRAY_SIZE(gxbb_aoclk_reset);
-	rstc->reset.of_node = dev->of_node;
-	ret = devm_reset_controller_register(dev, &rstc->reset);
-
-	/*
-	 * Populate regmap and register all clks
-	 */
-	for (clkid = 0; clkid < ARRAY_SIZE(gxbb_aoclk_gate); clkid++) {
-		gxbb_aoclk_gate[clkid]->map = regmap;
-
-		ret = devm_clk_hw_register(dev,
-					   gxbb_aoclk_onecell_data.hws[clkid]);
-		if (ret)
-			return ret;
+		return PTR_ERR(regmap);
 	}
 
 	/* Specific clocks */
 	cec_32k_ao.regmap = regmap;
 	ret = devm_clk_hw_register(dev, &cec_32k_ao.hw);
+	if (ret) {
+		dev_err(&pdev->dev, "clk cec_32k_ao register failed.\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct meson_aoclk_data gxbb_aoclkc_data = {
+	.reset_reg	= AO_RTI_GEN_CNTL_REG0,
+	.num_reset	= ARRAY_SIZE(gxbb_aoclk_reset),
+	.reset		= gxbb_aoclk_reset,
+	.num_clks	= ARRAY_SIZE(gxbb_aoclk_gate),
+	.clks		= gxbb_aoclk_gate,
+	.hw_data	= &gxbb_aoclk_onecell_data,
+};
+
+static int gxbb_aoclkc_probe(struct platform_device *pdev)
+{
+	int ret = gxbb_register_cec_ao_32k(pdev);
 	if (ret)
 		return ret;
 
-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
-			&gxbb_aoclk_onecell_data);
+	return meson_aoclkc_probe(pdev);
 }
 
 static const struct of_device_id gxbb_aoclkc_match_table[] = {
-	{ .compatible = "amlogic,meson-gx-aoclkc" },
+	{
+		.compatible	= "amlogic,meson-gx-aoclkc",
+		.data		= &gxbb_aoclkc_data,
+	},
 	{ }
 };
 
diff --git a/drivers/clk/meson/gxbb-aoclk.h b/drivers/clk/meson/gxbb-aoclk.h
index badc4c22b4ee..b031f1a0213e 100644
--- a/drivers/clk/meson/gxbb-aoclk.h
+++ b/drivers/clk/meson/gxbb-aoclk.h
@@ -8,6 +8,10 @@
 #ifndef __GXBB_AOCLKC_H
 #define __GXBB_AOCLKC_H
 
+#include "meson-aoclk.h"
+
+#define NR_CLKS	7
+
 /* AO Configuration Clock registers offsets */
 #define AO_RTI_PWR_CNTL_REG1	0x0c
 #define AO_RTI_PWR_CNTL_REG0	0x10
@@ -26,4 +30,7 @@ struct aoclk_cec_32k {
 
 extern const struct clk_ops meson_aoclk_cec_32k_ops;
 
+#include <dt-bindings/clock/gxbb-aoclkc.h>
+#include <dt-bindings/reset/gxbb-aoclkc.h>
+
 #endif /* __GXBB_AOCLKC_H */
diff --git a/drivers/clk/meson/meson-aoclk.c b/drivers/clk/meson/meson-aoclk.c
new file mode 100644
index 000000000000..74338ef0a62b
--- /dev/null
+++ b/drivers/clk/meson/meson-aoclk.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic Meson-AXG Clock Controller Driver
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Qiufang Dai <qiufang.dai@amlogic.com>
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/mfd/syscon.h>
+#include <linux/init.h>
+#include <linux/of_device.h>
+#include "clk-regmap.h"
+#include "meson-aoclk.h"
+
+static int meson_aoclk_do_reset(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct meson_aoclk_reset_controller *rstc =
+		container_of(rcdev, struct meson_aoclk_reset_controller, reset);
+
+	return regmap_write(rstc->regmap, rstc->data->reset_reg,
+			    BIT(rstc->data->reset[id]));
+}
+
+static const struct reset_control_ops meson_aoclk_reset_ops = {
+	.reset = meson_aoclk_do_reset,
+};
+
+int meson_aoclkc_probe(struct platform_device *pdev)
+{
+	struct meson_aoclk_reset_controller *rstc;
+	struct meson_aoclk_data *data;
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap;
+	int ret, clkid;
+
+	data = (struct meson_aoclk_data *) of_device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
+	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
+	if (!rstc)
+		return -ENOMEM;
+
+	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "failed to get regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	/* Reset Controller */
+	rstc->data = data;
+	rstc->regmap = regmap;
+	rstc->reset.ops = &meson_aoclk_reset_ops;
+	rstc->reset.nr_resets = data->num_reset,
+	rstc->reset.of_node = dev->of_node;
+	ret = devm_reset_controller_register(dev, &rstc->reset);
+	if (ret) {
+		dev_err(dev, "failed to register reset controller\n");
+		return ret;
+	}
+
+	/*
+	 * Populate regmap and register all clks
+	 */
+	for (clkid = 0; clkid < data->num_clks; clkid++) {
+		data->clks[clkid]->map = regmap;
+
+		ret = devm_clk_hw_register(dev, data->hw_data->hws[clkid]);
+		if (ret)
+			return ret;
+	}
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+		(void *) data->hw_data);
+}
diff --git a/drivers/clk/meson/meson-aoclk.h b/drivers/clk/meson/meson-aoclk.h
new file mode 100644
index 000000000000..ec59e027dd37
--- /dev/null
+++ b/drivers/clk/meson/meson-aoclk.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2017 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Qiufang Dai <qiufang.dai@amlogic.com>
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ */
+
+#ifndef __MESON_AOCLK_H__
+#define __MESON_AOCLK_H__
+
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include "clk-regmap.h"
+
+struct meson_aoclk_data {
+	const unsigned int			reset_reg;
+	const int				num_reset;
+	const unsigned int			*reset;
+	int					num_clks;
+	struct clk_regmap			**clks;
+	const struct clk_hw_onecell_data	*hw_data;
+};
+
+struct meson_aoclk_reset_controller {
+	struct reset_controller_dev		reset;
+	const struct meson_aoclk_data		*data;
+	struct regmap				*regmap;
+};
+
+int meson_aoclkc_probe(struct platform_device *pdev);
+#endif
+
-- 
2.15.1

[PATCH v5 3/7] dt-bindings: clock: axg-aoclkc: New binding for Meson-AXG SoC

[Yixun Lan]

Update the dt-binding documentation to support new compatible string
for the Amlogic's Meson-AXG SoC.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt
index 786dc39ca904..3a880528030e 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt
+++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt
@@ -9,6 +9,7 @@ Required Properties:
 	- GXBB (S905) : "amlogic,meson-gxbb-aoclkc"
 	- GXL (S905X, S905D) : "amlogic,meson-gxl-aoclkc"
 	- GXM (S912) : "amlogic,meson-gxm-aoclkc"
+	- AXG (A113D, A113X) : "amlogic,meson-axg-aoclkc"
 	followed by the common "amlogic,meson-gx-aoclkc"
 
 - #clock-cells: should be 1.
-- 
2.15.1

[PATCH v5 4/7] dt-bindings: clock: reset: Add AXG AO Clock and Reset Bindings

[Yixun Lan]

Add dt-bindings headers for the Meson-AXG's AO clock and
reset controller.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 include/dt-bindings/clock/axg-aoclkc.h | 26 ++++++++++++++++++++++++++
 include/dt-bindings/reset/axg-aoclkc.h | 20 ++++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 include/dt-bindings/clock/axg-aoclkc.h
 create mode 100644 include/dt-bindings/reset/axg-aoclkc.h

diff --git a/include/dt-bindings/clock/axg-aoclkc.h b/include/dt-bindings/clock/axg-aoclkc.h
new file mode 100644
index 000000000000..61955016a55b
--- /dev/null
+++ b/include/dt-bindings/clock/axg-aoclkc.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
+/*
+ * Copyright (c) 2016 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Qiufang Dai <qiufang.dai@amlogic.com>
+ */
+
+#ifndef DT_BINDINGS_CLOCK_AMLOGIC_MESON_AXG_AOCLK
+#define DT_BINDINGS_CLOCK_AMLOGIC_MESON_AXG_AOCLK
+
+#define CLKID_AO_REMOTE		0
+#define CLKID_AO_I2C_MASTER	1
+#define CLKID_AO_I2C_SLAVE	2
+#define CLKID_AO_UART1		3
+#define CLKID_AO_UART2		4
+#define CLKID_AO_IR_BLASTER	5
+#define CLKID_AO_SAR_ADC	6
+#define CLKID_AO_CLK81		7
+#define CLKID_AO_SAR_ADC_SEL	8
+#define CLKID_AO_SAR_ADC_DIV	9
+#define CLKID_AO_SAR_ADC_CLK	10
+#define CLKID_AO_ALT_XTAL	11
+
+#endif
diff --git a/include/dt-bindings/reset/axg-aoclkc.h b/include/dt-bindings/reset/axg-aoclkc.h
new file mode 100644
index 000000000000..d342c0b6b2a7
--- /dev/null
+++ b/include/dt-bindings/reset/axg-aoclkc.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
+/*
+ * Copyright (c) 2016 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Qiufang Dai <qiufang.dai@amlogic.com>
+ */
+
+#ifndef DT_BINDINGS_RESET_AMLOGIC_MESON_AXG_AOCLK
+#define DT_BINDINGS_RESET_AMLOGIC_MESON_AXG_AOCLK
+
+#define RESET_AO_REMOTE		0
+#define RESET_AO_I2C_MASTER	1
+#define RESET_AO_I2C_SLAVE	2
+#define RESET_AO_UART1		3
+#define RESET_AO_UART2		4
+#define RESET_AO_IR_BLASTER	5
+
+#endif
-- 
2.15.1

[PATCH v5 5/7] clk: meson-axg: Add AO Clock and Reset controller driver

[Yixun Lan]

From: Qiufang Dai <qiufang.dai@amlogic.com>

Adds a Clock and Reset controller driver for the Always-On part
of the Amlogic Meson-AXG SoC.

Signed-off-by: Qiufang Dai <qiufang.dai@amlogic.com>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/clk/meson/Kconfig     |   1 +
 drivers/clk/meson/Makefile    |   2 +-
 drivers/clk/meson/axg-aoclk.c | 164 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/axg-aoclk.h |  31 ++++++++
 4 files changed, 197 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/meson/axg-aoclk.c
 create mode 100644 drivers/clk/meson/axg-aoclk.h

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index fddc7ec7b820..815659eebea3 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -38,6 +38,7 @@ config COMMON_CLK_AXG
 	bool
 	depends on COMMON_CLK_AMLOGIC
 	select RESET_CONTROLLER
+	select COMMON_CLK_MESON_AO
 	select COMMON_CLK_REGMAP_MESON
 	select MFD_SYSCON
 	help
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 0a8df284f4e7..d0d13aeb369a 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -6,5 +6,5 @@ obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
 obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
 obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
 obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
-obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o
+obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
 obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
diff --git a/drivers/clk/meson/axg-aoclk.c b/drivers/clk/meson/axg-aoclk.c
new file mode 100644
index 000000000000..cb56d809d3df
--- /dev/null
+++ b/drivers/clk/meson/axg-aoclk.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic Meson-AXG Clock Controller Driver
+ *
+ * Copyright (c) 2016 Baylibre SAS.
+ * Author: Michael Turquette <mturquette@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Qiufang Dai <qiufang.dai@amlogic.com>
+ */
+#include <linux/clk-provider.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/mfd/syscon.h>
+#include <linux/init.h>
+#include "clkc.h"
+#include "axg-aoclk.h"
+
+#define AXG_AO_GATE(_name, _bit)					\
+static struct clk_regmap _name##_ao = {					\
+	.data = &(struct clk_regmap_gate_data) {			\
+		.offset = (AO_RTI_GEN_CNTL_REG0),			\
+		.bit_idx = (_bit),					\
+	},								\
+	.hw.init = &(struct clk_init_data) {				\
+		.name = #_name "_ao",					\
+		.ops = &clk_regmap_gate_ops,				\
+		.parent_names = (const char *[]){ "clk81" },		\
+		.num_parents = 1,					\
+		.flags = CLK_IGNORE_UNUSED,				\
+	},								\
+}
+
+AXG_AO_GATE(remote, 0);
+AXG_AO_GATE(i2c_master, 1);
+AXG_AO_GATE(i2c_slave, 2);
+AXG_AO_GATE(uart1, 3);
+AXG_AO_GATE(uart2, 5);
+AXG_AO_GATE(ir_blaster, 6);
+AXG_AO_GATE(saradc, 7);
+
+static struct clk_regmap ao_clk81 = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = AO_RTI_PWR_CNTL_REG0,
+		.mask = 0x1,
+		.shift = 8,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "ao_clk81",
+		.ops = &clk_regmap_mux_ro_ops,
+		.parent_names = (const char *[]){ "clk81", "ao_alt_xtal"},
+		.num_parents = 2,
+	},
+};
+
+static struct clk_regmap axg_saradc_mux = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = AO_SAR_CLK,
+		.mask = 0x3,
+		.shift = 9,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "axg_saradc_mux",
+		.ops = &clk_regmap_mux_ops,
+		.parent_names = (const char *[]){ "xtal", "ao_clk81" },
+		.num_parents = 2,
+	},
+};
+
+static struct clk_regmap axg_saradc_div = {
+	.data = &(struct clk_regmap_div_data) {
+		.offset = AO_SAR_CLK,
+		.shift = 0,
+		.width = 8,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "axg_saradc_div",
+		.ops = &clk_regmap_divider_ops,
+		.parent_names = (const char *[]){ "axg_saradc_mux" },
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap axg_saradc_gate = {
+	.data = &(struct clk_regmap_gate_data) {
+		.offset = AO_SAR_CLK,
+		.bit_idx = 8,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "axg_saradc_gate",
+		.ops = &clk_regmap_gate_ops,
+		.parent_names = (const char *[]){ "axg_saradc_div" },
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static const unsigned int axg_aoclk_reset[] = {
+	[RESET_AO_REMOTE] = 16,
+	[RESET_AO_I2C_MASTER] = 18,
+	[RESET_AO_I2C_SLAVE] = 19,
+	[RESET_AO_UART1] = 17,
+	[RESET_AO_UART2] = 22,
+	[RESET_AO_IR_BLASTER] = 23,
+};
+
+static struct clk_regmap *axg_aoclk_regmap[] = {
+	[CLKID_AO_REMOTE]	= &remote_ao,
+	[CLKID_AO_I2C_MASTER]	= &i2c_master_ao,
+	[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao,
+	[CLKID_AO_UART1]	= &uart1_ao,
+	[CLKID_AO_UART2]	= &uart2_ao,
+	[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao,
+	[CLKID_AO_SAR_ADC]	= &saradc_ao,
+	[CLKID_AO_CLK81]	= &ao_clk81,
+	[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux,
+	[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div,
+	[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate,
+};
+
+static const struct clk_hw_onecell_data axg_aoclk_onecell_data = {
+	.hws = {
+		[CLKID_AO_REMOTE]	= &remote_ao.hw,
+		[CLKID_AO_I2C_MASTER]	= &i2c_master_ao.hw,
+		[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao.hw,
+		[CLKID_AO_UART1]	= &uart1_ao.hw,
+		[CLKID_AO_UART2]	= &uart2_ao.hw,
+		[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao.hw,
+		[CLKID_AO_SAR_ADC]	= &saradc_ao.hw,
+		[CLKID_AO_CLK81]	= &ao_clk81.hw,
+		[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux.hw,
+		[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div.hw,
+		[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate.hw,
+	},
+	.num = NR_CLKS,
+};
+
+static const struct meson_aoclk_data axg_aoclkc_data = {
+	.reset_reg	= AO_RTI_GEN_CNTL_REG0,
+	.num_reset	= ARRAY_SIZE(axg_aoclk_reset),
+	.reset		= axg_aoclk_reset,
+	.num_clks	= ARRAY_SIZE(axg_aoclk_regmap),
+	.clks		= axg_aoclk_regmap,
+	.hw_data	= &axg_aoclk_onecell_data,
+};
+
+static const struct of_device_id axg_aoclkc_match_table[] = {
+	{
+		.compatible	= "amlogic,meson-axg-aoclkc",
+		.data		= &axg_aoclkc_data,
+	},
+	{ }
+};
+
+static struct platform_driver axg_aoclkc_driver = {
+	.probe		= meson_aoclkc_probe,
+	.driver		= {
+		.name	= "axg-aoclkc",
+		.of_match_table = axg_aoclkc_match_table,
+	},
+};
+
+builtin_platform_driver(axg_aoclkc_driver);
diff --git a/drivers/clk/meson/axg-aoclk.h b/drivers/clk/meson/axg-aoclk.h
new file mode 100644
index 000000000000..396cd3023064
--- /dev/null
+++ b/drivers/clk/meson/axg-aoclk.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2017 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Qiufang Dai <qiufang.dai@amlogic.com>
+ */
+
+#ifndef __AXG_AOCLKC_H
+#define __AXG_AOCLKC_H
+
+#include "meson-aoclk.h"
+
+#define NR_CLKS	11
+/* AO Configuration Clock registers offsets
+ * Register offsets from the data sheet must be multiplied by 4.
+ */
+#define AO_RTI_PWR_CNTL_REG1	0x0C
+#define AO_RTI_PWR_CNTL_REG0	0x10
+#define AO_RTI_GEN_CNTL_REG0	0x40
+#define AO_OSCIN_CNTL		0x58
+#define AO_CRT_CLK_CNTL1	0x68
+#define AO_SAR_CLK		0x90
+#define AO_RTC_ALT_CLK_CNTL0	0x94
+#define AO_RTC_ALT_CLK_CNTL1	0x98
+
+#include <dt-bindings/clock/axg-aoclkc.h>
+#include <dt-bindings/reset/axg-aoclkc.h>
+
+#endif /* __AXG_AOCLKC_H */
-- 
2.15.1

[PATCH v5 6/7] clk: meson: drop CLK_SET_RATE_PARENT flag

[Yixun Lan]

The clk81 is not expected to be changed, so drop this flag.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/clk/meson/gxbb-aoclk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
index f2e0640a7f74..b3c90a17c0e0 100644
--- a/drivers/clk/meson/gxbb-aoclk.c
+++ b/drivers/clk/meson/gxbb-aoclk.c
@@ -70,7 +70,7 @@ static struct clk_regmap _name##_ao = {					\
 		.ops = &clk_regmap_gate_ops,				\
 		.parent_names = (const char *[]){ "clk81" },		\
 		.num_parents = 1,					\
-		.flags = (CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED),	\
+		.flags = CLK_IGNORE_UNUSED,				\
 	},								\
 }
 
-- 
2.15.1

[PATCH v5 7/7] clk: meson: drop CLK_IGNORE_UNUSED flag

[Yixun Lan]

Rely on drivers to request the clock explicitly.

Previous the kernel will leave the clock on while
bootloader adready initilized the clock, this wasn't
optimal way, so fix it here.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/clk/meson/axg-aoclk.c  | 1 -
 drivers/clk/meson/gxbb-aoclk.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/clk/meson/axg-aoclk.c b/drivers/clk/meson/axg-aoclk.c
index cb56d809d3df..8457b64b9b7c 100644
--- a/drivers/clk/meson/axg-aoclk.c
+++ b/drivers/clk/meson/axg-aoclk.c
@@ -27,7 +27,6 @@ static struct clk_regmap _name##_ao = {					\
 		.ops = &clk_regmap_gate_ops,				\
 		.parent_names = (const char *[]){ "clk81" },		\
 		.num_parents = 1,					\
-		.flags = CLK_IGNORE_UNUSED,				\
 	},								\
 }
 
diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
index b3c90a17c0e0..2327eb609fbe 100644
--- a/drivers/clk/meson/gxbb-aoclk.c
+++ b/drivers/clk/meson/gxbb-aoclk.c
@@ -70,7 +70,6 @@ static struct clk_regmap _name##_ao = {					\
 		.ops = &clk_regmap_gate_ops,				\
 		.parent_names = (const char *[]){ "clk81" },		\
 		.num_parents = 1,					\
-		.flags = CLK_IGNORE_UNUSED,				\
 	},								\
 }
 
-- 
2.15.1

Re: [PATCH v5 4/7] dt-bindings: clock: reset: Add AXG AO Clock and Reset Bindings

[Jerome Brunet]

On Mon, 2018-04-09 at 22:37 +0800, Yixun Lan wrote:
> Add dt-bindings headers for the Meson-AXG's AO clock and
> reset controller.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  include/dt-bindings/clock/axg-aoclkc.h | 26 ++++++++++++++++++++++++++
>  include/dt-bindings/reset/axg-aoclkc.h | 20 ++++++++++++++++++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 include/dt-bindings/clock/axg-aoclkc.h
>  create mode 100644 include/dt-bindings/reset/axg-aoclkc.h

Hi Philipp,

Is OK if we take this through clock tree ?
The related reset controller is actually part of a clock controller driver.

Best Regards
Jerome

> 
> diff --git a/include/dt-bindings/clock/axg-aoclkc.h b/include/dt-bindings/clock/axg-aoclkc.h
> new file mode 100644
> index 000000000000..61955016a55b
> --- /dev/null
> +++ b/include/dt-bindings/clock/axg-aoclkc.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> +/*
> + * Copyright (c) 2016 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + */
> +
> +#ifndef DT_BINDINGS_CLOCK_AMLOGIC_MESON_AXG_AOCLK
> +#define DT_BINDINGS_CLOCK_AMLOGIC_MESON_AXG_AOCLK
> +
> +#define CLKID_AO_REMOTE		0
> +#define CLKID_AO_I2C_MASTER	1
> +#define CLKID_AO_I2C_SLAVE	2
> +#define CLKID_AO_UART1		3
> +#define CLKID_AO_UART2		4
> +#define CLKID_AO_IR_BLASTER	5
> +#define CLKID_AO_SAR_ADC	6
> +#define CLKID_AO_CLK81		7
> +#define CLKID_AO_SAR_ADC_SEL	8
> +#define CLKID_AO_SAR_ADC_DIV	9
> +#define CLKID_AO_SAR_ADC_CLK	10
> +#define CLKID_AO_ALT_XTAL	11
> +
> +#endif
> diff --git a/include/dt-bindings/reset/axg-aoclkc.h b/include/dt-bindings/reset/axg-aoclkc.h
> new file mode 100644
> index 000000000000..d342c0b6b2a7
> --- /dev/null
> +++ b/include/dt-bindings/reset/axg-aoclkc.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> +/*
> + * Copyright (c) 2016 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + */
> +
> +#ifndef DT_BINDINGS_RESET_AMLOGIC_MESON_AXG_AOCLK
> +#define DT_BINDINGS_RESET_AMLOGIC_MESON_AXG_AOCLK
> +
> +#define RESET_AO_REMOTE		0
> +#define RESET_AO_I2C_MASTER	1
> +#define RESET_AO_I2C_SLAVE	2
> +#define RESET_AO_UART1		3
> +#define RESET_AO_UART2		4
> +#define RESET_AO_IR_BLASTER	5
> +
> +#endif

Re: [PATCH v5 4/7] dt-bindings: clock: reset: Add AXG AO Clock and Reset Bindings

[Philipp Zabel]

Hi Jerome,

On Mon, 2018-04-16 at 11:59 +0200, Jerome Brunet wrote:
> On Mon, 2018-04-09 at 22:37 +0800, Yixun Lan wrote:
> > Add dt-bindings headers for the Meson-AXG's AO clock and
> > reset controller.
> > 
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> > ---
> >  include/dt-bindings/clock/axg-aoclkc.h | 26 ++++++++++++++++++++++++++
> >  include/dt-bindings/reset/axg-aoclkc.h | 20 ++++++++++++++++++++
> >  2 files changed, 46 insertions(+)
> >  create mode 100644 include/dt-bindings/clock/axg-aoclkc.h
> >  create mode 100644 include/dt-bindings/reset/axg-aoclkc.h
> 
> Hi Philipp,
> 
> Is OK if we take this through clock tree ?
> The related reset controller is actually part of a clock controller driver.

Absolutely, this should go in together with the driver.
I see no risk of merge conflicts here.

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v5 2/7] clk: meson: aoclk: refactor common code into dedicated file

[Jerome Brunet]

On Mon, 2018-04-09 at 22:37 +0800, Yixun Lan wrote:
> We try to refactor the common code into one dedicated file,
> while preparing to add new Meson-AXG aoclk driver, this would
> help us to better share the code by all aoclk drivers.
> 
> Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/clk/meson/Kconfig       |  7 ++++
>  drivers/clk/meson/Makefile      |  1 +
>  drivers/clk/meson/gxbb-aoclk.c  | 91 ++++++++++++++---------------------------
>  drivers/clk/meson/gxbb-aoclk.h  |  7 ++++
>  drivers/clk/meson/meson-aoclk.c | 82 +++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/meson-aoclk.h | 35 ++++++++++++++++
>  6 files changed, 163 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/clk/meson/meson-aoclk.c
>  create mode 100644 drivers/clk/meson/meson-aoclk.h
> 
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index d5cbec522aec..fddc7ec7b820 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -3,6 +3,12 @@ config COMMON_CLK_AMLOGIC
>  	depends on OF
>  	depends on ARCH_MESON || COMPILE_TEST
>  
> +config COMMON_CLK_MESON_AO
> +	bool
> +	depends on OF
> +	depends on ARCH_MESON || COMPILE_TEST
> +	select COMMON_CLK_REGMAP_MESON
> +
>  config COMMON_CLK_REGMAP_MESON
>  	bool
>  	select REGMAP
> @@ -21,6 +27,7 @@ config COMMON_CLK_GXBB
>  	bool
>  	depends on COMMON_CLK_AMLOGIC
>  	select RESET_CONTROLLER
> +	select COMMON_CLK_MESON_AO
>  	select COMMON_CLK_REGMAP_MESON
>  	select MFD_SYSCON
>  	help
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index ffee82e60b7a..0a8df284f4e7 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -3,6 +3,7 @@
>  #
>  
>  obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
> +obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>  obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o
> diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
> index eebb580b9e0f..f2e0640a7f74 100644
> --- a/drivers/clk/meson/gxbb-aoclk.c
> +++ b/drivers/clk/meson/gxbb-aoclk.c
> @@ -52,39 +52,13 @@
>   * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
> -#include <linux/clk-provider.h>
> -#include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/reset-controller.h>
>  #include <linux/mfd/syscon.h>
> -#include <linux/regmap.h>
>  #include <linux/init.h>
> -#include <linux/delay.h>
> -#include <dt-bindings/clock/gxbb-aoclkc.h>
> -#include <dt-bindings/reset/gxbb-aoclkc.h>
>  #include "clk-regmap.h"
>  #include "gxbb-aoclk.h"
>  
> -struct gxbb_aoclk_reset_controller {
> -	struct reset_controller_dev reset;
> -	unsigned int *data;
> -	struct regmap *regmap;
> -};
> -
> -static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
> -			       unsigned long id)
> -{
> -	struct gxbb_aoclk_reset_controller *reset =
> -		container_of(rcdev, struct gxbb_aoclk_reset_controller, reset);
> -
> -	return regmap_write(reset->regmap, AO_RTI_GEN_CNTL_REG0,
> -			    BIT(reset->data[id]));
> -}
> -
> -static const struct reset_control_ops gxbb_aoclk_reset_ops = {
> -	.reset = gxbb_aoclk_do_reset,
> -};
> -
>  #define GXBB_AO_GATE(_name, _bit)					\
>  static struct clk_regmap _name##_ao = {					\
>  	.data = &(struct clk_regmap_gate_data) {			\
> @@ -117,7 +91,7 @@ static struct aoclk_cec_32k cec_32k_ao = {
>  	},
>  };
>  
> -static unsigned int gxbb_aoclk_reset[] = {
> +static const unsigned int gxbb_aoclk_reset[] = {
>  	[RESET_AO_REMOTE] = 16,
>  	[RESET_AO_I2C_MASTER] = 18,
>  	[RESET_AO_I2C_SLAVE] = 19,
> @@ -135,7 +109,7 @@ static struct clk_regmap *gxbb_aoclk_gate[] = {
>  	[CLKID_AO_IR_BLASTER] = &ir_blaster_ao,
>  };
>  
> -static struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
> +static const struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
>  	.hws = {
>  		[CLKID_AO_REMOTE] = &remote_ao.hw,
>  		[CLKID_AO_I2C_MASTER] = &i2c_master_ao.hw,
> @@ -145,58 +119,55 @@ static struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
>  		[CLKID_AO_IR_BLASTER] = &ir_blaster_ao.hw,
>  		[CLKID_AO_CEC_32K] = &cec_32k_ao.hw,
>  	},
> -	.num = 7,
> +	.num = NR_CLKS,
>  };
>  
> -static int gxbb_aoclkc_probe(struct platform_device *pdev)
> +static int gxbb_register_cec_ao_32k(struct platform_device *pdev)
>  {
> -	struct gxbb_aoclk_reset_controller *rstc;
>  	struct device *dev = &pdev->dev;
>  	struct regmap *regmap;
> -	int ret, clkid;
> -
> -	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
> -	if (!rstc)
> -		return -ENOMEM;
> +	int ret;
>  
>  	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
>  	if (IS_ERR(regmap)) {
>  		dev_err(dev, "failed to get regmap\n");
> -		return -ENODEV;
> -	}
> -
> -	/* Reset Controller */
> -	rstc->regmap = regmap;
> -	rstc->data = gxbb_aoclk_reset;
> -	rstc->reset.ops = &gxbb_aoclk_reset_ops;
> -	rstc->reset.nr_resets = ARRAY_SIZE(gxbb_aoclk_reset);
> -	rstc->reset.of_node = dev->of_node;
> -	ret = devm_reset_controller_register(dev, &rstc->reset);
> -
> -	/*
> -	 * Populate regmap and register all clks
> -	 */
> -	for (clkid = 0; clkid < ARRAY_SIZE(gxbb_aoclk_gate); clkid++) {
> -		gxbb_aoclk_gate[clkid]->map = regmap;
> -
> -		ret = devm_clk_hw_register(dev,
> -					   gxbb_aoclk_onecell_data.hws[clkid]);
> -		if (ret)
> -			return ret;
> +		return PTR_ERR(regmap);
>  	}
>  
>  	/* Specific clocks */
>  	cec_32k_ao.regmap = regmap;
>  	ret = devm_clk_hw_register(dev, &cec_32k_ao.hw);
> +	if (ret) {
> +		dev_err(&pdev->dev, "clk cec_32k_ao register failed.\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct meson_aoclk_data gxbb_aoclkc_data = {
> +	.reset_reg	= AO_RTI_GEN_CNTL_REG0,
> +	.num_reset	= ARRAY_SIZE(gxbb_aoclk_reset),
> +	.reset		= gxbb_aoclk_reset,
> +	.num_clks	= ARRAY_SIZE(gxbb_aoclk_gate),
> +	.clks		= gxbb_aoclk_gate,
> +	.hw_data	= &gxbb_aoclk_onecell_data,
> +};
> +
> +static int gxbb_aoclkc_probe(struct platform_device *pdev)
> +{
> +	int ret = gxbb_register_cec_ao_32k(pdev);
>  	if (ret)
>  		return ret;
>  
> -	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> -			&gxbb_aoclk_onecell_data);
> +	return meson_aoclkc_probe(pdev);
>  }
>  
>  static const struct of_device_id gxbb_aoclkc_match_table[] = {
> -	{ .compatible = "amlogic,meson-gx-aoclkc" },
> +	{
> +		.compatible	= "amlogic,meson-gx-aoclkc",
> +		.data		= &gxbb_aoclkc_data,
> +	},
>  	{ }
>  };
>  
> diff --git a/drivers/clk/meson/gxbb-aoclk.h b/drivers/clk/meson/gxbb-aoclk.h
> index badc4c22b4ee..b031f1a0213e 100644
> --- a/drivers/clk/meson/gxbb-aoclk.h
> +++ b/drivers/clk/meson/gxbb-aoclk.h
> @@ -8,6 +8,10 @@
>  #ifndef __GXBB_AOCLKC_H
>  #define __GXBB_AOCLKC_H
>  
> +#include "meson-aoclk.h"
> +
> +#define NR_CLKS	7
> +
>  /* AO Configuration Clock registers offsets */
>  #define AO_RTI_PWR_CNTL_REG1	0x0c
>  #define AO_RTI_PWR_CNTL_REG0	0x10
> @@ -26,4 +30,7 @@ struct aoclk_cec_32k {
>  
>  extern const struct clk_ops meson_aoclk_cec_32k_ops;
>  
> +#include <dt-bindings/clock/gxbb-aoclkc.h>
> +#include <dt-bindings/reset/gxbb-aoclkc.h>
> +
>  #endif /* __GXBB_AOCLKC_H */
> diff --git a/drivers/clk/meson/meson-aoclk.c b/drivers/clk/meson/meson-aoclk.c
> new file mode 100644
> index 000000000000..74338ef0a62b
> --- /dev/null
> +++ b/drivers/clk/meson/meson-aoclk.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Amlogic Meson-AXG Clock Controller Driver
> + *
> + * Copyright (c) 2016 BayLibre, SAS.
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/init.h>
> +#include <linux/of_device.h>
> +#include "clk-regmap.h"
> +#include "meson-aoclk.h"
> +
> +static int meson_aoclk_do_reset(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct meson_aoclk_reset_controller *rstc =
> +		container_of(rcdev, struct meson_aoclk_reset_controller, reset);
> +
> +	return regmap_write(rstc->regmap, rstc->data->reset_reg,
> +			    BIT(rstc->data->reset[id]));
> +}
> +
> +static const struct reset_control_ops meson_aoclk_reset_ops = {
> +	.reset = meson_aoclk_do_reset,
> +};
> +
> +int meson_aoclkc_probe(struct platform_device *pdev)
> +{
> +	struct meson_aoclk_reset_controller *rstc;
> +	struct meson_aoclk_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	int ret, clkid;
> +
> +	data = (struct meson_aoclk_data *) of_device_get_match_data(dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
> +	if (!rstc)
> +		return -ENOMEM;
> +
> +	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "failed to get regmap\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	/* Reset Controller */
> +	rstc->data = data;
> +	rstc->regmap = regmap;
> +	rstc->reset.ops = &meson_aoclk_reset_ops;
> +	rstc->reset.nr_resets = data->num_reset,
> +	rstc->reset.of_node = dev->of_node;
> +	ret = devm_reset_controller_register(dev, &rstc->reset);
> +	if (ret) {
> +		dev_err(dev, "failed to register reset controller\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Populate regmap and register all clks
> +	 */
> +	for (clkid = 0; clkid < data->num_clks; clkid++) {
> +		data->clks[clkid]->map = regmap;
> +
> +		ret = devm_clk_hw_register(dev, data->hw_data->hws[clkid]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +		(void *) data->hw_data);
> +}

Missing new line here. Please run checkpatch and pay attention to git warnings
about whitespace errors.

> diff --git a/drivers/clk/meson/meson-aoclk.h b/drivers/clk/meson/meson-aoclk.h
> new file mode 100644
> index 000000000000..ec59e027dd37
> --- /dev/null
> +++ b/drivers/clk/meson/meson-aoclk.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2017 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#ifndef __MESON_AOCLK_H__
> +#define __MESON_AOCLK_H__
> +
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include "clk-regmap.h"
> +
> +struct meson_aoclk_data {
> +	const unsigned int			reset_reg;
> +	const int				num_reset;
> +	const unsigned int			*reset;
> +	int					num_clks;
> +	struct clk_regmap			**clks;
> +	const struct clk_hw_onecell_data	*hw_data;
> +};
> +
> +struct meson_aoclk_reset_controller {
> +	struct reset_controller_dev		reset;
> +	const struct meson_aoclk_data		*data;
> +	struct regmap				*regmap;
> +};
> +
> +int meson_aoclkc_probe(struct platform_device *pdev);
> +#endif
> +

Re: [PATCH v5 2/7] clk: meson: aoclk: refactor common code into dedicated file

[Jerome Brunet]

On Mon, 2018-04-09 at 22:37 +0800, Yixun Lan wrote:
> We try to refactor the common code into one dedicated file,
> while preparing to add new Meson-AXG aoclk driver, this would
> help us to better share the code by all aoclk drivers.
> 
> Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/clk/meson/Kconfig       |  7 ++++
>  drivers/clk/meson/Makefile      |  1 +
>  drivers/clk/meson/gxbb-aoclk.c  | 91 ++++++++++++++---------------------------
>  drivers/clk/meson/gxbb-aoclk.h  |  7 ++++
>  drivers/clk/meson/meson-aoclk.c | 82 +++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/meson-aoclk.h | 35 ++++++++++++++++
>  6 files changed, 163 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/clk/meson/meson-aoclk.c
>  create mode 100644 drivers/clk/meson/meson-aoclk.h
> 
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index d5cbec522aec..fddc7ec7b820 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -3,6 +3,12 @@ config COMMON_CLK_AMLOGIC
>  	depends on OF
>  	depends on ARCH_MESON || COMPILE_TEST
>  
> +config COMMON_CLK_MESON_AO
> +	bool
> +	depends on OF
> +	depends on ARCH_MESON || COMPILE_TEST
> +	select COMMON_CLK_REGMAP_MESON
> +
>  config COMMON_CLK_REGMAP_MESON
>  	bool
>  	select REGMAP
> @@ -21,6 +27,7 @@ config COMMON_CLK_GXBB
>  	bool
>  	depends on COMMON_CLK_AMLOGIC
>  	select RESET_CONTROLLER
> +	select COMMON_CLK_MESON_AO
>  	select COMMON_CLK_REGMAP_MESON
>  	select MFD_SYSCON
>  	help
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index ffee82e60b7a..0a8df284f4e7 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -3,6 +3,7 @@
>  #
>  
>  obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
> +obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>  obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o
> diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
> index eebb580b9e0f..f2e0640a7f74 100644
> --- a/drivers/clk/meson/gxbb-aoclk.c
> +++ b/drivers/clk/meson/gxbb-aoclk.c
> @@ -52,39 +52,13 @@
>   * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
> -#include <linux/clk-provider.h>
> -#include <linux/of_address.h>
>  #include <linux/platform_device.h>

>  #include <linux/reset-controller.h>
Not required anymore

>  #include <linux/mfd/syscon.h>
> -#include <linux/regmap.h>
>  #include <linux/init.h>
Not required

> -#include <linux/delay.h>

> -#include <dt-bindings/clock/gxbb-aoclkc.h>
> -#include <dt-bindings/reset/gxbb-aoclkc.h>
Please keep these 2 here

>  #include "clk-regmap.h"
>  #include "gxbb-aoclk.h"
>  
> -struct gxbb_aoclk_reset_controller {
> -	struct reset_controller_dev reset;
> -	unsigned int *data;
> -	struct regmap *regmap;
> -};
> -
> -static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
> -			       unsigned long id)
> -{
> -	struct gxbb_aoclk_reset_controller *reset =
> -		container_of(rcdev, struct gxbb_aoclk_reset_controller, reset);
> -
> -	return regmap_write(reset->regmap, AO_RTI_GEN_CNTL_REG0,
> -			    BIT(reset->data[id]));
> -}
> -
> -static const struct reset_control_ops gxbb_aoclk_reset_ops = {
> -	.reset = gxbb_aoclk_do_reset,
> -};
> -
>  #define GXBB_AO_GATE(_name, _bit)					\
>  static struct clk_regmap _name##_ao = {					\
>  	.data = &(struct clk_regmap_gate_data) {			\
> @@ -117,7 +91,7 @@ static struct aoclk_cec_32k cec_32k_ao = {
>  	},
>  };
>  
> -static unsigned int gxbb_aoclk_reset[] = {
> +static const unsigned int gxbb_aoclk_reset[] = {
>  	[RESET_AO_REMOTE] = 16,
>  	[RESET_AO_I2C_MASTER] = 18,
>  	[RESET_AO_I2C_SLAVE] = 19,
> @@ -135,7 +109,7 @@ static struct clk_regmap *gxbb_aoclk_gate[] = {
>  	[CLKID_AO_IR_BLASTER] = &ir_blaster_ao,
>  };
>  
> -static struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
> +static const struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
>  	.hws = {
>  		[CLKID_AO_REMOTE] = &remote_ao.hw,
>  		[CLKID_AO_I2C_MASTER] = &i2c_master_ao.hw,
> @@ -145,58 +119,55 @@ static struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
>  		[CLKID_AO_IR_BLASTER] = &ir_blaster_ao.hw,
>  		[CLKID_AO_CEC_32K] = &cec_32k_ao.hw,
>  	},
> -	.num = 7,
> +	.num = NR_CLKS,
>  };
>  
> -static int gxbb_aoclkc_probe(struct platform_device *pdev)
> +static int gxbb_register_cec_ao_32k(struct platform_device *pdev)
>  {
> -	struct gxbb_aoclk_reset_controller *rstc;
>  	struct device *dev = &pdev->dev;
>  	struct regmap *regmap;
> -	int ret, clkid;
> -
> -	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
> -	if (!rstc)
> -		return -ENOMEM;
> +	int ret;
>  
>  	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
>  	if (IS_ERR(regmap)) {
>  		dev_err(dev, "failed to get regmap\n");
> -		return -ENODEV;
> -	}
> -
> -	/* Reset Controller */
> -	rstc->regmap = regmap;
> -	rstc->data = gxbb_aoclk_reset;
> -	rstc->reset.ops = &gxbb_aoclk_reset_ops;
> -	rstc->reset.nr_resets = ARRAY_SIZE(gxbb_aoclk_reset);
> -	rstc->reset.of_node = dev->of_node;
> -	ret = devm_reset_controller_register(dev, &rstc->reset);
> -
> -	/*
> -	 * Populate regmap and register all clks
> -	 */
> -	for (clkid = 0; clkid < ARRAY_SIZE(gxbb_aoclk_gate); clkid++) {
> -		gxbb_aoclk_gate[clkid]->map = regmap;
> -
> -		ret = devm_clk_hw_register(dev,
> -					   gxbb_aoclk_onecell_data.hws[clkid]);
> -		if (ret)
> -			return ret;
> +		return PTR_ERR(regmap);
>  	}
>  
>  	/* Specific clocks */
>  	cec_32k_ao.regmap = regmap;
>  	ret = devm_clk_hw_register(dev, &cec_32k_ao.hw);
> +	if (ret) {
> +		dev_err(&pdev->dev, "clk cec_32k_ao register failed.\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct meson_aoclk_data gxbb_aoclkc_data = {
> +	.reset_reg	= AO_RTI_GEN_CNTL_REG0,
> +	.num_reset	= ARRAY_SIZE(gxbb_aoclk_reset),
> +	.reset		= gxbb_aoclk_reset,
> +	.num_clks	= ARRAY_SIZE(gxbb_aoclk_gate),
> +	.clks		= gxbb_aoclk_gate,
> +	.hw_data	= &gxbb_aoclk_onecell_data,
> +};
> +
> +static int gxbb_aoclkc_probe(struct platform_device *pdev)
> +{
> +	int ret = gxbb_register_cec_ao_32k(pdev);
>  	if (ret)
>  		return ret;
>  
> -	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> -			&gxbb_aoclk_onecell_data);
> +	return meson_aoclkc_probe(pdev);
>  }
>  
>  static const struct of_device_id gxbb_aoclkc_match_table[] = {
> -	{ .compatible = "amlogic,meson-gx-aoclkc" },
> +	{
> +		.compatible	= "amlogic,meson-gx-aoclkc",
> +		.data		= &gxbb_aoclkc_data,
> +	},
>  	{ }
>  };
>  
> diff --git a/drivers/clk/meson/gxbb-aoclk.h b/drivers/clk/meson/gxbb-aoclk.h
> index badc4c22b4ee..b031f1a0213e 100644
> --- a/drivers/clk/meson/gxbb-aoclk.h
> +++ b/drivers/clk/meson/gxbb-aoclk.h
> @@ -8,6 +8,10 @@
>  #ifndef __GXBB_AOCLKC_H
>  #define __GXBB_AOCLKC_H
>  
> +#include "meson-aoclk.h"

You are not using any of the structure defined in meson-aoclk.h in here.
Why do you include this here ?

> +
> +#define NR_CLKS	7
> +
>  /* AO Configuration Clock registers offsets */
>  #define AO_RTI_PWR_CNTL_REG1	0x0c
>  #define AO_RTI_PWR_CNTL_REG0	0x10
> @@ -26,4 +30,7 @@ struct aoclk_cec_32k {
>  
>  extern const struct clk_ops meson_aoclk_cec_32k_ops;
>  
> +#include <dt-bindings/clock/gxbb-aoclkc.h>
> +#include <dt-bindings/reset/gxbb-aoclkc.h>
Please drop this.

> +
>  #endif /* __GXBB_AOCLKC_H */
> diff --git a/drivers/clk/meson/meson-aoclk.c b/drivers/clk/meson/meson-aoclk.c
> new file mode 100644
> index 000000000000..74338ef0a62b
> --- /dev/null
> +++ b/drivers/clk/meson/meson-aoclk.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Amlogic Meson-AXG Clock Controller Driver
> + *
> + * Copyright (c) 2016 BayLibre, SAS.
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/init.h>
Not required

> +#include <linux/of_device.h>
> +#include "clk-regmap.h"
> +#include "meson-aoclk.h"
> +
> +static int meson_aoclk_do_reset(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct meson_aoclk_reset_controller *rstc =
> +		container_of(rcdev, struct meson_aoclk_reset_controller, reset);
> +
> +	return regmap_write(rstc->regmap, rstc->data->reset_reg,
> +			    BIT(rstc->data->reset[id]));
> +}
> +
> +static const struct reset_control_ops meson_aoclk_reset_ops = {
> +	.reset = meson_aoclk_do_reset,
> +};
> +
> +int meson_aoclkc_probe(struct platform_device *pdev)
> +{
> +	struct meson_aoclk_reset_controller *rstc;
> +	struct meson_aoclk_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	int ret, clkid;
> +
> +	data = (struct meson_aoclk_data *) of_device_get_match_data(dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
> +	if (!rstc)
> +		return -ENOMEM;
> +
> +	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "failed to get regmap\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	/* Reset Controller */
> +	rstc->data = data;
> +	rstc->regmap = regmap;
> +	rstc->reset.ops = &meson_aoclk_reset_ops;
> +	rstc->reset.nr_resets = data->num_reset,
> +	rstc->reset.of_node = dev->of_node;
> +	ret = devm_reset_controller_register(dev, &rstc->reset);
> +	if (ret) {
> +		dev_err(dev, "failed to register reset controller\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Populate regmap and register all clks
> +	 */
> +	for (clkid = 0; clkid < data->num_clks; clkid++) {
> +		data->clks[clkid]->map = regmap;
> +
> +		ret = devm_clk_hw_register(dev, data->hw_data->hws[clkid]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +		(void *) data->hw_data);
> +}
> diff --git a/drivers/clk/meson/meson-aoclk.h b/drivers/clk/meson/meson-aoclk.h
> new file mode 100644
> index 000000000000..ec59e027dd37
> --- /dev/null
> +++ b/drivers/clk/meson/meson-aoclk.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2017 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#ifndef __MESON_AOCLK_H__
> +#define __MESON_AOCLK_H__
> +
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include "clk-regmap.h"
> +
> +struct meson_aoclk_data {
> +	const unsigned int			reset_reg;
> +	const int				num_reset;
> +	const unsigned int			*reset;
> +	int					num_clks;
> +	struct clk_regmap			**clks;
> +	const struct clk_hw_onecell_data	*hw_data;
> +};
> +
> +struct meson_aoclk_reset_controller {
> +	struct reset_controller_dev		reset;
> +	const struct meson_aoclk_data		*data;
> +	struct regmap				*regmap;
> +};
> +
> +int meson_aoclkc_probe(struct platform_device *pdev);
> +#endif
> +

Re: [PATCH v5 5/7] clk: meson-axg: Add AO Clock and Reset controller driver

[Jerome Brunet]

On Mon, 2018-04-09 at 22:37 +0800, Yixun Lan wrote:
> From: Qiufang Dai <qiufang.dai@amlogic.com>
> 
> Adds a Clock and Reset controller driver for the Always-On part
> of the Amlogic Meson-AXG SoC.
> 
> Signed-off-by: Qiufang Dai <qiufang.dai@amlogic.com>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/clk/meson/Kconfig     |   1 +
>  drivers/clk/meson/Makefile    |   2 +-
>  drivers/clk/meson/axg-aoclk.c | 164 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/axg-aoclk.h |  31 ++++++++
>  4 files changed, 197 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/meson/axg-aoclk.c
>  create mode 100644 drivers/clk/meson/axg-aoclk.h
> 
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index fddc7ec7b820..815659eebea3 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -38,6 +38,7 @@ config COMMON_CLK_AXG
>  	bool
>  	depends on COMMON_CLK_AMLOGIC
>  	select RESET_CONTROLLER
> +	select COMMON_CLK_MESON_AO
>  	select COMMON_CLK_REGMAP_MESON
>  	select MFD_SYSCON
>  	help
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 0a8df284f4e7..d0d13aeb369a 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -6,5 +6,5 @@ obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
>  obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
> -obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o
> +obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
> diff --git a/drivers/clk/meson/axg-aoclk.c b/drivers/clk/meson/axg-aoclk.c
> new file mode 100644
> index 000000000000..cb56d809d3df
> --- /dev/null
> +++ b/drivers/clk/meson/axg-aoclk.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Amlogic Meson-AXG Clock Controller Driver
> + *
> + * Copyright (c) 2016 Baylibre SAS.
> + * Author: Michael Turquette <mturquette@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + */
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/init.h>
Not required 

> +#include "clkc.h"
Why do you need this? looks like clk-regmap.h would be enough here ?

> +#include "axg-aoclk.h"
> +
> +#define AXG_AO_GATE(_name, _bit)					\
> +static struct clk_regmap _name##_ao = {					\
> +	.data = &(struct clk_regmap_gate_data) {			\
> +		.offset = (AO_RTI_GEN_CNTL_REG0),			\
> +		.bit_idx = (_bit),					\
> +	},								\
> +	.hw.init = &(struct clk_init_data) {				\
> +		.name = #_name "_ao",					\
> +		.ops = &clk_regmap_gate_ops,				\
> +		.parent_names = (const char *[]){ "clk81" },		\
> +		.num_parents = 1,					\
> +		.flags = CLK_IGNORE_UNUSED,				\
> +	},								\
> +}
> +
> +AXG_AO_GATE(remote, 0);
> +AXG_AO_GATE(i2c_master, 1);
> +AXG_AO_GATE(i2c_slave, 2);
> +AXG_AO_GATE(uart1, 3);
> +AXG_AO_GATE(uart2, 5);
> +AXG_AO_GATE(ir_blaster, 6);
> +AXG_AO_GATE(saradc, 7);
> +
> +static struct clk_regmap ao_clk81 = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = AO_RTI_PWR_CNTL_REG0,
> +		.mask = 0x1,
> +		.shift = 8,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "ao_clk81",
> +		.ops = &clk_regmap_mux_ro_ops,
> +		.parent_names = (const char *[]){ "clk81", "ao_alt_xtal"},
> +		.num_parents = 2,
> +	},
> +};
> +
> +static struct clk_regmap axg_saradc_mux = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = AO_SAR_CLK,
> +		.mask = 0x3,
> +		.shift = 9,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "axg_saradc_mux",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_names = (const char *[]){ "xtal", "ao_clk81" },
> +		.num_parents = 2,
> +	},
> +};
> +
> +static struct clk_regmap axg_saradc_div = {
> +	.data = &(struct clk_regmap_div_data) {
> +		.offset = AO_SAR_CLK,
> +		.shift = 0,
> +		.width = 8,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "axg_saradc_div",
> +		.ops = &clk_regmap_divider_ops,
> +		.parent_names = (const char *[]){ "axg_saradc_mux" },
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static struct clk_regmap axg_saradc_gate = {
> +	.data = &(struct clk_regmap_gate_data) {
> +		.offset = AO_SAR_CLK,
> +		.bit_idx = 8,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "axg_saradc_gate",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_names = (const char *[]){ "axg_saradc_div" },
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static const unsigned int axg_aoclk_reset[] = {
> +	[RESET_AO_REMOTE] = 16,
> +	[RESET_AO_I2C_MASTER] = 18,
> +	[RESET_AO_I2C_SLAVE] = 19,
> +	[RESET_AO_UART1] = 17,
> +	[RESET_AO_UART2] = 22,
> +	[RESET_AO_IR_BLASTER] = 23,
> +};
> +
> +static struct clk_regmap *axg_aoclk_regmap[] = {
> +	[CLKID_AO_REMOTE]	= &remote_ao,
> +	[CLKID_AO_I2C_MASTER]	= &i2c_master_ao,
> +	[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao,
> +	[CLKID_AO_UART1]	= &uart1_ao,
> +	[CLKID_AO_UART2]	= &uart2_ao,
> +	[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao,
> +	[CLKID_AO_SAR_ADC]	= &saradc_ao,
> +	[CLKID_AO_CLK81]	= &ao_clk81,
> +	[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux,
> +	[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div,
> +	[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate,

Naming is not very consistent here, sometimes ao at beginning, sometimes the
end, with or w/o axg.

I would prefer if you prefix everything with axg_ao[clk] 
ex: axg_aoclk_remote, axg_aoclk_saradc_mux, etc ...

> +};
> +
> +static const struct clk_hw_onecell_data axg_aoclk_onecell_data = {
> +	.hws = {
> +		[CLKID_AO_REMOTE]	= &remote_ao.hw,
> +		[CLKID_AO_I2C_MASTER]	= &i2c_master_ao.hw,
> +		[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao.hw,
> +		[CLKID_AO_UART1]	= &uart1_ao.hw,
> +		[CLKID_AO_UART2]	= &uart2_ao.hw,
> +		[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao.hw,
> +		[CLKID_AO_SAR_ADC]	= &saradc_ao.hw,
> +		[CLKID_AO_CLK81]	= &ao_clk81.hw,
> +		[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux.hw,
> +		[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div.hw,
> +		[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate.hw,
> +	},
> +	.num = NR_CLKS,
> +};
> +
> +static const struct meson_aoclk_data axg_aoclkc_data = {
> +	.reset_reg	= AO_RTI_GEN_CNTL_REG0,
> +	.num_reset	= ARRAY_SIZE(axg_aoclk_reset),
> +	.reset		= axg_aoclk_reset,
> +	.num_clks	= ARRAY_SIZE(axg_aoclk_regmap),
> +	.clks		= axg_aoclk_regmap,
> +	.hw_data	= &axg_aoclk_onecell_data,
> +};
> +
> +static const struct of_device_id axg_aoclkc_match_table[] = {
> +	{
> +		.compatible	= "amlogic,meson-axg-aoclkc",
> +		.data		= &axg_aoclkc_data,
> +	},
> +	{ }
> +};
> +
> +static struct platform_driver axg_aoclkc_driver = {
> +	.probe		= meson_aoclkc_probe,
> +	.driver		= {
> +		.name	= "axg-aoclkc",
> +		.of_match_table = axg_aoclkc_match_table,
> +	},
> +};
> +
> +builtin_platform_driver(axg_aoclkc_driver);
> diff --git a/drivers/clk/meson/axg-aoclk.h b/drivers/clk/meson/axg-aoclk.h
> new file mode 100644
> index 000000000000..396cd3023064
> --- /dev/null
> +++ b/drivers/clk/meson/axg-aoclk.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2017 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + */
> +
> +#ifndef __AXG_AOCLKC_H
> +#define __AXG_AOCLKC_H
> +
> +#include "meson-aoclk.h"
> +
> +#define NR_CLKS	11
> +/* AO Configuration Clock registers offsets
> + * Register offsets from the data sheet must be multiplied by 4.
> + */
> +#define AO_RTI_PWR_CNTL_REG1	0x0C
> +#define AO_RTI_PWR_CNTL_REG0	0x10
> +#define AO_RTI_GEN_CNTL_REG0	0x40
> +#define AO_OSCIN_CNTL		0x58
> +#define AO_CRT_CLK_CNTL1	0x68
> +#define AO_SAR_CLK		0x90
> +#define AO_RTC_ALT_CLK_CNTL0	0x94
> +#define AO_RTC_ALT_CLK_CNTL1	0x98
> +
> +#include <dt-bindings/clock/axg-aoclkc.h>
> +#include <dt-bindings/reset/axg-aoclkc.h>
> +
> +#endif /* __AXG_AOCLKC_H */

Re: [PATCH v5 2/7] clk: meson: aoclk: refactor common code into dedicated file

[Yixun Lan]

Hi Jerome

On 04/16/18 18:38, Jerome Brunet wrote:
>> +	 */
>> +	for (clkid = 0; clkid < data->num_clks; clkid++) {
>> +		data->clks[clkid]->map = regmap;
>> +
>> +		ret = devm_clk_hw_register(dev, data->hw_data->hws[clkid]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> +		(void *) data->hw_data);
>> +}
> Missing new line here. Please run checkpatch and pay attention to git warnings
> about whitespace errors.
> 


I think we actually need to avoid this extra new line, or what's the
benefits with new line?

and besides， I didn't see any complaint from checkpatch.pl ..

 ./scripts/checkpatch.pl
0002-clk-meson-aoclk-refactor-common-code-into-dedicated-.patch
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#235:
new file mode 100644

total: 0 errors, 1 warnings, 302 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or
--fix-inplace.

0002-clk-meson-aoclk-refactor-common-code-into-dedicated-.patch has
style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


Yixun

Re: [PATCH v5 2/7] clk: meson: aoclk: refactor common code into dedicated file

[Yixun Lan]

Hi jerome

On 04/16/18 19:34, Jerome Brunet wrote:
> On Mon, 2018-04-09 at 22:37 +0800, Yixun Lan wrote:
>> We try to refactor the common code into one dedicated file,
>> while preparing to add new Meson-AXG aoclk driver, this would
>> help us to better share the code by all aoclk drivers.
>>
>> Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  drivers/clk/meson/Kconfig       |  7 ++++
>>  drivers/clk/meson/Makefile      |  1 +
>>  drivers/clk/meson/gxbb-aoclk.c  | 91 ++++++++++++++---------------------------
>>  drivers/clk/meson/gxbb-aoclk.h  |  7 ++++
>>  drivers/clk/meson/meson-aoclk.c | 82 +++++++++++++++++++++++++++++++++++++
>>  drivers/clk/meson/meson-aoclk.h | 35 ++++++++++++++++
>>  6 files changed, 163 insertions(+), 60 deletions(-)
>>  create mode 100644 drivers/clk/meson/meson-aoclk.c
>>  create mode 100644 drivers/clk/meson/meson-aoclk.h
>>
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index d5cbec522aec..fddc7ec7b820 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -3,6 +3,12 @@ config COMMON_CLK_AMLOGIC
>>  	depends on OF
>>  	depends on ARCH_MESON || COMPILE_TEST
>>  
>> +config COMMON_CLK_MESON_AO
>> +	bool
>> +	depends on OF
>> +	depends on ARCH_MESON || COMPILE_TEST
>> +	select COMMON_CLK_REGMAP_MESON
>> +
>>  config COMMON_CLK_REGMAP_MESON
>>  	bool
>>  	select REGMAP
>> @@ -21,6 +27,7 @@ config COMMON_CLK_GXBB
>>  	bool
>>  	depends on COMMON_CLK_AMLOGIC
>>  	select RESET_CONTROLLER
>> +	select COMMON_CLK_MESON_AO
>>  	select COMMON_CLK_REGMAP_MESON
>>  	select MFD_SYSCON
>>  	help
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index ffee82e60b7a..0a8df284f4e7 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -3,6 +3,7 @@
>>  #
>>  
>>  obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
>> +obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
>>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>>  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>>  obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o
>> diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
>> index eebb580b9e0f..f2e0640a7f74 100644
>> --- a/drivers/clk/meson/gxbb-aoclk.c
>> +++ b/drivers/clk/meson/gxbb-aoclk.c
>> @@ -52,39 +52,13 @@
>>   * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>   */
>> -#include <linux/clk-provider.h>
>> -#include <linux/of_address.h>
>>  #include <linux/platform_device.h>
> 
>>  #include <linux/reset-controller.h>
> Not required anymore
> 
will remove

>>  #include <linux/mfd/syscon.h>
>> -#include <linux/regmap.h>
>>  #include <linux/init.h>
> Not required
> 
will remove
>> -#include <linux/delay.h>
> 
>> -#include <dt-bindings/clock/gxbb-aoclkc.h>
>> -#include <dt-bindings/reset/gxbb-aoclkc.h>
> Please keep these 2 here
> 
 see comments below
>>  #include "clk-regmap.h"
>>  #include "gxbb-aoclk.h"
>>  
>> -struct gxbb_aoclk_reset_controller {
>> -	struct reset_controller_dev reset;
>> -	unsigned int *data;
>> -	struct regmap *regmap;
>> -};
>> -
>> -static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
>> -			       unsigned long id)
>> -{
>> -	struct gxbb_aoclk_reset_controller *reset =
>> -		container_of(rcdev, struct gxbb_aoclk_reset_controller, reset);
>> -
>> -	return regmap_write(reset->regmap, AO_RTI_GEN_CNTL_REG0,
>> -			    BIT(reset->data[id]));
>> -}
>> -
>> -static const struct reset_control_ops gxbb_aoclk_reset_ops = {
>> -	.reset = gxbb_aoclk_do_reset,
>> -};
>> -
>>  #define GXBB_AO_GATE(_name, _bit)					\
>>  static struct clk_regmap _name##_ao = {					\
>>  	.data = &(struct clk_regmap_gate_data) {			\
>> @@ -117,7 +91,7 @@ static struct aoclk_cec_32k cec_32k_ao = {
>>  	},
>>  };
>>  
>> -static unsigned int gxbb_aoclk_reset[] = {
>> +static const unsigned int gxbb_aoclk_reset[] = {
>>  	[RESET_AO_REMOTE] = 16,
>>  	[RESET_AO_I2C_MASTER] = 18,
>>  	[RESET_AO_I2C_SLAVE] = 19,
>> @@ -135,7 +109,7 @@ static struct clk_regmap *gxbb_aoclk_gate[] = {
>>  	[CLKID_AO_IR_BLASTER] = &ir_blaster_ao,
>>  };
>>  
>> -static struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
>> +static const struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
>>  	.hws = {
>>  		[CLKID_AO_REMOTE] = &remote_ao.hw,
>>  		[CLKID_AO_I2C_MASTER] = &i2c_master_ao.hw,
>> @@ -145,58 +119,55 @@ static struct clk_hw_onecell_data gxbb_aoclk_onecell_data = {
>>  		[CLKID_AO_IR_BLASTER] = &ir_blaster_ao.hw,
>>  		[CLKID_AO_CEC_32K] = &cec_32k_ao.hw,
>>  	},
>> -	.num = 7,
>> +	.num = NR_CLKS,
>>  };
>>  
>> -static int gxbb_aoclkc_probe(struct platform_device *pdev)
>> +static int gxbb_register_cec_ao_32k(struct platform_device *pdev)
>>  {
>> -	struct gxbb_aoclk_reset_controller *rstc;
>>  	struct device *dev = &pdev->dev;
>>  	struct regmap *regmap;
>> -	int ret, clkid;
>> -
>> -	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
>> -	if (!rstc)
>> -		return -ENOMEM;
>> +	int ret;
>>  
>>  	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
>>  	if (IS_ERR(regmap)) {
>>  		dev_err(dev, "failed to get regmap\n");
>> -		return -ENODEV;
>> -	}
>> -
>> -	/* Reset Controller */
>> -	rstc->regmap = regmap;
>> -	rstc->data = gxbb_aoclk_reset;
>> -	rstc->reset.ops = &gxbb_aoclk_reset_ops;
>> -	rstc->reset.nr_resets = ARRAY_SIZE(gxbb_aoclk_reset);
>> -	rstc->reset.of_node = dev->of_node;
>> -	ret = devm_reset_controller_register(dev, &rstc->reset);
>> -
>> -	/*
>> -	 * Populate regmap and register all clks
>> -	 */
>> -	for (clkid = 0; clkid < ARRAY_SIZE(gxbb_aoclk_gate); clkid++) {
>> -		gxbb_aoclk_gate[clkid]->map = regmap;
>> -
>> -		ret = devm_clk_hw_register(dev,
>> -					   gxbb_aoclk_onecell_data.hws[clkid]);
>> -		if (ret)
>> -			return ret;
>> +		return PTR_ERR(regmap);
>>  	}
>>  
>>  	/* Specific clocks */
>>  	cec_32k_ao.regmap = regmap;
>>  	ret = devm_clk_hw_register(dev, &cec_32k_ao.hw);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "clk cec_32k_ao register failed.\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct meson_aoclk_data gxbb_aoclkc_data = {
>> +	.reset_reg	= AO_RTI_GEN_CNTL_REG0,
>> +	.num_reset	= ARRAY_SIZE(gxbb_aoclk_reset),
>> +	.reset		= gxbb_aoclk_reset,
>> +	.num_clks	= ARRAY_SIZE(gxbb_aoclk_gate),
>> +	.clks		= gxbb_aoclk_gate,
>> +	.hw_data	= &gxbb_aoclk_onecell_data,
>> +};
>> +
>> +static int gxbb_aoclkc_probe(struct platform_device *pdev)
>> +{
>> +	int ret = gxbb_register_cec_ao_32k(pdev);
>>  	if (ret)
>>  		return ret;
>>  
>> -	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> -			&gxbb_aoclk_onecell_data);
>> +	return meson_aoclkc_probe(pdev);
>>  }
>>  
>>  static const struct of_device_id gxbb_aoclkc_match_table[] = {
>> -	{ .compatible = "amlogic,meson-gx-aoclkc" },
>> +	{
>> +		.compatible	= "amlogic,meson-gx-aoclkc",
>> +		.data		= &gxbb_aoclkc_data,
>> +	},
>>  	{ }
>>  };
>>  
>> diff --git a/drivers/clk/meson/gxbb-aoclk.h b/drivers/clk/meson/gxbb-aoclk.h
>> index badc4c22b4ee..b031f1a0213e 100644
>> --- a/drivers/clk/meson/gxbb-aoclk.h
>> +++ b/drivers/clk/meson/gxbb-aoclk.h
>> @@ -8,6 +8,10 @@
>>  #ifndef __GXBB_AOCLKC_H
>>  #define __GXBB_AOCLKC_H
>>  
>> +#include "meson-aoclk.h"
> 
> You are not using any of the structure defined in meson-aoclk.h in here.
> Why do you include this here ?
> 
my initial intention was trying to avoid inlcude it in gxbb-aoclk.c
I could move to gxbb-aoclk.c if you prefer?

>> +
>> +#define NR_CLKS	7
>> +
>>  /* AO Configuration Clock registers offsets */
>>  #define AO_RTI_PWR_CNTL_REG1	0x0c
>>  #define AO_RTI_PWR_CNTL_REG0	0x10
>> @@ -26,4 +30,7 @@ struct aoclk_cec_32k {
>>  
>>  extern const struct clk_ops meson_aoclk_cec_32k_ops;
>>  
>> +#include <dt-bindings/clock/gxbb-aoclkc.h>
>> +#include <dt-bindings/reset/gxbb-aoclkc.h>
> Please drop this.
> 
according to your last comment[1], you suggested to put these files
here? and I thought it was a good idea..

or do you think it's kind of messy if we put too many private files into
the C file (which has similar functionality and can be squashed together
?) ..


[1] https://lkml.kernel.org/r/1518449571.2883.30.camel@baylibre.com

> +#include <dt-bindings/clock/axg-aoclkc.h>
> +#include <dt-bindings/reset/axg-aoclkc.h>

I think those should be included in the same fashion as axg.h, gxbb.h and
meson8.h ... which is at the end of the private header.


>> +
>>  #endif /* __GXBB_AOCLKC_H */
>> diff --git a/drivers/clk/meson/meson-aoclk.c b/drivers/clk/meson/meson-aoclk.c
>> new file mode 100644
>> index 000000000000..74338ef0a62b
>> --- /dev/null
>> +++ b/drivers/clk/meson/meson-aoclk.c
>> @@ -0,0 +1,82 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Amlogic Meson-AXG Clock Controller Driver
>> + *
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/init.h>
> Not required
> 
will drop it

>> +#include <linux/of_device.h>
>> +#include "clk-regmap.h"
>> +#include "meson-aoclk.h"
>> +
>> +static int meson_aoclk_do_reset(struct reset_controller_dev *rcdev,
>> +			       unsigned long id)
>> +{
>> +	struct meson_aoclk_reset_controller *rstc =
>> +		container_of(rcdev, struct meson_aoclk_reset_controller, reset);
>> +
>> +	return regmap_write(rstc->regmap, rstc->data->reset_reg,
>> +			    BIT(rstc->data->reset[id]));
>> +}
>> +
>> +static const struct reset_control_ops meson_aoclk_reset_ops = {
>> +	.reset = meson_aoclk_do_reset,
>> +};
>> +
>> +int meson_aoclkc_probe(struct platform_device *pdev)
>> +{
>> +	struct meson_aoclk_reset_controller *rstc;
>> +	struct meson_aoclk_data *data;
>> +	struct device *dev = &pdev->dev;
>> +	struct regmap *regmap;
>> +	int ret, clkid;
>> +
>> +	data = (struct meson_aoclk_data *) of_device_get_match_data(dev);
>> +	if (!data)
>> +		return -ENODEV;
>> +
>> +	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
>> +	if (!rstc)
>> +		return -ENOMEM;
>> +
>> +	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
>> +	if (IS_ERR(regmap)) {
>> +		dev_err(dev, "failed to get regmap\n");
>> +		return PTR_ERR(regmap);
>> +	}
>> +
>> +	/* Reset Controller */
>> +	rstc->data = data;
>> +	rstc->regmap = regmap;
>> +	rstc->reset.ops = &meson_aoclk_reset_ops;
>> +	rstc->reset.nr_resets = data->num_reset,
>> +	rstc->reset.of_node = dev->of_node;
>> +	ret = devm_reset_controller_register(dev, &rstc->reset);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register reset controller\n");
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * Populate regmap and register all clks
>> +	 */
>> +	for (clkid = 0; clkid < data->num_clks; clkid++) {
>> +		data->clks[clkid]->map = regmap;
>> +
>> +		ret = devm_clk_hw_register(dev, data->hw_data->hws[clkid]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> +		(void *) data->hw_data);
>> +}
>> diff --git a/drivers/clk/meson/meson-aoclk.h b/drivers/clk/meson/meson-aoclk.h
>> new file mode 100644
>> index 000000000000..ec59e027dd37
>> --- /dev/null
>> +++ b/drivers/clk/meson/meson-aoclk.h
>> @@ -0,0 +1,35 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2017 BayLibre, SAS
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#ifndef __MESON_AOCLK_H__
>> +#define __MESON_AOCLK_H__
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include "clk-regmap.h"
>> +
>> +struct meson_aoclk_data {
>> +	const unsigned int			reset_reg;
>> +	const int				num_reset;
>> +	const unsigned int			*reset;
>> +	int					num_clks;
>> +	struct clk_regmap			**clks;
>> +	const struct clk_hw_onecell_data	*hw_data;
>> +};
>> +
>> +struct meson_aoclk_reset_controller {
>> +	struct reset_controller_dev		reset;
>> +	const struct meson_aoclk_data		*data;
>> +	struct regmap				*regmap;
>> +};
>> +
>> +int meson_aoclkc_probe(struct platform_device *pdev);
>> +#endif
>> +
> 
> .
> 

Re: [PATCH v5 5/7] clk: meson-axg: Add AO Clock and Reset controller driver

[Yixun Lan]

Hi Jerome:

see my comments below

On 04/16/18 19:45, Jerome Brunet wrote:
> On Mon, 2018-04-09 at 22:37 +0800, Yixun Lan wrote:
>> From: Qiufang Dai <qiufang.dai@amlogic.com>
>>
>> Adds a Clock and Reset controller driver for the Always-On part
>> of the Amlogic Meson-AXG SoC.
>>
>> Signed-off-by: Qiufang Dai <qiufang.dai@amlogic.com>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  drivers/clk/meson/Kconfig     |   1 +
>>  drivers/clk/meson/Makefile    |   2 +-
>>  drivers/clk/meson/axg-aoclk.c | 164 ++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/meson/axg-aoclk.h |  31 ++++++++
>>  4 files changed, 197 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/clk/meson/axg-aoclk.c
>>  create mode 100644 drivers/clk/meson/axg-aoclk.h
>>
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index fddc7ec7b820..815659eebea3 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -38,6 +38,7 @@ config COMMON_CLK_AXG
>>  	bool
>>  	depends on COMMON_CLK_AMLOGIC
>>  	select RESET_CONTROLLER
>> +	select COMMON_CLK_MESON_AO
>>  	select COMMON_CLK_REGMAP_MESON
>>  	select MFD_SYSCON
>>  	help
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index 0a8df284f4e7..d0d13aeb369a 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -6,5 +6,5 @@ obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
>>  obj-$(CONFIG_COMMON_CLK_MESON_AO) += meson-aoclk.o
>>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>>  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>> -obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o
>> +obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
>>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
>> diff --git a/drivers/clk/meson/axg-aoclk.c b/drivers/clk/meson/axg-aoclk.c
>> new file mode 100644
>> index 000000000000..cb56d809d3df
>> --- /dev/null
>> +++ b/drivers/clk/meson/axg-aoclk.c
>> @@ -0,0 +1,164 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Amlogic Meson-AXG Clock Controller Driver
>> + *
>> + * Copyright (c) 2016 Baylibre SAS.
>> + * Author: Michael Turquette <mturquette@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
>> + */
>> +#include <linux/clk-provider.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/init.h>
> Not required 
> 
will remove it

>> +#include "clkc.h"
> Why do you need this? looks like clk-regmap.h would be enough here ?
> 
you right, I will adjust

>> +#include "axg-aoclk.h"
>> +
>> +#define AXG_AO_GATE(_name, _bit)					\
>> +static struct clk_regmap _name##_ao = {					\
>> +	.data = &(struct clk_regmap_gate_data) {			\
>> +		.offset = (AO_RTI_GEN_CNTL_REG0),			\
>> +		.bit_idx = (_bit),					\
>> +	},								\
>> +	.hw.init = &(struct clk_init_data) {				\
>> +		.name = #_name "_ao",					\
>> +		.ops = &clk_regmap_gate_ops,				\
>> +		.parent_names = (const char *[]){ "clk81" },		\
>> +		.num_parents = 1,					\
>> +		.flags = CLK_IGNORE_UNUSED,				\
>> +	},								\
>> +}
>> +
>> +AXG_AO_GATE(remote, 0);
>> +AXG_AO_GATE(i2c_master, 1);
>> +AXG_AO_GATE(i2c_slave, 2);
>> +AXG_AO_GATE(uart1, 3);
>> +AXG_AO_GATE(uart2, 5);
>> +AXG_AO_GATE(ir_blaster, 6);
>> +AXG_AO_GATE(saradc, 7);
>> +
>> +static struct clk_regmap ao_clk81 = {
>> +	.data = &(struct clk_regmap_mux_data) {
>> +		.offset = AO_RTI_PWR_CNTL_REG0,
>> +		.mask = 0x1,
>> +		.shift = 8,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "ao_clk81",
>> +		.ops = &clk_regmap_mux_ro_ops,
>> +		.parent_names = (const char *[]){ "clk81", "ao_alt_xtal"},
>> +		.num_parents = 2,
>> +	},
>> +};
>> +
>> +static struct clk_regmap axg_saradc_mux = {
>> +	.data = &(struct clk_regmap_mux_data) {
>> +		.offset = AO_SAR_CLK,
>> +		.mask = 0x3,
>> +		.shift = 9,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "axg_saradc_mux",
>> +		.ops = &clk_regmap_mux_ops,
>> +		.parent_names = (const char *[]){ "xtal", "ao_clk81" },
>> +		.num_parents = 2,
>> +	},
>> +};
>> +
>> +static struct clk_regmap axg_saradc_div = {
>> +	.data = &(struct clk_regmap_div_data) {
>> +		.offset = AO_SAR_CLK,
>> +		.shift = 0,
>> +		.width = 8,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "axg_saradc_div",
>> +		.ops = &clk_regmap_divider_ops,
>> +		.parent_names = (const char *[]){ "axg_saradc_mux" },
>> +		.num_parents = 1,
>> +		.flags = CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
>> +static struct clk_regmap axg_saradc_gate = {
>> +	.data = &(struct clk_regmap_gate_data) {
>> +		.offset = AO_SAR_CLK,
>> +		.bit_idx = 8,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "axg_saradc_gate",
>> +		.ops = &clk_regmap_gate_ops,
>> +		.parent_names = (const char *[]){ "axg_saradc_div" },
>> +		.num_parents = 1,
>> +		.flags = CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
>> +static const unsigned int axg_aoclk_reset[] = {
>> +	[RESET_AO_REMOTE] = 16,
>> +	[RESET_AO_I2C_MASTER] = 18,
>> +	[RESET_AO_I2C_SLAVE] = 19,
>> +	[RESET_AO_UART1] = 17,
>> +	[RESET_AO_UART2] = 22,
>> +	[RESET_AO_IR_BLASTER] = 23,
>> +};
>> +
>> +static struct clk_regmap *axg_aoclk_regmap[] = {
>> +	[CLKID_AO_REMOTE]	= &remote_ao,
>> +	[CLKID_AO_I2C_MASTER]	= &i2c_master_ao,
>> +	[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao,
>> +	[CLKID_AO_UART1]	= &uart1_ao,
>> +	[CLKID_AO_UART2]	= &uart2_ao,
>> +	[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao,
>> +	[CLKID_AO_SAR_ADC]	= &saradc_ao,
>> +	[CLKID_AO_CLK81]	= &ao_clk81,
>> +	[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux,
>> +	[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div,
>> +	[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate,
> 
> Naming is not very consistent here, sometimes ao at beginning, sometimes the
> end, with or w/o axg.
> 
> I would prefer if you prefix everything with axg_ao[clk] 
> ex: axg_aoclk_remote, axg_aoclk_saradc_mux, etc ...
> 
I will adjust this..

but. I'd like to make short as 'axg_ao_' prefix
since we've already adopt this, see drivers/clk/meson/axg.c:823

does this sounds good to you?

>> +};
>> +
>> +static const struct clk_hw_onecell_data axg_aoclk_onecell_data = {
>> +	.hws = {
>> +		[CLKID_AO_REMOTE]	= &remote_ao.hw,
>> +		[CLKID_AO_I2C_MASTER]	= &i2c_master_ao.hw,
>> +		[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao.hw,
>> +		[CLKID_AO_UART1]	= &uart1_ao.hw,
>> +		[CLKID_AO_UART2]	= &uart2_ao.hw,
>> +		[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao.hw,
>> +		[CLKID_AO_SAR_ADC]	= &saradc_ao.hw,
>> +		[CLKID_AO_CLK81]	= &ao_clk81.hw,
>> +		[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux.hw,
>> +		[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div.hw,
>> +		[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate.hw,
>> +	},
>> +	.num = NR_CLKS,
>> +};
>> +
>> +static const struct meson_aoclk_data axg_aoclkc_data = {
>> +	.reset_reg	= AO_RTI_GEN_CNTL_REG0,
>> +	.num_reset	= ARRAY_SIZE(axg_aoclk_reset),
>> +	.reset		= axg_aoclk_reset,
>> +	.num_clks	= ARRAY_SIZE(axg_aoclk_regmap),
>> +	.clks		= axg_aoclk_regmap,
>> +	.hw_data	= &axg_aoclk_onecell_data,
>> +};
>> +
>> +static const struct of_device_id axg_aoclkc_match_table[] = {
>> +	{
>> +		.compatible	= "amlogic,meson-axg-aoclkc",
>> +		.data		= &axg_aoclkc_data,
>> +	},
>> +	{ }
>> +};
>> +
>> +static struct platform_driver axg_aoclkc_driver = {
>> +	.probe		= meson_aoclkc_probe,
>> +	.driver		= {
>> +		.name	= "axg-aoclkc",
>> +		.of_match_table = axg_aoclkc_match_table,
>> +	},
>> +};
>> +
>> +builtin_platform_driver(axg_aoclkc_driver);
>> diff --git a/drivers/clk/meson/axg-aoclk.h b/drivers/clk/meson/axg-aoclk.h
>> new file mode 100644
>> index 000000000000..396cd3023064
>> --- /dev/null
>> +++ b/drivers/clk/meson/axg-aoclk.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2017 BayLibre, SAS
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
>> + */
>> +
>> +#ifndef __AXG_AOCLKC_H
>> +#define __AXG_AOCLKC_H
>> +
>> +#include "meson-aoclk.h"
>> +
>> +#define NR_CLKS	11
>> +/* AO Configuration Clock registers offsets
>> + * Register offsets from the data sheet must be multiplied by 4.
>> + */
>> +#define AO_RTI_PWR_CNTL_REG1	0x0C
>> +#define AO_RTI_PWR_CNTL_REG0	0x10
>> +#define AO_RTI_GEN_CNTL_REG0	0x40
>> +#define AO_OSCIN_CNTL		0x58
>> +#define AO_CRT_CLK_CNTL1	0x68
>> +#define AO_SAR_CLK		0x90
>> +#define AO_RTC_ALT_CLK_CNTL0	0x94
>> +#define AO_RTC_ALT_CLK_CNTL1	0x98
>> +
>> +#include <dt-bindings/clock/axg-aoclkc.h>
>> +#include <dt-bindings/reset/axg-aoclkc.h>
>> +
>> +#endif /* __AXG_AOCLKC_H */
> 
> .
> 

Re: [PATCH v5 2/7] clk: meson: aoclk: refactor common code into dedicated file

[Jerome Brunet]

On Tue, 2018-04-17 at 16:16 +0800, Yixun Lan wrote:
> Hi Jerome
> 
> On 04/16/18 18:38, Jerome Brunet wrote:
> > > +	 */
> > > +	for (clkid = 0; clkid < data->num_clks; clkid++) {
> > > +		data->clks[clkid]->map = regmap;
> > > +
> > > +		ret = devm_clk_hw_register(dev, data->hw_data->hws[clkid]);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> > > +		(void *) data->hw_data);
> > > +}
> > 
> > Missing new line here. Please run checkpatch and pay attention to git warnings
> > about whitespace errors.
> > 
> 

Google this if you want, As far as I know the C language requires it and makes
things nice when I cat the file.

Anyway, I don't make those rules, It is just how it is.

> 
> I think we actually need to avoid this extra new line, or what's the
> benefits with new line?
> 
> and besides， I didn't see any complaint from checkpatch.pl ..
> 
>  ./scripts/checkpatch.pl
> 0002-clk-meson-aoclk-refactor-common-code-into-dedicated-.patch
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #235:
> new file mode 100644
> 
> total: 0 errors, 1 warnings, 302 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or
> --fix-inplace.
> 
> 0002-clk-meson-aoclk-refactor-common-code-into-dedicated-.patch has
> style problems, please review.
> 
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 
> 
> Yixun