Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753351AbbDNIyl (ORCPT ); Tue, 14 Apr 2015 04:54:41 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:6016 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751976AbbDNIy2 (ORCPT ); Tue, 14 Apr 2015 04:54:28 -0400 Message-ID: <552CD599.4010705@hisilicon.com> Date: Tue, 14 Apr 2015 16:53:45 +0800 From: YiPing Xu User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Arnd Bergmann , CC: Bintian , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH v2 4/6] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC References: <1428916660-25910-1-git-send-email-bintian.wang@huawei.com> <2254597.TWaxeZsKvK@wuerfel> <552BCB5A.8010705@huawei.com> <3183355.UnoLoN8dF8@wuerfel> In-Reply-To: <3183355.UnoLoN8dF8@wuerfel> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.142.157.68] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020204.552CD5AF.0175,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 095eff5ef19328ca22ea28ee02370e27 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3178 Lines: 84 在 2015/4/13 23:34, Arnd Bergmann 写道: > On Monday 13 April 2015 21:57:46 Bintian wrote: >> Hello Arnd, >> >> Thanks for your code review. >> >> On 2015/4/13 21:30, Arnd Bergmann wrote: >>> On Monday 13 April 2015 17:17:38 Bintian Wang wrote: >>>> +#define HI6220_CFG_CSI2PHY 8 >>>> +#define HI6220_ISP_SCLK_GATE 9 >>>> +#define HI6220_ISP_SCLK_GATE1 10 >>>> +#define HI6220_ADE_CORE_GATE 11 >>>> +#define HI6220_CODEC_VPU_GATE 12 >>>> +#define HI6220_MED_SYSPLL 13 >>>> + >>>> +/* mux clocks */ >>>> +#define HI6220_1440_1200 20 >>>> +#define HI6220_1000_1200 21 >>>> +#define HI6220_1000_1440 22 >>>> + >>>> +/* divider clocks */ >>>> +#define HI6220_CODEC_JPEG 30 >>>> +#define HI6220_ISP_SCLK_SRC 31 >>>> +#define HI6220_ISP_SCLK1 32 >>>> >>> >>> The numbers seem rather arbitrary, and you have both holes as well as duplicate >>> numbers here. I would suggest you do one of two things instead: > >> I just worry about some special clocks may be added later so keep some >> holes for them; >> >> The duplicate numbers means clocks belong to different system control >> domains. > > I don't understand. How would there be additional clocks added later? > Wouldn't that be a new chip? There are some clocks not used in linux system. e.g, some clocks are used in base band processor. So, some numbers are not defined here. > If you have separate system control domains, doesn't that mean that you > also have separate DT bindings? > >>> a) have a separate header file per clock driver and make all the >>> numbers unique and consecutive within that header >>> >>> b) use the same numbers as the hardware registers so you can put the >>> numbers directly into the dts and don't need a header to create >>> an artificial ABI between the clock driver and the boot loader. >> This header file will be used by device tree (I like using the clock >> name instead "magic number" in dts :) ) > > That's not how it works though: The dts file is the place to define > the hardware numbers, we do that for all sorts of numbers: interrupts, > gpios, register ranges etc are all defined in dts to avoid putting > magic numbers in external header files. > > There are some cases where it gets too ugly for clock controllers > that are highly irregular, but yours doesn't seem to be that kind. > > E.g. all the fixed rate clocks should just be separate device nodes, > which lets you remove the binding for that node. > >> so how about keep them in one header file and let dts just include >> one header file (not four files), but remove the holes? > > The header files constantly cause problems with merge dependencies, > and we have some other companies that keep releasing new chips > that each time require a new header file. If hisilicon plans to make > more chips like this one, please consider coming up with more > generic bindings to avoid this. > > Arnd > > . > -- 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/