On Sat, Nov 08, 2003 at 09:11:57PM -0500, Rick Wash wrote:
> I'd appreciate any comments on the driver.  I don't have the resources to
> test it on anything but by T30 laptop.  Also, this is my first NetBSD device
> driver, so any advice on coding it, things that I screwed up, etc. would be
> appreciated.

Rick,

I see a few ways to improve the driver. In tcpa_init,

        x = splhigh();
        /* talk directly to chip */
        outb(TPM_ADDR, 0x0D);       /* unlock 4F */
        outb(TPM_DATA, 0x55);
        outb(TPM_ADDR, 0x0A);           /* int disable */
        outb(TPM_DATA, 0x00);
        outb(TPM_ADDR, 0x08);           /* base addr lo */
        outb(TPM_DATA, sc->base & 0xFF);
        outb(TPM_ADDR, 0x09);           /* base addr hi */
        outb(TPM_DATA, (sc->base & 0xFF00) >> 8);
        outb(TPM_ADDR, 0x0D);          /* lock 4F */
        outb(TPM_DATA, 0xAA);
        splx(x);

Is it necessary to synchronize tcpa_init's access to those registers
with splhigh()? It seems like tcpa_init() is only run once during
auto-configuration.

Name constants like 0xAA and 0x55 so that the driver is
"self-documenting."  It might be appropriate to name the individual bits
of the constants, and then bitwise-OR them together, like BIT_ABC|BIT_XYZ.

The use of outb()/inb() is not *necessarily* wrong, but it is not
ordinarily used by NetBSD drivers.  The bus_space_ routines are preferred
because they are architecture/bus-independent.

Your driver seems to expect for the TPM registers to always map to the
same I/O addresses---TPM_ADDR and TPM_DATA. In general, I don't think
that you can count on that. Again, bus_space_ routines will "do the
right thing." The actual mapping ought to be provided by the autoconf
system to tcpa_pci_attach.

There are also bus_space_ routines which sometimes will replace loops
such as this one,

        /* Write everything but the last byte */
        for (i = 0; i < sc->len; i++) {
                outb(sc->base, sc->buffer[i]);
        }

There is not a clean break between the PCI front-end and the
bus-independent code in dev/ic/tcpa.c. You should not be passing a
pci_attach_args to tcpaattach.

DELAY(x) delays not for x milliseconds, but for x microseconds.

In the kernel, there is a "standard" macro called be32toh for converting
from big-endian to host-endian 32-bit words, such as decode_word does.

This declaration will probably make it hard to "tag" to the tcpa_softc
definition in dev/ic/tcpa.h,

 struct tcpa_softc *tcpa_softc;

Remove the dead code, like tcpa_recv. =)

Without knowing much about the opencrypto framework, my guess is that
its API suits the functions of the TPM.

Dave

-- 
David Young             OJC Technologies
dyoung@ojctech.com      Urbana, IL * (217) 278-3933