Re: [PATCH] Altix : ioc4 serial driver support

Patrick Gefre
2005-02-08T16:57:52Z

I've update the patch with changes from the comments below.

ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support

Christoph Hellwig wrote:
> On Mon, Feb 07, 2005 at 09:58:33AM -0600, Patrick Gefre wrote:
>
>>Latest version with review mods:
>>ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
>
>
>
> - still not __iomem annotations.

I *think* I have this right ??

> - still a ->remove method
>
> more comments (mostly nipicks I missed last time, nothing too exciting):
>
>
> +#define DEVICE_NAME_DYNAMIC "ttyIOC0" /* need full name for misc_register */
>
> this one is completely unused.
>
> +#define PENDING(_p) readl(&(_p)->ip_mem->sio_ir) & _p->ip_ienb
>
> probably wants some braces around the macro body
>
> +static struct ioc4_port *get_ioc4_port(struct uart_port *the_port)
> +{
> + struct ioc4_control *control = dev_get_drvdata(the_port->dev);
> + int ii;
> +
> + if (control) {
> + for ( ii = 0; ii < IOC4_NUM_SERIAL_PORTS; ii++ ) {
> + if (!control->ic_port[ii].icp_port)
> + continue;
> + if (the_port == control->ic_port[ii].icp_port->ip_port)
> + return control->ic_port[ii].icp_port;
> + }
> + }
> + return (struct ioc4_port *)0;
>
> just return NULL here.
>
> +static irqreturn_t ioc4_intr(int irq, void *arg, struct pt_regs *regs)
> +{
> + struct ioc4_soft *soft;
> + uint32_t this_ir, this_mir;
> + int xx, num_intrs = 0;
> + int intr_type;
> + int handled = 0;
> + struct ioc4_intr_info *ii;
> +
> + soft = (struct ioc4_soft *)arg;
> + if (!soft)
> + return IRQ_NONE; /* Polled but no ioc4 registered */
>
> no need to cast. and it can't be NULL either.
>
> + spin_lock_irqsave(&port->ip_lock, port_flags);
> + wake_up_interruptible(&info->delta_msr_wait);
> + spin_unlock_irqrestore(&port->ip_lock, port_flags);
>
> no need to lock around a wake_up()
>
> + /* Start up the serial port */
> + spin_lock_irqsave(&port->ip_lock, port_flags);
> + retval = ic4_startup_local(the_port);
> + if (retval) {
> + spin_unlock_irqrestore(&port->ip_lock, port_flags);
> + return retval;
> + }
> + spin_unlock_irqrestore(&port->ip_lock, port_flags);
> + return 0;
>
> what about just
>
> spin_lock_irqsave(&port->ip_lock, port_flags);
> retval = ic4_startup_local(the_port);
> spin_unlock_irqrestore(&port->ip_lock, port_flags);
> return reval;
>
> ?
>
> + struct ioc4_port *port = get_ioc4_port(the_port);
> + unsigned long port_flags;
> +
> + spin_lock_irqsave(&port->ip_lock, port_flags);
> + ioc4_change_speed(the_port, termios, old_termios);
> + spin_unlock_irqrestore(&port->ip_lock, port_flags);
> + return;
>
> no need for empty returns at the end of void functions
>
> +static struct uart_driver ioc4_uart = {
> + .owner = THIS_MODULE,
> + .driver_name = "ioc4_serial",
> + .dev_name = DEVICE_NAME,
> + .major = DEVICE_MAJOR,
> + .minor = DEVICE_MINOR,
> + .nr = IOC4_NUM_CARDS * IOC4_NUM_SERIAL_PORTS,
> + .cons = NULL,
> +};
>
> no need to initialize .cons to zero, the compiler does that for you.
>
> + if ( !request_region(tmp_addr, sizeof(struct ioc4_mem), "sioc4_mem")) {
>
> superflous space before the !
>
> + if (!request_irq(pdev->irq, ioc4_intr, SA_SHIRQ,
> + "sgi-ioc4serial", (void *)soft)) {
> + control->ic_irq = pdev->irq;
> + } else {
> + printk(KERN_WARNING
> + "%s : request_irq fails for IRQ 0x%x\n ",
> + __FUNCTION__, pdev->irq);
> + }
>
> Can the driver work without an irq?

Not in its current state.