Quoting Manivannan Sadhasivam (2019-08-16 20:55:57)

Hi Stephen,

On Wed, Aug 07, 2019 at 10:15:59PM -0700, Stephen Boyd wrote:

quoted

Quoting Manivannan Sadhasivam (2019-07-05 08:14:39)

quoted

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index fc1e0cf44995..ffc61ed85ade 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig

@@ -304,6 +304,12 @@ config COMMON_CLK_FIXED_MMIO
        help
          Support for Memory Mapped IO Fixed clocks
 
+config COMMON_CLK_BM1880
+       bool "Clock driver for Bitmain BM1880 SoC"
+       depends on ARCH_BITMAIN || COMPILE_TEST
+       help
+         This driver supports the clocks on Bitmain BM1880 SoC.

Can you add this config somewhere else besides the end? Preferably
close to alphabetically in this file.

Okay. I got confused by the fact that Makefile is sorted but not the
Kconfig.

Ok. I'll make a reminder to sort the Kconfig after -rc1 next time.

quoted

quoted

+
 source "drivers/clk/actions/Kconfig"
 source "drivers/clk/analogbits/Kconfig"
 source "drivers/clk/bcm/Kconfig"

diff --git a/drivers/clk/clk-bm1880.c b/drivers/clk/clk-bm1880.c
new file mode 100644
index 000000000000..26cdb75bb936
--- /dev/null
+++ b/drivers/clk/clk-bm1880.c

[....]

quoted

quoted

+
+struct clk *bm1880_clk_register_pll(const struct bm1880_pll_clock *pll_clk,
+                                   void __iomem *sys_base)
+{
+       struct bm1880_pll_hw_clock *pll_hw;
+       struct clk_init_data init;
+       struct clk_hw *hw;
+       int err;
+
+       pll_hw = kzalloc(sizeof(*pll_hw), GFP_KERNEL);
+       if (!pll_hw)
+               return ERR_PTR(-ENOMEM);
+
+       init.name = pll_clk->name;
+       init.ops = &bm1880_pll_ops;
+       init.flags = pll_clk->flags;
+       init.parent_names = &pll_clk->parent;

Can you use the new way of specifying parents instead of using strings
for everything?

Sure, will do it for clocks which doesn't use helper APIs.

quoted

quoted

+       init.num_parents = 1;
+
+       pll_hw->hw.init = &init;
+       pll_hw->pll.reg = pll_clk->reg;
+       pll_hw->base = sys_base;
+
+       hw = &pll_hw->hw;
+       err = clk_hw_register(NULL, hw);
+
+       if (err) {
+               kfree(pll_hw);
+               return ERR_PTR(err);
+       }
+
+       return hw->clk;

Can this return the clk_hw pointer instead?

What is the benefit? I see that only hw:init is going to be NULL in future.

Eventually we will remove ->clk from struct clk_hw and then this will
break. It also clearly makes this driver a clk provider driver and not a
clk consumer.

So, I'll keep it as it is.

Please no!

quoted

quoted

+               bm1880_clk_unregister_pll(data->clk_data.clks[clks[i].id]);
+
+       return PTR_ERR(clk);
+}
+
+int bm1880_clk_register_mux(const struct bm1880_mux_clock *clks,
+                           int num_clks, struct bm1880_clock_data *data)
+{
+       struct clk *clk;
+       void __iomem *sys_base = data->sys_base;
+       int i;
+
+       for (i = 0; i < num_clks; i++) {
+               clk = clk_register_mux(NULL, clks[i].name,

Can you use the clk_hw based APIs for generic type clks?

IMO using helper APIs greatly reduce code size and makes the driver
look more clean. So I prefer to use the helpers wherever applicable.
When you plan to deprecate those, I'll switch over to plain clk_hw APIs.

We have clk_hw_register_mux(). Please use it. The clk based registration
APIs are deprecated.

quoted

quoted

+       kfree(clk_data);
+}
+
+CLK_OF_DECLARE_DRIVER(bm1880_clk, "bitmain,bm1880-clk", bm1880_clk_init);

Is there a reason why it can't be a platform driver?

Hmm, I looked into the majority of drivers which live under `driver/clk/`.
Most of them are using CLK_OF_DECLARE_DRIVER, so I thought that only drivers
which have a separate directory are preferred by the maintainers to use
platform driver way.

Anyway, I can switch over to platform driver and that's what I prefer.

Yes please use a platform driver unless it doesn't work for some reason.
Even then, use a platform driver and CLK_OF_DECLARE_DRIVER() in
conjunction to register the early clks from the OF_DECLARED section and
then adopt the rest to the proper device driver later on. This way we
gain the benefits of driver core.