Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752151AbcJJKbL (ORCPT ); Mon, 10 Oct 2016 06:31:11 -0400 Received: from mail-lf0-f42.google.com ([209.85.215.42]:33493 "EHLO mail-lf0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751570AbcJJKbK (ORCPT ); Mon, 10 Oct 2016 06:31:10 -0400 Subject: Re: [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI To: =?UTF-8?Q?Rados=c5=82aw_Pietrzyk?= References: <1475791294-5804-1-git-send-email-user@localhost> <2d6d5c10-346e-0541-1bc0-b75ef5b40d3f@linaro.org> Cc: Michael Turquette , sboyd@codeaurora.org, Maxime Coquelin , Alexandre Torgue , linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org From: Daniel Thompson Message-ID: Date: Mon, 10 Oct 2016 11:31:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2118 Lines: 54 On 10/10/16 10:56, Radosław Pietrzyk wrote: > Hi, > all plls have the same clock parent which is after a main divider. > Currently the divider and multiplier are connected together within vco > clock and therefore there is no chance to reuse the divider and clearly > state where the conncetion "really" is. We can arrange all of them > separately but than the divider will be hidden for all of them separately. Quoting my last mail "I can see the value of naming the "/M" pre-division separately". In other words I agree with the idea of the patch. To more explicitly state my review comments... > From: Radoslaw Pietrzyk Please add a explanation of the problem and solution in the patch description. > Signed-off-by: Radoslaw Pietrzyk > --- > drivers/clk/clk-stm32f4.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c > index 02d6810..1fd3eac 100644 > --- a/drivers/clk/clk-stm32f4.c > +++ b/drivers/clk/clk-stm32f4.c > @@ -245,9 +245,10 @@ static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) > const char *pllsrc = pllcfgr & BIT(22) ? hse_clk : hsi_clk; > unsigned long pllq = (pllcfgr >> 24) & 0xf; > > - clk_register_fixed_factor(NULL, "vco", pllsrc, 0, plln, pllm); > - clk_register_fixed_factor(NULL, "pll", "vco", 0, 1, pllp); > - clk_register_fixed_factor(NULL, "pll48", "vco", 0, 1, pllq); > + clk_register_fixed_factor(NULL, "vco-div", pllsrc, 0, 1, pllm); This strikes me as a bad name for a clock that is shared by all three PLLs (the vco being an internal component of the PLL) however since the clock is not named in the datasheet we are forced to invent a name [I suspect that's why I gave up trying to name it when I wrote the driver originally ;-) ]. Perhaps "pllin-prediv"? > + clk_register_fixed_factor(NULL, "vco-mul", "vco-div", 0, plln, 1); Why rename this clock? Multiplying is a what the vco (and its control circuits) is *for*. Tagging it "-mul" is meaningless. Daniel.