* [PATCH] Nios2: Add Altera UART driver @ 2011-04-03 10:25 franck.jullien 2011-04-03 12:29 ` Marc Kleine-Budde 0 siblings, 1 reply; 6+ messages in thread From: franck.jullien @ 2011-04-03 10:25 UTC (permalink / raw) To: barebox From: Franck JULLIEN <franck.jullien@gmail.com> --- 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 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; + 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'); + + 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; + + 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; + + while (altera_serial_tstc(cdev) == 0); + return readl(&uart->rxdata) & 0x000000FF; +} + +static int altera_serial_probe(struct device_d *dev) +{ + struct console_device *cdev; + + cdev = malloc(sizeof(struct console_device)); + 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", + .probe = altera_serial_probe, +}; + +static int altera_serial_init(void) +{ + register_driver(&altera_serial_driver); + return 0; +} + +console_initcall(altera_serial_init); + -- 1.7.0.4 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Nios2: Add Altera UART driver 2011-04-03 10:25 [PATCH] Nios2: Add Altera UART driver franck.jullien @ 2011-04-03 12:29 ` Marc Kleine-Budde 2011-04-03 14:02 ` Franck JULLIEN 0 siblings, 1 reply; 6+ messages in thread From: Marc Kleine-Budde @ 2011-04-03 12:29 UTC (permalink / raw) To: franck.jullien; +Cc: barebox [-- Attachment #1.1: Type: text/plain, Size: 6791 bytes --] 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. 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. > 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. no volatiles please. > + 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.... > + > + 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 > + 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 > + 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 > + .probe = altera_serial_probe, > +}; > + > +static int altera_serial_init(void) > +{ > + register_driver(&altera_serial_driver); > + return 0; I think return register_driver(); is preferred. > +} > + > +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 | [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 262 bytes --] [-- Attachment #2: Type: text/plain, Size: 149 bytes --] _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Nios2: Add Altera UART driver 2011-04-03 12:29 ` Marc Kleine-Budde @ 2011-04-03 14:02 ` Franck JULLIEN [not found] ` <BANLkTikgKx7yb265QoYT0pNCPesu0E7E8A@mail.gmail.com> 2011-04-03 17:58 ` Marc Kleine-Budde 0 siblings, 2 replies; 6+ messages in thread From: Franck JULLIEN @ 2011-04-03 14:02 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: barebox [-- Attachment #1.1: Type: text/plain, Size: 8445 bytes --] 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. > > + 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: 12226 bytes --] [-- Attachment #2: Type: text/plain, Size: 149 bytes --] _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <BANLkTikgKx7yb265QoYT0pNCPesu0E7E8A@mail.gmail.com>]
* Re: [PATCH] Nios2: Add Altera UART driver [not found] ` <BANLkTikgKx7yb265QoYT0pNCPesu0E7E8A@mail.gmail.com> @ 2011-04-03 14:44 ` Franck JULLIEN 0 siblings, 0 replies; 6+ messages in thread From: Franck JULLIEN @ 2011-04-03 14:44 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: barebox [-- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Nios2: Add Altera UART driver 2011-04-03 14:02 ` Franck JULLIEN [not found] ` <BANLkTikgKx7yb265QoYT0pNCPesu0E7E8A@mail.gmail.com> @ 2011-04-03 17:58 ` Marc Kleine-Budde 2011-04-04 7:35 ` Sascha Hauer 1 sibling, 1 reply; 6+ messages in thread From: Marc Kleine-Budde @ 2011-04-03 17:58 UTC (permalink / raw) To: Franck JULLIEN; +Cc: barebox [-- Attachment #1.1: Type: text/plain, Size: 8530 bytes --] 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. >>> + 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 ? see below > > > >>> + .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 ? dito > > >>> +} >>> + >>> +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). Just fixing the coding style because it's bad is usually not done, because "git blame" will tell you about the coding style fixer not about the coder of that line. The rule of thumb is: Don't introduce new code with bad style. If you touch code fixing the style in a separate patch before you bring in your changes is usually accepted. 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 | [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 262 bytes --] [-- Attachment #2: Type: text/plain, Size: 149 bytes --] _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Nios2: Add Altera UART driver 2011-04-03 17:58 ` Marc Kleine-Budde @ 2011-04-04 7:35 ` Sascha Hauer 0 siblings, 0 replies; 6+ messages in thread From: Sascha Hauer @ 2011-04-04 7:35 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-04-04 7:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-03 10:25 [PATCH] Nios2: Add Altera UART driver franck.jullien
2011-04-03 12:29 ` Marc Kleine-Budde
2011-04-03 14:02 ` Franck JULLIEN
[not found] ` <BANLkTikgKx7yb265QoYT0pNCPesu0E7E8A@mail.gmail.com>
2011-04-03 14:44 ` Franck JULLIEN
2011-04-03 17:58 ` Marc Kleine-Budde
2011-04-04 7:35 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox