On 05/05, Bintian Wang wrote: > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 9897f35..935c44b 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -152,6 +152,8 @@ config COMMON_CLK_CDCE706 > > source "drivers/clk/qcom/Kconfig" > > +source "drivers/clk/hisilicon/Kconfig" > + Please move this above qcom to maintain alphabet sort. > endmenu > > source "drivers/clk/bcm/Kconfig" > diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig > new file mode 100644 > index 0000000..8034739 > --- /dev/null > +++ b/drivers/clk/hisilicon/Kconfig > @@ -0,0 +1,6 @@ > +config COMMON_CLK_HI6220 > + bool "Hi6220 Clock Driver" > + depends on OF && ARCH_HISI > + default y Can this be depends on ARCH_HISI || COMPILE_TEST default ARCH_HISI instead? I'd like to increase build coverage. > + help > + Build the Hisilicon Hi6220 clock driver based on the common clock framework. > diff --git a/drivers/clk/hisilicon/clk-hi6220.c b/drivers/clk/hisilicon/clk-hi6220.c > new file mode 100644 > index 0000000..91b1cd7 > --- /dev/null > +++ b/drivers/clk/hisilicon/clk-hi6220.c > @@ -0,0 +1,292 @@ > +/* > + * Hisilicon Hi6220 clock driver > + * > + * Copyright (c) 2015 Hisilicon Limited. > + * > + * Author: Bintian Wang <bintian.wang@xxxxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/kernel.h> > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/slab.h> > +#include <linux/clk.h> Do we need to include linux/clk.h? I don't see any consumer usage here. > + > +#include <dt-bindings/clock/hi6220-clock.h> > + > +#include "clk.h" > + > diff --git a/drivers/clk/hisilicon/clk.c b/drivers/clk/hisilicon/clk.c > index a078e84..5d2305c 100644 > --- a/drivers/clk/hisilicon/clk.c > +++ b/drivers/clk/hisilicon/clk.c > @@ -108,4 +123,6 @@ void __init hisi_clk_register_gate(struct hisi_gate_clock *, > int, struct hisi_clock_data *); > void __init hisi_clk_register_gate_sep(struct hisi_gate_clock *, > int, struct hisi_clock_data *); > +void __init hi6220_clk_register_divider(struct hi6220_divider_clock *, > + int, struct hisi_clock_data *); __init markings on function prototypes are useless. Please remove them. > #endif /* __HISI_CLK_H */ > diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c > + > +/** > + * struct hi6220_clk_divider - divider clock for hi6220 > + * > + * @hw: handle between common and hardware-specific interfaces > + * @reg: register containing divider > + * @shift: shift to the divider bit field > + * @width: width of the divider bit field > + * @mask: mask for setting divider rate > + * @table: the div table that the divider supports > + * @lock: register lock > + */ > +struct hi6220_clk_divider { > + struct clk_hw hw; > + void __iomem *reg; > + u8 shift; > + u8 width; > + u32 mask; > + const struct clk_div_table *table; > + spinlock_t *lock; > +}; The clk-divider.c code has been made "reusable". Can you please try to use the functions that it now exposes instead of copy/pasting it and modifying it to suit your needs? A lot of this code looks the same. > + > +#define to_hi6220_clk_divider(_hw) \ [..] > + > +static struct clk_ops hi6220_clkdiv_ops = { const? > + .recalc_rate = hi6220_clkdiv_recalc_rate, > + .round_rate = hi6220_clkdiv_round_rate, > + .set_rate = hi6220_clkdiv_set_rate, > +}; > + > +struct clk *hi6220_register_clkdiv(struct device *dev, const char *name, > + const char *parent_name, unsigned long flags, void __iomem *reg, > + u8 shift, u8 width, u32 mask_bit, spinlock_t *lock) > +{ > + struct hi6220_clk_divider *div; > + struct clk *clk; > + struct clk_init_data init; > + struct clk_div_table *table; > + u32 max_div, min_div; > + int i; > + > + /* allocate the divider */ > + div = kzalloc(sizeof(struct hi6220_clk_divider), GFP_KERNEL); > + if (!div) { > + pr_err("%s: could not allocate divider clk\n", __func__); Useless error message on allocation failure. Please run checkpatch. I've sent a patch to remove these from basic clock types. > + return ERR_PTR(-ENOMEM); > + } > + > + /* Init the divider table */ > + max_div = div_mask(width) + 1; > + min_div = 1; > + > + table = kzalloc(sizeof(struct clk_div_table) * (max_div + 1), GFP_KERNEL); > + if (!table) { > + kfree(div); > + pr_err("%s: failed to alloc divider table!\n", __func__); ditto. > + return ERR_PTR(-ENOMEM); > + } > + > + for (i = 0; i < max_div; i++) { > + table[i].div = min_div + i; > + table[i].val = table[i].div - 1; > + } > + > + init.name = name; > + init.ops = &hi6220_clkdiv_ops; > + init.flags = flags | CLK_IS_BASIC; It's basic? > + init.parent_names = parent_name ? &parent_name : NULL; > + init.num_parents = parent_name ? 1 : 0; -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html