lkml.org 
[lkml]   [2023]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 12/15] clk: nuvoton: Add clock driver for ma35d1 clock controller
    From
    Date
    Quoting Jacky Huang (2023-03-15 00:28:59)
    > diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c
    > new file mode 100644
    > index 000000000000..5f4791531e47
    > --- /dev/null
    > +++ b/drivers/clk/nuvoton/clk-ma35d1-divider.c
    > @@ -0,0 +1,144 @@
    > +// 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/slab.h>
    > +#include <linux/io.h>
    > +#include <linux/err.h>
    > +#include <linux/spinlock.h>
    > +
    > +#include "clk-ma35d1.h"
    > +
    > +#define div_mask(width) ((1 << (width)) - 1)

    This is clk_div_mask()

    > +
    > +struct ma35d1_adc_clk_divider {
    > + struct clk_hw hw;
    > + void __iomem *reg;
    > + u8 shift;
    > + u8 width;
    > + u32 mask;
    > + const struct clk_div_table *table;
    > + spinlock_t *lock;
    > +};
    > +
    > +#define to_ma35d1_adc_clk_divider(_hw) \
    > + container_of(_hw, struct ma35d1_adc_clk_divider, hw)
    > +
    > +static unsigned long ma35d1_clkdiv_recalc_rate(struct clk_hw *hw,
    > + unsigned long parent_rate)
    > +{
    > + unsigned int val;
    > + struct ma35d1_adc_clk_divider *dclk = to_ma35d1_adc_clk_divider(hw);
    > +
    > + val = readl_relaxed(dclk->reg) >> dclk->shift;
    > + val &= div_mask(dclk->width);
    > + val += 1;
    > + return divider_recalc_rate(hw, parent_rate, val, dclk->table,
    > + CLK_DIVIDER_ROUND_CLOSEST, dclk->width);
    > +}
    > +
    > +static long ma35d1_clkdiv_round_rate(struct clk_hw *hw, unsigned long rate,
    > + unsigned long *prate)
    > +{
    > + struct ma35d1_adc_clk_divider *dclk = to_ma35d1_adc_clk_divider(hw);
    > +
    > + return divider_round_rate(hw, rate, prate, dclk->table,
    > + dclk->width, CLK_DIVIDER_ROUND_CLOSEST);
    > +}
    > +
    > +static int ma35d1_clkdiv_set_rate(struct clk_hw *hw, unsigned long rate,
    > + unsigned long parent_rate)
    > +{
    > + int value;
    > + unsigned long flags = 0;
    > + u32 data;
    > + struct ma35d1_adc_clk_divider *dclk = to_ma35d1_adc_clk_divider(hw);
    > +
    > + value = divider_get_val(rate, parent_rate, dclk->table,
    > + dclk->width, CLK_DIVIDER_ROUND_CLOSEST);
    > +
    > + if (dclk->lock)
    > + spin_lock_irqsave(dclk->lock, flags);
    > +
    > + data = readl_relaxed(dclk->reg);
    > + data &= ~(div_mask(dclk->width) << dclk->shift);
    > + data |= (value - 1) << dclk->shift;
    > + data |= dclk->mask;
    > +
    > + writel_relaxed(data, dclk->reg);
    > +
    > + if (dclk->lock)
    > + spin_unlock_irqrestore(dclk->lock, flags);
    > +
    > + return 0;
    > +}
    > +
    > +static const struct clk_ops ma35d1_adc_clkdiv_ops = {
    > + .recalc_rate = ma35d1_clkdiv_recalc_rate,
    > + .round_rate = ma35d1_clkdiv_round_rate,
    > + .set_rate = ma35d1_clkdiv_set_rate,
    > +};
    > +
    > +struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name,
    > + const char *parent_name,
    > + unsigned long flags, void __iomem *reg,
    > + u8 shift, u8 width, u32 mask_bit)
    > +{
    > + struct ma35d1_adc_clk_divider *div;
    > + struct clk_init_data init;
    > + struct clk_div_table *table;
    > + u32 max_div, min_div;
    > + struct clk_hw *hw;
    > + int ret;
    > + int i;
    > +
    > + /* allocate the divider */

    Please remove useless comment.

    > + div = kzalloc(sizeof(*div), GFP_KERNEL);
    > + if (!div)
    > + return ERR_PTR(-ENOMEM);
    > +
    > + /* Init the divider table */

    Please remove useless comment.

    > + max_div = div_mask(width) + 1;
    > + min_div = 1;
    > +
    > + table = kcalloc(max_div + 1, sizeof(*table), GFP_KERNEL);

    Use devm_ allocations please.

    > + if (!table) {
    > + kfree(div);
    > + 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;
    > + init.num_parents = parent_name ? 1 : 0;
    > +
    > + /* struct ma35d1_adc_clk_divider assignments */

    Please remove useless comment.

    > + div->reg = reg;
    > + div->shift = shift;
    > + div->width = width;
    > + div->mask = mask_bit ? BIT(mask_bit) : 0;
    > + div->lock = &ma35d1_lock;
    > + div->hw.init = &init;
    > + div->table = table;
    > +
    > + /* Register the clock */

    Please remove useless comment.

    > + hw = &div->hw;
    > + ret = clk_hw_register(NULL, hw);

    Use devm_clk_hw_register()

    > + if (ret) {
    > + kfree(table);
    > + kfree(div);
    > + 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..79e724b148fa
    > --- /dev/null
    > +++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c
    > @@ -0,0 +1,534 @@
    > +// SPDX-License-Identifier: GPL-2.0-only
    > +/*
    > + * Copyright (C) 2023 Nuvoton Technology Corp.
    > + * Author: Chi-Fang Li <cfli0@nuvoton.com>
    > + */
    > +
    > +#include <linux/clk.h>

    Do you need to include this header?

    > +#include <linux/clk-provider.h>
    > +#include <linux/io.h>
    > +#include <linux/slab.h>
    > +#include <linux/bitfield.h>
    > +
    > +#include "clk-ma35d1.h"
    > +
    > +#define to_ma35d1_clk_pll(clk) \
    > + (container_of(clk, struct ma35d1_clk_pll, clk))
    > +
    > +#define PLL0CTL0_FBDIV_MSK GENMASK(7, 0)
    > +#define PLL0CTL0_INDIV_MSK GENMASK(11, 8)
    > +#define PLL0CTL0_OUTDIV_MSK GENMASK(13, 12)
    > +#define PLL0CTL0_PD_MSK BIT(16)
    > +#define PLL0CTL0_BP_MSK BIT(17)
    > +#define PLLXCTL0_FBDIV_MSK GENMASK(10, 0)
    > +#define PLLXCTL0_INDIV_MSK GENMASK(17, 12)
    > +#define PLLXCTL0_MODE_MSK GENMASK(19, 18)
    > +#define PLLXCTL0_SSRATE_MSK GENMASK(30, 20)
    > +#define PLLXCTL1_PD_MSK BIT(0)
    > +#define PLLXCTL1_BP_MSK BIT(1)
    > +#define PLLXCTL1_OUTDIV_MSK GENMASK(6, 4)
    > +#define PLLXCTL1_FRAC_MSK GENMASK(31, 8)
    > +#define PLLXCTL2_SLOPE_MSK GENMASK(23, 0)
    > +
    > +struct ma35d1_clk_pll {
    > + struct clk_hw hw;
    > + u8 type;
    > + u8 mode;
    > + unsigned long rate;
    > + void __iomem *ctl0_base;
    > + void __iomem *ctl1_base;
    > + void __iomem *ctl2_base;
    > + struct regmap *regmap;
    > +};
    > +
    > +struct vsipll_freq_conf_reg_tbl {
    > + unsigned long freq;
    > + u8 mode;
    > + u32 ctl0_reg;
    > + u32 ctl1_reg;
    > + u32 ctl2_reg;
    > +};
    > +
    > +static const struct vsipll_freq_conf_reg_tbl ma35d1pll_freq[] = {
    > + { 1000000000, VSIPLL_INTEGER_MODE, 0x307d, 0x10, 0 },
    > + { 884736000, VSIPLL_FRACTIONAL_MODE, 0x41024, 0xdd2f1b11, 0 },
    > + { 533000000, VSIPLL_SS_MODE, 0x12b8102c, 0x6aaaab20, 0x12317 },
    > + { }
    > +};
    > +
    > +static void CLK_UnLockReg(struct ma35d1_clk_pll *pll)

    Please don't use a mix of upper and lower case function names.
    Everything should be lower case. Maybe the name should be

    ma35d1_clk_pll_unlock_reg()

    > +{
    > + int ret;
    > +
    > + /* Unlock PLL registers */
    > + do {
    > + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x59);
    > + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x16);
    > + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x88);
    > + regmap_read(pll->regmap, REG_SYS_RLKTZNS, &ret);
    > + } while (ret == 0);
    > +}
    > +
    > +static void CLK_LockReg(struct ma35d1_clk_pll *pll)
    > +{

    ma35d1_clk_pll_lock_reg()

    > + /* Lock PLL registers */

    Remove these worthless comments.

    > + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x0);
    > +}
    > +
    > +/* SMIC PLL for CAPLL */
    > +unsigned long CLK_GetPLLFreq_SMICPLL(struct ma35d1_clk_pll *pll,
    > + unsigned long PllSrcClk)
    > +{
    > + u32 u32M, u32N, u32P, u32OutDiv;

    Variable names should not have the type in them. 'm', 'n', 'p', 'div'
    should suffice.

    > + u32 val;
    > + unsigned long u64PllClk;
    > + u32 clk_div_table[] = { 1, 2, 4, 8};
    > +
    > + val = __raw_readl(pll->ctl0_base);

    Why do you need to use __raw_readl()? Just use readl() here.

    > +
    > + u32N = FIELD_GET(PLL0CTL0_FBDIV_MSK, val);
    > + u32M = FIELD_GET(PLL0CTL0_INDIV_MSK, val);
    > + u32P = FIELD_GET(PLL0CTL0_OUTDIV_MSK, val);
    > + u32OutDiv = clk_div_table[u32P];
    > +
    > + if (val & PLL0CTL0_BP_MSK) {
    > + u64PllClk = PllSrcClk;
    > + } else {
    > + u64PllClk = PllSrcClk * u32N;
    > + do_div(u64PllClk, u32M * u32OutDiv);
    > + }

    Add a newline here.

    > + return u64PllClk;
    > +}
    > +
    > +/* VSI-PLL: INTEGER_MODE */

    I have no idea what this means.

    > +unsigned long CLK_CalPLLFreq_Mode0(unsigned long PllSrcClk,
    > + unsigned long u64PllFreq, u32 *u32Reg)

    Again, don't put types into the variable name.

    > +{
    > + u32 u32TmpM, u32TmpN, u32TmpP;
    > + u32 u32RngMinN, u32RngMinM, u32RngMinP;
    > + u32 u32RngMaxN, u32RngMaxM, u32RngMaxP;
    > + u32 u32Tmp, u32Min, u32MinN, u32MinM, u32MinP;
    > + unsigned long u64PllClk;
    > + unsigned long u64Con1, u64Con2, u64Con3;

    My eyes! Seriously, kernel style is not this way. Did checkpatch.pl pass
    on this?

    > +
    > + u64PllClk = 0;
    > + u32Min = (u32) -1;
    > +
    > + if (!((u64PllFreq >= VSIPLL_FCLKO_MIN_FREQ) &&
    > + (u64PllFreq <= VSIPLL_FCLKO_MAX_FREQ))) {
    > + u32Reg[0] = ma35d1pll_freq[0].ctl0_reg;
    > + u32Reg[1] = ma35d1pll_freq[0].ctl1_reg;
    > + u64PllClk = ma35d1pll_freq[0].freq;
    > + return u64PllClk;
    > + }
    > +
    > + u32RngMinM = 1UL;
    > + u32RngMaxM = 63UL;
    > + u32RngMinM = ((PllSrcClk / VSIPLL_FREFDIVM_MAX_FREQ) > 1) ?
    > + (PllSrcClk / VSIPLL_FREFDIVM_MAX_FREQ) : 1;
    > + u32RngMaxM = ((PllSrcClk / VSIPLL_FREFDIVM_MIN_FREQ0) < u32RngMaxM) ?
    > + (PllSrcClk / VSIPLL_FREFDIVM_MIN_FREQ0) : u32RngMaxM;
    > +
    > + for (u32TmpM = u32RngMinM; u32TmpM < (u32RngMaxM + 1); u32TmpM++) {
    > + u64Con1 = PllSrcClk / u32TmpM;
    > + u32RngMinN = 16UL;
    > + u32RngMaxN = 2047UL;
    > + u32RngMinN = ((VSIPLL_FCLK_MIN_FREQ / u64Con1) > u32RngMinN) ?
    > + (VSIPLL_FCLK_MIN_FREQ / u64Con1) : u32RngMinN;
    > + u32RngMaxN = ((VSIPLL_FCLK_MAX_FREQ / u64Con1) < u32RngMaxN) ?
    > + (VSIPLL_FCLK_MAX_FREQ / u64Con1) : u32RngMaxN;

    Is this clamp()?

    > +
    > + for (u32TmpN = u32RngMinN; u32TmpN < (u32RngMaxN + 1);
    > + u32TmpN++) {
    > + u64Con2 = u64Con1 * u32TmpN;
    > + u32RngMinP = 1UL;
    > + u32RngMaxP = 7UL;
    > + u32RngMinP = ((u64Con2 / VSIPLL_FCLKO_MAX_FREQ) > 1) ?
    > + (u64Con2 / VSIPLL_FCLKO_MAX_FREQ) : 1;

    Is this clamp()?

    > + u32RngMaxP = ((u64Con2 / VSIPLL_FCLKO_MIN_FREQ) <
    > + u32RngMaxP) ?
    > + (u64Con2 / VSIPLL_FCLKO_MIN_FREQ) :
    > + u32RngMaxP;
    > + for (u32TmpP = u32RngMinP; u32TmpP < (u32RngMaxP + 1);
    > + u32TmpP++) {
    > + u64Con3 = u64Con2 / u32TmpP;
    > + if (u64Con3 > u64PllFreq)
    > + u32Tmp = u64Con3 - u64PllFreq;
    > + else
    > + u32Tmp = u64PllFreq - u64Con3;
    > +
    > + if (u32Tmp < u32Min) {
    > + u32Min = u32Tmp;
    > + u32MinM = u32TmpM;
    > + u32MinN = u32TmpN;
    > + u32MinP = u32TmpP;
    > +
    > + if (u32Min == 0UL) {
    > + u32Reg[0] = (u32MinM << 12) |
    > + (u32MinN);
    > + u32Reg[1] = (u32MinP << 4);
    > + return ((PllSrcClk * u32MinN) /
    > + (u32MinP * u32MinM));
    > + }
    > + }
    > + }
    > + }
    > + }

    It's too hard to read this code.

    > +
    > + u32Reg[0] = (u32MinM << 12) | (u32MinN);

    FIELD_PREP?

    > + u32Reg[1] = (u32MinP << 4);

    ditto?

    > + u64PllClk = (PllSrcClk * u32MinN) / (u32MinP * u32MinM);
    > + return u64PllClk;
    > +}
    > +
    > +/* VSI-PLL: FRACTIONAL_MODE */
    > +unsigned long CLK_CalPLLFreq_Mode1(unsigned long PllSrcClk,
    > + unsigned long u64PllFreq, u32 *u32Reg)
    > +{
    > + unsigned long u64X, u64N, u64M, u64P, u64tmp;
    > + unsigned long u64PllClk, u64FCLKO;
    > + u32 u32FRAC;
    > +
    > + if (u64PllFreq > VSIPLL_FCLKO_MAX_FREQ) {
    > + u32Reg[0] = ma35d1pll_freq[1].ctl0_reg;
    > + u32Reg[1] = ma35d1pll_freq[1].ctl1_reg;
    > + u64PllClk = ma35d1pll_freq[1].freq;
    > + return u64PllClk;
    > + }
    > +
    > + if (u64PllFreq > (VSIPLL_FCLKO_MIN_FREQ/(100-1))) {

    Use a local variable for the right hand side of the comparison.

    > + u64FCLKO = u64PllFreq * ((VSIPLL_FCLKO_MIN_FREQ / u64PllFreq) +
    > + ((VSIPLL_FCLKO_MIN_FREQ % u64PllFreq) ? 1 : 0));

    Is this DIV_ROUND_UP() or something like that?

    > + } else {
    > + pr_err("Failed to set rate %ld\n", u64PllFreq);
    > + return 0;
    > + }
    > +
    > + u64P = (u64FCLKO >= VSIPLL_FCLK_MIN_FREQ) ? 1 :
    > + ((VSIPLL_FCLK_MIN_FREQ / u64FCLKO) +
    > + ((VSIPLL_FCLK_MIN_FREQ % u64FCLKO) ? 1 : 0));
    > +
    > + if ((PllSrcClk > (VSIPLL_FREFDIVM_MAX_FREQ * (64-1))) ||
    > + (PllSrcClk < VSIPLL_FREFDIVM_MIN_FREQ1))
    > + return 0;
    > +
    [...]
    > + break;
    > + }
    > +
    > + return pllfreq;
    > +}
    > +
    > +static long ma35d1_clk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
    > + unsigned long *prate)
    > +{
    > + return rate;

    This needs to do actual math and figure out that some rate will not
    work and calculate what the rate will actually be if clk_set_rate() is
    called with 'rate'.

    > +}
    > +
    > +static int ma35d1_clk_pll_is_prepared(struct clk_hw *hw)
    > +{
    > + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw);
    > + u32 val = __raw_readl(pll->ctl1_base);
    > +
    > + return (val & VSIPLLCTL1_PD_MSK) ? 0 : 1;
    > +}
    > +
    > +static int ma35d1_clk_pll_prepare(struct clk_hw *hw)
    > +{
    > + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw);
    > + u32 val;
    > +
    > + if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) {
    > + pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n");
    > + return -EACCES;
    > + }
    > +
    > + CLK_UnLockReg(pll);
    > + val = __raw_readl(pll->ctl1_base);
    > + val &= ~VSIPLLCTL1_PD_MSK;
    > + __raw_writel(val, pll->ctl1_base);
    > + CLK_LockReg(pll);
    > + return 0;
    > +}
    > +
    > +static void ma35d1_clk_pll_unprepare(struct clk_hw *hw)
    > +{
    > + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw);
    > + u32 val;
    > +
    > + if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) {
    > + pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n");
    > + } else {
    > + val = __raw_readl(pll->ctl1_base);
    > + val |= VSIPLLCTL1_PD_MSK;
    > + __raw_writel(val, pll->ctl1_base);
    > + }
    > +}
    > +
    > +static const struct clk_ops ma35d1_clk_pll_ops = {
    > + .is_prepared = ma35d1_clk_pll_is_prepared,
    > + .prepare = ma35d1_clk_pll_prepare,
    > + .unprepare = ma35d1_clk_pll_unprepare,
    > + .set_rate = ma35d1_clk_pll_set_rate,
    > + .recalc_rate = ma35d1_clk_pll_recalc_rate,
    > + .round_rate = ma35d1_clk_pll_round_rate,
    > +};
    > +
    > +struct clk_hw *ma35d1_reg_clk_pll(enum ma35d1_pll_type type,
    > + u8 u8mode, const char *name,
    > + const char *parent,
    > + unsigned long targetFreq,
    > + void __iomem *base,
    > + struct regmap *regmap)
    > +{
    > + struct ma35d1_clk_pll *pll;
    > + struct clk_hw *hw;
    > + struct clk_init_data init;
    > + int ret;
    > +
    > + pll = kmalloc(sizeof(*pll), GFP_KERNEL);
    > + if (!pll)
    > + return ERR_PTR(-ENOMEM);
    > +
    > + pll->type = type;
    > + pll->mode = u8mode;
    > + pll->rate = targetFreq;
    > + pll->ctl0_base = base + VSIPLL_CTL0;
    > + pll->ctl1_base = base + VSIPLL_CTL1;
    > + pll->ctl2_base = base + VSIPLL_CTL2;
    > + pll->regmap = regmap;
    > +
    > + init.name = name;
    > + init.flags = 0;
    > + init.parent_names = &parent;
    > + init.num_parents = 1;
    > + init.ops = &ma35d1_clk_pll_ops;
    > + pll->hw.init = &init;
    > + hw = &pll->hw;
    > +
    > + ret = clk_hw_register(NULL, hw);
    > + if (ret) {
    > + pr_err("failed to register vsi-pll clock!!!\n");
    > + kfree(pll);
    > + 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..ac8154458b81
    > --- /dev/null
    > +++ b/drivers/clk/nuvoton/clk-ma35d1.c
    > @@ -0,0 +1,970 @@
    > +// SPDX-License-Identifier: GPL-2.0-only
    > +/*
    > + * Copyright (C) 2023 Nuvoton Technology Corp.
    > + * Author: Chi-Fang Li <cfli0@nuvoton.com>
    > + */
    > +
    > +#include <linux/clk.h>

    I don't see any clk consumer usage. Please remove.

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

    I don't see any clkdev usage. Please remove.

    > +#include <linux/io.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"
    > +
    > +DEFINE_SPINLOCK(ma35d1_lock);

    Why not static?

    > +
    > +static const char *const ca35clk_sel_clks[] = {
    > + "hxt", "capll", "ddrpll", "dummy"

    Are these parent mappings? Please use 'struct clk_parent_data' instead
    if so.

    > +};
    > +
    > +static const char *const sysclk0_sel_clks[] = {
    > + "epll_div2", "syspll"
    > +};
    > +
    [...]
    > +
    > +static struct clk_hw **hws;
    > +static struct clk_hw_onecell_data *ma35d1_hw_data;

    Any reason to make these global pointers vs local pointers during probe?

    > +
    > +static int ma35d1_clocks_probe(struct platform_device *pdev)
    > +{
    > + int ret;
    > + struct device *dev = &pdev->dev;
    > + struct device_node *clk_node = dev->of_node;
    > + void __iomem *clk_base;
    > + struct regmap *regmap;
    > + u32 pllmode[5] = { 0, 0, 0, 0, 0 };
    > + u32 pllfreq[5] = { 0, 0, 0, 0, 0 };
    > +
    > + dev_info(&pdev->dev, "Nuvoton MA35D1 Clock Driver\n");

    Drop this banner message please.

    > + ma35d1_hw_data = devm_kzalloc(&pdev->dev, struct_size(ma35d1_hw_data,
    > + hws, CLK_MAX_IDX), GFP_KERNEL);
    > +
    > + if (WARN_ON(!ma35d1_hw_data))
    > + return -ENOMEM;
    > +
    > + ma35d1_hw_data->num = CLK_MAX_IDX;
    > + hws = ma35d1_hw_data->hws;
    > +
    > + clk_node = of_find_compatible_node(NULL, NULL, "nuvoton,ma35d1-clk");
    > + clk_base = of_iomap(clk_node, 0);

    Use platform_device APIs as you have a platform device here ('pdev').

    > + of_node_put(clk_node);
    > + if (!clk_base) {
    > + pr_err("%s: could not map region\n", __func__);
    > + return -ENOMEM;
    > + }
    > + regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
    > + "nuvoton,sys");

    Why is it a syscon?

    > + if (IS_ERR(regmap))
    > + pr_warn("%s: Unable to get syscon\n", __func__);

    How can we continue without the regmap?

    > +
    > + /* clock sources */
    > + hws[HXT] = ma35d1_clk_fixed("hxt", 24000000);
    [...]
    > + /* EADC */
    > + hws[EADC_DIV] = ma35d1_clk_divider_table("eadc_div", "pclk2",
    > + clk_base + REG_CLK_CLKDIV4,
    > + 0, 4, eadc_div_table);
    > + hws[EADC_GATE] = ma35d1_clk_gate("eadc_gate", "eadc_div",
    > + clk_base + REG_CLK_APBCLK2, 25);
    > +
    > + ret = of_clk_add_hw_provider(clk_node, of_clk_hw_onecell_get,

    Use devm_ variant.

    > + ma35d1_hw_data);
    > + if (ret < 0) {
    > + dev_err(dev, "failed to register hws for MA35D1\n");
    > + iounmap(clk_base);

    Use devm mapping APIs to avoid unmapping on error path.

    > + }
    > + return ret;
    > +}
    > +
    > +static const struct of_device_id ma35d1_clk_of_match[] = {
    > + { .compatible = "nuvoton,ma35d1-clk" },
    > + { },

    Drop comma above so nothing can come after this.

    > +};
    > +MODULE_DEVICE_TABLE(of, ma35d1_clk_of_match);
    > +
    > +static struct platform_driver ma35d1_clk_driver = {
    > + .probe = ma35d1_clocks_probe,
    > + .driver = {
    > + .name = "ma35d1-clk",
    > + .of_match_table = ma35d1_clk_of_match,
    > + },
    > +};
    > +
    > +static int __init ma35d1_clocks_init(void)
    > +{
    > + return platform_driver_register(&ma35d1_clk_driver);
    > +}
    > +
    > +postcore_initcall(ma35d1_clocks_init);
    > +
    > +MODULE_AUTHOR("Chi-Fang Li<cfli0@nuvoton.com>");
    > +MODULE_DESCRIPTION("NUVOTON MA35D1 Clock Driver");
    > +MODULE_LICENSE("GPL v2");
    > diff --git a/drivers/clk/nuvoton/clk-ma35d1.h b/drivers/clk/nuvoton/clk-ma35d1.h
    > new file mode 100644
    > index 000000000000..faae5a17e425
    > --- /dev/null
    > +++ b/drivers/clk/nuvoton/clk-ma35d1.h
    > @@ -0,0 +1,198 @@
    > +/* 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
    > +
    > +#include <linux/clk.h>
    > +#include <linux/clkdev.h>

    Are these includes used?

    > +#include <linux/clk-provider.h>
    > +#include <linux/spinlock.h>
    > +#include <linux/regmap.h>
    > +#include <linux/mfd/syscon.h>
    > +#include <linux/mfd/ma35d1-sys.h>

    These probably aren't needed to be included here. Just forward declare
    structs you need and include the headers in the C file.

    > +
    [...]
    > +
    > +struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev,
    > + const char *name,
    > + const char *parent_name,
    > + unsigned long flags,
    > + void __iomem *reg, u8 shift,
    > + u8 width, u32 mask_bit);
    > +
    > +extern spinlock_t ma35d1_lock;

    Why?

    \
     
     \ /
      Last update: 2023-03-27 01:02    [W:2.160 / U:0.104 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site