Re: [PATCH] bcm43xx: fix unexpected LED control values in BCM4303 sprom

[Michael Buesch <mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>]

On Wednesday 18 October 2006 06:38, Larry Finger wrote:
> The bcm43xx driver uses 4 locations in the devices sprom to determine
> the behavior of the leds. Certain defaults are assigned if all bits are
> set in those locations. On at least one BCM4303 chip, the sprom contains
> values other than the default, which executes an assertion placed in the
> default case of a following switch statement. This patch makes the leds
> on the above mentioned interface behave correctly. In addition, it limits
> the number of logged messages to 20 for the case of unexpected values in
> the sprom locations.
> 
> Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
> ---
> 
>  bcm43xx_leds.c |   12 +++++++++++-
>  bcm43xx_leds.h |    4 ++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> 
> John,
> 
> This patch should be applied to wireless-2.6 so as to be ready for 2.6.20.
> 
> Larry
> 
> 
> 
> 
> Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.c
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_leds.c
> +++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.c
> @@ -168,6 +168,8 @@ void bcm43xx_leds_exit(struct bcm43xx_pr
>  	bcm43xx_leds_switch_all(bcm, 0);
>  }
>  
> +static int bcm43xx_max_led_err = 20;
> +
>  void bcm43xx_leds_update(struct bcm43xx_private *bcm, int activity)
>  {
>  	struct bcm43xx_led *led;
> @@ -189,20 +191,24 @@ void bcm43xx_leds_update(struct bcm43xx_
>  		case BCM43xx_LED_INACTIVE:
>  			continue;
>  		case BCM43xx_LED_OFF:
> +		case BCM43xx_LED_BCM4303_3:
>  			break;
>  		case BCM43xx_LED_ON:
>  			turn_on = 1;
>  			break;
>  		case BCM43xx_LED_ACTIVITY:
> +		case BCM43xx_LED_BCM4303_0:
>  			turn_on = activity;
>  			break;
>  		case BCM43xx_LED_RADIO_ALL:
>  			turn_on = radio->enabled;
>  			break;
>  		case BCM43xx_LED_RADIO_A:
> +		case BCM43xx_LED_BCM4303_2:
>  			turn_on = (radio->enabled && phy->type == BCM43xx_PHYTYPE_A);
>  			break;
>  		case BCM43xx_LED_RADIO_B:
> +		case BCM43xx_LED_BCM4303_1:
>  			turn_on = (radio->enabled &&
>  				   (phy->type == BCM43xx_PHYTYPE_B ||
>  				    phy->type == BCM43xx_PHYTYPE_G));
> @@ -257,7 +263,11 @@ void bcm43xx_leds_update(struct bcm43xx_
>  			continue;
>  #endif /* CONFIG_BCM43XX_DEBUG */
>  		default:
> -			assert(0);
> +			if (bcm43xx_max_led_err) {
> +				printkl(KERN_INFO PFX "Bad value in leds_update,"
> +					" led->behaviour: 0x%x\n", led->behaviour);
> +				--bcm43xx_max_led_err;
> +			}

I'd call this message bloat. ;) This is the first time the assertion
triggers since it was added.
You could instead remove the assert(), remove bcm43xx_max_led_err
and use dprintkl instead of printkl.

>  		};
>  
>  		if (led->activelow)
> Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
> +++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
> @@ -24,6 +24,10 @@ struct bcm43xx_led {
>  
>  #define BCM43xx_LED_BEHAVIOUR		0x7F
>  #define BCM43xx_LED_ACTIVELOW		0x80
> +#define BCM43xx_LED_BCM4303_0		0x2B
> +#define BCM43xx_LED_BCM4303_1		0x78
> +#define BCM43xx_LED_BCM4303_2		0x2E
> +#define BCM43xx_LED_BCM4303_3		0x19

Add these to the enum below, instead, please.
The defines above are masks and not the behaviour values.

>  enum { /* LED behaviour values */
>  	BCM43xx_LED_OFF,
>  	BCM43xx_LED_ON,
> 
> ========
> 
> 

-- 
Greetings Michael.