RE: [PATCH V2] tty/serial: add support for Xilinx PS UART

John Linn
2011-04-22T13:36:36Z

> -----Original Message-----
> From: Michal Simek [mailto:michal.simek@petalogix.com]
> Sent: Friday, April 22, 2011 3:11 AM
> To: John Linn
> Cc: linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org;
> alan@lxorguk.ukuu.org.uk; greg@kroah.com; grant.likely@secretlab.ca
> Subject: Re: [PATCH V2] tty/serial: add support for Xilinx PS UART
>
> John Linn wrote:
> > The Xilinx PS Uart is used on the new ARM based SoC. This
> > UART is not compatible with others such that a seperate
> > driver is required.
> >
> > Signed-off-by: John Linn <john.linn@xilinx.com>
> > ---
> >
> > V2 Changes
> >
> > Updated based on Alan Cox and Greg KH comments. Thanks for their
> input.
> > I hope I corrected everything as expected.
> >
> > 1. Now using dynamic device nodes so Doc*/devices.txt removed from
> patch.
> > 2. Other minor updates based on Alan's input.
> >
> > drivers/tty/serial/Kconfig | 13 +
> > drivers/tty/serial/Makefile | 1 +
> > drivers/tty/serial/xilinx_uartps.c | 1121
> ++++++++++++++++++++++++++++++++++++
> > include/linux/serial_core.h | 3 +
> > 4 files changed, 1138 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/tty/serial/xilinx_uartps.c
> >
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index 80484af..84876ec 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -1612,4 +1612,17 @@ config SERIAL_MXS_AUART_CONSOLE
> > help
> > Enable a MXS AUART port to be the system console.
> >
> > +config SERIAL_XILINX_PS_UART
> > + tristate "Xilinx PS UART support"
> > + select SERIAL_CORE
> > + help
> > + This driver supports the Xilinx PS UART port.
> > +
> > +config SERIAL_XILINX_PS_UART_CONSOLE
> > + bool "Xilinx PS UART console support"
> > + depends on SERIAL_XILINX_PS_UART=y
> > + select SERIAL_CORE_CONSOLE
> > + help
> > + Enable a Xilinx PS UART port to be the system console.
> > +
> > endmenu
> > diff --git a/drivers/tty/serial/Makefile
> b/drivers/tty/serial/Makefile
> > index fee0690..aafddf1 100644
> > --- a/drivers/tty/serial/Makefile
> > +++ b/drivers/tty/serial/Makefile
> > @@ -94,3 +94,4 @@ obj-$(CONFIG_SERIAL_IFX6X60) += ifx6x60.o
> > obj-$(CONFIG_SERIAL_PCH_UART) += pch_uart.o
> > obj-$(CONFIG_SERIAL_MSM_SMD) += msm_smd_tty.o
> > obj-$(CONFIG_SERIAL_MXS_AUART) += mxs-auart.o
> > +obj-$(CONFIG_SERIAL_XILINX_PS_UART) += xilinx_uartps.o
> > diff --git a/drivers/tty/serial/xilinx_uartps.c
> b/drivers/tty/serial/xilinx_uartps.c
> > new file mode 100644
> > index 0000000..814fb5b
> > --- /dev/null
> > +++ b/drivers/tty/serial/xilinx_uartps.c
> > @@ -0,0 +1,1121 @@
> > +/*
> > + * Xilinx PS UART driver
> > + *
> > + * 2011 (c) Xilinx Inc.
> > + *
> > + * 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.
> > + *
> > + */

<snip>

> > +#define XUARTPS_SR_RXEMPTY 0x00000002 /* RX FIFO empty */
> > +#define XUARTPS_SR_TXEMPTY 0x00000008 /* TX FIFO empty */
> > +#define XUARTPS_SR_TXFULL 0x00000010 /* TX FIFO full */
> > +#define XUARTPS_SR_RXTRIG 0x00000001 /* Rx Trigger */
> > +
> > +
>
> empty line
>
> > +/**
> > + * xuartps_isr - Interrupt handler
> > + * @irq: Irq number
> > + * @dev_id: Id of the port
> > + *
> > + * Returns IRQHANDLED
> > + **/

<snip>

> > +
> > + if (percent_err >= 3)
> > + dev_err(port->dev, "Error too large, baud rate not
set\n");
> > + else {
> > + /* Set the values for the new baud rate */
> > + xuartps_writel(brgr_val, XUARTPS_BAUDGEN_OFFSET);
> > + xuartps_writel(brdiv_val, XUARTPS_BAUDDIV_OFFSET);
> > +
>
> remove this empty line.
>
> > + }
> > +}
> > +
> > +

<snip>

> > +
> > + /* Enable the TX Empty interrupt */
> > + xuartps_writel(XUARTPS_IXR_TXEMPTY, XUARTPS_IER_OFFSET);
> > +
> > + if (uart_circ_chars_pending(&port->state->xmit) < WAKEUP_CHARS)
> > + uart_write_wakeup(port);
> > +
>
> remove this empty line
>
> > +}
> > +
> > +/**
> > + * xuartps_stop_tx - Stop TX
> > + * @port: Handle to the uart port structure
> > + *
> > + **/
> > +static void xuartps_stop_tx(struct uart_port *port)
> > +{
> > + unsigned int regval;
> > +
> > + regval = xuartps_readl(XUARTPS_CR_OFFSET);
> > + regval |= XUARTPS_CR_TX_DIS;
> > + /* Disable the transmitter */
> > + xuartps_writel(regval, XUARTPS_CR_OFFSET);
> > +}
> > +
> > +/**
> > + * xuartps_stop_rx - Stop RX
> > + * @port: Handle to the uart port structure
> > + *
> > + **/
> > +static void xuartps_stop_rx(struct uart_port *port)
> > +{
> > + unsigned int regval;
> > +
> > + regval = xuartps_readl(XUARTPS_CR_OFFSET);
> > + regval |= XUARTPS_CR_RX_DIS;
> > + /* Disable the receiver */
> > + xuartps_writel(regval, XUARTPS_CR_OFFSET);
> > +}
> > +
> > +/**
> > + * xuartps_tx_empty - Check whether TX is empty
> > + * @port: Handle to the uart port structure
> > + *
> > + * Returns TIOCSER_TEMT on success, 0 otherwise
> > + **/
> > +static unsigned int xuartps_tx_empty(struct uart_port *port)
> > +{
> > + unsigned int status;
> > +
> > + status = xuartps_readl(XUARTPS_ISR_OFFSET) &
XUARTPS_IXR_TXEMPTY;
> > + return status ? TIOCSER_TEMT : 0;
> > +
>
> remove this empty line.
>
> > +}
> > +
> > +
>
> and this
>
> > +/**
> > + * xuartps_break_ctl - Based on the input ctl we have to start or
> stop
> > + * transmitting char breaks
> > + * @port: Handle to the uart port structure
> > + * @ctl: Value based on which start or stop decision is taken
> > + *
> > + **/
> > +static void xuartps_break_ctl(struct uart_port *port, int ctl)

<snip>

> > +/**
> > + * xuartps_config_port - Configure xuartps, called when the driver
> adds a
> > + * xuartps port
> > + * @port: Handle to the uart port structure
> > + * @flags: If any
> > + *
> > + **/
> > +static void xuartps_config_port(struct uart_port *port, int flags)
> > +{
> > + if (flags & UART_CONFIG_TYPE && xuartps_request_port(port) == 0)
> > + port->type = PORT_XUARTPS;
> > +
>
> empty line
>
> > +}
> > +
> > +/**
> > + * xuartps_get_mctrl - Get the modem control state
> > + *
> > + * @port: Handle to the uart port structure
> > + *
> > + * Returns the modem control state
> > + *
> > + **/
> > +static unsigned int xuartps_get_mctrl(struct uart_port *port)
> > +{
> > + return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
> > +}
> > +

<snip>

> > +/* Match table for of_platform binding */
> > +
> > +#ifdef CONFIG_OF
> > +static struct of_device_id xuartps_of_match[] __devinitdata = {
> > + { .compatible = "xlnx,xuartps", },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, xuartps_of_match);
> > +#endif
> > +
> > +static struct platform_driver xuartps_platform_driver = {
> > + .probe = xuartps_probe, /* Probe method */
> > + .remove = __exit_p(xuartps_remove), /* Detach method */
> > + .suspend = xuartps_suspend, /* Suspend */
> > + .resume = xuartps_resume, /* Resume after a
suspend */
> > + .driver = {
> > + .owner = THIS_MODULE,
> > + .name = XUARTPS_NAME, /* Driver name */
> > +#ifdef CONFIG_OF
> > + .of_match_table = xuartps_of_match,
> > +#endif
> > + },
> > +};
>

Hi Michal,

I don't have a problem removing the empty lines, but is there some
guideline on that
which I'm ignoring and should know?

> AFAIK. Grant proposed to write it in different style. I think he will
> agree with me.
>
> #ifdef CONFIG_OF
> static struct of_device_id xuartps_of_match[] __devinitdata = {
> { .compatible = "xlnx,xuartps", },
> {}
> };
> MODULE_DEVICE_TABLE(of, xuartps_of_match);
> #else
> #define xuartps_of_match NULL
> #endif
>
>
> static struct platform_driver xuartps_platform_driver = {
> .probe = xuartps_probe, /* Probe method */
> .remove = __exit_p(xuartps_remove), /* Detach method */
> .suspend = xuartps_suspend, /* Suspend */
> .resume = xuartps_resume, /* Resume after a
suspend */
> .driver = {
> .owner = THIS_MODULE,
> .name = XUARTPS_NAME, /* Driver name */
> .of_match_table = xuartps_of_match,
> },
> };
>

I'll check it out, that does look familiar to me now that you say it.
We've had this
driver around for a while so it just needs to be updated to the newer
style.

Easy enough.

Thanks for the review and input,
John

> Regards,
> Michal
>
>
>
> --
> Michal Simek, Ing. (M.Eng)
> PetaLogix - Linux Solutions for a Reconfigurable World
> w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-
> 30090663