Messages in this thread | | | Date | Sun, 22 Jan 2006 22:24:25 -0800 | From | Andrew Morton <> | Subject | Re: [PATCHv2] parport: add parallel port support for SGI O2 |
| |
Arnaud Giersch <arnaud.giersch@free.fr> wrote: > > From: Arnaud Giersch <arnaud.giersch@free.fr> > > Add support for the built-in parallel port on SGI O2 (a.k.a. IP32). > Define a new configuration option: PARPORT_IP32. The module is named > parport_ip32. > > Hardware support for SPP, EPP and ECP modes along with DMA support > when available are currently implemented.
Nice looking driver. Big.
It does rather a lot of
if (foo) do_something();
whereas we prefer
if (foo) do_something();
but if that was a blocker, we wouldn't have any drivers.
> +static void parport_ip32_dma_setup_context(unsigned int limit) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&parport_ip32_dma.lock, flags); > + if (parport_ip32_dma.left > 0) { > + volatile u64 __iomem *ctxreg = (parport_ip32_dma.ctx == 0) ? > + &mace->perif.ctrl.parport.context_a : > + &mace->perif.ctrl.parport.context_b;
Does this need to be volatile? writeq() should do the right thing.
> +static size_t parport_ip32_epp_read(void __iomem *eppreg, > + struct parport *p, void *buf, > + size_t len, int flags) > +{ > + struct parport_ip32_private * const priv = p->physport->private_data; > + size_t got; > + parport_ip32_set_mode(p, ECR_MODE_EPP); > + parport_ip32_data_reverse(p); > + parport_ip32_write_control(p, DCR_nINIT); > + if ((flags & PARPORT_EPP_FAST) && (len > 1)) { > + readsb(eppreg, buf, len);
readsb() is a mips thing, and doesn't seem to be documented. What does it do, and why does the driver use it (only) here?
> + writesb(eppreg, buf, len);
> +static unsigned int parport_ip32_fifo_wait_break(struct parport *p, > + unsigned long expire) > +{ > + cond_resched(); > + if (time_after(jiffies, expire)) { > + printk(KERN_DEBUG PPIP32 > + "%s: FIFO write timed out\n", p->name); > + return 1; > + } > + if (signal_pending(current)) { > + printk(KERN_DEBUG PPIP32 > + "%s: Signal pending\n", p->name); > + return 1; > + }
This printk could be a bit noisy, if someone hoses a signal stream at a printing program.
- 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/
| |