[PATCH] alter_ps2: Add devicetree support

[PATCH] alter_ps2: Add devicetree support

[Thomas Chou]

From: Walter Goossens <waltergoossens-CmkmPbn3yAE@public.gmane.org>

Signed-off-by: Walter Goossens <waltergoossens-CmkmPbn3yAE@public.gmane.org>
Signed-off-by: Thomas Chou <thomas-SDxUXYEhEBiCuPEqFHbRBg@public.gmane.org>
---
 drivers/input/serio/altera_ps2.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
index 7998560..93054a1 100644
--- a/drivers/input/serio/altera_ps2.c
+++ b/drivers/input/serio/altera_ps2.c
@@ -19,6 +19,9 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#ifdef CONFIG_OF
+#include <linux/of.h>
+#endif
 
 #define DRV_NAME "altera_ps2"
 
@@ -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",
+	},
+	{},
+}
+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

Re: [PATCH] alter_ps2: Add devicetree support

[Grant Likely]

On Sun, Jan 16, 2011 at 11:29 PM, Thomas Chou <thomas@wytron.com.tw> wrote:
> From: Walter Goossens <waltergoossens@home.nl>
>
> Signed-off-by: Walter Goossens <waltergoossens@home.nl>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>  drivers/input/serio/altera_ps2.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
> index 7998560..93054a1 100644
> --- a/drivers/input/serio/altera_ps2.c
> +++ b/drivers/input/serio/altera_ps2.c
> @@ -19,6 +19,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#ifdef CONFIG_OF
> +#include <linux/of.h>
> +#endif
>
>  #define DRV_NAME "altera_ps2"
>
> @@ -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).

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



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] alter_ps2: Add devicetree support

[Walter Goossens]

On 1/17/11 7:59 AM, Grant Likely wrote:
> On Sun, Jan 16, 2011 at 11:29 PM, Thomas Chou <thomas-SDxUXYEhEBiCuPEqFHbRBg@public.gmane.org> wrote:
>> From: Walter Goossens <waltergoossens-CmkmPbn3yAE@public.gmane.org>
>>
>> Signed-off-by: Walter Goossens <waltergoossens-CmkmPbn3yAE@public.gmane.org>
>> Signed-off-by: Thomas Chou <thomas-SDxUXYEhEBiCuPEqFHbRBg@public.gmane.org>
>> ---
>>  drivers/input/serio/altera_ps2.c |   16 ++++++++++++++++
>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
>> index 7998560..93054a1 100644
>> --- a/drivers/input/serio/altera_ps2.c
>> +++ b/drivers/input/serio/altera_ps2.c
>> @@ -19,6 +19,9 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/io.h>
>>  #include <linux/slab.h>
>> +#ifdef CONFIG_OF
>> +#include <linux/of.h>
>> +#endif
>>
>>  #define DRV_NAME "altera_ps2"
>>
>> @@ -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.

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

Re: [PATCH] alter_ps2: Add devicetree support

[Grant Likely]

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

Re: [PATCH] alter_ps2: Add devicetree support

[Walter Goossens]

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

Re: [PATCH] alter_ps2: Add devicetree support

[Thomas Chou]

On 01/18/2011 07:27 AM, Walter Goossens wrote:
>> 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.

Hi Walter,

Can we use "kind" and "version" attribute of a sopcinfo file to build 
the match string? This way, your dts converter won't need update for 
each new component.

eg,

kind="altera_avalon_spi" version="9.0" ==> "altera","avalon-spi-9.0"

kind="altera_up_avalon_ps2_classic" version="6.1" ==>
   "altera","up-avalon-ps2-classic-6.1"

- Thomas

Re: [PATCH] alter_ps2: Add devicetree support

[Dmitry Torokhov]

On Mon, Jan 17, 2011 at 02:29:20PM +0800, Thomas Chou wrote:
> From: Walter Goossens <waltergoossens@home.nl>
> 

For my education, why is this needed?

> Signed-off-by: Walter Goossens <waltergoossens@home.nl>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>  drivers/input/serio/altera_ps2.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
> index 7998560..93054a1 100644
> --- a/drivers/input/serio/altera_ps2.c
> +++ b/drivers/input/serio/altera_ps2.c
> @@ -19,6 +19,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#ifdef CONFIG_OF
> +#include <linux/of.h>
> +#endif

Does it have to be guarded?

>  
>  #define DRV_NAME "altera_ps2"
>  
> @@ -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[] = {

Not const?

> +	{ 
> +		.compatible = "altera,altera_ps2",
> +	},
> +	{},
> +}
> +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

Can't we spare 4 bytes per driver and have .of_match_table always
present?

-- 
Dmitry

Re: [PATCH] alter_ps2: Add devicetree support

[Grant Likely]

On Mon, Jan 17, 2011 at 01:31:01PM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 17, 2011 at 02:29:20PM +0800, Thomas Chou wrote:
> > From: Walter Goossens <waltergoossens-CmkmPbn3yAE@public.gmane.org>
> > 
> 
> For my education, why is this needed?
> 
> > Signed-off-by: Walter Goossens <waltergoossens-CmkmPbn3yAE@public.gmane.org>
> > Signed-off-by: Thomas Chou <thomas-SDxUXYEhEBiCuPEqFHbRBg@public.gmane.org>
> > ---
> >  drivers/input/serio/altera_ps2.c |   16 ++++++++++++++++
> >  1 files changed, 16 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
> > index 7998560..93054a1 100644
> > --- a/drivers/input/serio/altera_ps2.c
> > +++ b/drivers/input/serio/altera_ps2.c
> > @@ -19,6 +19,9 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/io.h>
> >  #include <linux/slab.h>
> > +#ifdef CONFIG_OF
> > +#include <linux/of.h>
> > +#endif
> 
> Does it have to be guarded?

Shouldn't need to be.

> 
> >  
> >  #define DRV_NAME "altera_ps2"
> >  
> > @@ -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[] = {
> 
> Not const?
> 
> > +	{ 
> > +		.compatible = "altera,altera_ps2",
> > +	},
> > +	{},
> > +}
> > +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
> 
> Can't we spare 4 bytes per driver and have .of_match_table always
> present?

Yeah, probably.  I've been trying to avoid it, but I suppose that is
much ado about a tiny amount of data.  I'll craft a patch.

g.

[PATCH v2] altera_ps2: Add devicetree support

[Thomas Chou]

From: Walter Goossens <waltergoossens@home.nl>

Signed-off-by: Walter Goossens <waltergoossens@home.nl>
Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
v2 use const and add compat version.

 drivers/input/serio/altera_ps2.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
index 7998560..63325ee 100644
--- a/drivers/input/serio/altera_ps2.c
+++ b/drivers/input/serio/altera_ps2.c
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #define DRV_NAME "altera_ps2"
 
@@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id altera_ps2_match[] = {
+	{ .compatible = "altr,ps2-1.0", },
+	{},
+}
+MODULE_DEVICE_TABLE(of, altera_ps2_match);
+
 /*
  * Our device driver structure
  */
@@ -182,6 +189,7 @@ static struct platform_driver altera_ps2_driver = {
 	.driver	= {
 		.name	= DRV_NAME,
 		.owner	= THIS_MODULE,
+		.of_match_table = altera_ps2_match,
 	},
 };
 
@@ -189,13 +197,12 @@ static int __init altera_ps2_init(void)
 {
 	return platform_driver_register(&altera_ps2_driver);
 }
+module_init(altera_ps2_init);
 
 static void __exit altera_ps2_exit(void)
 {
 	platform_driver_unregister(&altera_ps2_driver);
 }
-
-module_init(altera_ps2_init);
 module_exit(altera_ps2_exit);
 
 MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
-- 
1.7.3.4

Re: [PATCH v2] altera_ps2: Add devicetree support

[Grant Likely]

On Mon, Jan 24, 2011 at 01:58:13PM +0800, Thomas Chou wrote:
> From: Walter Goossens <waltergoossens@home.nl>
> 
> Signed-off-by: Walter Goossens <waltergoossens@home.nl>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
> v2 use const and add compat version.
> 
>  drivers/input/serio/altera_ps2.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
> index 7998560..63325ee 100644
> --- a/drivers/input/serio/altera_ps2.c
> +++ b/drivers/input/serio/altera_ps2.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  
>  #define DRV_NAME "altera_ps2"
>  
> @@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct of_device_id altera_ps2_match[] = {
> +	{ .compatible = "altr,ps2-1.0", },

I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch.  I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.

> +	{},
> +}
> +MODULE_DEVICE_TABLE(of, altera_ps2_match);
> +
>  /*
>   * Our device driver structure
>   */
> @@ -182,6 +189,7 @@ static struct platform_driver altera_ps2_driver = {
>  	.driver	= {
>  		.name	= DRV_NAME,
>  		.owner	= THIS_MODULE,
> +		.of_match_table = altera_ps2_match,

At the moment, this patch can only be applied against my
devicetree/next branch since there is a #ifdef CONFIG_OF around
of_match_table in mainline, and the patch to remove it is in my next
branch.  I'm okay with taking it through my tree if Dmitry agrees.

g.

>  	},
>  };
>  
> @@ -189,13 +197,12 @@ static int __init altera_ps2_init(void)
>  {
>  	return platform_driver_register(&altera_ps2_driver);
>  }
> +module_init(altera_ps2_init);
>  
>  static void __exit altera_ps2_exit(void)
>  {
>  	platform_driver_unregister(&altera_ps2_driver);
>  }
> -
> -module_init(altera_ps2_init);
>  module_exit(altera_ps2_exit);
>  
>  MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
> -- 
> 1.7.3.4
> 

Re: [PATCH v2] altera_ps2: Add devicetree support

[Grant Likely]

On Tue, Feb 1, 2011 at 9:31 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Jan 24, 2011 at 01:58:13PM +0800, Thomas Chou wrote:
>> From: Walter Goossens <waltergoossens@home.nl>
>>
>> Signed-off-by: Walter Goossens <waltergoossens@home.nl>
>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>> ---
>> v2 use const and add compat version.
>>
>>  drivers/input/serio/altera_ps2.c |   11 +++++++++--
>>  1 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
>> index 7998560..63325ee 100644
>> --- a/drivers/input/serio/altera_ps2.c
>> +++ b/drivers/input/serio/altera_ps2.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/io.h>
>>  #include <linux/slab.h>
>> +#include <linux/of.h>
>>
>>  #define DRV_NAME "altera_ps2"
>>
>> @@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> +static const struct of_device_id altera_ps2_match[] = {
>> +     { .compatible = "altr,ps2-1.0", },
>
> I thought I had seen 'altera' instead of an abbreviation being used in
> a previous patch.  I don't care much whether 'altr' or 'altera' is
> used, but I'd like to know that there is consensus from the Altera
> users so that all the drivers use the same prefix.

Oh, and this patch should add documentation to
Documentation/devicetree/bindings/spi specifying exactly what device
this compatible string represents and what properties are expected to
be in the node (if any) beyond the standard reg, interrupts, etc.  It
doesn't need to be huge and detailed, but it at least needs to show
that the string is in use.  This comment goes for the other patches
that add bindings too.

g.

[PATCH v3] altera_ps2: Add devicetree support

[Thomas Chou]

Signed-off-by: Walter Goossens <waltergoossens-CmkmPbn3yAE@public.gmane.org>
Signed-off-by: Thomas Chou <thomas-SDxUXYEhEBiCuPEqFHbRBg@public.gmane.org>
---
v2 use const and add compat version.
v3 change compatible vendor to ALTR.
   add dts binding doc.

 .../devicetree/bindings/serio/altera_ps2.txt       |    4 ++++
 drivers/input/serio/altera_ps2.c                   |   11 +++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serio/altera_ps2.txt

diff --git a/Documentation/devicetree/bindings/serio/altera_ps2.txt b/Documentation/devicetree/bindings/serio/altera_ps2.txt
new file mode 100644
index 0000000..4d9eecc
--- /dev/null
+++ b/Documentation/devicetree/bindings/serio/altera_ps2.txt
@@ -0,0 +1,4 @@
+Altera UP PS/2 controller
+
+Required properties:
+- compatible : should be "ALTR,ps2-1.0".
diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
index 7998560..d94b4a3 100644
--- a/drivers/input/serio/altera_ps2.c
+++ b/drivers/input/serio/altera_ps2.c
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #define DRV_NAME "altera_ps2"
 
@@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id altera_ps2_match[] = {
+	{ .compatible = "ALTR,ps2-1.0", },
+	{},
+}
+MODULE_DEVICE_TABLE(of, altera_ps2_match);
+
 /*
  * Our device driver structure
  */
@@ -182,6 +189,7 @@ static struct platform_driver altera_ps2_driver = {
 	.driver	= {
 		.name	= DRV_NAME,
 		.owner	= THIS_MODULE,
+		.of_match_table = altera_ps2_match,
 	},
 };
 
@@ -189,13 +197,12 @@ static int __init altera_ps2_init(void)
 {
 	return platform_driver_register(&altera_ps2_driver);
 }
+module_init(altera_ps2_init);
 
 static void __exit altera_ps2_exit(void)
 {
 	platform_driver_unregister(&altera_ps2_driver);
 }
-
-module_init(altera_ps2_init);
 module_exit(altera_ps2_exit);
 
 MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
-- 
1.7.3.5

Re: [PATCH v3] altera_ps2: Add devicetree support

[Grant Likely]

On Thu, Feb 03, 2011 at 11:05:04AM +0800, Thomas Chou wrote:
> Signed-off-by: Walter Goossens <waltergoossens@home.nl>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
> v2 use const and add compat version.
> v3 change compatible vendor to ALTR.
>    add dts binding doc.
> 
>  .../devicetree/bindings/serio/altera_ps2.txt       |    4 ++++
>  drivers/input/serio/altera_ps2.c                   |   11 +++++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/serio/altera_ps2.txt
> 
> diff --git a/Documentation/devicetree/bindings/serio/altera_ps2.txt b/Documentation/devicetree/bindings/serio/altera_ps2.txt
> new file mode 100644
> index 0000000..4d9eecc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serio/altera_ps2.txt
> @@ -0,0 +1,4 @@
> +Altera UP PS/2 controller
> +
> +Required properties:
> +- compatible : should be "ALTR,ps2-1.0".
> diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
> index 7998560..d94b4a3 100644
> --- a/drivers/input/serio/altera_ps2.c
> +++ b/drivers/input/serio/altera_ps2.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  
>  #define DRV_NAME "altera_ps2"
>  
> @@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct of_device_id altera_ps2_match[] = {
> +	{ .compatible = "ALTR,ps2-1.0", },
> +	{},
> +}
> +MODULE_DEVICE_TABLE(of, altera_ps2_match);
> +

Should be:

#ifdef CONFIG_OF
static const struct of_device_id altera_ps2_match[] = {
	{ .compatible = "ALTR,ps2-1.0", },
	{},
}
MODULE_DEVICE_TABLE(of, altera_ps2_match);
#else /* CONFIG_OF */
#define altera_ps2_match NULL
#endif /* CONFIG_OF */

Otherwise the driver will be advertising that it provides device tree
support when CONFIG_OF is disabled.

Otherwise, looks good to me.  Feel free to add my acked-by line.

g.

>  /*
>   * Our device driver structure
>   */
> @@ -182,6 +189,7 @@ static struct platform_driver altera_ps2_driver = {
>  	.driver	= {
>  		.name	= DRV_NAME,
>  		.owner	= THIS_MODULE,
> +		.of_match_table = altera_ps2_match,
>  	},
>  };
>  
> @@ -189,13 +197,12 @@ static int __init altera_ps2_init(void)
>  {
>  	return platform_driver_register(&altera_ps2_driver);
>  }
> +module_init(altera_ps2_init);
>  
>  static void __exit altera_ps2_exit(void)
>  {
>  	platform_driver_unregister(&altera_ps2_driver);
>  }
> -
> -module_init(altera_ps2_init);
>  module_exit(altera_ps2_exit);
>  
>  MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
> -- 
> 1.7.3.5
> 

Re: [PATCH v3] altera_ps2: Add devicetree support

[Thomas Chou]

On 02/12/2011 05:26 PM, Grant Likely wrote:
> On Thu, Feb 03, 2011 at 11:05:04AM +0800, Thomas Chou wrote:
>> Signed-off-by: Walter Goossens<waltergoossens@home.nl>
>> Signed-off-by: Thomas Chou<thomas@wytron.com.tw>
>> ---
>> v2 use const and add compat version.
>> v3 change compatible vendor to ALTR.
>>     add dts binding doc.
>>
>>   .../devicetree/bindings/serio/altera_ps2.txt       |    4 ++++
>>   drivers/input/serio/altera_ps2.c                   |   11 +++++++++--
>>   2 files changed, 13 insertions(+), 2 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/serio/altera_ps2.txt
>>
>> diff --git a/Documentation/devicetree/bindings/serio/altera_ps2.txt b/Documentation/devicetree/bindings/serio/altera_ps2.txt
>> new file mode 100644
>> index 0000000..4d9eecc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serio/altera_ps2.txt
>> @@ -0,0 +1,4 @@
>> +Altera UP PS/2 controller
>> +
>> +Required properties:
>> +- compatible : should be "ALTR,ps2-1.0".
>> diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
>> index 7998560..d94b4a3 100644
>> --- a/drivers/input/serio/altera_ps2.c
>> +++ b/drivers/input/serio/altera_ps2.c
>> @@ -19,6 +19,7 @@
>>   #include<linux/platform_device.h>
>>   #include<linux/io.h>
>>   #include<linux/slab.h>
>> +#include<linux/of.h>
>>
>>   #define DRV_NAME "altera_ps2"
>>
>> @@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>
>> +static const struct of_device_id altera_ps2_match[] = {
>> +	{ .compatible = "ALTR,ps2-1.0", },
>> +	{},
>> +}
>> +MODULE_DEVICE_TABLE(of, altera_ps2_match);
>> +
>
> Should be:
>
> #ifdef CONFIG_OF
> static const struct of_device_id altera_ps2_match[] = {
> 	{ .compatible = "ALTR,ps2-1.0", },
> 	{},
> }
> MODULE_DEVICE_TABLE(of, altera_ps2_match);
> #else /* CONFIG_OF */
> #define altera_ps2_match NULL
> #endif /* CONFIG_OF */
>
> Otherwise the driver will be advertising that it provides device tree
> support when CONFIG_OF is disabled.

Thanks. Will update all related drivers.

- Thomas

>
> Otherwise, looks good to me.  Feel free to add my acked-by line.
>
> g.
>
>>   /*
>>    * Our device driver structure
>>    */
>> @@ -182,6 +189,7 @@ static struct platform_driver altera_ps2_driver = {
>>   	.driver	= {
>>   		.name	= DRV_NAME,
>>   		.owner	= THIS_MODULE,
>> +		.of_match_table = altera_ps2_match,
>>   	},
>>   };
>>
>> @@ -189,13 +197,12 @@ static int __init altera_ps2_init(void)
>>   {
>>   	return platform_driver_register(&altera_ps2_driver);
>>   }
>> +module_init(altera_ps2_init);
>>
>>   static void __exit altera_ps2_exit(void)
>>   {
>>   	platform_driver_unregister(&altera_ps2_driver);
>>   }
>> -
>> -module_init(altera_ps2_init);
>>   module_exit(altera_ps2_exit);
>>
>>   MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
>> --
>> 1.7.3.5
>>
>

[PATCH v4] altera_ps2: Add devicetree support

[Thomas Chou]

From: Walter Goossens <waltergoossens@home.nl>

Signed-off-by: Walter Goossens <waltergoossens@home.nl>
Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
---
v2 use const and add compat version.
v3 change compatible vendor to ALTR.
    add dts binding doc.
v4 condition module device table export for of.

Grant, please take it through your tree as Dmitry agreed.

 .../devicetree/bindings/serio/altera_ps2.txt       |    4 ++++
 drivers/input/serio/altera_ps2.c                   |   15 +++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serio/altera_ps2.txt

diff --git a/Documentation/devicetree/bindings/serio/altera_ps2.txt b/Documentation/devicetree/bindings/serio/altera_ps2.txt
new file mode 100644
index 0000000..4d9eecc
--- /dev/null
+++ b/Documentation/devicetree/bindings/serio/altera_ps2.txt
@@ -0,0 +1,4 @@
+Altera UP PS/2 controller
+
+Required properties:
+- compatible : should be "ALTR,ps2-1.0".
diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
index 7998560..3fee88b 100644
--- a/drivers/input/serio/altera_ps2.c
+++ b/drivers/input/serio/altera_ps2.c
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #define DRV_NAME "altera_ps2"
 
@@ -173,6 +174,16 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id altera_ps2_match[] = {
+	{ .compatible = "ALTR,ps2-1.0", },
+	{},
+}
+MODULE_DEVICE_TABLE(of, altera_ps2_match);
+#else /* CONFIG_OF */
+#define altera_ps2_match NULL
+#endif /* CONFIG_OF */
+
 /*
  * Our device driver structure
  */
@@ -182,6 +193,7 @@ static struct platform_driver altera_ps2_driver = {
 	.driver	= {
 		.name	= DRV_NAME,
 		.owner	= THIS_MODULE,
+		.of_match_table = altera_ps2_match,
 	},
 };
 
@@ -189,13 +201,12 @@ static int __init altera_ps2_init(void)
 {
 	return platform_driver_register(&altera_ps2_driver);
 }
+module_init(altera_ps2_init);
 
 static void __exit altera_ps2_exit(void)
 {
 	platform_driver_unregister(&altera_ps2_driver);
 }
-
-module_init(altera_ps2_init);
 module_exit(altera_ps2_exit);
 
 MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
-- 
1.7.4

Re: [PATCH v4] altera_ps2: Add devicetree support

[Dmitry Torokhov]

On Mon, Feb 14, 2011 at 10:06:42AM +0800, Thomas Chou wrote:
> From: Walter Goossens <waltergoossens@home.nl>
> 
> Signed-off-by: Walter Goossens <waltergoossens@home.nl>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Dmitry Torokhov <dtor@mail.ru>

> ---
> v2 use const and add compat version.
> v3 change compatible vendor to ALTR.
>     add dts binding doc.
> v4 condition module device table export for of.
> 
> Grant, please take it through your tree as Dmitry agreed.

*nod* Please.

> 
>  .../devicetree/bindings/serio/altera_ps2.txt       |    4 ++++
>  drivers/input/serio/altera_ps2.c                   |   15 +++++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/serio/altera_ps2.txt
> 
> diff --git a/Documentation/devicetree/bindings/serio/altera_ps2.txt b/Documentation/devicetree/bindings/serio/altera_ps2.txt
> new file mode 100644
> index 0000000..4d9eecc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serio/altera_ps2.txt
> @@ -0,0 +1,4 @@
> +Altera UP PS/2 controller
> +
> +Required properties:
> +- compatible : should be "ALTR,ps2-1.0".
> diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
> index 7998560..3fee88b 100644
> --- a/drivers/input/serio/altera_ps2.c
> +++ b/drivers/input/serio/altera_ps2.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  
>  #define DRV_NAME "altera_ps2"
>  
> @@ -173,6 +174,16 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id altera_ps2_match[] = {
> +	{ .compatible = "ALTR,ps2-1.0", },
> +	{},
> +}
> +MODULE_DEVICE_TABLE(of, altera_ps2_match);
> +#else /* CONFIG_OF */
> +#define altera_ps2_match NULL
> +#endif /* CONFIG_OF */
> +
>  /*
>   * Our device driver structure
>   */
> @@ -182,6 +193,7 @@ static struct platform_driver altera_ps2_driver = {
>  	.driver	= {
>  		.name	= DRV_NAME,
>  		.owner	= THIS_MODULE,
> +		.of_match_table = altera_ps2_match,
>  	},
>  };
>  
> @@ -189,13 +201,12 @@ static int __init altera_ps2_init(void)
>  {
>  	return platform_driver_register(&altera_ps2_driver);
>  }
> +module_init(altera_ps2_init);
>  
>  static void __exit altera_ps2_exit(void)
>  {
>  	platform_driver_unregister(&altera_ps2_driver);
>  }
> -
> -module_init(altera_ps2_init);
>  module_exit(altera_ps2_exit);
>  
>  MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
> -- 
> 1.7.4
> 

-- 
Dmitry

Re: [PATCH v4] altera_ps2: Add devicetree support

[Grant Likely]

On Mon, Feb 14, 2011 at 10:06:42AM +0800, Thomas Chou wrote:
> From: Walter Goossens <waltergoossens@home.nl>
> 
> Signed-off-by: Walter Goossens <waltergoossens@home.nl>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

Merged, thanks,
g.

> ---
> v2 use const and add compat version.
> v3 change compatible vendor to ALTR.
>     add dts binding doc.
> v4 condition module device table export for of.
> 
> Grant, please take it through your tree as Dmitry agreed.
> 
>  .../devicetree/bindings/serio/altera_ps2.txt       |    4 ++++
>  drivers/input/serio/altera_ps2.c                   |   15 +++++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/serio/altera_ps2.txt
> 
> diff --git a/Documentation/devicetree/bindings/serio/altera_ps2.txt b/Documentation/devicetree/bindings/serio/altera_ps2.txt
> new file mode 100644
> index 0000000..4d9eecc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serio/altera_ps2.txt
> @@ -0,0 +1,4 @@
> +Altera UP PS/2 controller
> +
> +Required properties:
> +- compatible : should be "ALTR,ps2-1.0".
> diff --git a/drivers/input/serio/altera_ps2.c b/drivers/input/serio/altera_ps2.c
> index 7998560..3fee88b 100644
> --- a/drivers/input/serio/altera_ps2.c
> +++ b/drivers/input/serio/altera_ps2.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  
>  #define DRV_NAME "altera_ps2"
>  
> @@ -173,6 +174,16 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id altera_ps2_match[] = {
> +	{ .compatible = "ALTR,ps2-1.0", },
> +	{},
> +}
> +MODULE_DEVICE_TABLE(of, altera_ps2_match);
> +#else /* CONFIG_OF */
> +#define altera_ps2_match NULL
> +#endif /* CONFIG_OF */
> +
>  /*
>   * Our device driver structure
>   */
> @@ -182,6 +193,7 @@ static struct platform_driver altera_ps2_driver = {
>  	.driver	= {
>  		.name	= DRV_NAME,
>  		.owner	= THIS_MODULE,
> +		.of_match_table = altera_ps2_match,
>  	},
>  };
>  
> @@ -189,13 +201,12 @@ static int __init altera_ps2_init(void)
>  {
>  	return platform_driver_register(&altera_ps2_driver);
>  }
> +module_init(altera_ps2_init);
>  
>  static void __exit altera_ps2_exit(void)
>  {
>  	platform_driver_unregister(&altera_ps2_driver);
>  }
> -
> -module_init(altera_ps2_init);
>  module_exit(altera_ps2_exit);
>  
>  MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
> -- 
> 1.7.4
> 

Re: [PATCH v2] altera_ps2: Add devicetree support

[Dmitry Torokhov]

On Tue, Feb 01, 2011 at 09:31:21PM -0700, Grant Likely wrote:
> On Mon, Jan 24, 2011 at 01:58:13PM +0800, Thomas Chou wrote:
> > @@ -182,6 +189,7 @@ static struct platform_driver altera_ps2_driver = {
> >  	.driver	= {
> >  		.name	= DRV_NAME,
> >  		.owner	= THIS_MODULE,
> > +		.of_match_table = altera_ps2_match,
> 
> At the moment, this patch can only be applied against my
> devicetree/next branch since there is a #ifdef CONFIG_OF around
> of_match_table in mainline, and the patch to remove it is in my next
> branch.  I'm okay with taking it through my tree if Dmitry agrees.
> 

Actually I was going to ask you if you could do just that.

Thanks.

-- 
Dmitry

Re: [PATCH v2] altera_ps2: Add devicetree support

[Thomas Chou]

On 02/02/2011 12:31 PM, Grant Likely wrote:
>> +static const struct of_device_id altera_ps2_match[] = {
>> +	{ .compatible = "altr,ps2-1.0", },
>
> I thought I had seen 'altera' instead of an abbreviation being used in
> a previous patch.  I don't care much whether 'altr' or 'altera' is
> used, but I'd like to know that there is consensus from the Altera
> users so that all the drivers use the same prefix.
>

We had discussed on nios2-dev mailing list, and decided to use 'altr' as 
Walter suggested that it saves space.

About the documentation on dts binding, shall we have a single 
altera.txt to describe all Altera related binding, and opencores.txt to 
describe all OpenCores binding? Or separate file for each core in its 
driver class?

- Thomas

Re: [Nios2-dev] [PATCH v2] altera_ps2: Add devicetree support

[Tobias Klauser]

On Wed, 2 Feb 2011 19:48:58 +0800 Thomas Chou <thomas@wytron.com.tw> wrote:
> On 02/02/2011 12:31 PM, Grant Likely wrote:
> >> +static const struct of_device_id altera_ps2_match[] = {
> >> +	{ .compatible = "altr,ps2-1.0", },
> >
> > I thought I had seen 'altera' instead of an abbreviation being used in
> > a previous patch.  I don't care much whether 'altr' or 'altera' is
> > used, but I'd like to know that there is consensus from the Altera
> > users so that all the drivers use the same prefix.
> >
> 
> We had discussed on nios2-dev mailing list, and decided to use 'altr' as 
> Walter suggested that it saves space.
> 
> About the documentation on dts binding, shall we have a single 
> altera.txt to describe all Altera related binding, and opencores.txt to 
> describe all OpenCores binding? Or separate file for each core in its 
> driver class?

As I will submit the device tree support for the altera_uart driver
(and the altera_jtaguart driver sometime in the future) I was asking
myself the same question. I'd vote for the altera.txt solution, as we
could use that for documenting the uart drivers, other Altera component
specific drivers and also the Nios2 arch specific parameters in the
future.

Cheers
Tobias

Re: [PATCH v2] altera_ps2: Add devicetree support

[Grant Likely]

On Wed, Feb 02, 2011 at 07:48:58PM +0800, Thomas Chou wrote:
> On 02/02/2011 12:31 PM, Grant Likely wrote:
> >>+static const struct of_device_id altera_ps2_match[] = {
> >>+	{ .compatible = "altr,ps2-1.0", },
> >
> >I thought I had seen 'altera' instead of an abbreviation being used in
> >a previous patch.  I don't care much whether 'altr' or 'altera' is
> >used, but I'd like to know that there is consensus from the Altera
> >users so that all the drivers use the same prefix.
> >
> 
> We had discussed on nios2-dev mailing list, and decided to use
> 'altr' as Walter suggested that it saves space.
> 
> About the documentation on dts binding, shall we have a single
> altera.txt to describe all Altera related binding, and opencores.txt
> to describe all OpenCores binding? Or separate file for each core in
> its driver class?

Since I'm reorganizing the binding documentation to reflect subsystems
(bindings/spi, bindings/i2c, bindings/powerpc, etc) I'd prefer to see
a separate file for each type of core.

Eventually, I'd like to define a record format for documenting
bindings that can generate searchable and crosslinked output, but I've
not spent any time looking at that seriously yet.

g.

> 
> - Thomas

Re: [PATCH v2] altera_ps2: Add devicetree support

[Thomas Chou]

On 02/02/2011 11:38 PM, Grant Likely wrote:
> On Wed, Feb 02, 2011 at 07:48:58PM +0800, Thomas Chou wrote:
>> On 02/02/2011 12:31 PM, Grant Likely wrote:
>>>> +static const struct of_device_id altera_ps2_match[] = {
>>>> +	{ .compatible = "altr,ps2-1.0", },
>>>
>>> I thought I had seen 'altera' instead of an abbreviation being used in
>>> a previous patch.  I don't care much whether 'altr' or 'altera' is
>>> used, but I'd like to know that there is consensus from the Altera
>>> users so that all the drivers use the same prefix.
>>>
>>
>> We had discussed on nios2-dev mailing list, and decided to use
>> 'altr' as Walter suggested that it saves space.
>>
>> About the documentation on dts binding, shall we have a single
>> altera.txt to describe all Altera related binding, and opencores.txt
>> to describe all OpenCores binding? Or separate file for each core in
>> its driver class?
>
> Since I'm reorganizing the binding documentation to reflect subsystems
> (bindings/spi, bindings/i2c, bindings/powerpc, etc) I'd prefer to see
> a separate file for each type of core.

OK. We will use separate files. I will send an updated patch to include 
the dts binding.

As for nios2 cpu related binding, we shall use binding/nios2 dir. Right?

- Thomas

>
> Eventually, I'd like to define a record format for documenting
> bindings that can generate searchable and crosslinked output, but I've
> not spent any time looking at that seriously yet.
>
> g.
>
>>
>> - Thomas
>

Re: [PATCH v2] altera_ps2: Add devicetree support

[Grant Likely]

On Wed, Feb 02, 2011 at 07:48:58PM +0800, Thomas Chou wrote:
> On 02/02/2011 12:31 PM, Grant Likely wrote:
> >>+static const struct of_device_id altera_ps2_match[] = {
> >>+	{ .compatible = "altr,ps2-1.0", },
> >
> >I thought I had seen 'altera' instead of an abbreviation being used in
> >a previous patch.  I don't care much whether 'altr' or 'altera' is
> >used, but I'd like to know that there is consensus from the Altera
> >users so that all the drivers use the same prefix.
> >
> 
> We had discussed on nios2-dev mailing list, and decided to use
> 'altr' as Walter suggested that it saves space.

Is altr the stock ticker symbol?  The convention is to either use the
stock ticker in all uppercase (although the uppercase bit hasn't been
consistently applied), or to use the full name in lowercase.

g.

Re: [PATCH v2] altera_ps2: Add devicetree support

[Thomas Chou]

On 02/02/2011 11:39 PM, Grant Likely wrote:
> On Wed, Feb 02, 2011 at 07:48:58PM +0800, Thomas Chou wrote:
>> On 02/02/2011 12:31 PM, Grant Likely wrote:
>>>> +static const struct of_device_id altera_ps2_match[] = {
>>>> +	{ .compatible = "altr,ps2-1.0", },
>>>
>>> I thought I had seen 'altera' instead of an abbreviation being used in
>>> a previous patch.  I don't care much whether 'altr' or 'altera' is
>>> used, but I'd like to know that there is consensus from the Altera
>>> users so that all the drivers use the same prefix.
>>>
>>
>> We had discussed on nios2-dev mailing list, and decided to use
>> 'altr' as Walter suggested that it saves space.
>
> Is altr the stock ticker symbol?  The convention is to either use the
> stock ticker in all uppercase (although the uppercase bit hasn't been
> consistently applied), or to use the full name in lowercase.

Yes, it is the stock ticker symbol. Then we shall follow the convention, 
and change it to uppercase.

- Thomas

>
> g.
>
>

Re: [PATCH v2] altera_ps2: Add devicetree support

[Walter Goossens]

On 2/2/11 4:39 PM, Grant Likely wrote:
> On Wed, Feb 02, 2011 at 07:48:58PM +0800, Thomas Chou wrote:
>> On 02/02/2011 12:31 PM, Grant Likely wrote:
>>>> +static const struct of_device_id altera_ps2_match[] = {
>>>> +	{ .compatible = "altr,ps2-1.0", },
>>> I thought I had seen 'altera' instead of an abbreviation being used in
>>> a previous patch.  I don't care much whether 'altr' or 'altera' is
>>> used, but I'd like to know that there is consensus from the Altera
>>> users so that all the drivers use the same prefix.
>>>
>> We had discussed on nios2-dev mailing list, and decided to use
>> 'altr' as Walter suggested that it saves space.
> Is altr the stock ticker symbol?  The convention is to either use the
> stock ticker in all uppercase (although the uppercase bit hasn't been
> consistently applied), or to use the full name in lowercase.
>
> g.
>
>
Risking my limbs here by breaking in this late in the discussion... (I 
wasn't able to reply earlier) but where does it state it needs to be 
uppercase? I found a bunch of microblaze code which seems to use the 
lowercase xlnx and freescale seems happy with fsl. Unless I'm missing 
something obvious here I guess ALTR would actually be the first to use 
uppercase.
The only reference to uppercase I found in the ePAPR docs was chapter 
1.6 that talks about uppercase hex-characters as an OUI.
Not that I terribly mind either way, but I want to double-check before 
we go ahead and change all altera-related devicetree stuff to uppercase.

Greetz
Walter

Re: [PATCH v2] altera_ps2: Add devicetree support

[Mitch Bradley]

On 2/3/2011 12:27 PM, Walter Goossens wrote:
> On 2/2/11 4:39 PM, Grant Likely wrote:
>> On Wed, Feb 02, 2011 at 07:48:58PM +0800, Thomas Chou wrote:
>>> On 02/02/2011 12:31 PM, Grant Likely wrote:
>>>>> +static const struct of_device_id altera_ps2_match[] = {
>>>>> + { .compatible = "altr,ps2-1.0", },
>>>> I thought I had seen 'altera' instead of an abbreviation being used in
>>>> a previous patch. I don't care much whether 'altr' or 'altera' is
>>>> used, but I'd like to know that there is consensus from the Altera
>>>> users so that all the drivers use the same prefix.
>>>>
>>> We had discussed on nios2-dev mailing list, and decided to use
>>> 'altr' as Walter suggested that it saves space.
>> Is altr the stock ticker symbol? The convention is to either use the
>> stock ticker in all uppercase (although the uppercase bit hasn't been
>> consistently applied), or to use the full name in lowercase.
>>
>> g.
>>
>>
> Risking my limbs here by breaking in this late in the discussion... (I
> wasn't able to reply earlier) but where does it state it needs to be
> uppercase? I found a bunch of microblaze code which seems to use the
> lowercase xlnx and freescale seems happy with fsl. Unless I'm missing
> something obvious here I guess ALTR would actually be the first to use
> uppercase.
> The only reference to uppercase I found in the ePAPR docs was chapter
> 1.6 that talks about uppercase hex-characters as an OUI.
> Not that I terribly mind either way, but I want to double-check before
> we go ahead and change all altera-related devicetree stuff to uppercase.


The relevant text in IEEE 1275-1994 is in the description of the "name" 
property in Annex A.  If a node name begins with a sequence of from one 
to five uppercase letters followed by a comma, that means a stock symbol 
on some exchange whose names do not conflict with NYSE or NASDAQ.

A lower-case prefix is okay, but it does not necessarily mean that it is 
a ticker symbol.  So in some sense, a lower case prefix provides less 
protection against collisions than an upper case prefix, which comes 
from an externally-arbitrated name space.  Case is explicitly 
significant in node names.

In practice, the important thing is that names must not conflict.  Name 
collisions haven't been much of a problem so far.

>
> Greetz
> Walter
>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>

Re: [PATCH v2] altera_ps2: Add devicetree support

[Walter Goossens]

On 2/3/11 11:53 PM, Mitch Bradley wrote:
> On 2/3/2011 12:27 PM, Walter Goossens wrote:
>> On 2/2/11 4:39 PM, Grant Likely wrote:
>>> On Wed, Feb 02, 2011 at 07:48:58PM +0800, Thomas Chou wrote:
>>>> On 02/02/2011 12:31 PM, Grant Likely wrote:
>>>>>> +static const struct of_device_id altera_ps2_match[] = {
>>>>>> + { .compatible = "altr,ps2-1.0", },
>>>>> I thought I had seen 'altera' instead of an abbreviation being 
>>>>> used in
>>>>> a previous patch. I don't care much whether 'altr' or 'altera' is
>>>>> used, but I'd like to know that there is consensus from the Altera
>>>>> users so that all the drivers use the same prefix.
>>>>>
>>>> We had discussed on nios2-dev mailing list, and decided to use
>>>> 'altr' as Walter suggested that it saves space.
>>> Is altr the stock ticker symbol? The convention is to either use the
>>> stock ticker in all uppercase (although the uppercase bit hasn't been
>>> consistently applied), or to use the full name in lowercase.
>>>
>>> g.
>>>
>>>
>> Risking my limbs here by breaking in this late in the discussion... (I
>> wasn't able to reply earlier) but where does it state it needs to be
>> uppercase? I found a bunch of microblaze code which seems to use the
>> lowercase xlnx and freescale seems happy with fsl. Unless I'm missing
>> something obvious here I guess ALTR would actually be the first to use
>> uppercase.
>> The only reference to uppercase I found in the ePAPR docs was chapter
>> 1.6 that talks about uppercase hex-characters as an OUI.
>> Not that I terribly mind either way, but I want to double-check before
>> we go ahead and change all altera-related devicetree stuff to uppercase.
>
>
> The relevant text in IEEE 1275-1994 is in the description of the 
> "name" property in Annex A.  If a node name begins with a sequence of 
> from one to five uppercase letters followed by a comma, that means a 
> stock symbol on some exchange whose names do not conflict with NYSE or 
> NASDAQ.
>
> A lower-case prefix is okay, but it does not necessarily mean that it 
> is a ticker symbol.  So in some sense, a lower case prefix provides 
> less protection against collisions than an upper case prefix, which 
> comes from an externally-arbitrated name space.  Case is explicitly 
> significant in node names.
>
> In practice, the important thing is that names must not conflict.  
> Name collisions haven't been much of a problem so far.
>
Excellent!
Must have missed that one. Good reason. We'll change 'm!

Thanks
Walter
>>
>> Greetz
>> Walter
>>
>>
>> _______________________________________________
>> devicetree-discuss mailing list
>> devicetree-discuss@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/devicetree-discuss
>>
>