lkml.org 
[lkml]   [2011]   [Oct]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] tty/serial: atmel_serial: add device tree support
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


\
 
 \ /
  Last update: 2011-10-04 15:11    [W:0.148 / U:0.536 seconds]
©2003-2014 Jasper Spaans. Advertise on this site