[Linux-parport] Re: [PATCHv2] parport: add parallel port support for SGI O2

[Arnaud Giersch]

Lundi 23 janvier 2006, vers 07:24:25 (+0100), Andrew Morton a écrit:

>> From: Arnaud Giersch <arnaud.giersch at 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