[PATCH] Nios2: Add Altera UART driver
Franck JULLIEN
franck.jullien at gmail.com
Sun Apr 3 10:44:34 EDT 2011
2011/4/3 Franck JULLIEN <franck.jullien at gmail.com>
>
>
> 2011/4/3 Franck JULLIEN <franck.jullien at gmail.com>
>
>> Hello Marc,
>>
>> 2011/4/3 Marc Kleine-Budde <mkl at pengutronix.de>
>>
>>> On 04/03/2011 12:25 PM, franck.jullien at gmail.com wrote:
>>> > From: Franck JULLIEN <franck.jullien at gmail.com>
>>>
>>> Please add your S-o-b to the driver. See Documentation/SubmittingPatches
>>> in a Linux-kernel tree.
>>>
>>>
>> Ok I will.
>>
>>
>>
>>> See comments inline.
>>> >
>>> > ---
>>> > drivers/serial/Kconfig | 5 ++
>>> > drivers/serial/Makefile | 25 +++++-----
>>> > drivers/serial/serial_altera.c | 97
>>> ++++++++++++++++++++++++++++++++++++++++
>>> > 3 files changed, 115 insertions(+), 12 deletions(-)
>>> > create mode 100644 drivers/serial/serial_altera.c
>>> >
>>> > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>>> > index ffd877a..9e05f4b 100644
>>> > --- a/drivers/serial/Kconfig
>>> > +++ b/drivers/serial/Kconfig
>>> > @@ -43,6 +43,11 @@ config DRIVER_SERIAL_BLACKFIN
>>> > default y
>>> > bool "Blackfin serial driver"
>>> >
>>> > +config DRIVER_SERIAL_ALTERA
>>> > + depends on NIOS2
>>> > + default y
>>> > + bool "Altera serial driver"
>>> > +
>>> > config DRIVER_SERIAL_NS16550
>>> > default n
>>> > bool "NS16550 serial driver"
>>> > diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
>>> > index 9f0e12b..8067b74 100644
>>> > --- a/drivers/serial/Makefile
>>> > +++ b/drivers/serial/Makefile
>>> > @@ -4,15 +4,16 @@
>>> > # serial_max3100.o
>>> > # serial_pl010.o
>>> > # serial_xuartlite.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_ARM_DCC) += arm_dcc.o
>>> > -obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_IMX) += serial_imx.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_STM378X) += stm-serial.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_ATMEL) += atmel.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_NETX) += serial_netx.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_LINUX_COMSOLE) += linux_console.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_MPC5XXX) += serial_mpc5xxx.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_BLACKFIN) += serial_blackfin.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_NS16550) += serial_ns16550.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_PL010) += serial_pl010.o
>>> > -obj-$(CONFIG_DRIVER_SERIAL_S3C24X0) += serial_s3c24x0.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_ARM_DCC) += arm_dcc.o
>>> > +obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_IMX) += serial_imx.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_STM378X) += stm-serial.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_ATMEL) += atmel.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_NETX) += serial_netx.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_LINUX_COMSOLE) += linux_console.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_MPC5XXX) += serial_mpc5xxx.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_BLACKFIN) += serial_blackfin.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_NS16550) += serial_ns16550.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_PL010) += serial_pl010.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_S3C24X0) += serial_s3c24x0.o
>>> > +obj-$(CONFIG_DRIVER_SERIAL_ALTERA) += serial_altera.o
>>>
>>> Why do you reformat the whole file? Please don't do that.
>>>
>>
>>
>> Because there is a mix between tabs and spaces before the "+= ....". I
>> think it's better to have spaces here in order
>> to maintain alignment no matter what the tab size is. Tell me if you're
>> agree, if not, I'll just put my new line at the bottom..... :)
>>
>>
>>>
>>> > diff --git a/drivers/serial/serial_altera.c
>>> b/drivers/serial/serial_altera.c
>>> > new file mode 100644
>>> > index 0000000..03e48d1
>>> > --- /dev/null
>>> > +++ b/drivers/serial/serial_altera.c
>>> > @@ -0,0 +1,97 @@
>>> > +/*
>>> > + * (C) Copyright 2011, Franck JULLIEN, <elec4fun at gmail.com>
>>> > + *
>>> > + * See file CREDITS for list of people who contributed to this
>>> > + * project.
>>> > + *
>>> > + * This program is free software; you can redistribute it and/or
>>> > + * modify it under the terms of the GNU General Public License as
>>> > + * published by the Free Software Foundation; either version 2 of
>>> > + * the License, or (at your option) any later version.
>>> > + *
>>> > + * This program is distributed in the hope that it will be useful,
>>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> > + * GNU General Public License for more details.
>>> > + *
>>> > + * You should have received a copy of the GNU General Public License
>>> > + * along with this program; if not, write to the Free Software
>>> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>> > + * MA 02111-1307 USA
>>> > + */
>>> > +
>>> > +#include <common.h>
>>> > +#include <driver.h>
>>> > +#include <init.h>
>>> > +#include <malloc.h>
>>> > +#include <asm/io.h>
>>> > +#include <asm/nios2-io.h>
>>> > +
>>> > +static int altera_serial_setbaudrate(struct console_device *cdev, int
>>> baudrate)
>>> > +{
>>> > + volatile struct nios_uart * uart = (struct nios_uart
>>> *)cdev->dev->map_base;
>>> ^
>>>
>>> please remove this space.
>>>
>>>
>> Damn, I used the checkpatch script though, should have seen it....
>>
>>
>>> no volatiles please.
>>>
>>>
>>
>> Yeah I know but last time I removed those volatile, the driver didn't work
>> anymore ?!? I've done a test after I read your reply and it
>> works without so I'll remove them.
>>
>>
> In Documentation/volatile-considered-harmful.txt, one can read:
>
> There are still a few rare situations where volatile makes sense in the
> kernel:
>
> - The above-mentioned accessor functions might use volatile on
> architectures where direct I/O memory access does work.
> Essentially,
> each accessor call becomes a little critical section on its own
> and
> ensures that the access happens as expected by the programmer.
>
> (......)
>
> - Pointers to data structures in coherent memory which might be
> modified
> by I/O devices can, sometimes, legitimately be volatile. A ring
> buffer
> used by a network adapter, where that adapter changes pointers to
> indicate which descriptors have been processed, is an example of
> this
> type of situation.
>
> So is it that bad to use volatile ?
>
>
Ok, never mind, if we use readw, readl,...... the volatile is implicit.
>
>
>
>>
>>
>>> > + unsigned div;
>>>
>>> > +
>>> > + div = (CPU_FREQ / baudrate) - 1;
>>> > + writel(div, &uart->divisor);
>>> > +
>>> > + return 0;
>>> > +}
>>> > +
>>> > +static void altera_serial_putc(struct console_device *cdev, char c)
>>> > +{
>>> > + volatile struct nios_uart *uart = (struct nios_uart
>>> *)cdev->dev->map_base;
>>> > +
>>> > + if (c == '\n')
>>> > + altera_serial_putc(cdev, '\r');
>>>
>>> Why this? Only the at91rm9200 driver does this and I don't know why....
>>>
>>>
>> Because I copied it from U-Boot :) Removed.
>>
>>
>>
>>> > +
>>> > + while ((readl(&uart->status) & NIOS_UART_TRDY) == 0);
>>>
>>> > + writel(c, &uart->txdata);
>>> > +}
>>> > +
>>> > +static int altera_serial_tstc(struct console_device *cdev)
>>> > +{
>>> > + volatile struct nios_uart *uart = (struct nios_uart
>>> *)cdev->dev->map_base;
>>>
>>> No volatiles
>>>
>>> > +
>>> > + return readl(&uart->status) & NIOS_UART_RRDY;
>>> > +}
>>> > +
>>> > +static int altera_serial_getc(struct console_device *cdev)
>>> > +{
>>> > + volatile struct nios_uart *uart = (struct nios_uart
>>> *)cdev->dev->map_base;
>>> dito
>>> > +
>>> > + while (altera_serial_tstc(cdev) == 0);
>>> please add a comment, or at least an empty line after while
>>>
>>>
>> Do you guys put the semi colon on the next line also ?
>>
>>
>>
>>> > + return readl(&uart->rxdata) & 0x000000FF;
>>>
>>> > +}
>>> > +
>>> > +static int altera_serial_probe(struct device_d *dev)
>>> > +{
>>> > + struct console_device *cdev;
>>> > +
>>> > + cdev = malloc(sizeof(struct console_device));
>>>
>>> malloc can fail, use xmalloc instead
>>>
>>>
>> Done.
>>
>>
>>> > + dev->type_data = cdev;
>>>
>>> > + cdev->dev = dev;
>>> > + cdev->f_caps = CONSOLE_STDIN | CONSOLE_STDOUT | CONSOLE_STDERR;
>>> > + cdev->tstc = altera_serial_tstc;
>>> > + cdev->putc = altera_serial_putc;
>>> > + cdev->getc = altera_serial_getc;
>>> > + cdev->setbrg = altera_serial_setbaudrate;
>>> > +
>>> > + console_register(cdev);
>>> > +
>>> > + return 0;
>>> > +}
>>> > +
>>> > +static struct driver_d altera_serial_driver = {
>>> > + .name = "altera_serial",
>>> ^^
>>> one space is enough
>>>
>>>
>> Copied from blackfin :) Patch it ?
>>
>>
>>
>>> > + .probe = altera_serial_probe,
>>>
>>> > +};
>>> > +
>>> > +static int altera_serial_init(void)
>>> > +{
>>> > + register_driver(&altera_serial_driver);
>>> > + return 0;
>>>
>>> I think return register_driver(); is preferred.
>>>
>>>
>>
>> Copied from blackfin :) Patch it ?
>>
>>
>>> > +}
>>> > +
>>> > +console_initcall(altera_serial_init);
>>> > +
>>>
>>> cheers, Marc
>>>
>>> --
>>> Pengutronix e.K. | Marc Kleine-Budde |
>>> Industrial Linux Solutions | Phone: +49-231-2826-924 |
>>> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
>>> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>>>
>>>
>> Do you want me to patch files each time I see a wrong coding style in the
>> code ? Because I'm so focused
>> on that since I'm working on the nios2 port (and since I display tabs and
>> white spaces in my favorite editor :))
>> that I can see coding style mistakes in a lot of files :) (for
>> example, leading spaces in serial_imx line 222, 228,
>> 245, 313,.... checkpatch gives 9 errors and 44 warnings on this file).
>>
>> Regards,
>> Franck.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/barebox/attachments/20110403/cf452934/attachment-0001.html>
More information about the barebox
mailing list