[alliance-iosk] Comments on IOSK LM spec

[vhandel@chem.vu.nl (Ramon van Handel)]

Here are my comments on the IOSK LM spec.

1. My biggest complaint is error reporting.  Look at these functions:

      BOOLEAN LMinit(LMinitData *);
      BOOLEAN LMwrite(VOID *Buffer, UWORD32 Size);
      BOOLEAN LMread(VOID *Buffer, UWORD32 Size);

   The BOOLEAN type is simply inadequate for error reporting reason.  When these
   functions return, I know whether they went right or wrong, but if it went
   wrong I don't know why...  For instance, take this piece of code from the
   parallell LM:

      if(deviceStatus & (PAPEROUT|ERROR)) {     /* An error is occurred? */
          return(FALSE);                        /* Yes, exit aborting    */
      }

   Say the printer is out of paper.  Wouldn't it be nice if the spooler
   application (actually, coming to think of there's nowhere in our design
where
   we've put the printer spooler...  I guess that has to do with OpenDoc) would
   be able to print a nice dialog box on the screen, "Printer is out of paper" ?
   But it can't, because it doesn't know what went wrong.

2. The LMinit prototype:

      typedef struct {
          STRING *(*LMProbe)(VOID);
          BOOLEAN (*LMRead)(VOID *, UWORD32);
      } LMinitData;

      BOOLEAN LMinit(LMinitData *);

   I think it is better to give the function parameters directly, like this:

      typedef STRING *(*LMProbeType)(VOID);
      typedef BOOLEAN (*LMReadType)(VOID *, UWORD32);

      BOOLEAN LMinit(LMProbeType, LMReadType);

   OR to give the struct as a direct parameter:

      BOOLEAN LMinit(LMinitData);

   Otherwise, the IOSK has to allocate such a structure for each LM it
   initialises, while for the LM it might not be useful at all to have such a
   structure.  You're better off giving the pointers and letting the LM manage
   where it stores them.  Note that I'm assuming that these callback functions
   should never change during the lifetime of the IOSK (which is, I believe,
   a good assumption.)

3. Minor textual correction:

      In order to be generic enough to cover all possible situations,
      present and future, when IOSK want to know if a loaded LM can work and
      if so, what can do, IOSK interrogate the LM calling a probing
      function:

   The IOSK *HAS* to call the probe function ONCE at least AFTER it calls the
   init function, and BEFORE it calls any LM functions (read, write, etc.)
   As the init function isn't allowed to contain hardware initialisation, this
   has to be in the probe function, so it has to be certain that this function
   will be called before any requests to hardware access.  I think you did imply
   this, but it's best to clarify these things.

   There has to be another certainty here, but there are two options:  either
   1.  The LMprobe() function in the LM *HAS* to be non-destructive to any
       pending requests;  i.e., the LMprobe() function can be called at ANY time
       without influencing the other functions;  or
   2.  The IOSK promises only to call the probe function
          - ONCE right after the init function was called, and afterwards
          - ONLY if the LM called the probe callback function.
       After probe callback, the IOSK is then not allowed to make *any*
       requests until it has probed the LM.  The LM is itself responsible for
       suspending pending requests until probe.


Another little thing about the directory naming:  you put the parallell
port LM in a directory called ibm-pc.  Now in principle, we should divide
hardware drivers per hardware platform, but in practice hardware platforms
are not as unambiguous as processors.  For instance, the IBM-PC compatibles
as well as the Digital Alpha, and perhaps also sparcs and macs (not sure)
use the same serial 16650 UART and parallell port hardware.  If you divide
on the basis of ibm-pc, that would mean inefficient code reuse.

I don't have a solution ready for this, it's simply something we need to
consider.  Perhaps we should simply divide hardware drivers per chip - Ie,

LM/
LM/IOSK
LM/IOSK/Storage
LM/IOSK/Comms
...
LM/IOSK/Storage/AM53C974
LM/IOSK/Storage/NCR53C8xx
...
LM/IOSK/Comms/16650A
...

What do you think ?

Anyway, for the rest, I think it's a great spec !!!  Now you just need to
get it to actually work together with the IOSK.

Ramon




---
Ramon van Handel   <vhandel@chem.vu.nl>
Chemistry Student, OS Programmer and all-round Weirdo
The ant has made himself illustrious / Through constant industry industrious.
So what? Would you be calm and placid / If you were full of formic acid?
(Ogden Nash)


-
Alliance-IOSK:	http://iosk.allos.org/
Archive:	http://humbolt.nl.linux.org/lists/