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    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site