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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
