Re: [PATCH] alter_ps2: Add devicetree support

[Walter Goossens <waltergoossens@home.nl>]

On 1/17/11 11:02 PM, Grant Likely wrote:
> 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.
>
Ahh ok.
That does look like a good solution. I'll try and cook something up for 
that.
Thanks for the explanation!

Walter

> 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
>>>>
>>>>
>>>