From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756711AbbFPPie (ORCPT ); Tue, 16 Jun 2015 11:38:34 -0400 Received: from mail-bn1on0138.outbound.protection.outlook.com ([157.56.110.138]:46656 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756427AbbFPPiV (ORCPT ); Tue, 16 Jun 2015 11:38:21 -0400 Authentication-Results: elphel.com; dkim=none (message not signed) header.d=none; Message-ID: <558042E6.2090100@freescale.com> Date: Tue, 16 Jun 2015 08:38:14 -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> <55803E29.2040800@freescale.com> In-Reply-To: <55803E29.2040800@freescale.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [192.88.168.49] X-ClientProxiedBy: BY1PR0501CA0014.namprd05.prod.outlook.com (25.162.139.24) To BL2PR03MB148.namprd03.prod.outlook.com (10.255.230.27) X-Microsoft-Exchange-Diagnostics: 1;BL2PR03MB148;2:9cV+0bT6j2draarZ3RxlvrTkBX5V3vneU/kMmRvAnNTl+PGg3GZojwDvqKtDjKIc;2:FYN82xlMumWUIquUhcT5sdbJXScyMMABB2Haaw1tw2vrpnyQpBNidgSf/gIc3WI3K7gn2Al0gFITHO279fALm24FUmyF4/OqUEk7jlb72V74RGqVxnL3rfqlkbRa832mTjlykB5XDNFkgWcrYJqi8g==;6:oX7K8vKfW8H9s3+Ce+gnzcyaGT0fLn5cUbugZMj4M6PXlBceNuSxln4JQHLK0zA5DIQojpnRTOCi0bcyoIXfkckx/KO6APT90UyGKloW6W0PD9j6aO4Lfd8fOH0VjQIl/qcq1EgX/FxpGnSg4D8QPxR9Is0FpWJHZa6YrKNqL7LYPuM6KYOkQD8AE6xr7IMrlem7wMej47AsSw5b8YuMjhaBHOq7/TBdDHcnIVCdT+54lL7DM4u0YFXS5t/89h7OPCqvFVzKOmtmFPCD7pwpxZ71NMzMc1b+8AEkh1mQojJYSni1OExVhreKyIf2av7D380hfIip9wDrLLJnvh/PW+KkmDKVQSkfIGZl0Pxq3aSq4p7ef6zBLVPaF2W5XB8DjeWrEJ20aBQOOsuW6ywkTUc+G8qAk+gLhA6VTBMm3qdWfi85hqCiDnYDWz/pwzoLfnNE3fwnx4UPeuhdXgk+UVZLJ9SyvWW3qKdfcHq9QYmFv7HS1hq+znPN2RcOunUYdOa/lAi57i1tH04sQ4hjK5//TlQammT4DGAhO4QMfmFuF6WPMv2Xa5ETKhmeKpnmKFMihTZ2gn9yPONJOpINQOMDiznVtbD6bjyaxe2DAMc= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BL2PR03MB148; 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:BL2PR03MB148;BCL:0;PCL:0;RULEID:;SRVR:BL2PR03MB148; X-Microsoft-Exchange-Diagnostics: 1;BL2PR03MB148;3:5lvcJZnfUhTYnxrj3yc3LaYzMqr/IyHKThaSWqgZenG5bqXhTgT0iM825C4SAok7bbL2Xzc5ZaYned9rpwxbb69TXgbg34JVdoLa2/6O3A6zP8K7iO7Zy1eOkesVeIDaXFoEy6Qv29TOlvfsddX1+vEMOKM9V9tKpjkpvwRbcKtENVO9UnMy6I6oEfwPl5kzMAuIdokfjFqWd/S99Z5viQb/XcL6/kY9qFcSUelnSSNQMhCV/mLpVECraaEqcvQqKxFapMIWv7WOPD+UGr+e5eJiebBDKf+drZdPN4c/g+/Uns+VluPRYMwGzCSNDYo4 X-Forefront-PRVS: 06098A2863 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(6009001)(6049001)(24454002)(479174004)(377454003)(377424004)(51704005)(52604005)(87266999)(54356999)(50986999)(76176999)(65816999)(33656002)(42186005)(19580405001)(19580395003)(87976001)(83506001)(86362001)(64126003)(92566002)(80316001)(122386002)(62966003)(77156002)(40100003)(65806001)(66066001)(47776003)(36756003)(5001920100001)(4001350100001)(99136001)(50466002)(46102003)(5001960100002)(110136002)(189998001)(77096005)(2950100001)(23676002);DIR:OUT;SFP:1102;SCL:1;SRVR:BL2PR03MB148;H:[10.214.81.216];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtCTDJQUjAzTUIxNDg7OTp4K3p2Ky83TGZ0TkJ3VU02eTVubzQ4dVJrUUIv?= =?utf-8?B?OE8yWC95dzJ4aHlDTUx4TzRyRlU0clBGTjVyRVhmZ3ZtaXJKRitnUHVxdzlX?= =?utf-8?B?cnhTQ3ZOMTI5OVd5azBEb3RlL1BYQXdKYUFtTWVGbkpzUG5SbDloTTRmeTd1?= =?utf-8?B?dzhVVXlraWd1WVovT094anQ1eU5LYjBnSFFwcXRUKzFwNnhEUnUraS9NaGRx?= =?utf-8?B?bDJxQUpLNHpRRm9CUnFEcnJnanJPUXF6c3JMOGxDQmNvWXJpell3ZitiVWdj?= =?utf-8?B?TWNLVXpPMldpakVkcTh0c0Mwb2lreGNRUDhXMFkvb1BGUDNaTzg5alMwM21G?= =?utf-8?B?Z2Q1VDBxeERuTmt6cWJ3Umk5TTl5L213YUN2enV4ZEEwYkZDT25ielFBL08v?= =?utf-8?B?VzBHa1VCR01vM1llUzFOOUh4bDZ4bWZTRVY3VmFQY2Y5NkUvK1pQOWxwNGZi?= =?utf-8?B?M0JDN2FERjlkK3lLVkNsMFgzcWl0VHExUmVjZXgxbnRkNlN5Rm04RWpaSUZ6?= =?utf-8?B?WUNDbElWaEtFS05QcGRwYzRxVWYvQnNPb2RXRzlTQjRtZmNQUGh2THRqcTRz?= =?utf-8?B?cXhkYkoyZFY5NEJuZXdVM24yZnBiaFlINis5dVU1UE9hZk8xL3Bmc2R4Nm1p?= =?utf-8?B?UFdSWk5wZ1FCN0Z4MS9rYTdDam5mb2twZHBUZzBpYTF6dlFOa0RRTDNSQVgy?= =?utf-8?B?N05JODRmSmU3Y05HK2JzczRjZm9BczZrbDFad2JnU0p6ZGxMTDFoMi95b0hX?= =?utf-8?B?NWwwVkhDMk9MUmR1eE5OaVhvM0RVK0w2QlBGYU12c3BJbWNhdGNkQW96Tyt1?= =?utf-8?B?VjYwWXNobWNKMnJmMjBEbm5YV1NrNVpmNnU3QStPVFJwMkdROGRlT0t3VSty?= =?utf-8?B?OXBEYWhMNFVLZ3hKRm5kZEZPdElyQTRrdVh0SHBWR1kzSk1xZFJYWmtNazl3?= =?utf-8?B?blN6a0h2OEtHaHVBblo1bDk0RndRaVZMa2dGTVE1SDE1RGhDQ2twSnFIeGxU?= =?utf-8?B?ZElQeE91VzgrbmNWV0xYY1Vlb216ckVwRTBTbHNoVE9od0xmVDBLVjNjZFpO?= =?utf-8?B?Q3l3bVgvWExrYlg5dFh2d0k0NEk4bndScFlKdVlCdU1HUXFwZmtyQWxqNDdz?= =?utf-8?B?Tk5CeS9wODNoVWlQN2gwcWlQMFNNc3Y0Wk5OdXNna09oWklkbDM4Qks3UXFl?= =?utf-8?B?MURzanFBd0RSY3JiY2M3SUxGSGVqKyt4OHZmK0l1SEhmRkNtZmdyVE5uME1a?= =?utf-8?B?MHlhSkFWQ1RYaE1BM2JxbXp1Zks5VytEdUZNT1RjRzVySGhSMm5uRk9acFJJ?= =?utf-8?B?emJhZ3oyVjhHZ0VhSW1MZk1PNzZSMFl5dGlHNllIS09KQUYwTGxSbkNjSjRY?= =?utf-8?B?MERJM3lETmR6WkorTC9FSHJIQ2F3OUwyeU9uMEt6MExhNXB2bXR0TUxja1ZI?= =?utf-8?B?RWplb1VzYnRndmkrdzBVdU1vSUFIZ2dyYThFbVRsckIwZzUrQjlrNEp6TjMr?= =?utf-8?B?V3c9PQ==?= X-Microsoft-Exchange-Diagnostics: 1;BL2PR03MB148;3:ONJmxSBh8J3pFLZlR4gyCpEck2dvOIL2s+unG3X7emPseJz/ZJsOk5tDg7VxeCIx7sBaW+zYqrZMqjzYNFDQJwTGh3L3n6+eCaDipwpvXgHyd/kjjHUzxuo+tweZ01AUgbK0SI443qoIaDqf5ClohA==;10:moLPrqexch8itb9QeOS8j096mJYMXXVbpUFrbza53y6hPeS5xmguQPgQPYczMricH5pchq3XTwOAs2XK+d0nQQRyC9zo746JisDjTmc59SM=;6:kv9b1ZefqHHWn/tcx1wb2J+2aHu9ZyiG2/RdSefeIL3gbC7ReLHctpg+S6oObKTP X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jun 2015 15:38:17.6685 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL2PR03MB148 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/16/2015 08:18 AM, York Sun wrote: > 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. > Now I remember why I did this. COMMON_CLK wasn't an option users can select, because it is a bool and only selected by some platforms. I think it should be a tristate so one can build a driver with it. When it is selected, some drivers are built, either into kernel or as modules, up to user's choice. They are needed by common clock framework. I should add explanation in commit message. Or separate it into an individual patch. Which one is preferred? 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:38:14 -0700 Message-ID: <558042E6.2090100@freescale.com> References: <1434388051-23814-1-git-send-email-yorksun@freescale.com> <1434442897.2069.66.camel@x220> <55803E29.2040800@freescale.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55803E29.2040800-KZfg59tc24xl57MIdRCFDg@public.gmane.org> 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 On 06/16/2015 08:18 AM, York Sun wrote: > 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. > Now I remember why I did this. COMMON_CLK wasn't an option users can select, because it is a bool and only selected by some platforms. I think it should be a tristate so one can build a driver with it. When it is selected, some drivers are built, either into kernel or as modules, up to user's choice. They are needed by common clock framework. I should add explanation in commit message. Or separate it into an individual patch. Which one is preferred? York