Hi Thomas,
Thanks a lot for your comments.
On 2011-02-17 at 03:24:01 +0100, Thomas Chou <thomas@xxxxxxxxxxxxx> wrote:
> On 02/17/2011 12:12 AM, Tobias Klauser wrote:
>> With the recent switch of the (currently still out-of-tree) Nios2 Linux
>> port to devicetree we want to be able to retreive the resources and
>> properties from dts.
>>
>> The old method to retreive resources and properties from platform data
>> is still supported.
>>
>> Cc: Grant Likely<grant.likely@xxxxxxxxxxxx>
>> Signed-off-by: Tobias Klauser<tklauser@xxxxxxxxxx>
>> ---
>> Thanks to Grant Likely for the review.
>>
>> changes in v2:
>> - fall back to uartclk from platform data if the device tree property
>> is not available.
>> - Only include MODULE_DEVICE_TABLE if CONFIG_OF is set so we don't
>> advertise device tree support if CONFIG_OF isn't active.
>> - change vendor prefix in match table to be uppercase for consistency
>> with documentation
>>
>> .../devicetree/bindings/serial/altera_uart.txt | 7 +++
>> drivers/tty/serial/altera_uart.c | 50 ++++++++++++++++++--
>> 2 files changed, 53 insertions(+), 4 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/serial/altera_uart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/serial/altera_uart.txt b/Documentation/devicetree/bindings/serial/altera_uart.txt
>> new file mode 100644
>> index 0000000..71cae3f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serial/altera_uart.txt
>> @@ -0,0 +1,7 @@
>> +Altera UART
>> +
>> +Required properties:
>> +- compatible : should be "ALTR,uart-1.0"
>> +
>> +Optional properties:
>> +- clock-frequency : frequency of the clock input to the UART
>> diff --git a/drivers/tty/serial/altera_uart.c b/drivers/tty/serial/altera_uart.c
>> index 5e80977..4f0898c 100644
>> --- a/drivers/tty/serial/altera_uart.c
>> +++ b/drivers/tty/serial/altera_uart.c
>> @@ -24,6 +24,7 @@
>> #include<linux/serial.h>
>> #include<linux/serial_core.h>
>> #include<linux/platform_device.h>
>> +#include<linux/of.h>
>> #include<linux/io.h>
>> #include<linux/altera_uart.h>
>>
>> @@ -484,6 +485,29 @@ static struct uart_driver altera_uart_driver = {
>> .cons = ALTERA_UART_CONSOLE,
>> };
>>
>> +#ifdef CONFIG_OF
>> +static int altera_uart_get_of_uartclk(struct platform_device *pdev,
>> + struct uart_port *port)
>> +{
>> + int len;
>> + const __be32 *clk;
>> +
>> + clk = of_get_property(pdev->dev.of_node, "clock-frequency",&len);
>> + if (!clk || len< sizeof(__be32))
>> + return -ENODEV;
>> +
>> + port->uartclk = be32_to_cpup(clk);
>> +
>> + return 0;
>> +}
>> +#else
>> +static int altera_uart_get_of_uartclk(struct platform_device *pdev,
>> + struct uart_port *port)
>> +{
>> + return -ENODEV;
>> +}
>> +#endif /* CONFIG_OF */
>> +
>> static int __devinit altera_uart_probe(struct platform_device *pdev)
>> {
>> struct altera_uart_platform_uart *platp = pdev->dev.platform_data;
>> @@ -491,6 +515,7 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
>> struct resource *res_mem;
>> struct resource *res_irq;
>> int i = pdev->id;
>> + int ret;
>>
>> /* -1 emphasizes that the platform must have one port, no .N suffix */
>> if (i == -1)
>> @@ -515,6 +540,14 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
>> else if (platp->irq)
>> port->irq = platp->irq;
>>
>> + /* Try to get the uartclk from devicetree, fall back to platform data
>> + * otherwise */
>
> Please follow multi-line comment style.
>
> /*
> * xxxx
> * xxxx
> */
I will fix this.
>> + ret = altera_uart_get_of_uartclk(pdev, port);
>> + if (ret&& platp)
>> + port->uartclk = platp->uartclk;
>> + else if (ret)
>> + return ret;
>> +
>
> Better reverse the priority, with platform data checked first.
>
> if (platp)
> port->uartclk = platp->uartclk;
> else {
> ret = altera_uart_get_of_uartclk(pdev, port);
> if (ret)
> return ret;
> }
Do you have a specific reasoning for this? I thought it might make sense
to do it in the same order as with the resources above, but I have no
problem changing it to the way you suggest.
>
>> port->membase = ioremap(port->mapbase, ALTERA_UART_SIZE);
>> if (!port->membase)
>> return -ENOMEM;
>> @@ -527,7 +560,6 @@ static int __devinit altera_uart_probe(struct platform_device *pdev)
>> port->line = i;
>> port->type = PORT_ALTERA_UART;
>> port->iotype = SERIAL_IO_MEM;
>> - port->uartclk = platp->uartclk;
>> port->ops =&altera_uart_ops;
>> port->flags = UPF_BOOT_AUTOCONF;
>>
>> @@ -550,13 +582,23 @@ static int __devexit altera_uart_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_OF
>> +static struct of_device_id altera_uart_match[] = {
>> + { .compatible = "ALTR,uart-1.0", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, altera_uart_match);
>> +#else
>> +#define altera_uart_match NULL
>> +#endif /* CONFIG_OF */
>> +
>> static struct platform_driver altera_uart_platform_driver = {
>> .probe = altera_uart_probe,
>> .remove = __devexit_p(altera_uart_remove),
>> .driver = {
>> - .name = DRV_NAME,
>> - .owner = THIS_MODULE,
>> - .pm = NULL,
>> + .name = DRV_NAME,
>> + .owner = THIS_MODULE,
>> + .of_match_table = altera_uart_match,
>> },
>> };
Cheers
Tobias
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
