lkml.org 
[lkml]   [2006]   [Apr]   [3]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
/
DateMon, 3 Apr 2006 09:50:58 +0100
FromRussell King <>
SubjectRe: [PATCH 2.6.16-git] [SERIAL] Adds DCC(JTAG) serial and the console emulation support (revised)
On Mon, Apr 03, 2006 at 05:20:02PM +0900, Hyok S. Choi wrote:

Thanks for looking at the previous points, this driver is much better.
However, there's still some bits which could be even better.

> +/* use ttyJ0(128) */> +#define SERIAL_DCC_NAME		"ttyJ"> +#define SERIAL_DCC_MINOR	(64 + 64)> +#endif> +#define SERIAL_DCC_MAJOR	TTY_MAJOR

Why not just get a proper legal allocation from LANANA ?

> +static inline u32> +__dcc_getstatus(void)

Column space is not at a premium.

static inline u32 __dcc_getstatus(void)

will do just as well.  Does the double underscore here serve any purpose?

> +{> +	u32 __ret;

The double underscore here serves no purpose.

> +> +	asm("mrc p14, 0, %0, c0, c0	@ read comms ctrl reg"
> +		: "=r" (__ret));
> +
> +	return __ret;
> +}
> +
> +static inline char
> +__dcc_getchar(void)
> +{
> +	char __c;

Ditto.

> +> +	asm("mrc p14, 0, %0, c1, c0	@ read comms data reg"
> +		: "=r" (__c));
> +
> +	return __c;
> +}
> +
> +static inline void
> +__dcc_putchar(char c)
> +{
> +	asm("mcr p14, 0, %0, c1, c0	@ write a char"
> +		: /* no output register */
> +		: "r" (c));
> +}
> +
> +static void
> +dcc_putchar(struct uart_port *port, int ch)
> +{
> +	while (__dcc_getstatus() & DCC_STATUS_TX)
> +		cpu_relax();
> +	__dcc_putchar((char)(ch & 0xFF));

Since this is the only place __dcc_putchar is used, might be an idea to
move it into this function?

> +}> +> +static inline void> +xmit_string(struct uart_port *port, char *p, int len)
> +{> +	for ( ; len; len--, p++)> +		dcc_putchar(port, *p);> +}> +> +static inline void> +dcc_transmit_buffer(struct uart_port *port)> +{> +	struct circ_buf *xmit = &port->info->xmit;
> +	int pendings = uart_circ_chars_pending(xmit);> +> +	if(pendings + xmit->tail > UART_XMIT_SIZE)
> +	{

Mixture of bracing styles, and there should be a space between if and (.

	if (pendings + xmit->tail > UART_XMIT_SIZE) {
> +		xmit_string(port, &(xmit->buf[xmit->tail]),
> +			UART_XMIT_SIZE - xmit->tail);> +		xmit_string(port, &(xmit->buf[0]), xmit->head);
> +	} else> +		xmit_string(port, &(xmit->buf[xmit->tail]), pendings);
> +	> +	xmit->tail = (xmit->tail + pendings) & (UART_XMIT_SIZE-1);
> +	port->icount.tx += pendings;
> +}
>...
> +dcc_rx_chars(struct uart_port *port)
> +{
> +	unsigned char ch;
> +	struct tty_struct *tty = port->info->tty;
> +
> +	/*
> +	 * check input.
> +	 * checking JTAG flag is better to resolve the status test.
> +	 * incount is NOT used for JTAG1 protocol.
> +	 */
> +
> +	if (__dcc_getstatus() & DCC_STATUS_RX)
> +	{

Mixture of bracing styles again.  Either always place the { at the end of
the previous line (preferred) or separately on the next line, but don't
do both in the same file.
> +		/* for JTAG 1 protocol, incount is always 1. */> +		ch = __dcc_getchar();> +> +		if (tty) {> +			tty_insert_flip_char(tty, ch, TTY_NORMAL);> +			port->icount.rx++;> +			tty_flip_buffer_push(tty);> +		}> +	}> +}> +> +static inline void> +dcc_overrun_chars(struct uart_port *port)> +{> +	port->icount.overrun++;> +}

This doesn't seem to be used anywhere.

> +static int> +dcc_startup(struct uart_port *port)> +{> +#ifdef DCC_IRQ_USED /* real IRQ used */> +	int retval;> +> +	/* Allocate the IRQ */> +	retval = request_irq(port->irq, dcc_int, SA_INTERRUPT,
> +			     "serial_dcc", port);> +	if (retval)> +		return retval;> +#else /* emulation */> +	/* Initialize the work, and shcedule it. */> +	INIT_WORK(&dcc_poll_task, dcc_poll, port);> +	spin_lock(&port->lock);> +	if (dcc_poll_state != DCC_POLL_RUN)> +		dcc_poll_state = DCC_POLL_RUN;> +	schedule_delayed_work(&dcc_poll_task, 1);> +	spin_unlock(&port->lock);

Hmm, this looks over complex:

	if (dcc_poll_state != DCC_POLL_RUN)
		dcc_poll_state = DCC_POLL_RUN;
and unnecessary (see dcc_shutdown comments.)

Secondly, wouldn't it be better to have INIT_WORK() in the driver
initialisation?

> +#endif> +> +	return 0;> +}> +> +static void> +dcc_shutdown(struct uart_port *port)> +{> +#ifdef DCC_IRQ_USED /* real IRQ used */> +	free_irq(port->irq, port);> +#else> +	spin_lock(&port->lock);> +	dcc_poll_state = DCC_POLL_STOP;> +	spin_unlock(&port->lock);

Rather than having this dcc_poll_state, wouldn't the use of:

	cancel_rearming_delayed_work(&dcc_poll_task);

suffice?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-04-03 12:37    [from the cache]
©2003-2008