Re: [PATCH] 2.6 Altix : rs422 support for ioc4 serial driver

[Andrew Morton <akpm@osdl.org>]

Pat Gefre <pfg@sgi.com> wrote:
>
> This patch adds rs422 support to the Altix ioc4 serial driver.
> 
> +
> +#define PORT_IS_ACTIVE(_x, _y)	((_x->ip_flags & PORT_ACTIVE) \
> +					&& (_x->ip_port == _y))
> +

- Forgets to parenthesise macro args

- Evaluates args multiple times

- ugleeeee


This:

/*
 * Nice comment goes here
 */
static inline int port_is_active(struct ioc4_port *current_port,
				struct ioc4_port *my_port)
{
	...
}

Is more pleasing, no?

> +	if (port && PORT_IS_ACTIVE(port, the_port)) {

And in every case the test for port==NULL can be folded into port_is_active().

> +int ioc4_serial_remove_one(struct ioc4_driver_data *idd)

Should have static scope.

> +{
> +	int ii, jj;
> +	struct ioc4_control *control;
> +	struct uart_port *the_port;
> +	struct ioc4_port *port;
> +	struct ioc4_soft *soft;
> +
> +	control = idd->idd_serial_data;
> +
> +	for (ii = 0; ii < IOC4_NUM_SERIAL_PORTS; ii++) {
> +		for (jj = UART_PORT_MIN; jj < UART_PORT_COUNT; jj++) {
> +			the_port = &control->ic_port[ii].icp_uart_port[jj];
> +			if (the_port) {
> +				switch (jj) {
> +				case UART_PORT_RS422:
> +					uart_remove_one_port(&ioc4_uart_rs422,
> +							the_port);
> +					break;
> +				default:
> +				case UART_PORT_RS232:
> +					uart_remove_one_port(&ioc4_uart_rs232,
> +							the_port);
> +					break;
> +				}
> +			}
> +		}
> +		port = control->ic_port[ii].icp_port;
> +		if (!(ii & 1) && port) {
> +			pci_free_consistent(port->ip_pdev,
> +					TOTAL_RING_BUF_SIZE,
> +					(void *)port->ip_cpu_ringbuf,
> +					port->ip_dma_ringbuf);
> +			kfree(port);
> +		}
> +	}

Choosing more meaningful identifiers than `ii' and `jj' would help
understandability here.

> +		free_irq(control->ic_irq, (void *)soft);

The typecast is unneeded.