* Re: [PATCH 3/4] tty: serial: altera_uart: Add devicetree support
2011-02-09 9:58 ` [PATCH 3/4] tty: serial: altera_uart: Add devicetree support Tobias Klauser
@ 2011-02-16 4:32 ` Grant Likely
2011-02-16 7:43 ` Tobias Klauser
2011-02-16 16:12 ` [PATHV v2] " Tobias Klauser
2011-02-18 10:35 ` [PATCH 3/4 v3] " Tobias Klauser
2 siblings, 1 reply; 29+ messages in thread
From: Grant Likely @ 2011-02-16 4:32 UTC (permalink / raw)
To: Tobias Klauser
Cc: Greg Kroah-Hartman, linux-serial, nios2-dev, devicetree-discuss,
linux-kernel
On Wed, Feb 09, 2011 at 10:58:13AM +0100, Tobias Klauser wrote:
> With the recent switch of the (currently still out-of-tree) Nios2 Linux
> port to devicetree we want to be able to retreive the resources and
> properties from dts.
>
> The old method to retreive resources and properties from platform data
> is still supported.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---
> .../devicetree/bindings/serial/altera_uart.txt | 7 +++
> drivers/tty/serial/altera_uart.c | 47 ++++++++++++++++++--
> 2 files changed, 50 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/serial/altera_uart.txt
>
> diff --git a/Documentation/devicetree/bindings/serial/altera_uart.txt b/Documentation/devicetree/bindings/serial/altera_uart.txt
> new file mode 100644
> index 0000000..2ab151c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/altera_uart.txt
> @@ -0,0 +1,7 @@
> +Altera UART
> +
> +Required properties:
> +- compatible : should be "ALTR,uart-1.0"
> +
> +Optional properties:
> +- clock-frequency : frequency of the clock input to the UART
> diff --git a/drivers/tty/serial/altera_uart.c b/drivers/tty/serial/altera_uart.c
> index 3a57352..5b46ca7 100644
> --- a/drivers/tty/serial/altera_uart.c
> +++ b/drivers/tty/serial/altera_uart.c
> @@ -24,6 +24,7 @@
> #include <linux/serial.h>
> #include <linux/serial_core.h>
> #include <linux/platform_device.h>
> +#include <linux/of.h>
> #include <linux/io.h>
> #include <linux/altera_uart.h>
>
> @@ -507,6 +508,34 @@ static struct uart_driver altera_uart_driver = {
> .cons = ALTERA_UART_CONSOLE,
> };
>
> +#ifdef CONFIG_OF
> +static int altera_uart_get_uartclk(struct platform_device *pdev,
> + struct uart_port *port)
> +{
> + const __be32 *clk = of_get_property(pdev->dev.of_node, "clock-frequency", NULL);
> +
> + if (!clk)
> + return -ENODEV;
> +
> + port->uartclk = be32_to_cpup(clk);
> +
> + return 0;
> +}
> +#else
> +static int altera_uart_get_uartclk(struct platform_device *pdev,
> + struct uart_port *port)
> +{
> + struct altera_uart_platform_uart *platp = pdev->dev.platform_data;
> +
> + if (!platp)
> + return -ENODEV;
> +
> + port->uartclk = platp->uartclk;
> +
> + return 0;
> +}
> +#endif /* CONFIG_OF */
This either/or situation isn't really desirable. The driver should be
written so that platform_data works regardless of whether or not
CONFIG_OF is set. So, if the of_node pointer is set, use it to get
the configuration, but fall back to platform data if it is NULL.
> +
> static int __devinit altera_uart_probe(struct platform_device *pdev)
> {
> struct altera_uart_platform_uart *platp = pdev->dev.platform_data;
> @@ -514,6 +543,7 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
> struct resource *res_mem;
> struct resource *res_irq;
> int i = pdev->id;
> + int ret;
>
> /* -1 emphasizes that the platform must have one port, no .N suffix */
> if (i == -1)
> @@ -538,6 +568,10 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
> else if (platp->irq)
> port->irq = platp->irq;
>
> + ret = altera_uart_get_uartclk(pdev, port);
> + if (ret)
> + return ret;
> +
> port->membase = ioremap(port->mapbase, ALTERA_UART_SIZE);
> if (!port->membase)
> return -ENOMEM;
> @@ -550,7 +584,6 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
> port->line = i;
> port->type = PORT_ALTERA_UART;
> port->iotype = SERIAL_IO_MEM;
> - port->uartclk = platp->uartclk;
> port->ops = &altera_uart_ops;
> port->flags = UPF_BOOT_AUTOCONF;
>
> @@ -573,13 +606,19 @@ static int __devexit altera_uart_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static struct of_device_id altera_uart_match[] = {
> + { .compatible = "altr,uart-1.0", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, altera_uart_match);
Need to protect the MODULE_DEVICE_TABLE with #ifdef/#endif. You don't
want to advertise device tree support when CONFIG_OF isn't selected.
> +
> static struct platform_driver altera_uart_platform_driver = {
> .probe = altera_uart_probe,
> .remove = __devexit_p(altera_uart_remove),
> .driver = {
> - .name = DRV_NAME,
> - .owner = THIS_MODULE,
> - .pm = NULL,
> + .name = DRV_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = altera_uart_match,
> },
> };
>
> --
> 1.7.0.4
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 3/4] tty: serial: altera_uart: Add devicetree support
2011-02-16 4:32 ` Grant Likely
@ 2011-02-16 7:43 ` Tobias Klauser
2011-02-16 12:07 ` Grant Likely
0 siblings, 1 reply; 29+ messages in thread
From: Tobias Klauser @ 2011-02-16 7:43 UTC (permalink / raw)
To: Grant Likely
Cc: Greg Kroah-Hartman, linux-serial, nios2-dev, devicetree-discuss,
linux-kernel
On 2011-02-16 at 05:32:01 +0100, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 09, 2011 at 10:58:13AM +0100, Tobias Klauser wrote:
> > With the recent switch of the (currently still out-of-tree) Nios2 Linux
> > port to devicetree we want to be able to retreive the resources and
> > properties from dts.
> >
> > The old method to retreive resources and properties from platform data
> > is still supported.
> >
> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > ---
> > .../devicetree/bindings/serial/altera_uart.txt | 7 +++
> > drivers/tty/serial/altera_uart.c | 47 ++++++++++++++++++--
> > 2 files changed, 50 insertions(+), 4 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/serial/altera_uart.txt
> >
> > diff --git a/Documentation/devicetree/bindings/serial/altera_uart.txt b/Documentation/devicetree/bindings/serial/altera_uart.txt
> > new file mode 100644
> > index 0000000..2ab151c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serial/altera_uart.txt
> > @@ -0,0 +1,7 @@
> > +Altera UART
> > +
> > +Required properties:
> > +- compatible : should be "ALTR,uart-1.0"
> > +
> > +Optional properties:
> > +- clock-frequency : frequency of the clock input to the UART
> > diff --git a/drivers/tty/serial/altera_uart.c b/drivers/tty/serial/altera_uart.c
> > index 3a57352..5b46ca7 100644
> > --- a/drivers/tty/serial/altera_uart.c
> > +++ b/drivers/tty/serial/altera_uart.c
> > @@ -24,6 +24,7 @@
> > #include <linux/serial.h>
> > #include <linux/serial_core.h>
> > #include <linux/platform_device.h>
> > +#include <linux/of.h>
> > #include <linux/io.h>
> > #include <linux/altera_uart.h>
> >
> > @@ -507,6 +508,34 @@ static struct uart_driver altera_uart_driver = {
> > .cons = ALTERA_UART_CONSOLE,
> > };
> >
> > +#ifdef CONFIG_OF
> > +static int altera_uart_get_uartclk(struct platform_device *pdev,
> > + struct uart_port *port)
> > +{
> > + const __be32 *clk = of_get_property(pdev->dev.of_node, "clock-frequency", NULL);
> > +
> > + if (!clk)
> > + return -ENODEV;
> > +
> > + port->uartclk = be32_to_cpup(clk);
> > +
> > + return 0;
> > +}
> > +#else
> > +static int altera_uart_get_uartclk(struct platform_device *pdev,
> > + struct uart_port *port)
> > +{
> > + struct altera_uart_platform_uart *platp = pdev->dev.platform_data;
> > +
> > + if (!platp)
> > + return -ENODEV;
> > +
> > + port->uartclk = platp->uartclk;
> > +
> > + return 0;
> > +}
> > +#endif /* CONFIG_OF */
>
> This either/or situation isn't really desirable. The driver should be
> written so that platform_data works regardless of whether or not
> CONFIG_OF is set. So, if the of_node pointer is set, use it to get
> the configuration, but fall back to platform data if it is NULL.
Agreed, I'll fix this. Though that case will most probably never happen,
we either use device tree data or platform data.
> > +
> > static int __devinit altera_uart_probe(struct platform_device *pdev)
> > {
> > struct altera_uart_platform_uart *platp = pdev->dev.platform_data;
> > @@ -514,6 +543,7 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
> > struct resource *res_mem;
> > struct resource *res_irq;
> > int i = pdev->id;
> > + int ret;
> >
> > /* -1 emphasizes that the platform must have one port, no .N suffix */
> > if (i == -1)
> > @@ -538,6 +568,10 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
> > else if (platp->irq)
> > port->irq = platp->irq;
> >
> > + ret = altera_uart_get_uartclk(pdev, port);
> > + if (ret)
> > + return ret;
> > +
> > port->membase = ioremap(port->mapbase, ALTERA_UART_SIZE);
> > if (!port->membase)
> > return -ENOMEM;
> > @@ -550,7 +584,6 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
> > port->line = i;
> > port->type = PORT_ALTERA_UART;
> > port->iotype = SERIAL_IO_MEM;
> > - port->uartclk = platp->uartclk;
> > port->ops = &altera_uart_ops;
> > port->flags = UPF_BOOT_AUTOCONF;
> >
> > @@ -573,13 +606,19 @@ static int __devexit altera_uart_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +static struct of_device_id altera_uart_match[] = {
> > + { .compatible = "altr,uart-1.0", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, altera_uart_match);
>
> Need to protect the MODULE_DEVICE_TABLE with #ifdef/#endif. You don't
> want to advertise device tree support when CONFIG_OF isn't selected.
Shall I put the #ifdef around the whole table and define it as NULL if
CONFIG_OF is not defined - like this:
#ifdef CONFIG_OF
static struct of_device_id altera_uart_match[] = {
{ .compatible = "altr,uart-1.0", },
{},
};
MODULE_DEVICE_TABLE(of, altera_uart_match);
#else
#define altera_uart_match NULL
#endif /* CONFIG_OF */
or will it be sufficient to just #ifdef the MODULE_DEVICE_TABLE:
static struct of_device_id altera_uart_match[] = {
{ .compatible = "altr,uart-1.0", },
{},
};
#ifdef CONFIG_OF
MODULE_DEVICE_TABLE(of, altera_uart_match);
#endif
> > +
> > static struct platform_driver altera_uart_platform_driver = {
> > .probe = altera_uart_probe,
> > .remove = __devexit_p(altera_uart_remove),
> > .driver = {
> > - .name = DRV_NAME,
> > - .owner = THIS_MODULE,
> > - .pm = NULL,
> > + .name = DRV_NAME,
> > + .owner = THIS_MODULE,
> > + .of_match_table = altera_uart_match,
> > },
> > };
> >
> > --
> > 1.7.0.4
> >
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 3/4] tty: serial: altera_uart: Add devicetree support
2011-02-16 7:43 ` Tobias Klauser
@ 2011-02-16 12:07 ` Grant Likely
0 siblings, 0 replies; 29+ messages in thread
From: Grant Likely @ 2011-02-16 12:07 UTC (permalink / raw)
To: Tobias Klauser
Cc: Greg Kroah-Hartman, linux-serial, nios2-dev, devicetree-discuss,
linux-kernel
On Wed, Feb 16, 2011 at 08:43:21AM +0100, Tobias Klauser wrote:
> On 2011-02-16 at 05:32:01 +0100, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Wed, Feb 09, 2011 at 10:58:13AM +0100, Tobias Klauser wrote:
> > > With the recent switch of the (currently still out-of-tree) Nios2 Linux
> > > port to devicetree we want to be able to retreive the resources and
> > > properties from dts.
> > >
> > > The old method to retreive resources and properties from platform data
> > > is still supported.
> > >
> > > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > > ---
[...]
> > > +static struct of_device_id altera_uart_match[] = {
> > > + { .compatible = "altr,uart-1.0", },
> > > + {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, altera_uart_match);
> >
> > Need to protect the MODULE_DEVICE_TABLE with #ifdef/#endif. You don't
> > want to advertise device tree support when CONFIG_OF isn't selected.
>
> Shall I put the #ifdef around the whole table and define it as NULL if
> CONFIG_OF is not defined - like this:
>
> #ifdef CONFIG_OF
> static struct of_device_id altera_uart_match[] = {
> { .compatible = "altr,uart-1.0", },
> {},
> };
> MODULE_DEVICE_TABLE(of, altera_uart_match);
> #else
> #define altera_uart_match NULL
> #endif /* CONFIG_OF */
>
> or will it be sufficient to just #ifdef the MODULE_DEVICE_TABLE:
>
> static struct of_device_id altera_uart_match[] = {
> { .compatible = "altr,uart-1.0", },
> {},
> };
> #ifdef CONFIG_OF
> MODULE_DEVICE_TABLE(of, altera_uart_match);
> #endif
Either is fine, but the first will have a smaller memory footprint
when CONFIG_OF is deselected.
g.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATHV v2] tty: serial: altera_uart: Add devicetree support
2011-02-09 9:58 ` [PATCH 3/4] tty: serial: altera_uart: Add devicetree support Tobias Klauser
2011-02-16 4:32 ` Grant Likely
@ 2011-02-16 16:12 ` Tobias Klauser
2011-02-17 2:24 ` [Nios2-dev] " Thomas Chou
2011-02-18 10:35 ` [PATCH 3/4 v3] " Tobias Klauser
2 siblings, 1 reply; 29+ messages in thread
From: Tobias Klauser @ 2011-02-16 16:12 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-serial
Cc: nios2-dev, devicetree-discuss, linux-kernel, Grant Likely
With the recent switch of the (currently still out-of-tree) Nios2 Linux
port to devicetree we want to be able to retreive the resources and
properties from dts.
The old method to retreive resources and properties from platform data
is still supported.
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
Thanks to Grant Likely for the review.
changes in v2:
- fall back to uartclk from platform data if the device tree property
is not available.
- Only include MODULE_DEVICE_TABLE if CONFIG_OF is set so we don't
advertise device tree support if CONFIG_OF isn't active.
- change vendor prefix in match table to be uppercase for consistency
with documentation
.../devicetree/bindings/serial/altera_uart.txt | 7 +++
drivers/tty/serial/altera_uart.c | 50 ++++++++++++++++++--
2 files changed, 53 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/serial/altera_uart.txt
diff --git a/Documentation/devicetree/bindings/serial/altera_uart.txt b/Documentation/devicetree/bindings/serial/altera_uart.txt
new file mode 100644
index 0000000..71cae3f
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/altera_uart.txt
@@ -0,0 +1,7 @@
+Altera UART
+
+Required properties:
+- compatible : should be "ALTR,uart-1.0"
+
+Optional properties:
+- clock-frequency : frequency of the clock input to the UART
diff --git a/drivers/tty/serial/altera_uart.c b/drivers/tty/serial/altera_uart.c
index 5e80977..4f0898c 100644
--- a/drivers/tty/serial/altera_uart.c
+++ b/drivers/tty/serial/altera_uart.c
@@ -24,6 +24,7 @@
#include <linux/serial.h>
#include <linux/serial_core.h>
#include <linux/platform_device.h>
+#include <linux/of.h>
#include <linux/io.h>
#include <linux/altera_uart.h>
@@ -484,6 +485,29 @@ static struct uart_driver altera_uart_driver = {
.cons = ALTERA_UART_CONSOLE,
};
+#ifdef CONFIG_OF
+static int altera_uart_get_of_uartclk(struct platform_device *pdev,
+ struct uart_port *port)
+{
+ int len;
+ const __be32 *clk;
+
+ clk = of_get_property(pdev->dev.of_node, "clock-frequency", &len);
+ if (!clk || len < sizeof(__be32))
+ return -ENODEV;
+
+ port->uartclk = be32_to_cpup(clk);
+
+ return 0;
+}
+#else
+static int altera_uart_get_of_uartclk(struct platform_device *pdev,
+ struct uart_port *port)
+{
+ return -ENODEV;
+}
+#endif /* CONFIG_OF */
+
static int __devinit altera_uart_probe(struct platform_device *pdev)
{
struct altera_uart_platform_uart *platp = pdev->dev.platform_data;
@@ -491,6 +515,7 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
struct resource *res_mem;
struct resource *res_irq;
int i = pdev->id;
+ int ret;
/* -1 emphasizes that the platform must have one port, no .N suffix */
if (i == -1)
@@ -515,6 +540,14 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
else if (platp->irq)
port->irq = platp->irq;
+ /* Try to get the uartclk from devicetree, fall back to platform data
+ * otherwise */
+ ret = altera_uart_get_of_uartclk(pdev, port);
+ if (ret && platp)
+ port->uartclk = platp->uartclk;
+ else if (ret)
+ return ret;
+
port->membase = ioremap(port->mapbase, ALTERA_UART_SIZE);
if (!port->membase)
return -ENOMEM;
@@ -527,7 +560,6 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
port->line = i;
port->type = PORT_ALTERA_UART;
port->iotype = SERIAL_IO_MEM;
- port->uartclk = platp->uartclk;
port->ops = &altera_uart_ops;
port->flags = UPF_BOOT_AUTOCONF;
@@ -550,13 +582,23 @@ static int __devexit altera_uart_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_OF
+static struct of_device_id altera_uart_match[] = {
+ { .compatible = "ALTR,uart-1.0", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, altera_uart_match);
+#else
+#define altera_uart_match NULL
+#endif /* CONFIG_OF */
+
static struct platform_driver altera_uart_platform_driver = {
.probe = altera_uart_probe,
.remove = __devexit_p(altera_uart_remove),
.driver = {
- .name = DRV_NAME,
- .owner = THIS_MODULE,
- .pm = NULL,
+ .name = DRV_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = altera_uart_match,
},
};
--
1.7.0.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Nios2-dev] [PATHV v2] tty: serial: altera_uart: Add devicetree support
2011-02-16 16:12 ` [PATHV v2] " Tobias Klauser
@ 2011-02-17 2:24 ` Thomas Chou
2011-02-17 7:48 ` Tobias Klauser
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Chou @ 2011-02-17 2:24 UTC (permalink / raw)
To: nios2-dev
Cc: Tobias Klauser, Greg Kroah-Hartman, linux-serial, Grant Likely,
devicetree-discuss, linux-kernel
Hi Tobias,
On 02/17/2011 12:12 AM, Tobias Klauser wrote:
> With the recent switch of the (currently still out-of-tree) Nios2 Linux
> port to devicetree we want to be able to retreive the resources and
> properties from dts.
>
> The old method to retreive resources and properties from platform data
> is still supported.
>
> Cc: Grant Likely<grant.likely@secretlab.ca>
> Signed-off-by: Tobias Klauser<tklauser@distanz.ch>
> ---
> Thanks to Grant Likely for the review.
>
> changes in v2:
> - fall back to uartclk from platform data if the device tree property
> is not available.
> - Only include MODULE_DEVICE_TABLE if CONFIG_OF is set so we don't
> advertise device tree support if CONFIG_OF isn't active.
> - change vendor prefix in match table to be uppercase for consistency
> with documentation
>
> .../devicetree/bindings/serial/altera_uart.txt | 7 +++
> drivers/tty/serial/altera_uart.c | 50 ++++++++++++++++++--
> 2 files changed, 53 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/serial/altera_uart.txt
>
> diff --git a/Documentation/devicetree/bindings/serial/altera_uart.txt b/Documentation/devicetree/bindings/serial/altera_uart.txt
> new file mode 100644
> index 0000000..71cae3f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/altera_uart.txt
> @@ -0,0 +1,7 @@
> +Altera UART
> +
> +Required properties:
> +- compatible : should be "ALTR,uart-1.0"
> +
> +Optional properties:
> +- clock-frequency : frequency of the clock input to the UART
> diff --git a/drivers/tty/serial/altera_uart.c b/drivers/tty/serial/altera_uart.c
> index 5e80977..4f0898c 100644
> --- a/drivers/tty/serial/altera_uart.c
> +++ b/drivers/tty/serial/altera_uart.c
> @@ -24,6 +24,7 @@
> #include<linux/serial.h>
> #include<linux/serial_core.h>
> #include<linux/platform_device.h>
> +#include<linux/of.h>
> #include<linux/io.h>
> #include<linux/altera_uart.h>
>
> @@ -484,6 +485,29 @@ static struct uart_driver altera_uart_driver = {
> .cons = ALTERA_UART_CONSOLE,
> };
>
> +#ifdef CONFIG_OF
> +static int altera_uart_get_of_uartclk(struct platform_device *pdev,
> + struct uart_port *port)
> +{
> + int len;
> + const __be32 *clk;
> +
> + clk = of_get_property(pdev->dev.of_node, "clock-frequency",&len);
> + if (!clk || len< sizeof(__be32))
> + return -ENODEV;
> +
> + port->uartclk = be32_to_cpup(clk);
> +
> + return 0;
> +}
> +#else
> +static int altera_uart_get_of_uartclk(struct platform_device *pdev,
> + struct uart_port *port)
> +{
> + return -ENODEV;
> +}
> +#endif /* CONFIG_OF */
> +
> static int __devinit altera_uart_probe(struct platform_device *pdev)
> {
> struct altera_uart_platform_uart *platp = pdev->dev.platform_data;
> @@ -491,6 +515,7 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
> struct resource *res_mem;
> struct resource *res_irq;
> int i = pdev->id;
> + int ret;
>
> /* -1 emphasizes that the platform must have one port, no .N suffix */
> if (i == -1)
> @@ -515,6 +540,14 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
> else if (platp->irq)
> port->irq = platp->irq;
>
> + /* Try to get the uartclk from devicetree, fall back to platform data
> + * otherwise */
Please follow multi-line comment style.
/*
* xxxx
* xxxx
*/
> + ret = altera_uart_get_of_uartclk(pdev, port);
> + if (ret&& platp)
> + port->uartclk = platp->uartclk;
> + else if (ret)
> + return ret;
> +
Better reverse the priority, with platform data checked first.
if (platp)
port->uartclk = platp->uartclk;
else {
ret = altera_uart_get_of_uartclk(pdev, port);
if (ret)
return ret;
}
> port->membase = ioremap(port->mapbase, ALTERA_UART_SIZE);
> if (!port->membase)
> return -ENOMEM;
> @@ -527,7 +560,6 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
> port->line = i;
> port->type = PORT_ALTERA_UART;
> port->iotype = SERIAL_IO_MEM;
> - port->uartclk = platp->uartclk;
> port->ops =&altera_uart_ops;
> port->flags = UPF_BOOT_AUTOCONF;
>
> @@ -550,13 +582,23 @@ static int __devexit altera_uart_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static struct of_device_id altera_uart_match[] = {
> + { .compatible = "ALTR,uart-1.0", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, altera_uart_match);
> +#else
> +#define altera_uart_match NULL
> +#endif /* CONFIG_OF */
> +
> static struct platform_driver altera_uart_platform_driver = {
> .probe = altera_uart_probe,
> .remove = __devexit_p(altera_uart_remove),
> .driver = {
> - .name = DRV_NAME,
> - .owner = THIS_MODULE,
> - .pm = NULL,
> + .name = DRV_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = altera_uart_match,
> },
> };
>
Regards,
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Nios2-dev] [PATHV v2] tty: serial: altera_uart: Add devicetree support
2011-02-17 2:24 ` [Nios2-dev] " Thomas Chou
@ 2011-02-17 7:48 ` Tobias Klauser
2011-02-18 1:28 ` Thomas Chou
0 siblings, 1 reply; 29+ messages in thread
From: Tobias Klauser @ 2011-02-17 7:48 UTC (permalink / raw)
To: Thomas Chou
Cc: nios2-dev, Greg Kroah-Hartman, linux-serial, Grant Likely,
devicetree-discuss, linux-kernel
Hi Thomas,
Thanks a lot for your comments.
On 2011-02-17 at 03:24:01 +0100, Thomas Chou <thomas@wytron.com.tw> wrote:
> On 02/17/2011 12:12 AM, Tobias Klauser wrote:
>> With the recent switch of the (currently still out-of-tree) Nios2 Linux
>> port to devicetree we want to be able to retreive the resources and
>> properties from dts.
>>
>> The old method to retreive resources and properties from platform data
>> is still supported.
>>
>> Cc: Grant Likely<grant.likely@secretlab.ca>
>> Signed-off-by: Tobias Klauser<tklauser@distanz.ch>
>> ---
>> Thanks to Grant Likely for the review.
>>
>> changes in v2:
>> - fall back to uartclk from platform data if the device tree property
>> is not available.
>> - Only include MODULE_DEVICE_TABLE if CONFIG_OF is set so we don't
>> advertise device tree support if CONFIG_OF isn't active.
>> - change vendor prefix in match table to be uppercase for consistency
>> with documentation
>>
>> .../devicetree/bindings/serial/altera_uart.txt | 7 +++
>> drivers/tty/serial/altera_uart.c | 50 ++++++++++++++++++--
>> 2 files changed, 53 insertions(+), 4 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/serial/altera_uart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/serial/altera_uart.txt b/Documentation/devicetree/bindings/serial/altera_uart.txt
>> new file mode 100644
>> index 0000000..71cae3f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serial/altera_uart.txt
>> @@ -0,0 +1,7 @@
>> +Altera UART
>> +
>> +Required properties:
>> +- compatible : should be "ALTR,uart-1.0"
>> +
>> +Optional properties:
>> +- clock-frequency : frequency of the clock input to the UART
>> diff --git a/drivers/tty/serial/altera_uart.c b/drivers/tty/serial/altera_uart.c
>> index 5e80977..4f0898c 100644
>> --- a/drivers/tty/serial/altera_uart.c
>> +++ b/drivers/tty/serial/altera_uart.c
>> @@ -24,6 +24,7 @@
>> #include<linux/serial.h>
>> #include<linux/serial_core.h>
>> #include<linux/platform_device.h>
>> +#include<linux/of.h>
>> #include<linux/io.h>
>> #include<linux/altera_uart.h>
>>
>> @@ -484,6 +485,29 @@ static struct uart_driver altera_uart_driver = {
>> .cons = ALTERA_UART_CONSOLE,
>> };
>>
>> +#ifdef CONFIG_OF
>> +static int altera_uart_get_of_uartclk(struct platform_device *pdev,
>> + struct uart_port *port)
>> +{
>> + int len;
>> + const __be32 *clk;
>> +
>> + clk = of_get_property(pdev->dev.of_node, "clock-frequency",&len);
>> + if (!clk || len< sizeof(__be32))
>> + return -ENODEV;
>> +
>> + port->uartclk = be32_to_cpup(clk);
>> +
>> + return 0;
>> +}
>> +#else
>> +static int altera_uart_get_of_uartclk(struct platform_device *pdev,
>> + struct uart_port *port)
>> +{
>> + return -ENODEV;
>> +}
>> +#endif /* CONFIG_OF */
>> +
>> static int __devinit altera_uart_probe(struct platform_device *pdev)
>> {
>> struct altera_uart_platform_uart *platp = pdev->dev.platform_data;
>> @@ -491,6 +515,7 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
>> struct resource *res_mem;
>> struct resource *res_irq;
>> int i = pdev->id;
>> + int ret;
>>
>> /* -1 emphasizes that the platform must have one port, no .N suffix */
>> if (i == -1)
>> @@ -515,6 +540,14 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
>> else if (platp->irq)
>> port->irq = platp->irq;
>>
>> + /* Try to get the uartclk from devicetree, fall back to platform data
>> + * otherwise */
>
> Please follow multi-line comment style.
>
> /*
> * xxxx
> * xxxx
> */
I will fix this.
>> + ret = altera_uart_get_of_uartclk(pdev, port);
>> + if (ret&& platp)
>> + port->uartclk = platp->uartclk;
>> + else if (ret)
>> + return ret;
>> +
>
> Better reverse the priority, with platform data checked first.
>
> if (platp)
> port->uartclk = platp->uartclk;
> else {
> ret = altera_uart_get_of_uartclk(pdev, port);
> if (ret)
> return ret;
> }
Do you have a specific reasoning for this? I thought it might make sense
to do it in the same order as with the resources above, but I have no
problem changing it to the way you suggest.
>
>> port->membase = ioremap(port->mapbase, ALTERA_UART_SIZE);
>> if (!port->membase)
>> return -ENOMEM;
>> @@ -527,7 +560,6 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
>> port->line = i;
>> port->type = PORT_ALTERA_UART;
>> port->iotype = SERIAL_IO_MEM;
>> - port->uartclk = platp->uartclk;
>> port->ops =&altera_uart_ops;
>> port->flags = UPF_BOOT_AUTOCONF;
>>
>> @@ -550,13 +582,23 @@ static int __devexit altera_uart_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_OF
>> +static struct of_device_id altera_uart_match[] = {
>> + { .compatible = "ALTR,uart-1.0", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, altera_uart_match);
>> +#else
>> +#define altera_uart_match NULL
>> +#endif /* CONFIG_OF */
>> +
>> static struct platform_driver altera_uart_platform_driver = {
>> .probe = altera_uart_probe,
>> .remove = __devexit_p(altera_uart_remove),
>> .driver = {
>> - .name = DRV_NAME,
>> - .owner = THIS_MODULE,
>> - .pm = NULL,
>> + .name = DRV_NAME,
>> + .owner = THIS_MODULE,
>> + .of_match_table = altera_uart_match,
>> },
>> };
Cheers
Tobias
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Nios2-dev] [PATHV v2] tty: serial: altera_uart: Add devicetree support
2011-02-17 7:48 ` Tobias Klauser
@ 2011-02-18 1:28 ` Thomas Chou
2011-02-18 8:08 ` Grant Likely
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Chou @ 2011-02-18 1:28 UTC (permalink / raw)
To: Tobias Klauser
Cc: nios2-dev, Greg Kroah-Hartman, linux-serial, Grant Likely,
devicetree-discuss, linux-kernel
On 02/17/2011 03:48 PM, Tobias Klauser wrote:
>>> + ret = altera_uart_get_of_uartclk(pdev, port);
>>> + if (ret&& platp)
>>> + port->uartclk = platp->uartclk;
>>> + else if (ret)
>>> + return ret;
>>> +
>>
>> Better reverse the priority, with platform data checked first.
>>
>> if (platp)
>> port->uartclk = platp->uartclk;
>> else {
>> ret = altera_uart_get_of_uartclk(pdev, port);
>> if (ret)
>> return ret;
>> }
>
> Do you have a specific reasoning for this? I thought it might make sense
> to do it in the same order as with the resources above, but I have no
> problem changing it to the way you suggest.
Not quite sure. But I see some drivers follow this order, and I just
followed, too.
- Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Nios2-dev] [PATHV v2] tty: serial: altera_uart: Add devicetree support
2011-02-18 1:28 ` Thomas Chou
@ 2011-02-18 8:08 ` Grant Likely
2011-02-18 8:15 ` Tobias Klauser
0 siblings, 1 reply; 29+ messages in thread
From: Grant Likely @ 2011-02-18 8:08 UTC (permalink / raw)
To: Thomas Chou
Cc: Tobias Klauser, nios2-dev, Greg Kroah-Hartman, linux-serial,
devicetree-discuss, linux-kernel
On Thu, Feb 17, 2011 at 6:28 PM, Thomas Chou <thomas@wytron.com.tw> wrote:
> On 02/17/2011 03:48 PM, Tobias Klauser wrote:
>>>>
>>>> + ret = altera_uart_get_of_uartclk(pdev, port);
>>>> + if (ret&& platp)
>>>> + port->uartclk = platp->uartclk;
>>>> + else if (ret)
>>>> + return ret;
>>>> +
>>>
>>> Better reverse the priority, with platform data checked first.
>>>
>>> if (platp)
>>> port->uartclk = platp->uartclk;
>>> else {
>>> ret = altera_uart_get_of_uartclk(pdev, port);
>>> if (ret)
>>> return ret;
>>> }
>>
>> Do you have a specific reasoning for this? I thought it might make sense
>> to do it in the same order as with the resources above, but I have no
>> problem changing it to the way you suggest.
>
> Not quite sure. But I see some drivers follow this order, and I just
> followed, too.
The reason to check for platform_data first is that if a device has
*both* platform data and a device node pointer, then more than likely
the platform_data is indented to override the device node data.
g.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Nios2-dev] [PATHV v2] tty: serial: altera_uart: Add devicetree support
2011-02-18 8:08 ` Grant Likely
@ 2011-02-18 8:15 ` Tobias Klauser
0 siblings, 0 replies; 29+ messages in thread
From: Tobias Klauser @ 2011-02-18 8:15 UTC (permalink / raw)
To: Grant Likely
Cc: Thomas Chou, nios2-dev, Greg Kroah-Hartman, linux-serial,
devicetree-discuss, linux-kernel
On 2011-02-18 at 09:08:53 +0100, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Feb 17, 2011 at 6:28 PM, Thomas Chou <thomas@wytron.com.tw> wrote:
> > On 02/17/2011 03:48 PM, Tobias Klauser wrote:
> >>>>
> >>>> + ret = altera_uart_get_of_uartclk(pdev, port);
> >>>> + if (ret&& platp)
> >>>> + port->uartclk = platp->uartclk;
> >>>> + else if (ret)
> >>>> + return ret;
> >>>> +
> >>>
> >>> Better reverse the priority, with platform data checked first.
> >>>
> >>> if (platp)
> >>> port->uartclk = platp->uartclk;
> >>> else {
> >>> ret = altera_uart_get_of_uartclk(pdev, port);
> >>> if (ret)
> >>> return ret;
> >>> }
> >>
> >> Do you have a specific reasoning for this? I thought it might make sense
> >> to do it in the same order as with the resources above, but I have no
> >> problem changing it to the way you suggest.
> >
> > Not quite sure. But I see some drivers follow this order, and I just
> > followed, too.
>
> The reason to check for platform_data first is that if a device has
> *both* platform data and a device node pointer, then more than likely
> the platform_data is indented to override the device node data.
Thanks Grant and Thomas for the explanation. I was just wondering. I'll
send an updated patch including the changes suggested by Thomas.
Tobias
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 3/4 v3] tty: serial: altera_uart: Add devicetree support
2011-02-09 9:58 ` [PATCH 3/4] tty: serial: altera_uart: Add devicetree support Tobias Klauser
2011-02-16 4:32 ` Grant Likely
2011-02-16 16:12 ` [PATHV v2] " Tobias Klauser
@ 2011-02-18 10:35 ` Tobias Klauser
2011-02-25 17:58 ` Greg KH
2 siblings, 1 reply; 29+ messages in thread
From: Tobias Klauser @ 2011-02-18 10:35 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-serial
Cc: nios2-dev, devicetree-discuss, linux-kernel, Grant Likely
With the recent switch of the (currently still out-of-tree) Nios2 Linux
port to devicetree we want to be able to retreive the resources and
properties from dts.
The old method to retreive resources and properties from platform data
is still supported.
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
Greg: This patch will have the same compile problem in your tree as the
altera_jtaguart patch has. As this patch depends on the previous ones of
this series it might be best to apply it once commit c9e358dfc
"driver-core: remove conditionals around devicetree pointers" from
Grant's tree is merged.
Thanks to Grant Likely and Thomas Chou for their comments.
v3:
- Change order for getting uartclk.
v2:
- fall back to uartclk from platform data if the device tree property
is not available.
- Only include MODULE_DEVICE_TABLE if CONFIG_OF is set so we don't
advertise device tree support if CONFIG_OF isn't active.
- change vendor prefix in match table to be uppercase for consistency
with documentation
.../devicetree/bindings/serial/altera_uart.txt | 7 +++
drivers/tty/serial/altera_uart.c | 51 ++++++++++++++++++--
2 files changed, 54 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/serial/altera_uart.txt
diff --git a/Documentation/devicetree/bindings/serial/altera_uart.txt b/Documentation/devicetree/bindings/serial/altera_uart.txt
new file mode 100644
index 0000000..71cae3f
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/altera_uart.txt
@@ -0,0 +1,7 @@
+Altera UART
+
+Required properties:
+- compatible : should be "ALTR,uart-1.0"
+
+Optional properties:
+- clock-frequency : frequency of the clock input to the UART
diff --git a/drivers/tty/serial/altera_uart.c b/drivers/tty/serial/altera_uart.c
index 5e80977..6a1ebd9 100644
--- a/drivers/tty/serial/altera_uart.c
+++ b/drivers/tty/serial/altera_uart.c
@@ -24,6 +24,7 @@
#include <linux/serial.h>
#include <linux/serial_core.h>
#include <linux/platform_device.h>
+#include <linux/of.h>
#include <linux/io.h>
#include <linux/altera_uart.h>
@@ -484,6 +485,29 @@ static struct uart_driver altera_uart_driver = {
.cons = ALTERA_UART_CONSOLE,
};
+#ifdef CONFIG_OF
+static int altera_uart_get_of_uartclk(struct platform_device *pdev,
+ struct uart_port *port)
+{
+ int len;
+ const __be32 *clk;
+
+ clk = of_get_property(pdev->dev.of_node, "clock-frequency", &len);
+ if (!clk || len < sizeof(__be32))
+ return -ENODEV;
+
+ port->uartclk = be32_to_cpup(clk);
+
+ return 0;
+}
+#else
+static int altera_uart_get_of_uartclk(struct platform_device *pdev,
+ struct uart_port *port)
+{
+ return -ENODEV;
+}
+#endif /* CONFIG_OF */
+
static int __devinit altera_uart_probe(struct platform_device *pdev)
{
struct altera_uart_platform_uart *platp = pdev->dev.platform_data;
@@ -491,6 +515,7 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
struct resource *res_mem;
struct resource *res_irq;
int i = pdev->id;
+ int ret;
/* -1 emphasizes that the platform must have one port, no .N suffix */
if (i == -1)
@@ -515,6 +540,15 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
else if (platp->irq)
port->irq = platp->irq;
+ /* Check platform data first so we can override device node data */
+ if (platp)
+ port->uartclk = platp->uartclk;
+ else {
+ ret = altera_uart_get_of_uartclk(pdev, port);
+ if (ret)
+ return ret;
+ }
+
port->membase = ioremap(port->mapbase, ALTERA_UART_SIZE);
if (!port->membase)
return -ENOMEM;
@@ -527,7 +561,6 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
port->line = i;
port->type = PORT_ALTERA_UART;
port->iotype = SERIAL_IO_MEM;
- port->uartclk = platp->uartclk;
port->ops = &altera_uart_ops;
port->flags = UPF_BOOT_AUTOCONF;
@@ -550,13 +583,23 @@ static int __devexit altera_uart_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_OF
+static struct of_device_id altera_uart_match[] = {
+ { .compatible = "ALTR,uart-1.0", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, altera_uart_match);
+#else
+#define altera_uart_match NULL
+#endif /* CONFIG_OF */
+
static struct platform_driver altera_uart_platform_driver = {
.probe = altera_uart_probe,
.remove = __devexit_p(altera_uart_remove),
.driver = {
- .name = DRV_NAME,
- .owner = THIS_MODULE,
- .pm = NULL,
+ .name = DRV_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = altera_uart_match,
},
};
--
1.7.0.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 3/4 v3] tty: serial: altera_uart: Add devicetree support
2011-02-18 10:35 ` [PATCH 3/4 v3] " Tobias Klauser
@ 2011-02-25 17:58 ` Greg KH
[not found] ` <20110225175856.GA17072-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 29+ messages in thread
From: Greg KH @ 2011-02-25 17:58 UTC (permalink / raw)
To: Tobias Klauser
Cc: Greg Kroah-Hartman, linux-serial, nios2-dev, devicetree-discuss,
linux-kernel, Grant Likely
On Fri, Feb 18, 2011 at 11:35:32AM +0100, Tobias Klauser wrote:
> With the recent switch of the (currently still out-of-tree) Nios2 Linux
> port to devicetree we want to be able to retreive the resources and
> properties from dts.
>
> The old method to retreive resources and properties from platform data
> is still supported.
>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---
>
> Greg: This patch will have the same compile problem in your tree as the
> altera_jtaguart patch has. As this patch depends on the previous ones of
> this series it might be best to apply it once commit c9e358dfc
> "driver-core: remove conditionals around devicetree pointers" from
> Grant's tree is merged.
Ok, I can't take this then, Grant, can you?
Please feel free to add:
Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
to it.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 29+ messages in thread