On 2/11/2020 12:29 AM, Stephen Boyd wrote:

Quoting Vignesh Raghavendra (2020-02-06 20:44:25)

quoted

diff --git a/drivers/clk/keystone/Kconfig b/drivers/clk/keystone/Kconfig
index 38aeefb1e808..69ca3db1a99e 100644
--- a/drivers/clk/keystone/Kconfig
+++ b/drivers/clk/keystone/Kconfig

@@ -26,3 +26,11 @@ config TI_SCI_CLK_PROBE_FROM_FW
          This is mostly only useful for debugging purposes, and will
          increase the boot time of the device. If you want the clocks probed
          from firmware, say Y. Otherwise, say N.
+
+config TI_SYSCON_CLK
+       tristate "Syscon based clock driver for K2/K3 SoCs"
+       depends on (ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST) && OF
+       default (ARCH_KEYSTONE || ARCH_K3)

Drop parenthesis. It's not useful. Also, not sure why OF is a build
dependency. Please drop it.

Yes, will drop..

	depends on ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
	default ARCH_KEYSTONE || ARCH_K3

quoted

+       help
+         This adds clock driver support for syscon based gate
+         clocks on TI's K2 and K3 SoCs.

diff --git a/drivers/clk/keystone/Makefile b/drivers/clk/keystone/Makefile
index d044de6f965c..0e426e648f7c 100644
--- a/drivers/clk/keystone/Makefile
+++ b/drivers/clk/keystone/Makefile

@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_COMMON_CLK_KEYSTONE)      += pll.o gate.o
 obj-$(CONFIG_TI_SCI_CLK)               += sci-clk.o
+obj-$(CONFIG_TI_SYSCON_CLK)            += syscon-clk.o

diff --git a/drivers/clk/keystone/syscon-clk.c b/drivers/clk/keystone/syscon-clk.c
new file mode 100644
index 000000000000..42e7416371ff
--- /dev/null
+++ b/drivers/clk/keystone/syscon-clk.c

@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com/
+//

These last three comment lines should be normal kernel style. /* */

quoted

+
+#include <linux/clk-provider.h>
+#include <linux/clk.h>

Is this used?

Will drop

quoted

+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>

Hopefully these two includes aren't needed.

Not anymore

quoted

+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+

[...]

quoted

+
+static int ti_syscon_gate_clk_probe(struct platform_device *pdev)
+{
+       const struct ti_syscon_gate_clk_data *data, *p;
+       struct clk_hw_onecell_data *hw_data;
+       struct device *dev = &pdev->dev;
+       struct regmap *regmap;
+       int num_clks = 0;

Please don't initialize here.

Ok

quoted

+       int i;
+
+       data = of_device_get_match_data(dev);

Use device_get_match_data() instead?

Sure

quoted

+       if (!data)
+               return -EINVAL;
+
+       regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
+                                                "ti,tbclk-syscon");
+       if (IS_ERR(regmap)) {
+               if (PTR_ERR(regmap) == -EPROBE_DEFER)
+                       return -EPROBE_DEFER;
+               dev_err(dev, "failed to find parent regmap\n");
+               return PTR_ERR(regmap);
+       }
+
+       for (p = data; p->name; p++)

Initialize num_clks here so we know it's a loop that's counting.

OK

quoted

+               num_clks++;
+
+       hw_data = devm_kzalloc(dev, struct_size(hw_data, hws, num_clks),
+                              GFP_KERNEL);
+       if (!hw_data)
+               return -ENOMEM;
+
+       hw_data->num = num_clks;
+
+       for (i = 0; i < num_clks; i++) {
+               hw_data->hws[i] = ti_syscon_gate_clk_register(dev, regmap,
+                                                             &data[i]);
+               if (IS_ERR(hw_data->hws[i]))
+                       dev_err(dev, "failed to register %s",

Add a newline?

Yes, will add

quoted

+                               data[i].name);

And we don't fail? So it really isn't a problem? Maybe dev_warn()
instead?

OK

quoted

+       }
+
+       return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+                                          hw_data);
+}
+
+#define TI_SYSCON_CLK_GATE(_name, _offset, _bit_idx)   \
+       {                                               \
+               .name = _name,                          \
+               .offset = (_offset),                    \
+               .bit_idx = (_bit_idx),                  \
+       }
+
+static const struct ti_syscon_gate_clk_data am654_clk_data[] = {
+       TI_SYSCON_CLK_GATE("ehrpwm_tbclk0", 0x0, 0),
+       TI_SYSCON_CLK_GATE("ehrpwm_tbclk1", 0x4, 0),
+       TI_SYSCON_CLK_GATE("ehrpwm_tbclk2", 0x8, 0),
+       TI_SYSCON_CLK_GATE("ehrpwm_tbclk3", 0xc, 0),
+       TI_SYSCON_CLK_GATE("ehrpwm_tbclk4", 0x10, 0),
+       TI_SYSCON_CLK_GATE("ehrpwm_tbclk5", 0x14, 0),
+       { /* Sentinel */ },
+};
+
+static const struct of_device_id ti_syscon_gate_clk_ids[] = {
+       {
+               .compatible = "ti,am654-ehrpwm-tbclk",
+               .data = &am654_clk_data,
+       },
+       { }
+};
+MODULE_DEVICE_TABLE(of, ti_syscon_gate_clk_ids);
+
+static struct platform_driver ti_syscon_gate_clk_driver = {
+       .probe = ti_syscon_gate_clk_probe,
+       .driver = {
+               .name = "ti-syscon-gate-clk",
+               .of_match_table = ti_syscon_gate_clk_ids,
+       },
+};
+

Nitpick: Drop the newline.

Ok

quoted

+module_platform_driver(ti_syscon_gate_clk_driver);
+

Thanks for the review!

Regards
Vignesh