Messages in this thread | | | Date | Tue, 18 Jan 2011 00:27:02 +0100 | From | Walter Goossens <> | Subject | Re: [PATCH] alter_ps2: Add devicetree support |
| |
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 >>>> >>>> >>>
| |