Re: [PATCH] serial: Add driver for OMAP UARTs

["George G. Davis" <gdavis@mvista.com>]

On Mon, Apr 09, 2007 at 11:44:33PM +0300, Juha Yrjölä wrote:
> --- /dev/null
> +++ b/drivers/serial/omap.c
> @@ -0,0 +1,893 @@


> +static int serial_omap_startup(struct uart_port *port)
> +{
> +	struct uart_omap_port *up = (struct uart_omap_port *)port;
> +	unsigned long flags;
> +	int retval;
> +
> +	enable_uart_clocks(up);
> +
> +	/*
> +	 * Allocate the IRQ
> +	 */
> +	retval = request_irq(up->port.irq, serial_omap_irq, 0, up->name, up);
> +	if (retval) {
> +		disable_uart_clocks(up);
> +		return retval;
> +	}
> +


> +}
> +
> +static void serial_omap_shutdown(struct uart_port *port)
> +{


> +	disable_uart_clocks(up);
> +
> +	clk_put(up->fclk);
> +	if (up->iclk != NULL)
> +		clk_put(up->iclk);
> +}


You should probably move {enable,disable}_uart_clocks() calls into the
uart_ops .pm function, serial_omap_pm() (which is currently a stub : ),
assuming clocks can be controlled per serial port.  In fact, if clocks
are controlled per serial port, then you may discover problems if you
do not move clock handling into the .pm function, e.g. try this when
one or more serial ports are not in use:

	cat /proc/tty/serial

If one or more serial ports are disabled, results should be garbage and
or the system may hang depending on behavior when serial port clocks
are disabled.  I expect you will need to s/serial/OMAP serial/ in the
above example but more on that below.


> +struct uart_ops serial_omap_pops = {
> +	.tx_empty	= serial_omap_tx_empty,
> +	.set_mctrl	= serial_omap_set_mctrl,
> +	.get_mctrl	= serial_omap_get_mctrl,
> +	.stop_tx	= serial_omap_stop_tx,
> +	.start_tx	= serial_omap_start_tx,
> +	.stop_rx	= serial_omap_stop_rx,
> +	.enable_ms	= serial_omap_enable_ms,
> +	.break_ctl	= serial_omap_break_ctl,
> +	.startup	= serial_omap_startup,
> +	.shutdown	= serial_omap_shutdown,
> +	.set_termios	= serial_omap_set_termios,
> +	.pm		= serial_omap_pm,
> +	.type		= serial_omap_type,
> +	.release_port	= serial_omap_release_port,
> +	.request_port	= serial_omap_request_port,
> +	.config_port	= serial_omap_config_port,
> +	.verify_port	= serial_omap_verify_port,
> +};
> +
> +static struct uart_driver serial_omap_reg = {
> +	.owner		= THIS_MODULE,
> +	.driver_name	= "OMAP serial",

You may want to reconsider ".driver_name" otherwise I suspect this may
appear as:

	"/proc/tty/driver/OMAP serial"

Yuck.  : )

> +	.dev_name	= "ttyS",
> +	.major		= TTY_MAJOR,
> +	.minor		= 64,
> +	.nr		= 3,
> +	.cons		= OMAP_CONSOLE,
> +};

--
Regards,
George