From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752160AbcA0DbP (ORCPT <rfc822;w@1wt.eu>);
	Tue, 26 Jan 2016 22:31:15 -0500
Received: from szxga01-in.huawei.com ([58.251.152.64]:43885 "EHLO
	szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751266AbcA0DbL (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 26 Jan 2016 22:31:11 -0500
Subject: Re: [PATCH v7 1/6] clk: hisilicon: add CRG driver for hi3519 soc
To: Paul Bolle <pebolle@tiscali.nl>
References: <1453690883-31220-1-git-send-email-xuejiancheng@huawei.com>
 <1453690883-31220-2-git-send-email-xuejiancheng@huawei.com>
 <1453771063.17181.56.camel@tiscali.nl>
CC: <mturquette@baylibre.com>, <sboyd@codeaurora.org>,
        <p.zabel@pengutronix.de>, <robh+dt@kernel.org>, <pawel.moll@arm.com>,
        <mark.rutland@arm.com>, <ijc+devicetree@hellion.org.uk>,
        <galak@codeaurora.org>, <linux@arm.linux.org.uk>, <khilman@linaro.org>,
        <arnd@arndb.de>, <olof@lixom.net>, <xuwei5@hisilicon.com>,
        <haojian.zhuang@linaro.org>, <zhangfei.gao@linaro.org>,
        <bintian.wang@huawei.com>, <linux-kernel@vger.kernel.org>,
        <linux-clk@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <yanhaifeng@hisilicon.com>,
        <yanghongwei@hisilicon.com>, <suwenping@hisilicon.com>,
        <raojun@hisilicon.com>, <ml.yang@hisilicon.com>,
        <gaofei@hisilicon.com>, <zhangzhenxing@hisilicon.com>,
        <xuejiancheng@hisilicon.com>, <lidongpo@hisilicon.com>
From: xuejiancheng <xuejiancheng@huawei.com>
Message-ID: <56A8394E.2040207@huawei.com>
Date: Wed, 27 Jan 2016 11:28:14 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <1453771063.17181.56.camel@tiscali.nl>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
X-Originating-IP: [10.67.217.211]
X-CFilter-Loop: Reflected
X-Mirapoint-Virus-RAPID-Raw: score=unknown(0),
	refid=str=0001.0A020204.56A83993.00E9,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0,
	ip=0.0.0.0,
	so=2013-06-18 04:22:30,
	dmn=2013-03-21 17:37:32
X-Mirapoint-Loop-Id: d94e9fd27d69bddb1766d402e898f4d7
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Paul Bolle，
   Thank you for your reply.

On 2016/1/26 9:17, Paul Bolle wrote:
> On ma, 2016-01-25 at 11:01 +0800, Jiancheng Xue wrote:
>> --- a/drivers/clk/hisilicon/Kconfig
>> +++ b/drivers/clk/hisilicon/Kconfig
> 
>> +config COMMON_CLK_HI3519
>> +	bool "Hi3519 Clock Driver"
>> +	depends on ARCH_HISI
>> +	default y
>> +	help
>> +	  Build the clock driver for hi3519.
> 
>> --- a/drivers/clk/hisilicon/Makefile
>> +++ b/drivers/clk/hisilicon/Makefile
> 
>> +obj-$(CONFIG_COMMON_CLK_HI3519)	+= clk-hi3519.o
> 
> If I parsed the above correctly clk-hi3519.o can only be built-in,
> right?
> 
Yes. You are right.

But this clock driver should be able to be compiled as a module.
Even though it is preferred to be built-in.

I'll fix it in next version. Thank you.

Regards,

Jiancheng

>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/clk-hi3519.c
> 
>> +#include <linux/module.h>
> 
> So is this include actually needed?
> 
>> +static int hi3519_clk_probe(struct platform_device *pdev)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +       struct hisi_clock_data *clk_data;
>> +
>> +       clk_data = hisi_clk_init(np, HI3519_NR_CLKS);
>> +       if (!clk_data)
>> +               return -ENODEV;
>> +
>> +       hisi_clk_register_fixed_rate(hi3519_fixed_rate_clks,
>> +                                   
>> ARRAY_SIZE(hi3519_fixed_rate_clks),
>> +                                    clk_data);
>> +       hisi_clk_register_mux(hi3519_mux_clks,
>> ARRAY_SIZE(hi3519_mux_clks),
>> +                                       clk_data);
>> +       hisi_clk_register_gate(hi3519_gate_clks,
>> +                       ARRAY_SIZE(hi3519_gate_clks), clk_data);
>> +
>> +       return hisi_reset_init(np);
>> +}
> 
> (evolution 3.16.5 makes replying to code quite a challenge.)
> 
>> +static const struct of_device_id hi3519_clk_match_table[] = {
>> +	{ .compatible = "hisilicon,hi3519-crg" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, hi3519_clk_match_table);
> 
> Last time I checked MODULE_DEVICE_TABLE is preprocessed away for built
> -in code.
> 
>> +static void __exit hi3519_clk_exit(void)
>> +{
>> +	platform_driver_unregister(&hi3519_clk_driver);
>> +}
>> +module_exit(hi3519_clk_exit);
> 
> Not needed for built-in only code.
> 
>> +MODULE_DESCRIPTION("HiSilicon Hi3519 Clock Driver");
> 
> Ditto.
> 
>> --- a/drivers/clk/hisilicon/clk.c
>> +++ b/drivers/clk/hisilicon/clk.c
> 
>> +EXPORT_SYMBOL(hisi_clk_init);
> 
> What module uses this export?
>  
>> +EXPORT_SYMBOL(hisi_clk_register_fixed_rate);
> 
> Ditto.
> 
>> +EXPORT_SYMBOL(hisi_clk_register_fixed_factor);
> 
> Ditto.
>  
>> +EXPORT_SYMBOL(hisi_clk_register_mux);
> 
> Ditto.
> 
>> +EXPORT_SYMBOL(hisi_clk_register_divider);
> 
> Ditto.
> 
>> +EXPORT_SYMBOL(hisi_clk_register_gate);
> 
> Ditto.
> 
>> +EXPORT_SYMBOL(hisi_clk_register_gate_sep);
> 
> Ditto.
> 
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/reset.c
> 
>> +int hisi_reset_init(struct device_node *np)
>> +{
>> +	[...]
>> +}
>> +EXPORT_SYMBOL(hisi_reset_init);
> 
> Ditto.
> 
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/reset.h
> 
>> +#ifdef CONFIG_RESET_CONTROLLER
>> +int hisi_reset_init(struct device_node *np);
>> +#else
>> +static inline int hisi_reset_init(struct device_node *np)
>> +{
>> +	return 0;
>> +}
>> +#endif
> 
> Thanks,
> 
> 
> Paul Bolle
> 
> .
>