From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751950Ab3KJRll (ORCPT ); Sun, 10 Nov 2013 12:41:41 -0500 Received: from mail-ee0-f54.google.com ([74.125.83.54]:54789 "EHLO mail-ee0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751416Ab3KJRlb (ORCPT ); Sun, 10 Nov 2013 12:41:31 -0500 From: Tomasz Figa To: Vyacheslav Tyrtov Cc: linux-kernel@vger.kernel.org, Rob Herring , Pawel Moll , Mark Rutland , Stephen Warren , Ian Campbell , Rob Landley , Kukjin Kim , Russell King , Ben Dooks , Mike Turquette , Daniel Lezcano , Thomas Gleixner , Heiko Stuebner , Naour Romain , devicetree@vger.kernel.org, linux-doc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, Tarek Dakhran , Dave.Martin@arm.com, nicolas.pitre@linaro.org Subject: Re: [PATCH v3 2/4] clk: exynos5410: register clocks using common clock framework Date: Sun, 10 Nov 2013 18:41:28 +0100 Message-ID: <6379094.zWymlDc09O@flatron> User-Agent: KMail/4.11.3 (Linux/3.12.0-gentoo; KDE/4.11.3; x86_64; ; ) In-Reply-To: <1383811969-32712-3-git-send-email-v.tyrtov@samsung.com> References: <1383811969-32712-1-git-send-email-v.tyrtov@samsung.com> <1383811969-32712-3-git-send-email-v.tyrtov@samsung.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Thursday 07 of November 2013 12:12:47 Vyacheslav Tyrtov wrote: > From: Tarek Dakhran > > The EXYNOS5410 clocks are statically listed and registered > using the Samsung specific common clock helper functions. Thanks for keeping up with addressing the comments. However there are still few things that need to be corrected. Please see my comments inline. > Signed-off-by: Tarek Dakhran > Signed-off-by: Vyacheslav Tyrtov > --- > .../devicetree/bindings/clock/exynos5410-clock.txt | 37 ++++ > drivers/clk/samsung/Makefile | 1 + > drivers/clk/samsung/clk-exynos5410.c | 239 +++++++++++++++++++++ > include/dt-bindings/clock/exynos5410.h | 175 +++++++++++++++ > 4 files changed, 452 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/exynos5410-clock.txt > create mode 100644 drivers/clk/samsung/clk-exynos5410.c > create mode 100644 include/dt-bindings/clock/exynos5410.h > > diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt > new file mode 100644 > index 0000000..a462da231 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt > @@ -0,0 +1,37 @@ > +* Samsung Exynos5410 Clock Controller > + > +The Exynos5410 clock controller generates and supplies clock to various > +controllers within the Exynos5410 SoC. > + > +Required Properties: > + > +- compatible: should be "samsung,exynos5410-clock" > + > +- reg: physical base address of the controller and length of memory mapped > + region. > + > +- #clock-cells: should be 1. If there are any external clocks that need to be provided (and I believe there are), you should mention them in the documentation. Also, to make this more future-proof, I would add clock-names and clocks properties to the binding, listing all those external clocks. It's just about the binding definition - you don't have to implement this in the driver yet, as we don't have the framework to handle this at early system initialization. However, when we finally implement this in the Common Clock Framework, we will not have to change existing DT bindings. > + > +All available clocks are defined as preprocessor macros in > +dt-bindings/clock/exynos5410.h header and can be used in device > +tree sources. > + > +Example 1: An example of a clock controller node is listed below. > + > + clock: clock-controller@0x10010000 { > + compatible = "samsung,exynos5410-clock"; > + reg = <0x10010000 0x30000>; > + #clock-cells = <1>; > + }; > + > +Example 2: UART controller node that consumes the clock generated by the clock > + controller. Refer to the standard clock bindings for information > + about 'clocks' and 'clock-names' property. > + > + serial@12C20000 { > + compatible = "samsung,exynos4210-uart"; > + reg = <0x12C00000 0x100>; > + interrupts = <0 51 0>; > + clocks = <&clock CLK_UART0>, <&clock CLK_SCLK_UART0>; > + clock-names = "uart", "clk_uart_baud0"; > + }; [snip] > diff --git a/include/dt-bindings/clock/exynos5410.h b/include/dt-bindings/clock/exynos5410.h > new file mode 100644 > index 0000000..9b4a58b > --- /dev/null > +++ b/include/dt-bindings/clock/exynos5410.h > @@ -0,0 +1,175 @@ > +#ifndef _DT_BINDINGS_CLOCK_EXYNOS_5410_H > +#define _DT_BINDINGS_CLOCK_EXYNOS_5410_H > + > +/* core clocks */ > +#define CLK_FIN_PLL 1 This is external clock, not provided by this clock controller, isn't it? > +#define CLK_FOUT_APLL 2 > +#define CLK_FOUT_CPLL 3 > +#define CLK_FOUT_DPLL 4 > +#define CLK_FOUT_EPLL 5 [snip] > +#define CLK_ACLK_G3D 500 > +#define CLK_G3D 501 > +#define CLK_SMMU_MIXER 502 > + > +/* mux clocks */ > +#define CLK_MOUT_HDMI 640 This definition does not seem to be used anywhere in the driver itself. > + > +/* divider clocks */ > +#define CLK_DOUT_PIXEL 768 Ditto. Please don't define IDs for clocks that are not yet provided by the driver. Best regards, Tomasz