Re: [PATCH v3 2/5] clk: samsung: Add dt bindings for Exynos4412 ISP clock controller

[Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On Thu, Oct 12, 2017 at 09:08:32AM +0200, Krzysztof Kozlowski wrote:
> On Thu, Oct 12, 2017 at 8:47 AM, Marek Szyprowski
> <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> > Hi Krzysztof,
> >
> >
> > On 2017-10-11 19:05, Krzysztof Kozlowski wrote:
> >>
> >> On Wed, Oct 11, 2017 at 11:25:12AM +0200, Marek Szyprowski wrote:
> >>>
> >>> Some registers for the Exynos 4412 ISP (Camera subsystem) clocks are
> >>> located in the ISP power domain. Because those registers are also
> >>> located in a different memory region than the main clock controller,
> >>> support for them can be provided by a separate clock controller.
> >>>
> >>> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >>> ---
> >>>   .../devicetree/bindings/clock/exynos4-clock.txt    | 43
> >>> ++++++++++++++++++++++
> >>>   include/dt-bindings/clock/exynos4.h                | 35
> >>> ++++++++++++++++++
> >>>   2 files changed, 78 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/clock/exynos4-clock.txt
> >>> b/Documentation/devicetree/bindings/clock/exynos4-clock.txt
> >>> index f5a5b19ed3b2..bc61c952cb0b 100644
> >>> --- a/Documentation/devicetree/bindings/clock/exynos4-clock.txt
> >>> +++ b/Documentation/devicetree/bindings/clock/exynos4-clock.txt
> >>> @@ -41,3 +41,46 @@ Example 2: UART controller node that consumes the
> >>> clock generated by the clock
> >>>                 clocks = <&clock CLK_UART2>, <&clock CLK_SCLK_UART2>;
> >>>                 clock-names = "uart", "clk_uart_baud0";
> >>>         };
> >>> +
> >>> +Exynos4412 SoC contains some additional clocks for FIMC-ISP (Camera ISP)
> >>> +subsystem. Registers for those clocks are located in the ISP power
> >>> domain.
> >>> +Because those registers are also located in a different memory region
> >>> than
> >>> +the main clock controller, a separate clock controller has to be defined
> >>> for
> >>> +handling them.
> >>> +
> >>> +Required Properties:
> >>> +
> >>> +- compatible: should be "samsung,exynos4412-isp-clock".
> >>> +
> >>> +- reg: physical base address of the ISP clock controller and length of
> >>> memory
> >>> +  mapped region.
> >>> +
> >>> +- #clock-cells: should be 1.
> >>> +
> >>> +- clocks: list of the clock controller input clock identifiers,
> >>> +  from common clock bindings, should point to CLK_ACLK200 and
> >>> +  CLK_ACLK400_MCUISP clocks from the main clock controller.
> >>> +
> >>> +- clock-names: list of the clock controller input clock names,
> >>> +  as described in clock-bindings.txt, should be "aclk200" and
> >>> +  "aclk400_mcuisp".
> >>> +
> >>> +- power-domains: a phandle to ISP power domain node as described by
> >>> +  generic PM domain bindings.
> >>> +
> >>> +Example 3: The clock controllers bindings for Exynos4412 SoCs.
> >>> +
> >>> +       clock: clock-controller@10030000 {
> >>> +               compatible = "samsung,exynos4412-clock";
> >>> +               reg = <0x10030000 0x18000>;
> >>> +               #clock-cells = <1>;
> >>> +       };
> >>> +
> >>> +       isp_clock: clock-controller@10048000 {
> >>> +               compatible = "samsung,exynos4412-isp-clock";
> >>> +               reg = <0x10048000 0x1000>;
> >>> +               #clock-cells = <1>;
> >>> +               power-domains = <&pd_isp>;
> >>> +               clocks = <&clock CLK_ACLK200>, <&clock
> >>> CLK_ACLK400_MCUISP>;
> >>> +               clock-names = "aclk200", "aclk400_mcuisp";
> >>> +       };
> >>> diff --git a/include/dt-bindings/clock/exynos4.h
> >>> b/include/dt-bindings/clock/exynos4.h
> >>> index c40111f36d5e..e9f9d400c322 100644
> >>> --- a/include/dt-bindings/clock/exynos4.h
> >>> +++ b/include/dt-bindings/clock/exynos4.h
> >>> @@ -272,4 +272,39 @@
> >>>   /* must be greater than maximal clock id */
> >>>   #define CLK_NR_CLKS           461
> >>>   +/* Exynos4x12 ISP clocks */
> >>
> >> Shouldn't the clock IDs go with driver changes? I think only the
> >> Documentation/ part should be separate.
> >
> >
> > Well, Rob asked to move dt bindings and dt include to the separate patch.
> >
> > I see no value-added by such split, as both dt and driver patches will be
> > processed together anyway, but I didn't want to have this patchset blocked
> > by this issue.

First, I mainly review the bindings and not the drivers and splitting 
them more accurately captures what was reviewed. Second, for the DT only 
tree generated from the kernel using git-filter-branch[1], the commit 
history will be more logical if it is keeping and dropping entire 
commits rather than 1/2 a commit.

> I understand... actually I think it does not matter where the clock
> IDs would go. From my perspective both approaches look fine so in any
> case:
> Acked-by: Krzysztof Kozlowski <krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

The IDs are part of the ABI which is defined by the binding doc.

Rob

[1] https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html