From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756536AbbFPPSO (ORCPT ); Tue, 16 Jun 2015 11:18:14 -0400 Received: from mail-bn1bn0108.outbound.protection.outlook.com ([157.56.110.108]:23872 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753659AbbFPPSK (ORCPT ); Tue, 16 Jun 2015 11:18:10 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=yorksun@freescale.com; Message-ID: <55803E29.2040800@freescale.com> Date: Tue, 16 Jun 2015 08:18:01 -0700 From: York Sun User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Paul Bolle CC: , , , Sebastian Hesselbarth , Guenter Roeck , Andrey Filippov Subject: Re: [Patch v2] driver/clk/clk-si5338: Add common clock framework driver for si5338 References: <1434388051-23814-1-git-send-email-yorksun@freescale.com> <1434442897.2069.66.camel@x220> In-Reply-To: <1434442897.2069.66.camel@x220> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [192.88.168.49] X-ClientProxiedBy: BY1PR0501CA0031.namprd05.prod.outlook.com (25.162.139.41) To BL2PR03MB147.namprd03.prod.outlook.com (10.255.230.19) X-Microsoft-Exchange-Diagnostics: 1;BL2PR03MB147;2:gwMz8jVxFEjBrX6emdm2OY89yBiWldSFCzHqwHWh+dR+Z9+AIbwGpGxnSpK8F5gl;2:s+ckhtCrWeZCivSku5JPJYf47mkRLVurtXYnVc9ExgV52oi9mlYKy9XyyNrpiqDVdpBr0J3Mgoc7MEGJ28AIjz8qieUelSqEik32Ho52a+TIH/5dwIFjvFnLG9X/Nl/llojXFNE0sa3qQZTOo4gQhw==;6:By+dCSNSvsNV/nnDyma4YLBJJNINEWF1J8ANn0qeWafqcuIp5Vtd06LXUj7wOUguxS8wwe3HvX1hXBlcZXRqtptMW3UzpQyYcv6Sx5nag1saqfUF1mW8QCb0LkvllBo5dSVgndTTDXs3A9jNFilCBrf+JvEJZp7AlUuo7hfvdxc32jBQ4R57TFEaKzbknf7btM/uVg2pce37Eo3jwNXL+sTp+x6jwg1wTbeKDQzYU1r9jkgiLYJhVJJyzY+9FmsIdES5PY+s/tjJNNI02dfKpmFFd0QMe8pnq++xWQxji9TSLKE6JHE0FHvgk6Zu7iAPrshgr226Jn4xobiSkET3Lsq0auZRr5Dxl9o2ugnosixpfDlwUSsZ6SqJM6WJztR58Lw7Rq6ONlKsKjfDryAmyHPcKbdTr0qnXhUsWTueYZP0TKWv7zHyf2awintpe6Q4SngGs3tnoYZReuv5/df0pdhge+A0OzOCq/Tx20hr36QKIVtahclFJKqYLnHkFuFQqwgdEIemO7V/ojcUQSBDMUd8fgZAO4hVo2w+h7dti+KbCpV+AQKtg1nYHWcAL3rnZPpzBEsjPXj6PuaINm+mqcUyvWubWyIty686Oks8aK0= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BL2PR03MB147; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(520003)(5005006)(3002001);SRVR:BL2PR03MB147;BCL:0;PCL:0;RULEID:;SRVR:BL2PR03MB147; X-Microsoft-Exchange-Diagnostics: 1;BL2PR03MB147;3:idMB9MQya7PKqkL0MRUKBIRca+TEnpp7cnX5UGi2P3nqxFJuJVgDARV3nIn6bO0c1yNI3v9mXuDDL4fzObW2msMJKu5i6rT52CPQVixcz3BOYnRsp3DAtuYbLGeUaoxOBI7TCeUKfzwp2QLFWwP2go16Jko1TBlJ7NGLEpebG9vzm5tfvg+XPZCsKtWBfrM8Jng75aB3Eabn9hcbTJdsghuBdAvV7n1SX9s9wtIH7I5IHKd0MlKZM1K0KdG6AOjGGUi8wJFAqnOxVb/+l1xgczpiaEGK3VP2nOjQkADQ3ZqyN1lk6e/rn0kdaWMBvC+T X-Forefront-PRVS: 06098A2863 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(979002)(6009001)(6049001)(377424004)(24454002)(51704005)(52604005)(377454003)(479174004)(77096005)(23676002)(46102003)(189998001)(5001960100002)(110136002)(42186005)(2950100001)(4001350100001)(50466002)(65806001)(47776003)(66066001)(65956001)(92566002)(64126003)(80316001)(19580405001)(19580395003)(59896002)(33656002)(50986999)(65816999)(87266999)(54356999)(76176999)(87976001)(86362001)(77156002)(83506001)(40100003)(99136001)(62966003)(122386002)(36756003)(969003)(989001)(999001)(1009001)(1019001);DIR:OUT;SFP:1102;SCL:1;SRVR:BL2PR03MB147;H:[10.214.81.216];FPR:;SPF:None;MLV:ovrnspm;PTR:InfoNoRecords;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtCTDJQUjAzTUIxNDc7OTpLT0xsUzNBV2I2QnpaVUMyWmlQdkcxMHBucU9Q?= =?utf-8?B?SDZ1ei81ZkhtS2Z0cDg0eVl2MU1yakNIZkVpdnhLOEc0M0NhTTdPSkc5SjVH?= =?utf-8?B?WEQvUU45ektkVVRDNUU2VTE3WmVQWGFDdzJpcmwvNUxtcGxpT2dpVmhaeDl4?= =?utf-8?B?eHI5RitWOFZsc1BYT1U0MVVvWWtBRnpjeFhTUEdrZ0lSSEpNd3paTDF3bTVx?= =?utf-8?B?OGx3UFpKd0ZHVHpVNHhoUi9lNWFZeXZpZzZGUkFYWlpHYm5ZaWEraWpJdVpI?= =?utf-8?B?MzIybGhYOGdWWE9EbkhiZ0h2V0xIUFRtMG9NQzZyOFdUZ3VQMUR6Vzk0bjZB?= =?utf-8?B?cWlEbWdSVGdWNWhvd21lNFVlL25WbnhMbi9MSjh4UEg1YXNENHpMemhTYmJa?= =?utf-8?B?WVdCeFFiV04vOE52MlBFbCtFTjJ6V3VNQ2NiWElJQXY5ejNmSmQ0SHhsQXIx?= =?utf-8?B?ZnRjMll3UXptNGxOZHd0VjYxbFRBcXJpbUoxTnRnL25oTkczT1dpdUFEc3Nw?= =?utf-8?B?M3lKU3lWS1EzREI1V0hQWkVtT2dzTk9xTGtCelB3eFJQUEx3NkVRcW9GcTRV?= =?utf-8?B?UFdIWUpmZ2hlcHNRbkpUZTkxajRRbSt2Q085TW1jaG5YYjNxdVNhWi9JWmxG?= =?utf-8?B?T1lEbFhNRlp2RmU3RUdEeTRib2YyYjZ0UlF6SVZBZ25Rb1gyd1VNUVNhQkw2?= =?utf-8?B?UVVQUUMwazBwemNMdUZsVkVzMENrWC9NNG9WRWlDLzYxTDZvTXF2Y2JkZFlQ?= =?utf-8?B?cGM2MytiWHloemZBNzVLb2hyU3JZL3VlSGwwV0oxWDZnL2Ivb05MYmVVakZy?= =?utf-8?B?STFJZDF3T25ReFd3NXk4d1dQUzZ2NXRLMC8xS2l3WmdpVlFUQUJzR0lweHJC?= =?utf-8?B?c0JUNVFZcFRPUGFGNysrWXFYTFRNdGxOT1d0a3ZSbUxKTVJLK0RnUjhscCtY?= =?utf-8?B?YTIvaEVuMmJSWFlncEZoV0lvTFh5MU9sdDRTRUZHb0tHODl5dTU1N2EzRVNX?= =?utf-8?B?ZFhJd3Y3Qm5BVWVMaklWNUhsOG5zaXF6VHlBZEpGanVydlQ3ek5mcjlWWFRs?= =?utf-8?B?MjJSTkFHN3lDWittcGlicHdvYzZkZVdYRno2M0p0di9qbnViYXBaOWZCa2lO?= =?utf-8?B?SW8vendPZ3BrUm9MR0VXa25rcjFqdEJ5UENLYklVbmhEU05McEM4aW92cXV0?= =?utf-8?B?Q3RqaDNVMjRNdFYrQzRvR2I4a2V4WDRvZUpPUGp5RUViaUtwclREdVVBRjRG?= =?utf-8?B?Sno3RVJBS2p3Q2c2VWtGdTFhV2ZnSlJLRUlFQ0VkZXdKbXZvMExwWDRtN3N3?= =?utf-8?B?Zm1VUmdYSzBobVlHTmpCUkxSdkR2WGlzc0U4VU53SStVazZWQVNzZnQyY3BT?= =?utf-8?B?UGhHd1FJcHBFMkdUcjN2RmFEV2pLbWVHalNJUXBodjVCYk9KS3kvNitlRVRR?= =?utf-8?B?d0ZlZ2MwWjE1RG12dFBrNDF1ZURxNDFDYnVBREw3WEV4ZU8yU0Z1WU1FL2Ez?= =?utf-8?B?OEpzSEhRUmV4cE9wcjRwVkwwVEJNVlRYRUwxaFczbURVcVN1ODR1dTY0MVRZ?= =?utf-8?B?RXBSYUJVTkJ2UFkyRWhmbUVGYi9xVk5BMnpPUHcyWDJaaFBaWVBXZEtQZmNy?= =?utf-8?B?dmlpcWc4bHBzYk9YZnRrZyt5SHlkTUhrV00yc3h3eGErWEpQWTIxdz09?= X-Microsoft-Exchange-Diagnostics: 1;BL2PR03MB147;3:IR78oeG2zZ+9y06+NIk57+bqvZldyyRjxD0Cx3yFC6eEL0+QVJrvmqLD82xRo7iDaVbkySHNQJAo/g8RagznNcEBZE/vhbAngBLcIkI+4eH5BMTWZhg0q/3bOGyYgLf2AViRYv+vSiQu4d/jaYyyPA==;10:mXP+KdqHkKfi8+RMdk6TjrgP7474Wm/shnP/Bjt2rn1Oz6pUA7lOI8fOXQqC4CsDalsWyJpKSX2N/U4cCqx+ZbWgihovhnqfU0YXLpamcmQ=;6:rctSq/wR8usNvaP1YjuDHfKYu3Jl62AaRuQKG05VXjwmP6gsttDRiHNFC2ZhSxE5 X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jun 2015 15:18:05.6408 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL2PR03MB147 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Paul, Thanks for reviewing. On 06/16/2015 01:21 AM, Paul Bolle wrote: > One question and a few nits follow. > > On Mon, 2015-06-15 at 10:07 -0700, York Sun wrote: >> SI5338 is a programmable clock generator. It has 4 sets of inputs, >> PLL, multisynth and dividers to make 4 outputs. This driver splits >> them into multiple clocks to comply with common clock framework. >> >> See Documentation/devicetree/bindings/clock/silabs,si5338.txt for >> details. >> >> Signed-off-by: York Sun >> CC: Mike Turquette > > Apparently that's now mturquette@baylibre.com . Thanks. Will change. > >> CC: Sebastian Hesselbarth >> CC: Guenter Roeck >> CC: Andrey Filippov > >> --- a/drivers/clk/Kconfig >> +++ b/drivers/clk/Kconfig > >> config COMMON_CLK >> - bool >> + tristate "Common Clock" >> select HAVE_CLK_PREPARE >> select CLKDEV_LOOKUP >> select SRCU > > Why? The commit explanation doesn't mention this. Did you use an unclean > tree? If not, you just created over a dozen of new modules: Thanks for catching this. I was testing building the driver within and outside of kernel tree for another kernel version. If this driver is built-in, I don't need to make it tristate. Will revert in next version. > $ git grep -nw CONFIG_COMMON_CLK -- "*Makefile*" > arch/arm/mach-mmp/Makefile:13:ifeq ($(CONFIG_COMMON_CLK), ) > arch/arm/mach-shmobile/Makefile:21:ifndef CONFIG_COMMON_CLK > arch/powerpc/platforms/512x/Makefile:4:obj-$(CONFIG_COMMON_CLK) += clock-commonclk.o > drivers/clk/Makefile:4:obj-$(CONFIG_COMMON_CLK) += clk.o > drivers/clk/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk-divider.o > drivers/clk/Makefile:6:obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o > drivers/clk/Makefile:7:obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o > drivers/clk/Makefile:8:obj-$(CONFIG_COMMON_CLK) += clk-gate.o > drivers/clk/Makefile:9:obj-$(CONFIG_COMMON_CLK) += clk-mux.o > drivers/clk/Makefile:10:obj-$(CONFIG_COMMON_CLK) += clk-composite.o > drivers/clk/Makefile:11:obj-$(CONFIG_COMMON_CLK) += clk-fractional-divider.o > drivers/clk/Makefile:12:obj-$(CONFIG_COMMON_CLK) += clk-gpio-gate.o > drivers/clk/Makefile:14:obj-$(CONFIG_COMMON_CLK) += clk-conf.o > drivers/clk/Makefile:59:ifeq ($(CONFIG_COMMON_CLK), y) > drivers/clk/samsung/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o > drivers/gpu/drm/msm/Makefile:53:msm-$(CONFIG_COMMON_CLK) += mdp/mdp4/mdp4_lvds_pll.o > drivers/sh/Makefile:5:ifneq ($(CONFIG_COMMON_CLK),y) > >> +config COMMON_CLK_SI5338 >> + tristate "Clock driver for SiLabs 5338" >> + depends on I2C >> + select REGMAP_I2C >> + select RATIONAL >> + ---help--- >> + This driver supports Silicon Labs 5338 programmable clock generators, >> + using common clock framework. It needs parent clock as input(s). >> + Internal clocks are registered with unique names in case multiple >> + devices exist. See devicetree/bindings/clock/silabs,si5338.txt >> + under Documentation for details. > >> --- /dev/null >> +++ b/drivers/clk/clk-si5338.c > >> +unsigned long si5338_divrefclk_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + [...] >> +} > > Can't this be made static? It compiles cleanly with static too. Is there > some subtle issue I'm missing? > Absolutely. I must have missed them for some functions. >> +static const struct clk_ops si5338_divrefclk_ops = { >> + .recalc_rate = si5338_divrefclk_recalc_rate, >> +}; > >> +unsigned long si5338_divfbclk_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + [...] >> +} > > Ditto. > >> +static const struct clk_ops si5338_divfbclk_ops = { >> + .recalc_rate = si5338_divfbclk_recalc_rate, >> +}; > >> --- /dev/null >> +++ b/include/dt-bindings/clock/clk-si5338.h > >> +#ifndef _DT_BINDINGS_CLK_DSI5338_H >> +#define _DT_BINDINGS_CLK_DSI5338_H > > (I spotted a D that looks odd here.) Me, too. It takes fresh eyes to spot this non-sense error. > > And git am whines: > new blank line at EOF. Thanks. York From mboxrd@z Thu Jan 1 00:00:00 1970 From: York Sun Subject: Re: [Patch v2] driver/clk/clk-si5338: Add common clock framework driver for si5338 Date: Tue, 16 Jun 2015 08:18:01 -0700 Message-ID: <55803E29.2040800@freescale.com> References: <1434388051-23814-1-git-send-email-yorksun@freescale.com> <1434442897.2069.66.camel@x220> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1434442897.2069.66.camel@x220> Sender: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Paul Bolle Cc: mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Sebastian Hesselbarth , Guenter Roeck , Andrey Filippov List-Id: linux-i2c@vger.kernel.org Paul, Thanks for reviewing. On 06/16/2015 01:21 AM, Paul Bolle wrote: > One question and a few nits follow. > > On Mon, 2015-06-15 at 10:07 -0700, York Sun wrote: >> SI5338 is a programmable clock generator. It has 4 sets of inputs, >> PLL, multisynth and dividers to make 4 outputs. This driver splits >> them into multiple clocks to comply with common clock framework. >> >> See Documentation/devicetree/bindings/clock/silabs,si5338.txt for >> details. >> >> Signed-off-by: York Sun >> CC: Mike Turquette > > Apparently that's now mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org . Thanks. Will change. > >> CC: Sebastian Hesselbarth >> CC: Guenter Roeck >> CC: Andrey Filippov > >> --- a/drivers/clk/Kconfig >> +++ b/drivers/clk/Kconfig > >> config COMMON_CLK >> - bool >> + tristate "Common Clock" >> select HAVE_CLK_PREPARE >> select CLKDEV_LOOKUP >> select SRCU > > Why? The commit explanation doesn't mention this. Did you use an unclean > tree? If not, you just created over a dozen of new modules: Thanks for catching this. I was testing building the driver within and outside of kernel tree for another kernel version. If this driver is built-in, I don't need to make it tristate. Will revert in next version. > $ git grep -nw CONFIG_COMMON_CLK -- "*Makefile*" > arch/arm/mach-mmp/Makefile:13:ifeq ($(CONFIG_COMMON_CLK), ) > arch/arm/mach-shmobile/Makefile:21:ifndef CONFIG_COMMON_CLK > arch/powerpc/platforms/512x/Makefile:4:obj-$(CONFIG_COMMON_CLK) += clock-commonclk.o > drivers/clk/Makefile:4:obj-$(CONFIG_COMMON_CLK) += clk.o > drivers/clk/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk-divider.o > drivers/clk/Makefile:6:obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o > drivers/clk/Makefile:7:obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o > drivers/clk/Makefile:8:obj-$(CONFIG_COMMON_CLK) += clk-gate.o > drivers/clk/Makefile:9:obj-$(CONFIG_COMMON_CLK) += clk-mux.o > drivers/clk/Makefile:10:obj-$(CONFIG_COMMON_CLK) += clk-composite.o > drivers/clk/Makefile:11:obj-$(CONFIG_COMMON_CLK) += clk-fractional-divider.o > drivers/clk/Makefile:12:obj-$(CONFIG_COMMON_CLK) += clk-gpio-gate.o > drivers/clk/Makefile:14:obj-$(CONFIG_COMMON_CLK) += clk-conf.o > drivers/clk/Makefile:59:ifeq ($(CONFIG_COMMON_CLK), y) > drivers/clk/samsung/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o > drivers/gpu/drm/msm/Makefile:53:msm-$(CONFIG_COMMON_CLK) += mdp/mdp4/mdp4_lvds_pll.o > drivers/sh/Makefile:5:ifneq ($(CONFIG_COMMON_CLK),y) > >> +config COMMON_CLK_SI5338 >> + tristate "Clock driver for SiLabs 5338" >> + depends on I2C >> + select REGMAP_I2C >> + select RATIONAL >> + ---help--- >> + This driver supports Silicon Labs 5338 programmable clock generators, >> + using common clock framework. It needs parent clock as input(s). >> + Internal clocks are registered with unique names in case multiple >> + devices exist. See devicetree/bindings/clock/silabs,si5338.txt >> + under Documentation for details. > >> --- /dev/null >> +++ b/drivers/clk/clk-si5338.c > >> +unsigned long si5338_divrefclk_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + [...] >> +} > > Can't this be made static? It compiles cleanly with static too. Is there > some subtle issue I'm missing? > Absolutely. I must have missed them for some functions. >> +static const struct clk_ops si5338_divrefclk_ops = { >> + .recalc_rate = si5338_divrefclk_recalc_rate, >> +}; > >> +unsigned long si5338_divfbclk_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + [...] >> +} > > Ditto. > >> +static const struct clk_ops si5338_divfbclk_ops = { >> + .recalc_rate = si5338_divfbclk_recalc_rate, >> +}; > >> --- /dev/null >> +++ b/include/dt-bindings/clock/clk-si5338.h > >> +#ifndef _DT_BINDINGS_CLK_DSI5338_H >> +#define _DT_BINDINGS_CLK_DSI5338_H > > (I spotted a D that looks odd here.) Me, too. It takes fresh eyes to spot this non-sense error. > > And git am whines: > new blank line at EOF. Thanks. York