[RFC PATCH] tty/serial: atmel_serial: add device tree support

[RFC PATCH] tty/serial: atmel_serial: add device tree support

[Nicolas Ferre]

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
Hi,

Here is a first attempt to add device tree support to atmel_serial driver.
RS485 data are not handled for the moment. My feeling is that they should be
added as a generic DT biding set.


 .../devicetree/bindings/tty/serial/atmel-usart.txt |   27 +++++++++
 drivers/tty/serial/atmel_serial.c                  |   56 +++++++++++++++++---
 2 files changed, 75 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/tty/serial/atmel-usart.txt

diff --git a/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt b/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
new file mode 100644
index 0000000..a49d9a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
@@ -0,0 +1,27 @@
+* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
+
+Required properties:
+- compatible: Should be "atmel,<chip>-usart"
+  The compatible <chip> indicated will be the first SoC to support an
+  additional mode or an USART new feature.
+- reg: Should contain registers location and length
+- interrupts: Should contain interrupt
+
+Optional properties:
+- atmel,use-dma-rx: use of PDC or DMA for receiving data
+- atmel,use-dma-tx: use of PDC or DMA for transmitting data
+
+<chip> compatible description:
+- at91rm9200:  legacy USART support
+- at91sam9260: generic USART implementation for SAM9 SoCs
+
+Example:
+
+	usart0: serial at fff8c000 {
+		compatible = "atmel,at91sam9260-usart";
+		reg = <0xfff8c000 0x4000>;
+		interrupts = <7>;
+		atmel,use-dma-rx;
+		atmel,use-dma-tx;
+	};
+
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 453cdb5..65f56c3 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -33,6 +33,8 @@
 #include <linux/sysrq.h>
 #include <linux/tty_flip.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/dma-mapping.h>
 #include <linux/atmel_pdc.h>
 #include <linux/atmel_serial.h>
@@ -162,6 +164,16 @@ static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
 static struct console atmel_console;
 #endif
 
+#if defined(CONFIG_OF)
+static const struct of_device_id atmel_serial_dt_ids[] = {
+	{ .compatible = "atmel,at91rm9200-usart" },
+	{ .compatible = "atmel,at91sam9260-usart" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, atmel_serial_dt_ids);
+#endif
+
 static inline struct atmel_uart_port *
 to_atmel_uart_port(struct uart_port *uart)
 {
@@ -1413,14 +1425,31 @@ static struct uart_ops atmel_pops = {
 static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
 				      struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	struct uart_port *port = &atmel_port->uart;
 	struct atmel_uart_data *pdata = pdev->dev.platform_data;
+	int ret;
+
+	if (np) {
+		ret = of_alias_get_id(np, "serial");
+		if (ret >= 0)
+			port->line = ret;
+		if (of_get_property(np, "atmel,use-dma-rx", NULL))
+			atmel_port->use_dma_rx	= 1;
+		if (of_get_property(np, "atmel,use-dma-tx", NULL))
+			atmel_port->use_dma_tx	= 1;
+	} else {
+		port->line = pdata->num;
+
+		atmel_port->use_dma_rx	= pdata->use_dma_rx;
+		atmel_port->use_dma_tx	= pdata->use_dma_tx;
+		atmel_port->rs485	= pdata->rs485;
+	}
 
 	port->iotype		= UPIO_MEM;
 	port->flags		= UPF_BOOT_AUTOCONF;
 	port->ops		= &atmel_pops;
 	port->fifosize		= 1;
-	port->line		= pdata->num;
 	port->dev		= &pdev->dev;
 	port->mapbase	= pdev->resource[0].start;
 	port->irq	= pdev->resource[1].start;
@@ -1430,7 +1459,7 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
 
 	memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
 
-	if (pdata->regs) {
+	if (pdata && pdata->regs) {
 		/* Already mapped by setup code */
 		port->membase = pdata->regs;
 	} else {
@@ -1447,10 +1476,6 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
 		/* only enable clock when USART is in use */
 	}
 
-	atmel_port->use_dma_rx	= pdata->use_dma_rx;
-	atmel_port->use_dma_tx	= pdata->use_dma_tx;
-	atmel_port->rs485	= pdata->rs485;
-
 	/* Use TXEMPTY for interrupt when rs485 else TXRDY or ENDTX|TXBUFE */
 	if (atmel_port->rs485.flags & SER_RS485_ENABLED)
 		atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
@@ -1710,13 +1735,27 @@ static int atmel_serial_resume(struct platform_device *pdev)
 static int __devinit atmel_serial_probe(struct platform_device *pdev)
 {
 	struct atmel_uart_port *port;
+	struct device_node *np = pdev->dev.of_node;
 	struct atmel_uart_data *pdata = pdev->dev.platform_data;
 	void *data;
 	int ret;
 
 	BUILD_BUG_ON(ATMEL_SERIAL_RINGSIZE & (ATMEL_SERIAL_RINGSIZE - 1));
 
-	port = &atmel_ports[pdata->num];
+	if (np) {
+		ret = of_alias_get_id(np, "serial");
+		if (ret < 0)
+			goto err;
+	} else {
+		if (pdata) {
+			ret = pdata->num;
+		} else {
+			ret = -ENODEV;
+			goto err;
+		}
+	}
+
+	port = &atmel_ports[ret];
 	port->backup_imr = 0;
 
 	atmel_init_port(port, pdev);
@@ -1763,7 +1802,7 @@ err_alloc_ring:
 		clk_put(port->clk);
 		port->clk = NULL;
 	}
-
+err:
 	return ret;
 }
 
@@ -1796,6 +1835,7 @@ static struct platform_driver atmel_serial_driver = {
 	.driver		= {
 		.name	= "atmel_usart",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(atmel_serial_dt_ids),
 	},
 };
 
-- 
1.7.3

[RFC PATCH] tty/serial: atmel_serial: add device tree support

[Rob Herring]

On 10/03/2011 04:51 AM, Nicolas Ferre wrote:
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> Hi,
> 
> Here is a first attempt to add device tree support to atmel_serial driver.
> RS485 data are not handled for the moment. My feeling is that they should be
> added as a generic DT biding set.

Feel free to propose something. :)

> 
> 
>  .../devicetree/bindings/tty/serial/atmel-usart.txt |   27 +++++++++
>  drivers/tty/serial/atmel_serial.c                  |   56 +++++++++++++++++---
>  2 files changed, 75 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
> 
> diff --git a/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt b/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
> new file mode 100644
> index 0000000..a49d9a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
> @@ -0,0 +1,27 @@
> +* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
> +
> +Required properties:
> +- compatible: Should be "atmel,<chip>-usart"
> +  The compatible <chip> indicated will be the first SoC to support an
> +  additional mode or an USART new feature.
> +- reg: Should contain registers location and length
> +- interrupts: Should contain interrupt
> +
> +Optional properties:
> +- atmel,use-dma-rx: use of PDC or DMA for receiving data
> +- atmel,use-dma-tx: use of PDC or DMA for transmitting data

Is this an internal DMA or separate DMA controller? If the latter, these
should be phandles to a DMA channel/request.

> +
> +<chip> compatible description:
> +- at91rm9200:  legacy USART support
> +- at91sam9260: generic USART implementation for SAM9 SoCs
> +
> +Example:
> +
> +	usart0: serial at fff8c000 {
> +		compatible = "atmel,at91sam9260-usart";
> +		reg = <0xfff8c000 0x4000>;
> +		interrupts = <7>;
> +		atmel,use-dma-rx;
> +		atmel,use-dma-tx;
> +	};
> +
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 453cdb5..65f56c3 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -33,6 +33,8 @@
>  #include <linux/sysrq.h>
>  #include <linux/tty_flip.h>
>  #include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/atmel_pdc.h>
>  #include <linux/atmel_serial.h>
> @@ -162,6 +164,16 @@ static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
>  static struct console atmel_console;
>  #endif
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id atmel_serial_dt_ids[] = {
> +	{ .compatible = "atmel,at91rm9200-usart" },
> +	{ .compatible = "atmel,at91sam9260-usart" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, atmel_serial_dt_ids);
> +#endif

This ifdef isn't necessary, but it really depends if long term this
driver will be built without OF (and you care about the extra data).

> +
>  static inline struct atmel_uart_port *
>  to_atmel_uart_port(struct uart_port *uart)
>  {
> @@ -1413,14 +1425,31 @@ static struct uart_ops atmel_pops = {
>  static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
>  				      struct platform_device *pdev)
>  {
> +	struct device_node *np = pdev->dev.of_node;
>  	struct uart_port *port = &atmel_port->uart;
>  	struct atmel_uart_data *pdata = pdev->dev.platform_data;
> +	int ret;
> +
> +	if (np) {
> +		ret = of_alias_get_id(np, "serial");
> +		if (ret >= 0)
> +			port->line = ret;
> +		if (of_get_property(np, "atmel,use-dma-rx", NULL))
> +			atmel_port->use_dma_rx	= 1;
> +		if (of_get_property(np, "atmel,use-dma-tx", NULL))
> +			atmel_port->use_dma_tx	= 1;
> +	} else {
> +		port->line = pdata->num;
> +
> +		atmel_port->use_dma_rx	= pdata->use_dma_rx;
> +		atmel_port->use_dma_tx	= pdata->use_dma_tx;
> +		atmel_port->rs485	= pdata->rs485;
> +	}
>  
>  	port->iotype		= UPIO_MEM;
>  	port->flags		= UPF_BOOT_AUTOCONF;
>  	port->ops		= &atmel_pops;
>  	port->fifosize		= 1;
> -	port->line		= pdata->num;
>  	port->dev		= &pdev->dev;
>  	port->mapbase	= pdev->resource[0].start;
>  	port->irq	= pdev->resource[1].start;
> @@ -1430,7 +1459,7 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
>  
>  	memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
>  
> -	if (pdata->regs) {
> +	if (pdata && pdata->regs) {
>  		/* Already mapped by setup code */
>  		port->membase = pdata->regs;
>  	} else {
> @@ -1447,10 +1476,6 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
>  		/* only enable clock when USART is in use */
>  	}
>  
> -	atmel_port->use_dma_rx	= pdata->use_dma_rx;
> -	atmel_port->use_dma_tx	= pdata->use_dma_tx;
> -	atmel_port->rs485	= pdata->rs485;
> -
>  	/* Use TXEMPTY for interrupt when rs485 else TXRDY or ENDTX|TXBUFE */
>  	if (atmel_port->rs485.flags & SER_RS485_ENABLED)
>  		atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
> @@ -1710,13 +1735,27 @@ static int atmel_serial_resume(struct platform_device *pdev)
>  static int __devinit atmel_serial_probe(struct platform_device *pdev)
>  {
>  	struct atmel_uart_port *port;
> +	struct device_node *np = pdev->dev.of_node;
>  	struct atmel_uart_data *pdata = pdev->dev.platform_data;
>  	void *data;
>  	int ret;
>  
>  	BUILD_BUG_ON(ATMEL_SERIAL_RINGSIZE & (ATMEL_SERIAL_RINGSIZE - 1));
>  
> -	port = &atmel_ports[pdata->num];
> +	if (np) {
> +		ret = of_alias_get_id(np, "serial");

I'll defer to Grant on this. There aren't any other drivers using this.

> +		if (ret < 0)
> +			goto err;
> +	} else {
> +		if (pdata) {
> +			ret = pdata->num;
> +		} else {
> +			ret = -ENODEV;
> +			goto err;
> +		}
> +	}

This would a bit more concise:

	} else if (pdata) {
		ret = pdata->num;
	} else {
		ret = -ENODEV;
		goto err;
	}


Rob

[RFC PATCH] tty/serial: atmel_serial: add device tree support

[Nicolas Ferre]

On 10/04/2011 03:58 AM, Rob Herring :
> On 10/03/2011 04:51 AM, Nicolas Ferre wrote:
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>> Hi,
>>
>> Here is a first attempt to add device tree support to atmel_serial driver.
>> RS485 data are not handled for the moment. My feeling is that they should be
>> added as a generic DT biding set.
> 
> Feel free to propose something. :)

Yes, I will have a look and practice my DT understanding on this...

>>  .../devicetree/bindings/tty/serial/atmel-usart.txt |   27 +++++++++
>>  drivers/tty/serial/atmel_serial.c                  |   56 +++++++++++++++++---
>>  2 files changed, 75 insertions(+), 8 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt b/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
>> new file mode 100644
>> index 0000000..a49d9a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
>> @@ -0,0 +1,27 @@
>> +* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
>> +
>> +Required properties:
>> +- compatible: Should be "atmel,<chip>-usart"
>> +  The compatible <chip> indicated will be the first SoC to support an
>> +  additional mode or an USART new feature.
>> +- reg: Should contain registers location and length
>> +- interrupts: Should contain interrupt
>> +
>> +Optional properties:
>> +- atmel,use-dma-rx: use of PDC or DMA for receiving data
>> +- atmel,use-dma-tx: use of PDC or DMA for transmitting data
> 
> Is this an internal DMA or separate DMA controller? If the latter, these
> should be phandles to a DMA channel/request.

Well, for now it relies on PDC (looks like an internal DMA). There is no
notion of channel/request so I think it is appropriate to keep it like this.

>> +<chip> compatible description:
>> +- at91rm9200:  legacy USART support
>> +- at91sam9260: generic USART implementation for SAM9 SoCs
>> +
>> +Example:
>> +
>> +	usart0: serial at fff8c000 {
>> +		compatible = "atmel,at91sam9260-usart";
>> +		reg = <0xfff8c000 0x4000>;
>> +		interrupts = <7>;
>> +		atmel,use-dma-rx;
>> +		atmel,use-dma-tx;
>> +	};
>> +
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 453cdb5..65f56c3 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -33,6 +33,8 @@
>>  #include <linux/sysrq.h>
>>  #include <linux/tty_flip.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/atmel_pdc.h>
>>  #include <linux/atmel_serial.h>
>> @@ -162,6 +164,16 @@ static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
>>  static struct console atmel_console;
>>  #endif
>>  
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id atmel_serial_dt_ids[] = {
>> +	{ .compatible = "atmel,at91rm9200-usart" },
>> +	{ .compatible = "atmel,at91sam9260-usart" },
>> +	{ /* sentinel */ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, atmel_serial_dt_ids);
>> +#endif
> 
> This ifdef isn't necessary, but it really depends if long term this
> driver will be built without OF (and you care about the extra data).

Yes, even in long term, this driver will be built without OF support. It
is true though that the extra data is not very big...

>>  static inline struct atmel_uart_port *
>>  to_atmel_uart_port(struct uart_port *uart)
>>  {
>> @@ -1413,14 +1425,31 @@ static struct uart_ops atmel_pops = {
>>  static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
>>  				      struct platform_device *pdev)
>>  {
>> +	struct device_node *np = pdev->dev.of_node;
>>  	struct uart_port *port = &atmel_port->uart;
>>  	struct atmel_uart_data *pdata = pdev->dev.platform_data;
>> +	int ret;
>> +
>> +	if (np) {
>> +		ret = of_alias_get_id(np, "serial");
>> +		if (ret >= 0)
>> +			port->line = ret;
>> +		if (of_get_property(np, "atmel,use-dma-rx", NULL))
>> +			atmel_port->use_dma_rx	= 1;
>> +		if (of_get_property(np, "atmel,use-dma-tx", NULL))
>> +			atmel_port->use_dma_tx	= 1;
>> +	} else {
>> +		port->line = pdata->num;
>> +
>> +		atmel_port->use_dma_rx	= pdata->use_dma_rx;
>> +		atmel_port->use_dma_tx	= pdata->use_dma_tx;
>> +		atmel_port->rs485	= pdata->rs485;
>> +	}
>>  
>>  	port->iotype		= UPIO_MEM;
>>  	port->flags		= UPF_BOOT_AUTOCONF;
>>  	port->ops		= &atmel_pops;
>>  	port->fifosize		= 1;
>> -	port->line		= pdata->num;
>>  	port->dev		= &pdev->dev;
>>  	port->mapbase	= pdev->resource[0].start;
>>  	port->irq	= pdev->resource[1].start;
>> @@ -1430,7 +1459,7 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
>>  
>>  	memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
>>  
>> -	if (pdata->regs) {
>> +	if (pdata && pdata->regs) {
>>  		/* Already mapped by setup code */
>>  		port->membase = pdata->regs;
>>  	} else {
>> @@ -1447,10 +1476,6 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
>>  		/* only enable clock when USART is in use */
>>  	}
>>  
>> -	atmel_port->use_dma_rx	= pdata->use_dma_rx;
>> -	atmel_port->use_dma_tx	= pdata->use_dma_tx;
>> -	atmel_port->rs485	= pdata->rs485;
>> -
>>  	/* Use TXEMPTY for interrupt when rs485 else TXRDY or ENDTX|TXBUFE */
>>  	if (atmel_port->rs485.flags & SER_RS485_ENABLED)
>>  		atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
>> @@ -1710,13 +1735,27 @@ static int atmel_serial_resume(struct platform_device *pdev)
>>  static int __devinit atmel_serial_probe(struct platform_device *pdev)
>>  {
>>  	struct atmel_uart_port *port;
>> +	struct device_node *np = pdev->dev.of_node;
>>  	struct atmel_uart_data *pdata = pdev->dev.platform_data;
>>  	void *data;
>>  	int ret;
>>  
>>  	BUILD_BUG_ON(ATMEL_SERIAL_RINGSIZE & (ATMEL_SERIAL_RINGSIZE - 1));
>>  
>> -	port = &atmel_ports[pdata->num];
>> +	if (np) {
>> +		ret = of_alias_get_id(np, "serial");
> 
> I'll defer to Grant on this. There aren't any other drivers using this.

I took the IMX serial driver as an example which is in devicetree/next
branch (tty/serial/imx.c).

Is it the proper way to use this?

>> +		if (ret < 0)
>> +			goto err;
>> +	} else {
>> +		if (pdata) {
>> +			ret = pdata->num;
>> +		} else {
>> +			ret = -ENODEV;
>> +			goto err;
>> +		}
>> +	}
> 
> This would a bit more concise:
> 
> 	} else if (pdata) {
> 		ret = pdata->num;
> 	} else {
> 		ret = -ENODEV;
> 		goto err;
> 	}

Ok.


Thanks for your review Rob.

Best regards,
-- 
Nicolas Ferre

[RFC PATCH] tty/serial: atmel_serial: add device tree support

[Rob Herring]

Nicolas,

On 10/04/2011 03:18 AM, Nicolas Ferre wrote:
> On 10/04/2011 03:58 AM, Rob Herring :
>> On 10/03/2011 04:51 AM, Nicolas Ferre wrote:
>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>> ---
>>> Hi,
>>>
>>> Here is a first attempt to add device tree support to atmel_serial driver.
>>> RS485 data are not handled for the moment. My feeling is that they should be
>>> added as a generic DT biding set.
>>
>> Feel free to propose something. :)
> 
> Yes, I will have a look and practice my DT understanding on this...
> 
>>>  .../devicetree/bindings/tty/serial/atmel-usart.txt |   27 +++++++++
>>>  drivers/tty/serial/atmel_serial.c                  |   56 +++++++++++++++++---
>>>  2 files changed, 75 insertions(+), 8 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt b/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
>>> new file mode 100644
>>> index 0000000..a49d9a1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/tty/serial/atmel-usart.txt
>>> @@ -0,0 +1,27 @@
>>> +* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
>>> +
>>> +Required properties:
>>> +- compatible: Should be "atmel,<chip>-usart"
>>> +  The compatible <chip> indicated will be the first SoC to support an
>>> +  additional mode or an USART new feature.
>>> +- reg: Should contain registers location and length
>>> +- interrupts: Should contain interrupt
>>> +
>>> +Optional properties:
>>> +- atmel,use-dma-rx: use of PDC or DMA for receiving data
>>> +- atmel,use-dma-tx: use of PDC or DMA for transmitting data
>>
>> Is this an internal DMA or separate DMA controller? If the latter, these
>> should be phandles to a DMA channel/request.
> 
> Well, for now it relies on PDC (looks like an internal DMA). There is no
> notion of channel/request so I think it is appropriate to keep it like this.
> 

Okay. Although, this is a bit of Linux creeping into DT. DT should
describe the h/w. You should really be describing whether the h/w
supports DMA or not rather than whether you want to use DMA or not.
Since DMA support is really tied to the SOC rather than board, you could
just use the compatible string to distinguish platforms that support DMA
or not. So what is determining this setting? Is it purely user choice?

>>> +<chip> compatible description:
>>> +- at91rm9200:  legacy USART support
>>> +- at91sam9260: generic USART implementation for SAM9 SoCs
>>> +
>>> +Example:
>>> +
>>> +	usart0: serial at fff8c000 {
>>> +		compatible = "atmel,at91sam9260-usart";
>>> +		reg = <0xfff8c000 0x4000>;
>>> +		interrupts = <7>;
>>> +		atmel,use-dma-rx;
>>> +		atmel,use-dma-tx;
>>> +	};
>>> +
>>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>>> index 453cdb5..65f56c3 100644
>>> --- a/drivers/tty/serial/atmel_serial.c
>>> +++ b/drivers/tty/serial/atmel_serial.c
>>> @@ -33,6 +33,8 @@
>>>  #include <linux/sysrq.h>
>>>  #include <linux/tty_flip.h>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>>  #include <linux/dma-mapping.h>
>>>  #include <linux/atmel_pdc.h>
>>>  #include <linux/atmel_serial.h>
>>> @@ -162,6 +164,16 @@ static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
>>>  static struct console atmel_console;
>>>  #endif
>>>  
>>> +#if defined(CONFIG_OF)
>>> +static const struct of_device_id atmel_serial_dt_ids[] = {
>>> +	{ .compatible = "atmel,at91rm9200-usart" },
>>> +	{ .compatible = "atmel,at91sam9260-usart" },
>>> +	{ /* sentinel */ }
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(of, atmel_serial_dt_ids);
>>> +#endif
>>
>> This ifdef isn't necessary, but it really depends if long term this
>> driver will be built without OF (and you care about the extra data).
> 
> Yes, even in long term, this driver will be built without OF support. It
> is true though that the extra data is not very big...
> 
>>>  static inline struct atmel_uart_port *
>>>  to_atmel_uart_port(struct uart_port *uart)
>>>  {
>>> @@ -1413,14 +1425,31 @@ static struct uart_ops atmel_pops = {
>>>  static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
>>>  				      struct platform_device *pdev)
>>>  {
>>> +	struct device_node *np = pdev->dev.of_node;
>>>  	struct uart_port *port = &atmel_port->uart;
>>>  	struct atmel_uart_data *pdata = pdev->dev.platform_data;
>>> +	int ret;
>>> +
>>> +	if (np) {
>>> +		ret = of_alias_get_id(np, "serial");
>>> +		if (ret >= 0)
>>> +			port->line = ret;
>>> +		if (of_get_property(np, "atmel,use-dma-rx", NULL))
>>> +			atmel_port->use_dma_rx	= 1;
>>> +		if (of_get_property(np, "atmel,use-dma-tx", NULL))
>>> +			atmel_port->use_dma_tx	= 1;
>>> +	} else {
>>> +		port->line = pdata->num;
>>> +
>>> +		atmel_port->use_dma_rx	= pdata->use_dma_rx;
>>> +		atmel_port->use_dma_tx	= pdata->use_dma_tx;
>>> +		atmel_port->rs485	= pdata->rs485;
>>> +	}
>>>  
>>>  	port->iotype		= UPIO_MEM;
>>>  	port->flags		= UPF_BOOT_AUTOCONF;
>>>  	port->ops		= &atmel_pops;
>>>  	port->fifosize		= 1;
>>> -	port->line		= pdata->num;
>>>  	port->dev		= &pdev->dev;
>>>  	port->mapbase	= pdev->resource[0].start;
>>>  	port->irq	= pdev->resource[1].start;
>>> @@ -1430,7 +1459,7 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
>>>  
>>>  	memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
>>>  
>>> -	if (pdata->regs) {
>>> +	if (pdata && pdata->regs) {
>>>  		/* Already mapped by setup code */
>>>  		port->membase = pdata->regs;
>>>  	} else {
>>> @@ -1447,10 +1476,6 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
>>>  		/* only enable clock when USART is in use */
>>>  	}
>>>  
>>> -	atmel_port->use_dma_rx	= pdata->use_dma_rx;
>>> -	atmel_port->use_dma_tx	= pdata->use_dma_tx;
>>> -	atmel_port->rs485	= pdata->rs485;
>>> -
>>>  	/* Use TXEMPTY for interrupt when rs485 else TXRDY or ENDTX|TXBUFE */
>>>  	if (atmel_port->rs485.flags & SER_RS485_ENABLED)
>>>  		atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
>>> @@ -1710,13 +1735,27 @@ static int atmel_serial_resume(struct platform_device *pdev)
>>>  static int __devinit atmel_serial_probe(struct platform_device *pdev)
>>>  {
>>>  	struct atmel_uart_port *port;
>>> +	struct device_node *np = pdev->dev.of_node;
>>>  	struct atmel_uart_data *pdata = pdev->dev.platform_data;
>>>  	void *data;
>>>  	int ret;
>>>  
>>>  	BUILD_BUG_ON(ATMEL_SERIAL_RINGSIZE & (ATMEL_SERIAL_RINGSIZE - 1));
>>>  
>>> -	port = &atmel_ports[pdata->num];
>>> +	if (np) {
>>> +		ret = of_alias_get_id(np, "serial");
>>
>> I'll defer to Grant on this. There aren't any other drivers using this.
> 
> I took the IMX serial driver as an example which is in devicetree/next
> branch (tty/serial/imx.c).
> 
> Is it the proper way to use this?
> 

Okay, I missed this. It should be fine.

Rob

[RFC PATCH] tty/serial: atmel_serial: add device tree support

[Grant Likely]

On Tue, Oct 04, 2011 at 08:08:27AM -0500, Rob Herring wrote:
> Nicolas,
> 
> On 10/04/2011 03:18 AM, Nicolas Ferre wrote:
> > On 10/04/2011 03:58 AM, Rob Herring :
> >> On 10/03/2011 04:51 AM, Nicolas Ferre wrote:
> >>> +Optional properties:
> >>> +- atmel,use-dma-rx: use of PDC or DMA for receiving data
> >>> +- atmel,use-dma-tx: use of PDC or DMA for transmitting data
> >>
> >> Is this an internal DMA or separate DMA controller? If the latter, these
> >> should be phandles to a DMA channel/request.
> > 
> > Well, for now it relies on PDC (looks like an internal DMA). There is no
> > notion of channel/request so I think it is appropriate to keep it like this.
> > 
> 
> Okay. Although, this is a bit of Linux creeping into DT. DT should
> describe the h/w. You should really be describing whether the h/w
> supports DMA or not rather than whether you want to use DMA or not.
> Since DMA support is really tied to the SOC rather than board, you could
> just use the compatible string to distinguish platforms that support DMA
> or not. So what is determining this setting? Is it purely user choice?

It is okay for a binding to still requires config properties like this
to state if the DMA can be used, even if compatible is enough to
distinquish.  However, you're right to bring it up.  It is a design
choice that should be made thoughtfully.

g.

[RFC PATCH] tty/serial: atmel_serial: add device tree support

[Nicolas Ferre]

On 10/04/2011 06:55 PM, Grant Likely :
> On Tue, Oct 04, 2011 at 08:08:27AM -0500, Rob Herring wrote:
>> Nicolas,
>>
>> On 10/04/2011 03:18 AM, Nicolas Ferre wrote:
>>> On 10/04/2011 03:58 AM, Rob Herring :
>>>> On 10/03/2011 04:51 AM, Nicolas Ferre wrote:
>>>>> +Optional properties:
>>>>> +- atmel,use-dma-rx: use of PDC or DMA for receiving data
>>>>> +- atmel,use-dma-tx: use of PDC or DMA for transmitting data
>>>>
>>>> Is this an internal DMA or separate DMA controller? If the latter, these
>>>> should be phandles to a DMA channel/request.
>>>
>>> Well, for now it relies on PDC (looks like an internal DMA). There is no
>>> notion of channel/request so I think it is appropriate to keep it like this.
>>>
>>
>> Okay. Although, this is a bit of Linux creeping into DT. DT should
>> describe the h/w. You should really be describing whether the h/w
>> supports DMA or not rather than whether you want to use DMA or not.
>> Since DMA support is really tied to the SOC rather than board, you could
>> just use the compatible string to distinguish platforms that support DMA
>> or not. So what is determining this setting? Is it purely user choice?
> 
> It is okay for a binding to still requires config properties like this
> to state if the DMA can be used, even if compatible is enough to
> distinquish.  However, you're right to bring it up.  It is a design
> choice that should be made thoughtfully.

Grant, Rob,

Thanks for your inputs.

I would like to keep this binding to allow the user to choose the way he
likes to use integrated DMA.

I plan to resend a patch series about this serial driver once I will be
able to propose a rs485 binding.

Cheers,
-- 
Nicolas Ferre

[RFC PATCH] tty/serial: atmel_serial: add device tree support

[Grant Likely]

On Mon, Oct 03, 2011 at 08:58:48PM -0500, Rob Herring wrote:
> On 10/03/2011 04:51 AM, Nicolas Ferre wrote:
> >  static int __devinit atmel_serial_probe(struct platform_device *pdev)
> >  {
> >  	struct atmel_uart_port *port;
> > +	struct device_node *np = pdev->dev.of_node;
> >  	struct atmel_uart_data *pdata = pdev->dev.platform_data;
> >  	void *data;
> >  	int ret;
> >  
> >  	BUILD_BUG_ON(ATMEL_SERIAL_RINGSIZE & (ATMEL_SERIAL_RINGSIZE - 1));
> >  
> > -	port = &atmel_ports[pdata->num];
> > +	if (np) {
> > +		ret = of_alias_get_id(np, "serial");
> > +		if (ret < 0)
> > +			goto err;
> 
> I'll defer to Grant on this. There aren't any other drivers using this.
> 

This is the correct thing to do.

One note however; I prefer driver to *not* require an alias to be
present.  By all means, use an alias if it is available, but the
driver should auto-enumerate if it is not.  There is a patch that
makes of_alias_get_id() do this for you, but it hasn't been mainlined
yet and it is still a bit in flux.  There will be something that takes
care of this for you, but be aware that the function name may change.

In the mean time, this patch is okay.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

g.

[RFC PATCH] tty/serial: atmel_serial: add device tree support

[Nicolas Ferre]

On 10/04/2011 06:52 PM, Grant Likely :
> On Mon, Oct 03, 2011 at 08:58:48PM -0500, Rob Herring wrote:
>> On 10/03/2011 04:51 AM, Nicolas Ferre wrote:
>>>  static int __devinit atmel_serial_probe(struct platform_device *pdev)
>>>  {
>>>  	struct atmel_uart_port *port;
>>> +	struct device_node *np = pdev->dev.of_node;
>>>  	struct atmel_uart_data *pdata = pdev->dev.platform_data;
>>>  	void *data;
>>>  	int ret;
>>>  
>>>  	BUILD_BUG_ON(ATMEL_SERIAL_RINGSIZE & (ATMEL_SERIAL_RINGSIZE - 1));
>>>  
>>> -	port = &atmel_ports[pdata->num];
>>> +	if (np) {
>>> +		ret = of_alias_get_id(np, "serial");
>>> +		if (ret < 0)
>>> +			goto err;
>>
>> I'll defer to Grant on this. There aren't any other drivers using this.
>>
> 
> This is the correct thing to do.
> 
> One note however; I prefer driver to *not* require an alias to be
> present.  By all means, use an alias if it is available, but the
> driver should auto-enumerate if it is not.  There is a patch that
> makes of_alias_get_id() do this for you, but it hasn't been mainlined
> yet and it is still a bit in flux.  There will be something that takes
> care of this for you, but be aware that the function name may change.
> 
> In the mean time, this patch is okay.
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

Grant,

I have reworked this atmel_serial device tree support so I did not add
your "Acked-by" to this new series. I repost it right now.
I hope this new implementation will be even better though ;-)

Bye,
-- 
Nicolas Ferre