Re: [PATCH v3 4/5] clk/exynos5260: add macros and documentation for exynos5260

[Tomasz Figa <tomasz.figa@gmail.com>]

Hi Rahul,

On 18.02.2014 12:56, Rahul Sharma wrote:
> Add macros which are used as Clock IDs in DT and clock file.
> It also adds the documentation for the exynos5260 clocks.
>
> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> ---
>   .../devicetree/bindings/clock/exynos5260-clock.txt |  299 ++++++++++++++++++++
>   include/dt-bindings/clk/exynos5260-clk.h           |  234 +++++++++++++++
>   2 files changed, 533 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/clock/exynos5260-clock.txt
>   create mode 100644 include/dt-bindings/clk/exynos5260-clk.h
>
> diff --git a/Documentation/devicetree/bindings/clock/exynos5260-clock.txt b/Documentation/devicetree/bindings/clock/exynos5260-clock.txt
> new file mode 100644
> index 0000000..ec180c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/exynos5260-clock.txt
> @@ -0,0 +1,299 @@
> +* Samsung Exynos5260 Clock Controller
> +
> +The Exynos5260 clock controller encalsulate all CMUs which are
> +instantiaited independently from the device-tree. As a whole,
> +these CMUs generates and supplies clocks to various controllers
> +within the Exynos5260 SoC.
> +
> +Required Properties:
> +
> +- compatible: should be one of the following.
> +  - First compatible should be one of the following
> +		"exynos5260-cmu-top"
> +		"exynos5260-cmu-peri"
> +		"exynos5260-cmu-egl"
> +		"exynos5260-cmu-kfc"
> +		"exynos5260-cmu-g2d"
> +		"exynos5260-cmu-mif"
> +		"exynos5260-cmu-mfc"
> +		"exynos5260-cmu-g3d"
> +		"exynos5260-cmu-fsys"
> +		"exynos5260-cmu-aud"
> +		"exynos5260-cmu-isp"
> +		"exynos5260-cmu-gscl"
> +		"exynos5260-cmu-disp"
> +  - Second compatible should be "samsung,exynos5260-clock".

This is no longer true, as we have decided to remove the second compatible.

> +
> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +
> +- #clock-cells: should be 1.
> +
> +The following is the list of clocks generated by the each controller. Each
> +clock is assigned a MACRO constant. These constants are defined in
> +"dt-bindings/clk/exynos5260-clk.h". DT client nodes use this MACRO to specify
> +the clock which they consume.

There is no need to list all the macros in this file, if there is a 
reference to the header where they are defined.

> +
> +-----------------------
> +  CMU_TOP clocks
> +-----------------------
> +
> +  FIN_PLL
> +  TOP_FOUT_DISP_PLL
> +  TOP_FOUT_AUD_PLL
> +  TOP_SCLK_MMC0

[snip]

> +  ISP_SCLK_SPI0_EXT
> +  ISP_SCLK_SPI1_EXT
> +  ISP_SCLK_UART_EXT
> +
> +
> +Example 1: An example of a clock controller node is listed below.
> +
> +	cmu_disp: clock-controller@0x14550000 {
> +		compatible = "exynos5260-cmu-disp", "samsung,exynos5260-clock";

Forgot to remove second compatible?

> +		reg = <0x14550000 0x10000>;
> +		#clock-cells = <1>;
> +	};
> +
> +Example 2: UART controller node that consumes the clock generated by the
> +		peri clock controller. Refer to the standard clock bindings for
> +		information about 'clocks' and 'clock-names' property.
> +
> +	serial@12C00000 {
> +		compatible = "samsung,exynos4210-uart";
> +		reg = <0x12C00000 0x100>;
> +		interrupts = <0 146 0>;
> +		clocks = <&cmu_peri PERI_PCLK_UART0>, <&cmu_peri PERI_SCLK_UART0>;
> +		clock-names = "uart", "clk_uart_baud0";
> +		status = "disabled";
> +	};
> +
> diff --git a/include/dt-bindings/clk/exynos5260-clk.h b/include/dt-bindings/clk/exynos5260-clk.h
> new file mode 100644
> index 0000000..d2fb7fa
> --- /dev/null
> +++ b/include/dt-bindings/clk/exynos5260-clk.h
> @@ -0,0 +1,234 @@
> +/*
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Provides Constants for Exynos5260 clocks.
> +*/
> +
> +#ifndef _DT_BINDINGS_CLK_EXYNOS5260_H
> +#define _DT_BINDINGS_CLK_EXYNOS5260_H
> +
> +#define	ID_NONE		0

This is not a part of DT binding and should not be defined here. 
Furthermore, the driver should simply use 0 directly.

> +
> +/*
> + * Clock names: XXXXXX_YYYYY_ZZZZZ
> + *                    |------| |----| |----|
> + *                       cmu       type      IP
> +*/
> +
> +/* list of clocks for CMU_TOP */
> +#define	FIN_PLL			1

Hmm, isn't this an external clock?

Best regards,
Tomasz