lkml.org 
[lkml]   [2012]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [03/22] Cyclades PC300 driver: Fix usage of types
Date
Hi,

Andrea Shepard <andrea@persephoneslair.org> writes:

> +++ 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.

> +#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.

> #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.

> 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


\
 
 \ /
  Last update: 2012-01-31 20:11    [W:0.055 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site