* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-03 9:51 ` Nicolas Ferre 0 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-03 9:51 UTC (permalink / raw) To: linux-arm-kernel, alan, linux-serial, devicetree-discuss Cc: Nicolas Ferre, linux-kernel 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@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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-03 9:51 ` Nicolas Ferre 0 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-03 9:51 UTC (permalink / raw) To: linux-arm-kernel, alan, linux-serial, devicetree-discuss Cc: linux-kernel, 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@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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-03 9:51 ` Nicolas Ferre 0 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-03 9:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] tty/serial: atmel_serial: add device tree support 2011-10-03 9:51 ` Nicolas Ferre @ 2011-10-04 1:58 ` Rob Herring -1 siblings, 0 replies; 19+ messages in thread From: Rob Herring @ 2011-10-04 1:58 UTC (permalink / raw) To: Nicolas Ferre Cc: linux-arm-kernel, alan, linux-serial, devicetree-discuss, linux-kernel 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@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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-04 1:58 ` Rob Herring 0 siblings, 0 replies; 19+ messages in thread From: Rob Herring @ 2011-10-04 1:58 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] tty/serial: atmel_serial: add device tree support 2011-10-04 1:58 ` Rob Herring (?) @ 2011-10-04 8:18 ` Nicolas Ferre -1 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-04 8:18 UTC (permalink / raw) To: Rob Herring, Grant Likely Cc: linux-kernel, devicetree-discuss, linux-serial, linux-arm-kernel, alan 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@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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-04 8:18 ` Nicolas Ferre 0 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-04 8:18 UTC (permalink / raw) To: Rob Herring, Grant Likely Cc: linux-arm-kernel, alan, linux-serial, devicetree-discuss, linux-kernel 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@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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-04 8:18 ` Nicolas Ferre 0 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-04 8:18 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] tty/serial: atmel_serial: add device tree support 2011-10-04 8:18 ` Nicolas Ferre @ 2011-10-04 13:08 ` Rob Herring -1 siblings, 0 replies; 19+ messages in thread From: Rob Herring @ 2011-10-04 13:08 UTC (permalink / raw) To: Nicolas Ferre Cc: Grant Likely, linux-arm-kernel, alan, linux-serial, devicetree-discuss, linux-kernel 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@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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-04 13:08 ` Rob Herring 0 siblings, 0 replies; 19+ messages in thread From: Rob Herring @ 2011-10-04 13:08 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] tty/serial: atmel_serial: add device tree support 2011-10-04 13:08 ` Rob Herring @ 2011-10-04 16:55 ` Grant Likely -1 siblings, 0 replies; 19+ messages in thread From: Grant Likely @ 2011-10-04 16:55 UTC (permalink / raw) To: Rob Herring Cc: Nicolas Ferre, linux-arm-kernel, alan, linux-serial, devicetree-discuss, linux-kernel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-04 16:55 ` Grant Likely 0 siblings, 0 replies; 19+ messages in thread From: Grant Likely @ 2011-10-04 16:55 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] tty/serial: atmel_serial: add device tree support 2011-10-04 16:55 ` Grant Likely @ 2011-10-11 16:04 ` Nicolas Ferre -1 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-11 16:04 UTC (permalink / raw) To: Grant Likely, Rob Herring Cc: linux-arm-kernel, alan, linux-serial, devicetree-discuss, linux-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-11 16:04 ` Nicolas Ferre 0 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-11 16:04 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] tty/serial: atmel_serial: add device tree support 2011-10-04 1:58 ` Rob Herring @ 2011-10-04 16:52 ` Grant Likely -1 siblings, 0 replies; 19+ messages in thread From: Grant Likely @ 2011-10-04 16:52 UTC (permalink / raw) To: Rob Herring Cc: Nicolas Ferre, linux-arm-kernel, alan, linux-serial, devicetree-discuss, linux-kernel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-04 16:52 ` Grant Likely 0 siblings, 0 replies; 19+ messages in thread From: Grant Likely @ 2011-10-04 16:52 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] tty/serial: atmel_serial: add device tree support 2011-10-04 16:52 ` Grant Likely (?) @ 2011-10-12 15:35 ` Nicolas Ferre -1 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-12 15:35 UTC (permalink / raw) To: Grant Likely Cc: devicetree-discuss, linux-kernel, linux-serial, linux-arm-kernel, alan 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-12 15:35 ` Nicolas Ferre 0 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-12 15:35 UTC (permalink / raw) To: Grant Likely Cc: Rob Herring, linux-arm-kernel, alan, linux-serial, devicetree-discuss, linux-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH] tty/serial: atmel_serial: add device tree support @ 2011-10-12 15:35 ` Nicolas Ferre 0 siblings, 0 replies; 19+ messages in thread From: Nicolas Ferre @ 2011-10-12 15:35 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2011-10-12 15:35 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-03 9:51 [RFC PATCH] tty/serial: atmel_serial: add device tree support Nicolas Ferre 2011-10-03 9:51 ` Nicolas Ferre 2011-10-03 9:51 ` Nicolas Ferre 2011-10-04 1:58 ` Rob Herring 2011-10-04 1:58 ` Rob Herring 2011-10-04 8:18 ` Nicolas Ferre 2011-10-04 8:18 ` Nicolas Ferre 2011-10-04 8:18 ` Nicolas Ferre 2011-10-04 13:08 ` Rob Herring 2011-10-04 13:08 ` Rob Herring 2011-10-04 16:55 ` Grant Likely 2011-10-04 16:55 ` Grant Likely 2011-10-11 16:04 ` Nicolas Ferre 2011-10-11 16:04 ` Nicolas Ferre 2011-10-04 16:52 ` Grant Likely 2011-10-04 16:52 ` Grant Likely 2011-10-12 15:35 ` Nicolas Ferre 2011-10-12 15:35 ` Nicolas Ferre 2011-10-12 15:35 ` Nicolas Ferre
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.