[PATCH RFC 01/12] s5p-csis: Add device tree support

[Sylwester Nawrocki]

Hello Grant,

On 12/11/2012 09:36 AM, Grant Likely wrote:
> On Mon, 10 Dec 2012 20:45:55 +0100, Sylwester Nawrocki <s.nawrocki at samsung.com> wrote:
>> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
>> (camera host interface DMA engine and image processor). This patch
>> adds support for instantiating the MIPI-CSIS devices from DT and
>> parsing all SoC and board specific properties from device tree.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
>> ---
>>  .../bindings/media/soc/samsung-mipi-csis.txt       |   82 +++++++++++
>>  drivers/media/platform/s5p-fimc/mipi-csis.c        |  155 +++++++++++++++-----
>>  drivers/media/platform/s5p-fimc/mipi-csis.h        |    1 +
>>  3 files changed, 202 insertions(+), 36 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>> new file mode 100644
>> index 0000000..f57cbdc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>> @@ -0,0 +1,82 @@
>> +Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
>> +-----------------------------------------------------
>> +
>> +Required properties:
>> +
>> +- compatible	  : "samsung,s5pv210-csis" for S5PV210 SoCs,
>> +		    "samsung,exynos4210-csis" for Exynos4210 and later SoCs;
>> +- reg		  : physical base address and size of the device memory mapped
>> +		    registers;
>> +- interrupts      : should contain MIPI CSIS interrupt; the format of the
>> +		    interrupt specifier depends on the interrupt controller;
>> +- max-data-lanes  : maximum number of data lanes supported (SoC specific);
>> +- vddio-supply    : MIPI CSIS I/O and PLL voltage supply (e.g. 1.8V);
>> +- vddcore-supply  : MIPI CSIS Core voltage supply (e.g. 1.1V).
>> +
>> +Optional properties:
>> +
>> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
>> +		    value when this property is not specified is 166 MHz;
>> +- samsung,csis,wclk : CSI-2 wrapper clock selection. If this property is present
>> +		    external clock from CMU will be used, if not bus clock will
>> +		    be selected.
>> +
>> +The device node should contain one 'port' child node with one child 'endpoint'
>> +node, as outlined in the common media bindings specification. See
>> +Documentation/devicetree/bindings/media/v4l2.txt for details. The following are
>> +properties specific to those nodes. (TODO: update the file path)
>> +
>> +port node
>> +---------
>> +
>> +- reg		  : (required) must be 2 for camera C input (CSIS0) or 3 for
>> +		    camera D input (CSIS1);
> 
> 'reg' has a very specific definition. If you're going to use a reg
> property here, then the parent nodes need to have
> #address-cells=<1>;#size-cells=<0>; properties to define the address
> specifier format.
> 
> However since you're identifying port numbers that aren't really
> addresses I would suggest simply changing this property to something
> like 'port-num'.
> 
> Otherwise the binding looks good.

Thank you for the review. Indeed I should have said about #address-cells,
#size-cells here. I thought using 'reg' was agreed during previous discussions
on the mailing lists (e.g. [1]), so I just carried on with 'reg'.
I should just have addressed the comments from Stephen and Rob, instead of
just resending same version of the documentation. I'll try to take care of
it in the next post. i.e. rename 'link' node to 'endpoint' and 'remote'
phandle to 'remote-endpoint'.

I'm not really sure what advantage 'reg' gives over a new property.
Would it still be OK to have port nodes names followed by indexes with
port-num instead of reg, e.g.

foo {
	port at 0 {
		port-num = <0>;
	};

	port at 1 {
		port-num = <1>;
	};
};
?


[1] http://www.spinics.net/lists/linux-sh/msg13383.html

--

Thanks,
Sylwester