diff mbox

[RFC,14/18] dt: bindings: Add bindings for omap3isp

Message ID 1425764475-27691-15-git-send-email-sakari.ailus@iki.fi
State Needs Review / ACK, archived
Headers show

Checks

Context Check Description
robh/checkpatch warning total: 1 errors, 0 warnings, 0 lines checked
robh/patch-applied success

Commit Message

Sakari Ailus March 7, 2015, 9:41 p.m. UTC
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 .../devicetree/bindings/media/ti,omap3isp.txt      |   64 ++++++++++++++++++++
 MAINTAINERS                                        |    1 +
 2 files changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,omap3isp.txt

Comments

Laurent Pinchart March 11, 2015, 11:39 p.m. UTC | #1
Hi Sakari,

Thank you for the patch.

On Saturday 07 March 2015 23:41:11 Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  .../devicetree/bindings/media/ti,omap3isp.txt      |   64 +++++++++++++++++
>  MAINTAINERS                                        |    1 +
>  2 files changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/ti,omap3isp.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/ti,omap3isp.txt
> b/Documentation/devicetree/bindings/media/ti,omap3isp.txt new file mode
> 100644
> index 0000000..2059524
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,omap3isp.txt
> @@ -0,0 +1,64 @@
> +OMAP 3 ISP Device Tree bindings
> +===============================
> +
> +More documentation on these bindings is available in
> +video-interfaces.txt in the same directory.
> +
> +Required properties
> +===================
> +
> +compatible	: "ti,omap3-isp"

I would rephrase that using the usual wording as "compatible: Must contain 
"ti,omap3-isp".

> +reg		: a set of two register block physical addresses and
> +		  lengths

We should describe what each set represents and contains.

> +interrupts	: the interrupt number

I would keep the wording generic and refer to interrupt specifier instead of 
interrupt number.

"interrupts: the ISP interrupt specifier"

> +iommus		: phandle of the IOMMU

Similarly,

"iommus: phandle and IOMMU specifier for the IOMMU that serves the ISP"

> +syscon		: syscon phandle and register offset

We should document what the register offset is.

> +ti,phy-type	: 0 -- 3430; 1 -- 3630

Would it make sense to add #define's for this ?

It could also make sense to document/name them "Complex I/O" and "CSIPHY" to 
avoid referring to the SoC that implements them, as the ISP is also found in 
SoCs other than 3430 and 3630.

Could the PHY type be derived from the ES revision that we query at runtime ?

We should also take into account the fact that the DM3730 has officially no 
CSIPHY, but still seems to implement them in practice.

> +#clock-cells	: Must be 1 --- the ISP provides two external clocks,
> +		  cam_xclka and cam_xclkb, at indices 0 and 1,
> +		  respectively. Please find more information on common
> +		  clock bindings in ../clock/clock-bindings.txt.
> +
> +Port nodes (optional)
> +---------------------

This should refer to Documentation/devicetree/bindings/media/video-
interfaces.txt.

> +reg		: The interface:
> +		  0 - parallel (CCDC)
> +		  1 - CSIPHY1 -- CSI2C / CCP2B on 3630;
> +		      CSI1 -- CSIb on 3430
> +		  2 - CSIPHY2 -- CSI2A / CCP2B on 3630;
> +		      CSI2 -- CSIa on 3430
> +
> +Optional properties
> +===================
> +
> +vdd-csiphy1-supply : voltage supply of the CSI-2 PHY 1
> +vdd-csiphy2-supply : voltage supply of the CSI-2 PHY 2
> +
> +Endpoint nodes
> +--------------
> +
> +lane-polarity	: lane polarity (required on CSI-2)
> +		  0 -- not inverted; 1 -- inverted
> +data-lanes	: an array of data lanes from 1 to 3. The length can
> +		  be either 1 or 2. (required CSI-2)

s/required/required on/ ?

> +clock-lanes	: the clock lane (from 1 to 3). (required on CSI-2)
> +
> +
> +Example
> +=======
> +
> +		omap3_isp: omap3_isp@480bc000 {

DT node names traditionally use - as a separator. Furthermore the phandle 
isn't needed. This should thus probably be

	omap3-isp@480bc000 {

> +			compatible = "ti,omap3-isp";
> +			reg = <0x480bc000 0x12fc
> +			       0x480bd800 0x0600>;
> +			interrupts = <24>;
> +			iommus = <&mmu_isp>;
> +			syscon = <&omap3_scm_general 0x2f0>;
> +			ti,phy-type = <1>;
> +			#clock-cells = <1>;
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ddc5a8c..cdeef39 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7079,6 +7079,7 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	drivers/media/platform/omap3isp/
>  F:	drivers/staging/media/omap4iss/
> +F:	Documentation/devicetree/bindings/media/ti,omap3isp.txt

I would move this line before the other F: entries to keep them alphabetically 
sorted.

>  OMAP USB SUPPORT
>  M:	Felipe Balbi <balbi@ti.com>
Sakari Ailus March 12, 2015, 11:03 p.m. UTC | #2
Hi Laurent,

On Thu, Mar 12, 2015 at 01:39:07AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Saturday 07 March 2015 23:41:11 Sakari Ailus wrote:
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  .../devicetree/bindings/media/ti,omap3isp.txt      |   64 +++++++++++++++++
> >  MAINTAINERS                                        |    1 +
> >  2 files changed, 65 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/ti,omap3isp.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/ti,omap3isp.txt
> > b/Documentation/devicetree/bindings/media/ti,omap3isp.txt new file mode
> > 100644
> > index 0000000..2059524
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/ti,omap3isp.txt
> > @@ -0,0 +1,64 @@
> > +OMAP 3 ISP Device Tree bindings
> > +===============================
> > +
> > +More documentation on these bindings is available in
> > +video-interfaces.txt in the same directory.
> > +
> > +Required properties
> > +===================
> > +
> > +compatible	: "ti,omap3-isp"
> 
> I would rephrase that using the usual wording as "compatible: Must contain 
> "ti,omap3-isp".

Fixed.

> > +reg		: a set of two register block physical addresses and
> > +		  lengths
> 
> We should describe what each set represents and contains.

As discussed:

reg             : the two registers sets (physical address and length) for the
                  ISP. The first set contains the core ISP registers up to
                  the end of the SBL block. The second set contains the
                  CSI PHYs and receivers registers.

> > +interrupts	: the interrupt number
> 
> I would keep the wording generic and refer to interrupt specifier instead of 
> interrupt number.
> 
> "interrupts: the ISP interrupt specifier"

Fixed.

> > +iommus		: phandle of the IOMMU
> 
> Similarly,
> 
> "iommus: phandle and IOMMU specifier for the IOMMU that serves the ISP"

Ditto.

> > +syscon		: syscon phandle and register offset
> 
> We should document what the register offset is.

This is SoC specific as is the base address. I'm not sure that would be
relevant here. If you think so, shouldn't we also add the device base
addresses and register block lengths?

> > +ti,phy-type	: 0 -- 3430; 1 -- 3630
> 
> Would it make sense to add #define's for this ?

I'll use OMAP3ISP_PHY_TYPE_COMPLEX_IO and OMAP3ISP_PHY_TYPE_CSIPHY as
discussed.

> It could also make sense to document/name them "Complex I/O" and "CSIPHY" to 
> avoid referring to the SoC that implements them, as the ISP is also found in 
> SoCs other than 3430 and 3630.
> 
> Could the PHY type be derived from the ES revision that we query at runtime ?

I think this would work on 3430 and 3630 but I'm not certain about others.

> We should also take into account the fact that the DM3730 has officially no 
> CSIPHY, but still seems to implement them in practice.

The DT sources are for 36xx, but I'd guess it works on 37xx as well, doesn't
it?

> > +#clock-cells	: Must be 1 --- the ISP provides two external clocks,
> > +		  cam_xclka and cam_xclkb, at indices 0 and 1,
> > +		  respectively. Please find more information on common
> > +		  clock bindings in ../clock/clock-bindings.txt.
> > +
> > +Port nodes (optional)
> > +---------------------
> 
> This should refer to Documentation/devicetree/bindings/media/video-
> interfaces.txt.

There's a reference to video-interfaces.txt in the beginning of the file. Do
you think that'd be enough?

> > +reg		: The interface:
> > +		  0 - parallel (CCDC)
> > +		  1 - CSIPHY1 -- CSI2C / CCP2B on 3630;
> > +		      CSI1 -- CSIb on 3430
> > +		  2 - CSIPHY2 -- CSI2A / CCP2B on 3630;
> > +		      CSI2 -- CSIa on 3430
> > +
> > +Optional properties
> > +===================
> > +
> > +vdd-csiphy1-supply : voltage supply of the CSI-2 PHY 1
> > +vdd-csiphy2-supply : voltage supply of the CSI-2 PHY 2
> > +
> > +Endpoint nodes
> > +--------------
> > +
> > +lane-polarity	: lane polarity (required on CSI-2)
> > +		  0 -- not inverted; 1 -- inverted
> > +data-lanes	: an array of data lanes from 1 to 3. The length can
> > +		  be either 1 or 2. (required CSI-2)
> 
> s/required/required on/ ?

Fixed.

> > +clock-lanes	: the clock lane (from 1 to 3). (required on CSI-2)
> > +
> > +
> > +Example
> > +=======
> > +
> > +		omap3_isp: omap3_isp@480bc000 {
> 
> DT node names traditionally use - as a separator. Furthermore the phandle 
> isn't needed. This should thus probably be
> 
> 	omap3-isp@480bc000 {

Fixed.

> > +			compatible = "ti,omap3-isp";
> > +			reg = <0x480bc000 0x12fc
> > +			       0x480bd800 0x0600>;
> > +			interrupts = <24>;
> > +			iommus = <&mmu_isp>;
> > +			syscon = <&omap3_scm_general 0x2f0>;
> > +			ti,phy-type = <1>;
> > +			#clock-cells = <1>;
> > +			ports {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +			};
> > +		};
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ddc5a8c..cdeef39 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7079,6 +7079,7 @@ L:	linux-media@vger.kernel.org
> >  S:	Maintained
> >  F:	drivers/media/platform/omap3isp/
> >  F:	drivers/staging/media/omap4iss/
> > +F:	Documentation/devicetree/bindings/media/ti,omap3isp.txt
> 
> I would move this line before the other F: entries to keep them alphabetically 
> sorted.

Fixed.

> >  OMAP USB SUPPORT
> >  M:	Felipe Balbi <balbi@ti.com>
>
Laurent Pinchart March 12, 2015, 11:11 p.m. UTC | #3
Hi Sakari,

On Friday 13 March 2015 01:03:21 Sakari Ailus wrote:
> On Thu, Mar 12, 2015 at 01:39:07AM +0200, Laurent Pinchart wrote:
> > On Saturday 07 March 2015 23:41:11 Sakari Ailus wrote:
> > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > ---
> > > 
> > >  .../devicetree/bindings/media/ti,omap3isp.txt      |   64 +++++++++++++
> > >  MAINTAINERS                                        |    1 +
> > >  2 files changed, 65 insertions(+)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/media/ti,omap3isp.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/ti,omap3isp.txt
> > > b/Documentation/devicetree/bindings/media/ti,omap3isp.txt new file mode
> > > 100644
> > > index 0000000..2059524
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/ti,omap3isp.txt

[snip]

> > > +syscon		: syscon phandle and register offset
> > 
> > We should document what the register offset is.
> 
> This is SoC specific as is the base address. I'm not sure that would be
> relevant here. If you think so, shouldn't we also add the device base
> addresses and register block lengths?

I meant something like

syscon:	the phandle and register offset to the Complex I/O or CSI-PHY 
register.

> > > +ti,phy-type	: 0 -- 3430; 1 -- 3630
> > 
> > Would it make sense to add #define's for this ?
> 
> I'll use OMAP3ISP_PHY_TYPE_COMPLEX_IO and OMAP3ISP_PHY_TYPE_CSIPHY as
> discussed.
> 
> > It could also make sense to document/name them "Complex I/O" and "CSIPHY"
> > to avoid referring to the SoC that implements them, as the ISP is also
> > found in SoCs other than 3430 and 3630.
> > 
> > Could the PHY type be derived from the ES revision that we query at
> > runtime ?
>
> I think this would work on 3430 and 3630 but I'm not certain about others.
> 
> > We should also take into account the fact that the DM3730 has officially
> > no CSIPHY, but still seems to implement them in practice.
> 
> The DT sources are for 36xx, but I'd guess it works on 37xx as well, doesn't
> it?

I think so.

> > > +#clock-cells	: Must be 1 --- the ISP provides two external clocks,
> > > +		  cam_xclka and cam_xclkb, at indices 0 and 1,
> > > +		  respectively. Please find more information on common
> > > +		  clock bindings in ../clock/clock-bindings.txt.
> > > +
> > > +Port nodes (optional)
> > > +---------------------
> > 
> > This should refer to Documentation/devicetree/bindings/media/video-
> > interfaces.txt.
> 
> There's a reference to video-interfaces.txt in the beginning of the file. Do
> you think that'd be enough?

I've missed that. I think you could move the reference here.

> > > +reg		: The interface:
> > > +		  0 - parallel (CCDC)
> > > +		  1 - CSIPHY1 -- CSI2C / CCP2B on 3630;
> > > +		      CSI1 -- CSIb on 3430
> > > +		  2 - CSIPHY2 -- CSI2A / CCP2B on 3630;
> > > +		      CSI2 -- CSIa on 3430
Sakari Ailus March 12, 2015, 11:43 p.m. UTC | #4
Hi Laurent,

On Fri, Mar 13, 2015 at 01:11:03AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Friday 13 March 2015 01:03:21 Sakari Ailus wrote:
> > On Thu, Mar 12, 2015 at 01:39:07AM +0200, Laurent Pinchart wrote:
> > > On Saturday 07 March 2015 23:41:11 Sakari Ailus wrote:
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > ---
> > > > 
> > > >  .../devicetree/bindings/media/ti,omap3isp.txt      |   64 +++++++++++++
> > > >  MAINTAINERS                                        |    1 +
> > > >  2 files changed, 65 insertions(+)
> > > >  create mode 100644
> > > >  Documentation/devicetree/bindings/media/ti,omap3isp.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/ti,omap3isp.txt
> > > > b/Documentation/devicetree/bindings/media/ti,omap3isp.txt new file mode
> > > > 100644
> > > > index 0000000..2059524
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/ti,omap3isp.txt
> 
> [snip]
> 
> > > > +syscon		: syscon phandle and register offset
> > > 
> > > We should document what the register offset is.
> > 
> > This is SoC specific as is the base address. I'm not sure that would be
> > relevant here. If you think so, shouldn't we also add the device base
> > addresses and register block lengths?
> 
> I meant something like
> 
> syscon:	the phandle and register offset to the Complex I/O or CSI-PHY 
> register.

Oh, I misunderstood you. I'll use that text.

> > > > +ti,phy-type	: 0 -- 3430; 1 -- 3630
> > > 
> > > Would it make sense to add #define's for this ?
> > 
> > I'll use OMAP3ISP_PHY_TYPE_COMPLEX_IO and OMAP3ISP_PHY_TYPE_CSIPHY as
> > discussed.
> > 
> > > It could also make sense to document/name them "Complex I/O" and "CSIPHY"
> > > to avoid referring to the SoC that implements them, as the ISP is also
> > > found in SoCs other than 3430 and 3630.
> > > 
> > > Could the PHY type be derived from the ES revision that we query at
> > > runtime ?
> >
> > I think this would work on 3430 and 3630 but I'm not certain about others.
> > 
> > > We should also take into account the fact that the DM3730 has officially
> > > no CSIPHY, but still seems to implement them in practice.
> > 
> > The DT sources are for 36xx, but I'd guess it works on 37xx as well, doesn't
> > it?
> 
> I think so.
> 
> > > > +#clock-cells	: Must be 1 --- the ISP provides two external clocks,
> > > > +		  cam_xclka and cam_xclkb, at indices 0 and 1,
> > > > +		  respectively. Please find more information on common
> > > > +		  clock bindings in ../clock/clock-bindings.txt.
> > > > +
> > > > +Port nodes (optional)
> > > > +---------------------
> > > 
> > > This should refer to Documentation/devicetree/bindings/media/video-
> > > interfaces.txt.
> > 
> > There's a reference to video-interfaces.txt in the beginning of the file. Do
> > you think that'd be enough?
> 
> I've missed that. I think you could move the reference here.

Done.
Sebastian Reichel March 13, 2015, 9:34 a.m. UTC | #5
Hi,

On Fri, Mar 13, 2015 at 01:03:21AM +0200, Sakari Ailus wrote:
> [...]
>
> > > +Required properties
> > > +===================
> > > +
> > > +compatible	: "ti,omap3-isp"
> > 
> > I would rephrase that using the usual wording as "compatible: Must contain 
> > "ti,omap3-isp".
>
> [...]
>
> > > +ti,phy-type	: 0 -- 3430; 1 -- 3630
> > 
> > Would it make sense to add #define's for this ?
> 
> I'll use OMAP3ISP_PHY_TYPE_COMPLEX_IO and OMAP3ISP_PHY_TYPE_CSIPHY as
> discussed.
> 
> > It could also make sense to document/name them "Complex I/O" and "CSIPHY" to 
> > avoid referring to the SoC that implements them, as the ISP is also found in 
> > SoCs other than 3430 and 3630.
> > 
> > Could the PHY type be derived from the ES revision that we query at runtime ?
> 
> I think this would work on 3430 and 3630 but I'm not certain about others.
> 
> > We should also take into account the fact that the DM3730 has officially no 
> > CSIPHY, but still seems to implement them in practice.
> 
> The DT sources are for 36xx, but I'd guess it works on 37xx as well, doesn't
> it?

In other drivers this kind of information is often extracted from the
compatible string. For example:

{ .compatible = "ti,omap34xx-isp", .data = OMAP3ISP_PHY_TYPE_COMPLEX_IO, },
{ .compatible = "ti,omap36xx-isp", .data = OMAP3ISP_PHY_TYPE_CSIPHY, },
...

> [...]
>
> > > +Example
> > > +=======
> > > +
> > > +		omap3_isp: omap3_isp@480bc000 {
> > 
> > DT node names traditionally use - as a separator. Furthermore the phandle 
> > isn't needed. This should thus probably be
> > 
> > 	omap3-isp@480bc000 {
> 
> Fixed.

According to ePAPR this should be a generic name (page 19); For
example the i2c node name should be "i2c@address" instead of
"omap3-i2c@address". There is no recommended generic term for an
image signal processor, "isp" looks ok to me and seems to be
already used in NVIDIA Tegra's device tree files. So maybe:

isp@480bc000 {

> [...]

-- Sebastian
Laurent Pinchart March 14, 2015, 12:33 a.m. UTC | #6
Hi Sebastian,

Thank you for the review.

On Friday 13 March 2015 10:34:53 Sebastian Reichel wrote:
> On Fri, Mar 13, 2015 at 01:03:21AM +0200, Sakari Ailus wrote:
> > [...]
> > 
> > > > +Required properties
> > > > +===================
> > > > +
> > > > +compatible	: "ti,omap3-isp"
> > > 
> > > I would rephrase that using the usual wording as "compatible: Must
> > > contain
> > > "ti,omap3-isp".
> > 
> > [...]
> > 
> > > > +ti,phy-type	: 0 -- 3430; 1 -- 3630
> > > 
> > > Would it make sense to add #define's for this ?
> > 
> > I'll use OMAP3ISP_PHY_TYPE_COMPLEX_IO and OMAP3ISP_PHY_TYPE_CSIPHY as
> > discussed.
> > 
> > > It could also make sense to document/name them "Complex I/O" and
> > > "CSIPHY" to avoid referring to the SoC that implements them, as the ISP
> > > is also found in SoCs other than 3430 and 3630.
> > > 
> > > Could the PHY type be derived from the ES revision that we query at
> > > runtime ?
> >
> > I think this would work on 3430 and 3630 but I'm not certain about others.
> > 
> > > We should also take into account the fact that the DM3730 has officially
> > > no CSIPHY, but still seems to implement them in practice.
> > 
> > The DT sources are for 36xx, but I'd guess it works on 37xx as well,
> > doesn't it?
> 
> In other drivers this kind of information is often extracted from the
> compatible string. For example:
> 
> { .compatible = "ti,omap34xx-isp", .data = OMAP3ISP_PHY_TYPE_COMPLEX_IO, },
> { .compatible = "ti,omap36xx-isp", .data = OMAP3ISP_PHY_TYPE_CSIPHY, },
> ...

That's an option too, which I've discussed with Sakari before. The reason why 
we have decided to go for a separate property is that the PHY type seems to be 
more an SoC integration property than an ISP model property. I'm open to 
reconsidering that though.

Another option that has been discussed was to infer the PHY type from the ISP 
revision number queried at runtime. That would be fine for the 3430, 3630 and 
3730, but it remains unclear at this point whether this scheme would work with 
other SoCs. It should also be noted that some OMAP3-based SoCs that 
incorporate the ISP officially don't include the CSI PHYs, but seem to have 
them in practice.

> > [...]
> > 
> > > > +Example
> > > > +=======
> > > > +
> > > > +		omap3_isp: omap3_isp@480bc000 {
> > > 
> > > DT node names traditionally use - as a separator. Furthermore the
> > > phandle isn't needed. This should thus probably be
> > > 
> > > 	omap3-isp@480bc000 {
> > 
> > Fixed.
> 
> According to ePAPR this should be a generic name (page 19); For
> example the i2c node name should be "i2c@address" instead of
> "omap3-i2c@address". There is no recommended generic term for an
> image signal processor, "isp" looks ok to me and seems to be
> already used in NVIDIA Tegra's device tree files. So maybe:
> 
> isp@480bc000 {

"isp" sounds good to me.
Sakari Ailus March 14, 2015, 2:10 p.m. UTC | #7
Hi Sebastian,

Thanks for the comments!

On Fri, Mar 13, 2015 at 10:34:53AM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Mar 13, 2015 at 01:03:21AM +0200, Sakari Ailus wrote:
> > [...]
> >
> > > > +Required properties
> > > > +===================
> > > > +
> > > > +compatible	: "ti,omap3-isp"
> > > 
> > > I would rephrase that using the usual wording as "compatible: Must contain 
> > > "ti,omap3-isp".
> >
> > [...]
> >
> > > > +ti,phy-type	: 0 -- 3430; 1 -- 3630
> > > 
> > > Would it make sense to add #define's for this ?
> > 
> > I'll use OMAP3ISP_PHY_TYPE_COMPLEX_IO and OMAP3ISP_PHY_TYPE_CSIPHY as
> > discussed.
> > 
> > > It could also make sense to document/name them "Complex I/O" and "CSIPHY" to 
> > > avoid referring to the SoC that implements them, as the ISP is also found in 
> > > SoCs other than 3430 and 3630.
> > > 
> > > Could the PHY type be derived from the ES revision that we query at runtime ?
> > 
> > I think this would work on 3430 and 3630 but I'm not certain about others.
> > 
> > > We should also take into account the fact that the DM3730 has officially no 
> > > CSIPHY, but still seems to implement them in practice.
> > 
> > The DT sources are for 36xx, but I'd guess it works on 37xx as well, doesn't
> > it?
> 
> In other drivers this kind of information is often extracted from the
> compatible string. For example:
> 
> { .compatible = "ti,omap34xx-isp", .data = OMAP3ISP_PHY_TYPE_COMPLEX_IO, },
> { .compatible = "ti,omap36xx-isp", .data = OMAP3ISP_PHY_TYPE_CSIPHY, },
> ...

As Laurent said, I'd prefer to keep it as it is now; they phy selection
isn't really a part of the ISP in hardware either. It just happens to be
that the first phy selection logic can be found in 3430 (isp rev 2.0) and
latter in 3630 (isp rev 15.0).

> > [...]
> >
> > > > +Example
> > > > +=======
> > > > +
> > > > +		omap3_isp: omap3_isp@480bc000 {
> > > 
> > > DT node names traditionally use - as a separator. Furthermore the phandle 
> > > isn't needed. This should thus probably be
> > > 
> > > 	omap3-isp@480bc000 {
> > 
> > Fixed.
> 
> According to ePAPR this should be a generic name (page 19); For
> example the i2c node name should be "i2c@address" instead of
> "omap3-i2c@address". There is no recommended generic term for an
> image signal processor, "isp" looks ok to me and seems to be
> already used in NVIDIA Tegra's device tree files. So maybe:
> 
> isp@480bc000 {

Thanks for the suggestion. I'll fix that.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/media/ti,omap3isp.txt b/Documentation/devicetree/bindings/media/ti,omap3isp.txt
new file mode 100644
index 0000000..2059524
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,omap3isp.txt
@@ -0,0 +1,64 @@ 
+OMAP 3 ISP Device Tree bindings
+===============================
+
+More documentation on these bindings is available in
+video-interfaces.txt in the same directory.
+
+Required properties
+===================
+
+compatible	: "ti,omap3-isp"
+reg		: a set of two register block physical addresses and
+		  lengths
+interrupts	: the interrupt number
+iommus		: phandle of the IOMMU
+syscon		: syscon phandle and register offset
+ti,phy-type	: 0 -- 3430; 1 -- 3630
+#clock-cells	: Must be 1 --- the ISP provides two external clocks,
+		  cam_xclka and cam_xclkb, at indices 0 and 1,
+		  respectively. Please find more information on common
+		  clock bindings in ../clock/clock-bindings.txt.
+
+Port nodes (optional)
+---------------------
+
+reg		: The interface:
+		  0 - parallel (CCDC)
+		  1 - CSIPHY1 -- CSI2C / CCP2B on 3630;
+		      CSI1 -- CSIb on 3430
+		  2 - CSIPHY2 -- CSI2A / CCP2B on 3630;
+		      CSI2 -- CSIa on 3430
+
+Optional properties
+===================
+
+vdd-csiphy1-supply : voltage supply of the CSI-2 PHY 1
+vdd-csiphy2-supply : voltage supply of the CSI-2 PHY 2
+
+Endpoint nodes
+--------------
+
+lane-polarity	: lane polarity (required on CSI-2)
+		  0 -- not inverted; 1 -- inverted
+data-lanes	: an array of data lanes from 1 to 3. The length can
+		  be either 1 or 2. (required CSI-2)
+clock-lanes	: the clock lane (from 1 to 3). (required on CSI-2)
+
+
+Example
+=======
+
+		omap3_isp: omap3_isp@480bc000 {
+			compatible = "ti,omap3-isp";
+			reg = <0x480bc000 0x12fc
+			       0x480bd800 0x0600>;
+			interrupts = <24>;
+			iommus = <&mmu_isp>;
+			syscon = <&omap3_scm_general 0x2f0>;
+			ti,phy-type = <1>;
+			#clock-cells = <1>;
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
diff --git a/MAINTAINERS b/MAINTAINERS
index ddc5a8c..cdeef39 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7079,6 +7079,7 @@  L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/media/platform/omap3isp/
 F:	drivers/staging/media/omap4iss/
+F:	Documentation/devicetree/bindings/media/ti,omap3isp.txt
 
 OMAP USB SUPPORT
 M:	Felipe Balbi <balbi@ti.com>