Re: [PATCH] Nios2: Add Altera UART driver

[Sascha Hauer <s.hauer@pengutronix.de>]

On Sun, Apr 03, 2011 at 07:58:51PM +0200, Marc Kleine-Budde wrote:
> On 04/03/2011 04:02 PM, Franck JULLIEN wrote:
> 
> [...]
> 
> >>> 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..... :)
> 
> The tab size is 8 by definition. I see that AMBA_PL011 does it
> different, but the other indent with tabs.
> 
> >>> 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.
> 
> You've found the answer yourself :) We don't do pointer derefs to iomem
> in C but use readl/writel instead.
> 
> > 
> > 
> > 
> >>> +     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 ?
> 
> Don't know.

Usually not.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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