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

[Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]