Messages in this thread | | | Subject | Re: [PATCHv2] parport: add parallel port support for SGI O2 | From | Arnaud Giersch <> | Date | Tue, 24 Jan 2006 00:38:57 +0100 |
| |
Lundi 23 janvier 2006, vers 07:24:25 (+0100), Andrew Morton a écrit:
>> 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. > > Nice looking driver. Big.
Thanks.
> It does rather a lot of > > if (foo) do_something(); > > whereas we prefer > > if (foo) > do_something();
Those are limited to the parport_ip32_dump_state() function. The rationale is that this function is only here for debugging purpose, and I did not want to make it longer than it already is.
I will correct the style.
>> +static void parport_ip32_dma_setup_context(unsigned int limit) [...] >> + 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.
The "volatile" is here for type consistency. Both variables "mace->perif.ctrl.parport.context_a" and "context_b" (defined in include/asm-mips/ip32/mace.h) are from type "volatile u64". Omitting the "volatile" qualifier for "ctxreg" would make gcc and sparse complain.
I will add a comment to explain it in the code.
>> +static size_t parport_ip32_epp_read(void __iomem *eppreg, [...] >> + 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);
readsb() and writesb() are like ioread8_rep() and iowrite8_rep(). The io{read,write} family functions are however not available in the linux-mips.org tree.
The usage of readsb() here is similar to that of insb() in the corresponding code of the parport_pc driver.
>> +static unsigned int parport_ip32_fifo_wait_break(struct parport *p, [...] >> + 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.
I did not realized that. I will try to correct the problem.
Thank you for your attention.
Arnaud - 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/
| |