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