Re: [PATCH 5/5] ath5k: Fix and clean mode initialization, prefer G for AR5212

[Jiri Slaby <jirislaby@gmail.com>]

On 10/13/2007 10:08 PM, Luis R. Rodriguez wrote:
> [PATCH 5/5] ath5k: Fix and clean mode initialization, prefer G for AR5212
[...]
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index 18ee995..db37ceb 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
> @@ -1896,7 +1896,7 @@ static void ath_dump_modes(struct ieee80211_hw_mode *modes)
>  {
>  	unsigned int m, i;
>  
> -	for (m = 0; m < NUM_IEEE80211_MODES; m++) {
> +	for (m = 0; m < NUM_DRIVER_MODES; m++) {
>  		printk(KERN_DEBUG "Mode %u: channels %d, rates %d\n", m,
>  				modes[m].num_channels, modes[m].num_rates);
>  		printk(KERN_DEBUG " channels:\n");
> @@ -1919,71 +1919,92 @@ static void ath_dump_modes(struct ieee80211_hw_mode *modes)
>  static inline void ath_dump_modes(struct ieee80211_hw_mode *modes) {}
>  #endif
>  
> +static inline int ath5k_register_mode(struct ieee80211_hw *hw, u8 m)
> +{
> +	struct ath_softc *sc = hw->priv;
> +	struct ieee80211_hw_mode *modes = sc->modes;
> +	int i, ret;

unsigned is usually used for positive iterators, it generates better code almost
for all platforms.

> +
> +	for (i = 0; i < NUM_DRIVER_MODES; i++) {
> +		if (modes[i].mode != m || !modes[i].num_channels)
> +			continue;
> +		ret = ieee80211_register_hwmode(hw, &modes[i]);
> +		if (ret) {
> +			printk(KERN_ERR "can't register hwmode %u\n", m);
> +			return ret;
> +		}
> +		return 0;
> +	}

Maybe we should oops here, since it is a bug to get that far. I think, you
should put BUG() here to not mess userspace with positive retvals.

> +	return 1;
> +}
> +
> +/* Only tries to register modes our EEPROM says it can support */
> +#define REGISTER_MODE(m) do { \
> +	if (test_bit(m, ah->ah_capabilities.cap_mode)) { \
> +		ret = ath5k_register_mode(hw, m); \
> +		if (ret) \
> +			return ret; \
> +	} \

I though you will put the test inside the new ath5k_register_mode function too
(and return 0 in the case when unsupported) and get rid of the macro. But ok,
not global macro we can tolerate return-ing from it :).

knowing to be a pain in the ass,
-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University