Hello Marc,<br><br><div class="gmail_quote">2011/4/3 Marc Kleine-Budde <span dir="ltr"><<a href="mailto:mkl@pengutronix.de">mkl@pengutronix.de</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">On 04/03/2011 12:25 PM, <a href="mailto:franck.jullien@gmail.com">franck.jullien@gmail.com</a> wrote:<br>
> From: Franck JULLIEN <<a href="mailto:franck.jullien@gmail.com">franck.jullien@gmail.com</a>><br>
<br>
</div>Please add your S-o-b to the driver. See Documentation/SubmittingPatches<br>
in a Linux-kernel tree.<br>
<br></blockquote><div><br></div><div>Ok I will. </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
See comments inline.<br>
<div><div></div><div class="h5">><br>
> ---<br>
> drivers/serial/Kconfig | 5 ++<br>
> drivers/serial/Makefile | 25 +++++-----<br>
> drivers/serial/serial_altera.c | 97 ++++++++++++++++++++++++++++++++++++++++<br>
> 3 files changed, 115 insertions(+), 12 deletions(-)<br>
> create mode 100644 drivers/serial/serial_altera.c<br>
><br>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig<br>
> index ffd877a..9e05f4b 100644<br>
> --- a/drivers/serial/Kconfig<br>
> +++ b/drivers/serial/Kconfig<br>
> @@ -43,6 +43,11 @@ config DRIVER_SERIAL_BLACKFIN<br>
> default y<br>
> bool "Blackfin serial driver"<br>
><br>
> +config DRIVER_SERIAL_ALTERA<br>
> + depends on NIOS2<br>
> + default y<br>
> + bool "Altera serial driver"<br>
> +<br>
> config DRIVER_SERIAL_NS16550<br>
> default n<br>
> bool "NS16550 serial driver"<br>
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile<br>
> index 9f0e12b..8067b74 100644<br>
> --- a/drivers/serial/Makefile<br>
> +++ b/drivers/serial/Makefile<br>
> @@ -4,15 +4,16 @@<br>
> # serial_max3100.o<br>
> # serial_pl010.o<br>
> # serial_xuartlite.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_ARM_DCC) += arm_dcc.o<br>
> -obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_IMX) += serial_imx.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_STM378X) += stm-serial.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_ATMEL) += atmel.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_NETX) += serial_netx.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_LINUX_COMSOLE) += linux_console.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_MPC5XXX) += serial_mpc5xxx.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_BLACKFIN) += serial_blackfin.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_NS16550) += serial_ns16550.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_PL010) += serial_pl010.o<br>
> -obj-$(CONFIG_DRIVER_SERIAL_S3C24X0) += serial_s3c24x0.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_ARM_DCC) += arm_dcc.o<br>
> +obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_IMX) += serial_imx.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_STM378X) += stm-serial.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_ATMEL) += atmel.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_NETX) += serial_netx.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_LINUX_COMSOLE) += linux_console.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_MPC5XXX) += serial_mpc5xxx.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_BLACKFIN) += serial_blackfin.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_NS16550) += serial_ns16550.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_PL010) += serial_pl010.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_S3C24X0) += serial_s3c24x0.o<br>
> +obj-$(CONFIG_DRIVER_SERIAL_ALTERA) += serial_altera.o<br>
<br>
</div></div>Why do you reformat the whole file? Please don't do that.<br></blockquote><div><br></div><div><br></div><div>Because there is a mix between tabs and spaces before the "+= ....". I think it's better to have spaces here in order</div>
<div>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..... :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5"><br>
> diff --git a/drivers/serial/serial_altera.c b/drivers/serial/serial_altera.c<br>
> new file mode 100644<br>
> index 0000000..03e48d1<br>
> --- /dev/null<br>
> +++ b/drivers/serial/serial_altera.c<br>
> @@ -0,0 +1,97 @@<br>
> +/*<br>
> + * (C) Copyright 2011, Franck JULLIEN, <<a href="mailto:elec4fun@gmail.com">elec4fun@gmail.com</a>><br>
> + *<br>
> + * See file CREDITS for list of people who contributed to this<br>
> + * project.<br>
> + *<br>
> + * This program is free software; you can redistribute it and/or<br>
> + * modify it under the terms of the GNU General Public License as<br>
> + * published by the Free Software Foundation; either version 2 of<br>
> + * the License, or (at your option) any later version.<br>
> + *<br>
> + * This program is distributed in the hope that it will be useful,<br>
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the<br>
> + * GNU General Public License for more details.<br>
> + *<br>
> + * You should have received a copy of the GNU General Public License<br>
> + * along with this program; if not, write to the Free Software<br>
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,<br>
> + * MA 02111-1307 USA<br>
> + */<br>
> +<br>
> +#include <common.h><br>
> +#include <driver.h><br>
> +#include <init.h><br>
> +#include <malloc.h><br>
> +#include <asm/io.h><br>
> +#include <asm/nios2-io.h><br>
> +<br>
> +static int altera_serial_setbaudrate(struct console_device *cdev, int baudrate)<br>
> +{<br>
> + volatile struct nios_uart * uart = (struct nios_uart *)cdev->dev->map_base;<br>
</div></div> ^<br>
<br>
please remove this space.<br>
<br></blockquote><div><br></div><div>Damn, I used the checkpatch script though, should have seen it....</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
no volatiles please.<br>
<div class="im"><br></div></blockquote><div><br></div><div><br></div><div>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</div>
<div>works without so I'll remove them.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">
> + unsigned div;<br>
> +<br>
> + div = (CPU_FREQ / baudrate) - 1;<br>
> + writel(div, &uart->divisor);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static void altera_serial_putc(struct console_device *cdev, char c)<br>
> +{<br>
> + volatile struct nios_uart *uart = (struct nios_uart *)cdev->dev->map_base;<br>
> +<br>
> + if (c == '\n')<br>
> + altera_serial_putc(cdev, '\r');<br>
<br>
</div>Why this? Only the at91rm9200 driver does this and I don't know why....<br>
<div class="im"><br></div></blockquote><div><br></div><div>Because I copied it from U-Boot :) Removed.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">
> +<br>
> + while ((readl(&uart->status) & NIOS_UART_TRDY) == 0);<br>
> + writel(c, &uart->txdata);<br>
> +}<br>
> +<br>
> +static int altera_serial_tstc(struct console_device *cdev)<br>
> +{<br>
> + volatile struct nios_uart *uart = (struct nios_uart *)cdev->dev->map_base;<br>
<br>
</div>No volatiles<br>
<div class="im"><br>
> +<br>
> + return readl(&uart->status) & NIOS_UART_RRDY;<br>
> +}<br>
> +<br>
> +static int altera_serial_getc(struct console_device *cdev)<br>
> +{<br>
> + volatile struct nios_uart *uart = (struct nios_uart *)cdev->dev->map_base;<br>
</div>dito<br>
<div class="im">> +<br>
> + while (altera_serial_tstc(cdev) == 0);<br>
</div>please add a comment, or at least an empty line after while<br>
<div class="im"><br></div></blockquote><div><br></div><div>Do you guys put the semi colon on the next line also ?</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">
> + return readl(&uart->rxdata) & 0x000000FF;<br>
> +}<br>
> +<br>
> +static int altera_serial_probe(struct device_d *dev)<br>
> +{<br>
> + struct console_device *cdev;<br>
> +<br>
> + cdev = malloc(sizeof(struct console_device));<br>
<br>
</div>malloc can fail, use xmalloc instead<br>
<div class="im"><br></div></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">
> + dev->type_data = cdev;<br>
> + cdev->dev = dev;<br>
> + cdev->f_caps = CONSOLE_STDIN | CONSOLE_STDOUT | CONSOLE_STDERR;<br>
> + cdev->tstc = altera_serial_tstc;<br>
> + cdev->putc = altera_serial_putc;<br>
> + cdev->getc = altera_serial_getc;<br>
> + cdev->setbrg = altera_serial_setbaudrate;<br>
> +<br>
> + console_register(cdev);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static struct driver_d altera_serial_driver = {<br>
> + .name = "altera_serial",<br>
</div> ^^<br>
one space is enough<br>
<div class="im"><br></div></blockquote><div><br></div><div>Copied from blackfin :) Patch it ?</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">
> + .probe = altera_serial_probe,<br>
> +};<br>
> +<br>
> +static int altera_serial_init(void)<br>
> +{<br>
> + register_driver(&altera_serial_driver);<br>
> + return 0;<br>
<br>
</div>I think return register_driver(); is preferred.<br>
<div class="im"><br></div></blockquote><div><br></div><div><br></div><div>Copied from blackfin :) Patch it ?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">
> +}<br>
> +<br>
> +console_initcall(altera_serial_init);<br>
> +<br>
<br>
</div>cheers, Marc<br>
<font color="#888888"><br>
--<br>
Pengutronix e.K. | Marc Kleine-Budde |<br>
Industrial Linux Solutions | Phone: +49-231-2826-924 |<br>
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |<br>
Amtsgericht Hildesheim, HRA 2686 | <a href="http://www.pengutronix.de" target="_blank">http://www.pengutronix.de</a> |<br>
<br>
</font></blockquote></div><br><div>Do you want me to patch files each time I see a wrong coding style in the code ? Because I'm so focused</div><div>on that since I'm working on the nios2 port (and since I display tabs and white spaces in my favorite editor :))</div>
<div>that I can see coding style mistakes in a lot of files :) (for example, leading spaces in serial_imx line 222, 228, </div><div>245, 313,.... checkpatch gives 9 errors and 44 warnings on this file).</div><div><br></div>
<div>Regards,</div><div>Franck.</div>