Re: [PARPORT] Ta-Da

From: Alan Cox (alan@lxorguk.ukuu.org.uk)
Date: Thu Mar 01 2001 - 13:41:02 EST

  • Next message: bad_knee: "Re: [PARPORT] paride not reporting correct drive size"

    > I'm glad to let you know, I've been allowed to release the following source
    > code under the terms of the Free Software Foundation's General Public
    > License. This code is a protocol module for the PARIDE sub-system that
    > allows it to communicate with the newer generation of the Micro Solutions
    > BACKPACK drives (including our CD-Rewriters).

    Excellent

    > #ifdef CONFIG_PARPORT_PC_MODULE=0A=
    > #define CONFIG_PARPORT_PC=0A=
    > #endif=0A=
    > =0A=
    > #ifdef CONFIG_PARPORT_PC=0A=

    I'd rather see

    #if defined(CONFIG_PARPORT_PC) || defined(CONFIG_PARPORT_PC_MODULE)

    > printk("<1> leaving probe\n");=0A=

    Swap the "<1> " for KERN_ macros

    > /* allocate a state structure for this item */=0A=
    > pi->private=3D(int)kmalloc(sizeof(PPC),GFP_KERNEL);=0A=

    Check pi->private != NULL - you might be out of memory

    > /* free after use count decrimented so that we aren't using it=0A=

                            decremented (pedantry.. )

    > #ifdef MODULE=0A=
    > /*module information*/=0A=
    > =0A=
    > int init_module(void)=0A=
    static int

    > {=0A=
    > printk("BACKPACK Protocol Driver V2.0.0\n");=0A=
    > printk("Copyright 2001 by Micro Solutions, Inc., DeKalb IL.\n");=0A=

    KERN_INFO on those

    > =0A=
    > if(verbose)=0A=
    > {=0A=
    > printk(KERN_DEBUG "VERBOSE ON!\n");=0A=
    > }=0A=

    General comment . Its best to put [modulename]: - ie

            KERN_DEBUG "backpack: verbose debug enabled.\n");

    > #define inp(x) inb((x))=0A=
    > #define inpw(x) inw((x))=0A=
    > #define inpd(x) inl((x))=0A=
    > #define outp(x,y) outb((y),(x))=0A=
    > #define outpw(x,y) outw((y),(x))=0A=
    > #define outpd(x,y) outl((y),(x))=0A=

    Please actually remove the macros and update the code so its clear to read

    > typedef unsigned char BYTE;=0A=
    > typedef unsigned short WORD;=0A=
    > typedef unsigned long DWORD;=0A=
    > typedef unsigned long ULONG;=0A=

    The types the kernel provide you want to use are

            u8, u16, u32, u64

    Don't assume long is 32bit, since your boards probably work on Alpha based
    boxes, hppa64 and many other systems that all use pc like par port

    > void wait_for_fifo(PPC *ppc);=0A=

    names like 'wait_for_fifo' are far to generic to be globals. They will clash
    with something one day. Maybe put ppc6 in front of them too ?

    Alan

    -- 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 2b29 : Thu Mar 01 2001 - 13:40:37 EST