Re: [early RFC] Device Tree bindings for OMAP3 Camera Subsystem

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

On Sun, Nov 03, 2013 at 10:03:15PM +0000, Sebastian Reichel wrote:
> Hi,
> 
> This is an early RFC for omap3isp DT support. For now i just created a potential DT
> binding documentation based on the existing platform data:
> 
> Binding for the OMAP3 Camera subsystem with the image signal processor (ISP) feature.
> 
> omap3isp node
> -------------
> 
> Required properties:
> 
> - compatible	: should be "ti,omap3isp" for OMAP3;
> - reg		: physical addresses and length of the registers set;
> - clocks	: list of clock specifiers, corresponding to entries in
> 		  clock-names property;
> - clock-names	: must contain "cam_ick", "cam_mclk", "csi2_96m_fck",
> 		  "l3_ick" entries, matching entries in the clocks property;
> - interrupts	: must contain mmu interrupt;
> - ti,iommu	: phandle to isp mmu;

s/;/./ (or s/;//)

> 
> Optional properties:
> 
> - VDD_CSIPHY1-supply	: regulator for csi phy1
> - VDD_CSIPHY2-supply	: regulator for csi phy2

I'd make these lower-case. Upper case is unusual, and lower-case is
preferred.

> - ti,isp-xclk-1		: device(s) attached to ISP's first external clock
> - ti,isp-xclk-2		: device(s) attached to ISP's second external clock

If the ISP is acting as a clock controller, it should have #clock-cells,
and export clocks to the consumers. They can in turn refer to th ISP via
the standard clocks property.

> 
> device-group subnode
> --------------------
> 
> Required properties:
> - ti,isp-interface-type	: Integer describing the interface type, one of the following
>    * 0 = ISP_INTERFACE_PARALLEL
>    * 1 = ISP_INTERFACE_CSI2A_PHY2
>    * 2 = ISP_INTERFACE_CCP2B_PHY1
>    * 3 = ISP_INTERFACE_CCP2B_PHY2
>    * 4 = ISP_INTERFACE_CSI2C_PHY1

Are these PHYs always present?

Are they external to the ISP?

It's not possible for several of these to be valid simultaneously?

> - ti,isp-devices	: Array of phandles to devices connected via the interface

Which devices are these? This looks backwards to me...

> - One of the following configuration nodes (depending on ti,isp-interface-type)
>  - ti,ccp2-bus-cfg	: CCP2 bus configuration (needed for ISP_INTERFACE_CCP*)
>  - ti,parallel-bus-cfg	: PARALLEL bus configuration (needed for ISP_INTERFACE_PARALLEL)
>  - ti,csi2-bus-cfg	: CSI bus configuration (needed for ISP_INTERFACE_CSI*)
> 
> ccp2-bus-cfg subnode
> --------------------
> 
> Required properties:
> - ti,video-port-clock-divisor	: integer; used for video port output clock control
> 
> Optional properties:
> - ti,inverted-clock		: boolean; clock/strobe signal is inverted
> - ti,enable-crc			: boolean; enable crc checking

Why can't this be a run-time option?

> - ti,ccp2-mode-mipi		: boolean; port is used in MIPI-CSI1 mode (default: CCP2 mode)
> - ti,phy-layer-is-strobe	: boolean; use data/strobe physical layer (default: data/clock physical layer)
> - ti,data-lane-configuration	: integer array with position and polarity information for lane 1 and 2
> - ti,clock-lane-configuration	: integer array with position and polarity information for clock lane

In what precise format?

> 
> parallel-bus-cfg subnode
> ------------------------
> 
> Required properties:
> - ti,data-lane-shift				: integer; shift data lanes by this amount
> 
> Optional properties:
> - ti,clock-falling-edge				: boolean; sample on falling edge (default: rising edge)
> - ti,horizontal-synchronization-active-low	: boolean; default: active high
> - ti,vertical-synchronization-active-low	: boolean; default: active high
> - ti,data-polarity-ones-complement		: boolean; data polarity is one's complement
> 
> csi2-bus-cfg subnode
> --------------------
> 
> Required properties:
> - ti,video-port-clock-divisor	: integer; used for video port output clock control
> 
> Optional properties:
> - ti,data-lane-configuration	: integer array with position and polarity information for lane 1 and 2
> - ti,clock-lane-configuration	: integer array with position and polarity information for clock lane
> - ti,enable-crc			: boolean; enable crc checking

Similarly, run-time selectable?

> 
> Example for Nokia N900
> ----------------------
> 
> omap3isp: isp@480BC000 {
> 	compatible = "ti,omap3isp";
> 	reg = <
> 		/* OMAP3430+ */
> 		0x480BC000 0x070	/* base */
> 		0x480BC100 0x078	/* cbuf */
> 		0x480BC400 0x1F0 	/* cpp2 */
> 		0x480BC600 0x0A8	/* ccdc */
> 		0x480BCA00 0x048	/* hist */
> 		0x480BCC00 0x060	/* h3a  */
> 		0x480BCE00 0x0A0	/* prev */
> 		0x480BD000 0x0AC	/* resz */
> 		0x480BD200 0x0FC	/* sbl  */
> 		0x480BD400 0x070	/* mmu  */
> 	>;

The binding implied a single contiguous reg entry. These look like they
are in a contiguous register space, is it not possible to describe them
via a single large contiguous entry?

Also, please bracket individual entries in a list like so:

reg = <0x0 0x4>,
      <0x44 0x27>,
      <0x800 0x63>;

It's far easier to read arbitrary lists when they're bracketed
consistently.

> 
> 	clocks = < &cam_ick &cam_mclk &csi2_96m_fck &l3_ick >;

Similarly here.

> 	clock-names = "cam_ick", "cam_mclk", "csi2_96m_fck", "l3_ick";
> 
> 	interrupts = <24>;
> 
> 	ti,iommu = <&mmu_isp>;
> 
> 	ti,isp-xclk-1 = <
> 		&et8ek8
> 		&smiapp_dfl
> 	>;

And here (though I think this property is unnecessary).

Thanks,
Mark.