Re: [PATCH v2] ARM: CSR: Adding CSR SiRFprimaII board support

On Wed, Jul 06, 2011 at 02:01:24PM +0800, Barry Song wrote:

2011/7/6 Barry Song [off-list ref]:

quoted

2011/7/6 Grant Likely [off-list ref]:

quoted

quoted

                      intc: interrupt-controller@0x80020000 {
                              #interrupt-cells = <1>;
                              interrupt-controller;
                              compatible = "sirf,intc", "sirf,prima2-intc";

This looks backwards.  Most specific values should appear first.
"sirf,intc" looks to be generic when compared to "sirf,prima2-intc".
I'm also concerned that "sirf,intc" is trying to be too generic.  As
much as possible compatible values should reflect a real
implementation of the interface, not a generic abstract name.  Future
hardware can always claim compatibility with existing older hardware.
You're probably better off dropping "sirf,intc" entirely, and doing
the same for similar properties throughout the file.

i have been confused by compatible in dts and drivers for some time.
if an intc ip core is shared by two chips, for example, prima2 and
atlas6. is the following right?

in dts for prima2: compatible = "sirf,prima2-intc";
in dts for atlas6:  compatible = "sirf,prima2-atlas6";

sorry for typo. in dts for atlas6:  compatible = "sirf,atlas6-intc";

quoted

in intc drivers shared for both:     compatible = "sirf,prima2-intc",
"sirf,prima2-atlas6";

sorry for typo.  compatible = "sirf,prima2-intc", "sirf,atlas6-intc";

Yes, that is one way to do it, just specify all the implementations
using the IP core.  If there are only a few, then this is easy.  If
there are a lot of implementations using the same core, then the list
gets rather large in a hurry.

The intent of compatible is to allow one device to claim compatibility
with another.  So, if the prima2 was already supported, and you were adding
atlas6 support, you could put the following into the atlas6 .dts:

	compatible = "sirf,atlas6-intc", "airf,prima2-intc";

and the existing driver would work.

The other option when working with IP cores it so encode the version
of the IP block into the compatible values, and make both boards use
that.  ie.  "sirf,intc-1.32".  *however* doing so requires that the
hardware folks actually do a good job of documenting the core versions
so that the compatible value you use accurately describes the
hardware.

It is also completely valid for a newer IP core to claim compatibility
with an older one in the .dts:

	ie: compatible = "sirf,intc-1.32", "sirf,intc-1.00";

Personally, I prefer anchoring compatible values to the specific
hardware implmentation rather than the IP core version.

quoted

in my understanding, dts for special soc/board contains special
compatible for the chip, drivers shared by multi-soc/board contain all
compatible lists which can be supported by them?

what's the generic way for this?

quoted

Also, for every new compatible value make sure you add documentation
for it to Documentation/devicetree/bindings. (You may have already
done so, it's just something I remind people about a lot).

These comments also apply through the rest of the file.