Re: [PATCH v9 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Sep 27, 2013 at 11:59:07AM +0100, Arun Kumar K wrote:
> The patch adds the DT binding documentation for Samsung
> Exynos5 SoC series imaging subsystem (FIMC-IS).
> 
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  .../devicetree/bindings/media/exynos5-fimc-is.txt  |   84 ++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
> new file mode 100644
> index 0000000..0525417
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
> @@ -0,0 +1,84 @@
> +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS)
> +------------------------------------------------------
> +
> +The camera subsystem on Samsung Exynos5 SoC has some changes relative
> +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and
> +FIMC-LITE IPs but has a much improved version of FIMC-IS which can
> +handle sensor controls and camera post-processing operations. The
> +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many
> +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two
> +dedicated scalers (SCC and SCP).
> +
> +fimc-is node
> +------------
> +
> +Required properties:
> +
> +- compatible        : must be "samsung,exynos5250-fimc-is"

s/must be/should contain/

> +- reg               : physical base address and size of the memory mapped
> +                      registers
> +- interrupt-parent  : parent interrupt controller

Is this actually necessary? Is it not implicit in most cases?

> +- interrupts        : fimc-is interrupt to the parent interrupt controller

- interrupts: interrupt-specifier for the sole interrupt generated by
  the device.

> +- clocks            : list of clock specifiers, corresponding to entries in
> +                      clock-names property

- clocks: A list of phandle + clock-specifier pairs corresponding to
  entries in clock-names

> +- clock-names       : must contain "isp", "mcu_isp", "isp_div0", "isp_div1",
> +                      "isp_divmpwm", "mcu_isp_div0", "mcu_isp_div1" entries,
> +                      matching entries in the clocks property
> +- samsung,pmu       : phandle to the Power Management Unit (PMU) node
> +

It may be worth point out that #address-cells, #size-cells, and ranges
need to be present as approriate for mapping sub-nodes.

> +i2c-isp (ISP I2C bus controller) nodes
> +--------------------------------------
> +The i2c-isp node is defined as the child node of fimc-is.
> +
> +Required properties:
> +
> +- compatible	: should be "samsung,exynos4212-i2c-isp" for Exynos4212,
> +		  Exynos4412 and Exynos5250 SoCs

s/should be/should contain/

> +- reg		: physical base address and length of the registers set
> +- clocks	: must contain gate clock specifier for this controller
> +- clock-names	: must contain "i2c_isp" entry

Please reword clocks to be in terms of clock-names, as above. e.g.

- clocks: A list of phandle + clock-specifier pairs corresponding to
  entries in clock-names
- clock-names: Should contain "i2c_isp" fot the gate clock

Otherwise, I think this looks ok.

Cheers,
Mark.