Re: [PATCH] alter_ps2: Add devicetree support

[Grant Likely <grant.likely@secretlab.ca>]

On Mon, Jan 17, 2011 at 10:04:03PM +0100, Walter Goossens wrote:
> On 1/17/11 7:59 AM, Grant Likely wrote:
> > On Sun, Jan 16, 2011 at 11:29 PM, Thomas Chou <thomas@wytron.com.tw> wrote:
> >> @@ -173,6 +176,16 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
> >>        return 0;
> >>  }
> >>
> >> +#ifdef CONFIG_OF
> >> +static struct of_device_id altera_ps2_match[] = {
> >> +       {
> >> +               .compatible = "altera,altera_ps2",
> >> +       },
> > So is this an FPGA soft core PS2 device?  Is there any kind of version
> > attached to the soft core?  The compatible value should specify an
> > exact version of the implementation that this driver works with.
> > (Newer core versions can claim compatibility with older ones, so the
> > driver's compatible list doesn't need to be exhaustive).
> >
> What's the preferred way of versioning components in a device-tree?
> Quite a few components inside an fpga will get a new version number with
> every release of the tools. For example components supplied by Altera
> will get a new number with every release of their IP library (approx.
> twice a year) even when (at least from a software point of view) there
> is nothing changed in the core. Should we add the number to the
> "compatible" name and possibly get slightly more bulky drivers, or add a
> version tag to the components where a driver can make decisions based on
> the version of the core (if needed)?
> Another way to reduce the number of lines in a compatible section would
> be to add both their versioned and unversioned compatible entry in the
> dts so drivers not needing a specific version don't need to supply the
> entire list.
> We do have the version numbers available when generating the DTS and
> NiosII is still quite new to device-tree so we are still flexible in
> fixing this in the best possible way.

A good rule of thumb is to always choose compatible values that
reflect real working hardware.  ie. "xlnx,xps-uartlite-1.00.a" instead
of trying to define a generic "xlnx,uartlite".  You can see this value
in drivers/serial/uartlite.c, and write the driver to match the value
of the device that you actually worked with.

Then, when you produce a .dts for a design, each device must specify
exactly what it is (the specific version) plus an optional list of
devices that it is 100% register-level backwards compatible with.  In
the example above, the uartlite has retained the exact same interface,
so all designs claim compatibility with xlnx,xps-uartlite-1.00.a
regardless of the actual version.

To take the example of the altera ps2 core; say altera has released
versions 1, 2, 3, 4 and 5 of the core, and say the behaviour changed
in a non-compatible way in version 3.  Then the driver might do
something like:


static struct of_device_id altera_ps2_match[] = {
       { .compatible = "altera,altera_ps2-1", .data = altera_ps2_1_ops, },
       { .compatible = "altera,altera_ps2-3", .data = altera_ps2_3_ops, },
       { }
};

Then the compatible values for each version in a .dts file would be:

v1: compatible = "altera,altera_ps2-1";
v2: compatible = "altera,altera_ps2-2", "altera,altera_ps2-1";
v3: compatible = "altera,altera_ps2-3";
v4: compatible = "altera,altera_ps2-4", "altera,altera_ps2-3";
v5: compatible = "altera,altera_ps2-5", "altera,altera_ps2-3";

so, instead of trying to us a 'generic' value like "altera,altera_ps2"
which has a bunch of ambiguity about which hardware actually
implements the behaviour, the values "altera,altera_ps2-1" and
"altera,altera_ps2-3" become the de-facto 'generic' values without any
messiness or ambiguity about what they mean.

Plus, each .dts file still specifies the exact version that is
implemented on the board so that the driver can still fixup
version-specific bugs if any are discovered in the future.

g.

> 
> > Otherwise, this patch looks correct.
> >
> > g.
> >
> >> +       {},
> >> +}
> >> +MODULE_DEVICE_TABLE(of, altera_jtaguart_match);
> >> +#endif /* CONFIG_OF */
> >> +
> >>  /*
> >>  * Our device driver structure
> >>  */
> >> @@ -182,6 +195,9 @@ static struct platform_driver altera_ps2_driver = {
> >>        .driver = {
> >>                .name   = DRV_NAME,
> >>                .owner  = THIS_MODULE,
> >> +#ifdef CONFIG_OF
> >> +               .of_match_table = altera_ps2_match,
> >> +#endif
> >>        },
> >>  };
> >>
> >> --
> >> 1.7.3.4
> >>
> >>
> >
> >
>