lkml.org 
[lkml]   [2007]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/4] atl1: Header files for Attansic L1 driver
    On Wed, Jan 10, 2007 at 06:41:37PM -0600, Jay Cliburn wrote:
    > +/**
    > + * atl1.h - atl1 main header

    Please remove these kind of comments, they get out of date far too soon
    and don't really help anything. (Also everywhere else in the driver)

    > +#include <linux/tcp.h>
    > +#include <linux/skbuff.h>
    > +#include <linux/netdevice.h>
    > +#include <linux/pci.h>
    > +#include <linux/spinlock_types.h>
    > +#include <linux/workqueue.h>
    > +#include <linux/timer.h>
    > +#include <linux/delay.h>
    > +#include <linux/if_vlan.h>
    > +
    > +#include <asm/types.h>

    Please always include <linux/types.h>

    >
    > +#include <asm/atomic.h>

    Please only includ headers where you use them - that's mostly the .c
    files unless you have lots of inlines or complex structures in the headers.



    > +#ifdef NETIF_F_TSO
    > +#include <net/checksum.h>
    > +#endif
    > +
    > +#ifdef SIOCGMIIPHY
    > +#include <linux/mii.h>
    > +#endif
    > +
    > +#ifdef SIOCETHTOOL
    > +#include <linux/ethtool.h>
    > +#endif

    Please remove all these ifdefs.

    > +#define BAR_0 0

    This looks etirely superflous.

    > +#define usec_delay(x) udelay(x)
    > +#ifndef msec_delay
    > +#define msec_delay(x) do { if(in_interrupt()) { \
    > + /* Don't mdelay in interrupt context!*/ \
    > + BUG(); \
    > + } else { \
    > + msleep(x); \
    > + }} while(0)
    > +/**
    > + * Some workarounds require millisecond delays and are run during interrupt
    > + * context. Most notably, when establishing link, the phy may need tweaking
    > + * but cannot process phy register reads/writes faster than millisecond
    > + * intervals...and we establish link due to a "link status change" interrupt.
    > + **/
    > +#define msec_delay_irq(x) mdelay(x)
    > +#endif

    Please kill all these wrappers.

    > +} _ATL1_ATTRIB_PACK_;

    All your structs seems to be properly packed from a first sight. Please
    doble-check this and get rid of the attribute packed.

    > +struct csum_param {
    > + unsigned buf_len:14;
    > + unsigned dma_int:1;
    > + unsigned pkt_int:1;
    > + u16 valan_tag;
    > + unsigned eop:1;
    > + /* command */
    > + unsigned coalese:1;
    > + unsigned ins_vlag:1;
    > + unsigned custom_chksum:1;
    > + unsigned segment:1;
    > + unsigned ip_chksum:1;
    > + unsigned tcp_chksum:1;
    > + unsigned udp_chksum:1;
    > + /* packet state */
    > + unsigned vlan_tagged:1;
    > + unsigned eth_type:1;
    > + unsigned iphl:4;
    > + unsigned:2;
    > + unsigned payload_offset:8;
    > + unsigned xsum_offset:8;
    > +} _ATL1_ATTRIB_PACK_;

    Bitfields should not be used for hardware datastructures ever.
    Please convert this to explicit masking and shifting.

    > +/* Structure containing variables used by the shared code */
    > +struct atl1_hw {
    > + u8 __iomem *hw_addr;
    > + void *back;

    This is definitly a kernel data structure. Shouldn't it be in atl1.h
    instead of _hw.h? Also does back really need to be a void pointer or
    can it be something typed?

    > + u16 dev_rev;
    > + u16 device_id;
    > + u16 vendor_id;
    > + u16 subsystem_id;
    > + u16 subsystem_vendor_id;
    > + u8 revision_id;

    Please just use the values from the pci_dev insead of duplicating them.

    > +/* formerly ATL1_WRITE_REG */
    > +static inline void atl1_write32(const struct atl1_hw *hw, int reg, u32 val)
    > +{
    > + writel(val, hw->hw_addr + reg);
    > +}
    > +
    > +/* formerly ATL1_READ_REG */
    > +static inline u32 atl1_read32(const struct atl1_hw *hw, int reg)
    > +{
    > + return readl(hw->hw_addr + reg);
    > +}

    Just kill all these wrappers. Also you probably want to convert to
    pci_iomap + ioread*/iowrite*.

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-01-11 10:55    [W:0.037 / U:30.044 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site