Messages in this thread | | | Date | Sat, 15 Apr 2023 11:58:57 +0800 | Subject | Re: [PATCH v7 09/12] clk: nuvoton: Add clock driver for ma35d1 clock controller | From | Jacky Huang <> |
| |
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@nuvoton.com> >> + */ >> + >> +#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@nuvoton.com> >> + */ >> + >> +#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@nuvoton.com> >> + */ >> + >> +#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
| |