Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933746AbbEOAZd (ORCPT ); Thu, 14 May 2015 20:25:33 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:50891 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933551AbbEOAZa (ORCPT ); Thu, 14 May 2015 20:25:30 -0400 Date: Thu, 14 May 2015 17:25:27 -0700 From: Stephen Boyd To: Bintian Wang Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, will.deacon@arm.com, devicetree@vger.kernel.org, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, khilman@linaro.org, mturquette@linaro.org, rob.herring@linaro.org, zhangfei.gao@linaro.org, haojian.zhuang@linaro.org, xuwei5@hisilicon.com, jh80.chung@samsung.com, olof@lixom.net, yanhaifeng@gmail.com, xuejiancheng@huawei.com, sledge.yanwei@huawei.com, tomeu.vizoso@collabora.com, linux@arm.linux.org.uk, guodong.xu@linaro.org, jorge.ramirez-ortiz@linaro.org, tyler.baker@linaro.org, khilman@kernel.org, pebolle@tiscali.nl, arnd@arndb.de, marc.zyngier@arm.com, xuyiping@hisilicon.com, wangbinghui@hisilicon.com, zhenwei.wang@hisilicon.com, victor.lixin@hisilicon.com, puck.chen@hisilicon.com, dan.zhao@hisilicon.com, huxinwei@huawei.com, z.liuxinliang@huawei.com, heyunlei@huawei.com, kong.kongxinwei@hisilicon.com, btw@mail.itp.ac.cn, w.f@huawei.com, liguozhu@hisilicon.com Subject: Re: [PATCH v4 4/5] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC Message-ID: <20150515002527.GD31753@codeaurora.org> References: <1430827599-11560-1-git-send-email-bintian.wang@huawei.com> <1430827599-11560-5-git-send-email-bintian.wang@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1430827599-11560-5-git-send-email-bintian.wang@huawei.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5418 Lines: 187 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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Do we need to include linux/clk.h? I don't see any consumer usage here. > + > +#include > + > +#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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/