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.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
