Re: [PATCH v7 09/12] clk: nuvoton: Add clock driver for ma35d1 clock controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dear Stephen,


On 2023/4/14 上午 04:27, Stephen Boyd wrote:
Quoting Jacky Huang (2023-04-11 22:38:21)
diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c
new file mode 100644
index 000000000000..8d573ba3dfd3
--- /dev/null
+++ b/drivers/clk/nuvoton/clk-ma35d1-divider.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
[...]
+struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name,
+                                    const char *parent_name,
+                                    spinlock_t *lock,
+                                    unsigned long flags, void __iomem *reg,
+                                    u8 shift, u8 width, u32 mask_bit)
+{
+       struct ma35d1_adc_clk_div *div;
+       struct clk_init_data init;
+       struct clk_div_table *table;
+       u32 max_div, min_div;
+       struct clk_hw *hw;
+       int ret;
+       int i;
+
+       div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+       if (!div)
+               return ERR_PTR(-ENOMEM);
+
+       max_div = clk_div_mask(width) + 1;
+       min_div = 1;
+
+       table = devm_kcalloc(dev, max_div + 1, sizeof(*table), GFP_KERNEL);
+       if (!table)
+               return ERR_PTR(-ENOMEM);
+
+       for (i = 0; i < max_div; i++) {
+               table[i].val = min_div + i;
+               table[i].div = 2 * table[i].val;
+       }
+       table[max_div].val = 0;
+       table[max_div].div = 0;
+
+       init.name = name;
+       init.ops = &ma35d1_adc_clkdiv_ops;
+       init.flags |= flags;
+       init.parent_names = parent_name ? &parent_name : NULL;
Can you use parent_data instead?

Sure, I will modify it.



+       init.num_parents = parent_name ? 1 : 0;
+
+       div->reg = reg;
+       div->shift = shift;
+       div->width = width;
+       div->mask = mask_bit ? BIT(mask_bit) : 0;
+       div->lock = lock;
+       div->hw.init = &init;
+       div->table = table;
+
+       hw = &div->hw;
+       ret = devm_clk_hw_register(dev, hw);
+       if (ret)
+               return ERR_PTR(ret);
+       return hw;
+}
diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c b/drivers/clk/nuvoton/clk-ma35d1-pll.c
new file mode 100644
index 000000000000..6de67c964a2d
--- /dev/null
+++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c
@@ -0,0 +1,315 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ * Author: Chi-Fang Li <cfli0@xxxxxxxxxxx>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+
+#include "clk-ma35d1.h"
+
+struct ma35d1_clk_pll {
+       struct clk_hw hw;
+       u8 type;
+       u8 mode;
+       void __iomem *ctl0_base;
+       void __iomem *ctl1_base;
+       void __iomem *ctl2_base;
+};
+
[..]
+struct clk_hw *ma35d1_reg_clk_pll(enum ma35d1_pll_type type,
+                                 struct device *dev,
+                                 u8 u8mode, const char *name,
+                                 const char *parent,
+                                 void __iomem *base)
+{
+       struct ma35d1_clk_pll *pll;
+       struct clk_hw *hw;
+       struct clk_init_data init = {};
+       int ret;
+
+       pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
+       if (!pll)
+               return ERR_PTR(-ENOMEM);
+
+       pll->type = type;
+       pll->mode = u8mode;
+       pll->ctl0_base = base + REG_PLL_CTL0_OFFSET;
+       pll->ctl1_base = base + REG_PLL_CTL1_OFFSET;
+       pll->ctl2_base = base + REG_PLL_CTL2_OFFSET;
+
+       init.name = name;
+       init.flags = 0;
+       init.parent_names = &parent;
Can you use parent_data instead?

I will fix it.



+       init.num_parents = 1;
+
+       if (type == MA35D1_CAPLL || type == MA35D1_DDRPLL)
+               init.ops = &ma35d1_clk_fixed_pll_ops;
+       else
+               init.ops = &ma35d1_clk_pll_ops;
+
+       pll->hw.init = &init;
+       hw = &pll->hw;
+
+       ret = devm_clk_hw_register(dev, hw);
+       if (ret)
+               return ERR_PTR(ret);
+       return hw;
+}
diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
new file mode 100644
index 000000000000..066e1c6f2d35
--- /dev/null
+++ b/drivers/clk/nuvoton/clk-ma35d1.c
@@ -0,0 +1,897 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ * Author: Chi-Fang Li <cfli0@xxxxxxxxxxx>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+
+#include "clk-ma35d1.h"
+
+static DEFINE_SPINLOCK(ma35d1_lock);
+
+static const struct clk_parent_data ca35clk_sel_clks[] = {
+       { .fw_name = "hxt", .name = "hxt" },
+       { .fw_name = "capll", .name = "capll" },
+       { .fw_name = "ddrpll", .name = "ddrpll" },
+       { .fw_name = "dummy", .name = "dummy" }
What is 'dummy'? Is that in the binding? Note, don't put both .fw_name
and .name in the binding. For new drivers, prefer to use .index or .hw
and never use .name to describe parents.


I will remove "dummy" and use .index or .hw instead.


+};
+
+static const char *const sysclk0_sel_clks[] = {
+       "epll_div2", "syspll"
[...]
diff --git a/drivers/clk/nuvoton/clk-ma35d1.h b/drivers/clk/nuvoton/clk-ma35d1.h
new file mode 100644
index 000000000000..28c60f081788
--- /dev/null
+++ b/drivers/clk/nuvoton/clk-ma35d1.h
@@ -0,0 +1,123 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ * Author: Chi-Fang Li <cfli0@xxxxxxxxxxx>
+ */
+
+#ifndef __DRV_CLK_NUVOTON_MA35D1_H
+#define __DRV_CLK_NUVOTON_MA35D1_H
Is this header included in one C file? If so, remove the header file and
put the contents in the C file.

Both clk-ma35d1.c and clk-ma35d1-pll.c include this header file.


Best regards,

Jacky Huang





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux