Re: [PATCH] Nios2: Add Altera UART driver

[Franck JULLIEN <franck.jullien@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 10134 bytes --]

2011/4/3 Franck JULLIEN <franck.jullien@gmail.com>

>
>
> 2011/4/3 Franck JULLIEN <franck.jullien@gmail.com>
>
>> Hello Marc,
>>
>> 2011/4/3 Marc Kleine-Budde <mkl@pengutronix.de>
>>
>>> On 04/03/2011 12:25 PM, franck.jullien@gmail.com wrote:
>>> > From: Franck JULLIEN <franck.jullien@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@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.
>>
>
>

[-- Attachment #1.2: Type: text/html, Size: 14622 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox