Riccardo Facchetti (fizban@tin.it)
Tue, 31 Mar 1998 20:28:37 +0200 (MET DST)
On Tue, 31 Mar 1998, Andrea Arcangeli wrote:
>
> On Fri, 27 Mar 1998, Tim Waugh wrote:
>
> >Hi guys,
> >
> >Please try out this patch:
> >
> ><ftp://ftp.torque.net/pub/parport/patch-2.1.91pre2-parport980327>
>
> The patch seems to be mainly wrong. It is completly wrong define the
> lp flags inside parport.h. These flags give us the semantic of how _lp_
> handle the 8255 signal lines. How can be useful PARPORT_STATUS_PAPEROUT
> for a parallel ZIP drive for example?
>
> All parport_timedout() macro should be also moved into lp.h and all must
> be renamed from PARPORT_xxx and parport_xxx to LP_xxx and lp_xxx.
>
> This only after a short read of the patch...
The patch is mainly wrong !
Ohh ... interesting. You have seen a thing that every one have read that
patch knows. PARPORT_xxx are wrong but, as I have alredy pointed out to
Tim, IMHO the parport driver (driver of a hardware port) is too much
intersected with the lp driver (driver of just one of many devices that
can be attached to that port). My one is an attempt to separate the code
related with parallel port from the code related with the lp. Of course I
can not pretend to do it fast and well, but this is a start.
After that, since the status byte is just that, 8 bits in a byte read from
the status port, I think we should make one level of abstraction for this
bits in parport.h and then a second, higher, level on lp.h. Just something
like this:
parport.h
#define PARPORT_STATUS_BIT0 0x01
#define PARPORT_STATUS_BIT1 0x08
#define PARPORT_STATUS_BIT2 0x10
....
lp.h
#define LP_PARPORT_STATUS_TIMEOUT PARPORT_STATUS_BIT0
#define LP_PARPORT_STATUS_ERROR PARPORT_STATUS_BIT1
#define LP_PARPORT_STATUS_SELECT PARPORT_STATUS_BIT2
....
/* and, move the test macros from parport.h to lp.h */
#define lp_parport_timedout(s) (s & LP_PARPORT_STATUS_TIMEOUT)
#define lp_parport_noerror(s) (s & LP_PARPORT_STATUS_ERROR)
#define lp_parport_selected(s) (s & LP_PARPORT_STATUS_SELECT)
/* and use the LP_PARPORT_STATUS_* defines in the lp.c */
but, IHMO we must NOT lose sight that *_PARPORT_STATUS_* is an hardware
thing: read a byte from parallel status I/O port and ONLY after that, it
is a message from the device attached to that port.
I think the parport driver should be on a lower level of abstraction than
the lp driver and the two should be as much as possible orthogonal, but I
may be wrong.
After said all the above, I think that the patch is more than moving
defines from here to there. You have not seen in your "short read of the
patch..." (ohhh ... a promise of further fighting against it ?) that it
countains a bug fix, into the /proc/parport/X/irq code, and some code
cleanups, so that "mainly wrong" referred to the entire patch is really
offensive.
Note that I have read your e-mail with the COMA issue in parport_procfs.c
The old code (before that patch) reads:
[...]
if (pp->irq != PARPORT_IRQ_NONE && !(pp->flags & PARPORT_FLAG_COMA)) {
if (pp->cad != NULL && pp->cad->irq_func != NULL)
free_irq(pp->irq, pp->cad->private);
else
free_irq(pp->irq, NULL);
}
oldirq = pp->irq;
pp->irq = newirq;
[...]
Look .. this is strange ... you are assigning newirq to a COMA parport !
Infact the assignment is not protected by a check for COMA flag !!
The behaviour of the code is not changed by the patch. It is only
a cleanup and reorganization of the code and this bug existed before.
Anyway that patch is very old, but this is not important as my main issue
with this e-mail is to reply to your unpolite comments.
Ciao,
Riccardo.
-- To unsubscribe, send mail to: linux-parport-request@torque.net --
-- with the single word "unsubscribe" in the body of the message. --
This archive was generated by hypermail 2.0b3 on Wed 30 Dec 1998 - 10:17:34 EST