Re: [03/22] Cyclades PC300 driver: Fix usage of types
flat view
From: Krzysztof Halasa <khc@pm.waw.pl>
Date: 2012-01-31 19:08:54
Also in:
lkml
Hi, Andrea Shepard [off-list ref] writes:
quoted hunk
+++ b/drivers/net/wan/hd64572.h@@ -172,11 +172,11 @@ /* Block Descriptor Structure */ typedef struct { - unsigned long next; /* pointer to next block descriptor */ - unsigned long ptbuf; /* buffer pointer */ - unsigned short len; /* data length */ - unsigned char status; /* status */ - unsigned char filler[5]; /* alignment filler (16 bytes) */ + u32 next; /* pointer to next block descriptor */ + u32 ptbuf; /* buffer pointer */ + u16 len; /* data length */ + u8 status; /* status */ + u8 filler[5]; /* alignment filler (16 bytes) */ } pcsca_bd_t;
Perhaps you could just use the following struct which is already in the file: /* Block Descriptor Structure */ typedef struct { u32 cp; /* pointer to next block descriptor */ u32 bp; /* buffer pointer */ u16 len; /* data length */ u8 stat; /* status */ u8 unused; /* pads to 4-byte boundary */ }pkt_desc; The difference is a different alignment (4 bytes vs 16 bytes) but I think it's been verified (both docs and real hw) that 32-bit alignment is ok with these chips.
quoted hunk
+#define cpc_writeb(port, val) {writeb((u8)(val), \ + (void __iomem *)(port)); mb(); } +#define cpc_writew(port, val) {writew((u16)(val), \ + (void __iomem *)(port)); mb(); } +#define cpc_writel(port, val) {writel((u32)(val), \ + (void __iomem *)(port)); mb(); }
Are these mb() needed? I think write[bwl] and read[bwl] are already memory barriers. PCI write posting is a different thing of course and you may need e.g. a PCI read in some timing-critical parts of the code.
quoted hunk
#else /* __KERNEL__ */ -#define cpc_writeb(port,val) (*(volatile ucchar *)(port) = (ucchar)(val)) -#define cpc_writew(port,val) (*(volatile ucshort *)(port) = (ucshort)(val)) -#define cpc_writel(port,val) (*(volatile uclong *)(port) = (uclong)(val)) +#define cpc_writeb(port, val) (*(u8 *)(port) = (u8)(val)) +#define cpc_writew(port, val) (*(u16 *)(port) = (u16)(val)) +#define cpc_writel(port, val) (*(u32 *)(port) = (u32)(val)) -#define cpc_readb(port) (*(volatile ucchar *)(port)) -#define cpc_readw(port) (*(volatile ucshort *)(port)) -#define cpc_readl(port) (*(volatile uclong *)(port)) +#define cpc_readb(port) (*(u8 *)(port)) +#define cpc_readw(port) (*(u16 *)(port)) +#define cpc_readl(port) (*(u32 *)(port))
I'm not sure the kernel headers are a good place for such things.
quoted hunk
void tx_dma_stop(pc300_t * card, int ch) { - uclong scabase = card->hw.scabase; - ucchar drr_ena_bit = 1 << (5 + 2 * ch); - ucchar drr_rst_bit = 1 << (1 + 2 * ch); + uintptr_t scabase = (uintptr_t)(card->hw.scabase); + u8 drr_ena_bit = 1 << (5 + 2 * ch); + u8 drr_rst_bit = 1 << (1 + 2 * ch);
uintptr_t? Is it simply an unsigned int *? -- Krzysztof Halasa