Re: [PATCH 1/4] Add new Atheros IEEE 802.11n ath9k driver

[Harvey Harrison <harvey.harrison@gmail.com>]

On Wed, 2008-07-23 at 21:43 +0200, Johannes Berg wrote:
> Only STA support is currently enabled
> +#ifndef howmany
> +#define howmany(x, y)   (((x)+((y)-1))/(y))
> +#endif

It's called DIV_ROUND_UP in kernel.h

> +#define REG_WRITE(_ah, _reg, _val) iowrite32(_val, _ah->ah_sh + _reg)
> +#define REG_READ(_ah, _reg) ioread32(_ah->ah_sh + _reg)
> 
> Do you really need macros for this? Static inlines that do
> type-checking are so much nicer.

Or just open-code the ioread/writes

> +#define LE_READ_2(p)							\
> +	((u_int16_t)							\
> +	 ((((const u_int8_t *)(p))[0]) | \
> +		(((const u_int8_t *)(p))[1] << 8)))
> +

get_unaligned_le16()

> +#define LE_READ_4(p)							\
> +	((u_int32_t)							\
> +	 ((((const u_int8_t *)(p))[0]) | \
> +		(((const u_int8_t *)(p))[1] << 8) | \
> +		(((const u_int8_t *)(p))[2] << 16) | \
> +			(((const u_int8_t *)(p))[3] << 24)))
> 

get_unaligned_le32()

> Harvey will flame you for that. Rightfully.

;-)

> +	/* XXX: spin_lock_bh should not be used here, but sparse bitches
> +	 * otherwise. We should fix sparse :) */
> +	spin_lock_bh(&mcastq->axq_lock);
> 
> First and foremost you should write correct code regardless of whether
> sparse complains about it or not. Then let others review it and possibly
> submit a bug report to sparse. This is the wrong way around.
> 

Indeed.

> +/* Macro to expand scalars to 64-bit objects */
> +
> +#define	ito64(x) (sizeof(x) == 8) ? \
> +	(((unsigned long long int)(x)) & (0xff)) : \
> +	(sizeof(x) == 16) ? \
> +	(((unsigned long long int)(x)) & 0xffff) : \
> +	((sizeof(x) == 32) ?			\
> +	 (((unsigned long long int)(x)) & 0xffffffff) : \
> +	(unsigned long long int)(x))
> 
> Eww.

Consider using s64/u64 if you really need 64-bits.

Also, for anything shared with the hardware in a fixed endianness,
le16/32/64 and be16/32/64 are your friends.  To reiterate what
Johannes mentioned elsewhere.

Cheers,

Harvey