[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [MiNT] lp driver patch for the Firebee to extend the parallel port strobe duration

On 03/10/2015 22:35, markus@mubf.de wrote:
please find attached a patch to MiNT's lp driver.

Very good, this was the thing to do.

However, a few remarks:

1) Your strobe_delay() routine takes the parameter from the stack, and cleans up the stack by itself. That's not the standard C convention, anyway it doesn't matter in assembler code. However, inside the routine, you immediately put the parameter into d0. It would have been more efficient to pass directly the parameter in d0, and document it in the function header. That would also get rid of the confusing stack usage.

2) FreeMiNT is compiled with gas (the GNU assembler). By design, gas doesn't automatically shorten the branches. This means that "bne" will always be 4 bytes, while "bne.s" would be 2 bytes only. Usually, the rule is: "use .s when possible". Ideally, the gas "jbne" macro should be used where possible, since it automatically choses the best branch size. Same with jbra, jbsr, etc. However such macros are currently never used in FreeMiNT, probably by design to avoid using too much gas specificities (which is a nonsense since the current code can' be compiled with anything else than gas, anyway).

Of course, the 2 above points are not critical, since speed doesn't matter in this case. But as a general rule, it is good to avoid suboptimal or misleading code.

3) AFAIK, "lp" is just a spooler which creates the /dev/lp device. Even without it, it should always be possible to print through the /dev/centr BIOS device (or its /dev/prn alias). So patching the "lp" driver is good, but be sure that the BIOS device also works. IIRC, Roger recently patched EmuTOS for that.

And to answer Alan's question:
Why can't this problem be entirely fixed within the FPGA and not resort
to these additional requirements ?

Fixing the FPGA would slow down the transitions 1->0 and 0->1, but would not change the actual delay where the strobe line is hold at 0. Increasing the transition time would just be an undesired side effect. Increasing the delay in the code is the right thing to do. Using nops is not very good, but as Markus said that's acceptable in this case.

Vincent Rivière