lkml.org 
[lkml]   [2006]   [Jan]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCHv2] parport: add parallel port support for SGI O2
From
Date
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/

\
 
 \ /
  Last update: 2006-01-24 00:41    [W:0.071 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site