[PATCH] zd1211rw-d80211: Use LED class

[PATCH] zd1211rw-d80211: Use LED class

[Michael Wu]

[-- Attachment #1: Type: text/plain, Size: 9680 bytes --]

This makes zd1211rw-d80211 register an LED class to control the link LED
instead of trying to determine when the LED should be on based on the
current bssid. No default trigger is set since d80211 doesn't currently
have a link on/off LED trigger.

Signed-off-by: Michael Wu <flamingice@sourmilk.net>
---
 drivers/net/wireless/d80211/zd1211rw/zd_chip.c |    2 +-
 drivers/net/wireless/d80211/zd1211rw/zd_chip.h |    2 +-
 drivers/net/wireless/d80211/zd1211rw/zd_mac.c  |   90 
++++++++++--------------
 drivers/net/wireless/d80211/zd1211rw/zd_mac.h  |   23 +++++--
 drivers/net/wireless/d80211/zd1211rw/zd_usb.c  |   19 +++---
 drivers/net/wireless/d80211/zd1211rw/zd_usb.h  |    2 -
 6 files changed, 65 insertions(+), 73 deletions(-)

diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_chip.c 
b/drivers/net/wireless/d80211/zd1211rw/zd_chip.c
index daec7b6..97ff136 100644
--- a/drivers/net/wireless/d80211/zd1211rw/zd_chip.c
+++ b/drivers/net/wireless/d80211/zd1211rw/zd_chip.c
@@ -1321,7 +1321,7 @@ int zd_chip_control_leds(struct zd_chip
 	other_led = chip->link_led == LED1 ? LED2 : LED1;
 
 	switch (status) {
-	case LED_OFF:
+	case LED_DISABLED:
 		ioreqs[0].value = FW_LINK_OFF;
 		ioreqs[1].value = v[1] & ~(LED1|LED2);
 		break;
diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_chip.h 
b/drivers/net/wireless/d80211/zd1211rw/zd_chip.h
index b9c225d..970b920 100644
--- a/drivers/net/wireless/d80211/zd1211rw/zd_chip.h
+++ b/drivers/net/wireless/d80211/zd1211rw/zd_chip.h
@@ -821,7 +821,7 @@ int zd_chip_lock_phy_regs(struct zd_chip
 int zd_chip_unlock_phy_regs(struct zd_chip *chip);
 
 enum led_status {
-	LED_OFF = 0,
+	LED_DISABLED = 0, /* linux/leds.h took LED_OFF.. */
 	LED_SCANNING = 1,
 	LED_ASSOCIATED = 2,
 };
diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_mac.c 
b/drivers/net/wireless/d80211/zd1211rw/zd_mac.c
index 6ee650f..1c76e62 100644
--- a/drivers/net/wireless/d80211/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/d80211/zd1211rw/zd_mac.c
@@ -29,10 +29,6 @@
 #include "zd_rf.h"
 #include "zd_util.h"
 
-static void housekeeping_init(struct zd_mac *mac);
-static void housekeeping_enable(struct zd_mac *mac);
-static void housekeeping_disable(struct zd_mac *mac);
-
 int zd_mac_init_hw(struct ieee80211_hw *dev, u8 device_type)
 {
 	int r;
@@ -137,7 +133,9 @@ static int zd_mac_open(struct ieee80211_
 	if (r < 0)
 		goto disable_rx;
 
-	housekeeping_enable(mac);
+#ifdef CONFIG_LEDS_CLASS
+	led_classdev_resume(&mac->link_led);
+#endif
 	return 0;
 disable_rx:
 	zd_chip_disable_rx(chip);
@@ -161,8 +159,10 @@ static int zd_mac_stop(struct ieee80211_
 	 * frames to be processed by softmac after we have stopped it.
 	 */
 
+#ifdef CONFIG_LEDS_CLASS
+	led_classdev_suspend(&mac->link_led);
+#endif
 	zd_chip_disable_rx(chip);
-	housekeeping_disable(mac);
 
 	zd_chip_disable_hwint(chip);
 	zd_chip_switch_radio_off(chip);
@@ -552,10 +552,6 @@ static int zd_mac_config(struct ieee8021
 static int zd_mac_config_interface(struct ieee80211_hw *dev, int if_id,
 				   struct ieee80211_if_conf *conf)
 {
-	struct zd_mac *mac = zd_dev_mac(dev);
-
-	mac->associated = is_valid_ether_addr(conf->bssid);
-
 	/* TODO: do hardware bssid filtering */
 	return 0;
 }
@@ -570,6 +566,37 @@ static struct ieee80211_ops zd_ops = {
 	.config_interface	= zd_mac_config_interface,
 };
 
+#ifdef CONFIG_LEDS_CLASS
+static void set_link_led(struct led_classdev *led_cdev,
+			 enum led_brightness brightness)
+{
+	struct zd_mac *mac = container_of(led_cdev, struct zd_mac, link_led);
+	int r;
+
+	r = zd_chip_control_leds(&mac->chip,
+		                 brightness ? LED_ASSOCIATED : LED_DISABLED);
+	if (r)
+		dev_err(zd_mac_dev(mac), "zd_chip_control_leds error %d\n", r);
+}
+
+int zd_mac_register_led(struct ieee80211_hw *dev)
+{
+	struct zd_mac *mac = zd_dev_mac(dev);
+	snprintf(mac->link_led_name, sizeof(mac->link_led_name),
+	         "wiphy%dlink", dev->index);
+	mac->link_led.name = mac->link_led_name;
+	mac->link_led.brightness_set = set_link_led;
+	mac->link_led.flags = LED_SUSPENDED;
+	return led_classdev_register(dev->dev, &mac->link_led);
+}
+
+void zd_mac_unregister_led(struct ieee80211_hw *dev)
+{
+	struct zd_mac *mac = zd_dev_mac(dev);
+	led_classdev_unregister(&mac->link_led);
+}
+#endif
+
 struct ieee80211_hw *zd_mac_alloc(struct usb_interface *intf)
 {
 	struct zd_mac *mac;
@@ -616,51 +643,8 @@ struct ieee80211_hw *zd_mac_alloc(struct
 
 	skb_queue_head_init(&mac->tx_queue);
 	zd_chip_init(&mac->chip, dev, intf);
-	housekeeping_init(mac);
 
 	SET_MODULE_OWNER(dev);
 	dev->dev = &intf->dev;
 	return dev;
 }
-
-#define LINK_LED_WORK_DELAY HZ
-
-static void link_led_handler(void *p)
-{
-	struct zd_mac *mac = p;
-	struct zd_chip *chip = &mac->chip;
-	int is_associated;
-	int r;
-
-	spin_lock_irq(&mac->lock);
-	is_associated = mac->associated;
-	spin_unlock_irq(&mac->lock);
-
-	r = zd_chip_control_leds(chip,
-		                 is_associated ? LED_ASSOCIATED : LED_SCANNING);
-	if (r)
-		dev_err(zd_mac_dev(mac), "zd_chip_control_leds error %d\n", r);
-
-	queue_delayed_work(zd_workqueue, &mac->housekeeping.link_led_work,
-		           LINK_LED_WORK_DELAY);
-}
-
-static void housekeeping_init(struct zd_mac *mac)
-{
-	INIT_WORK(&mac->housekeeping.link_led_work, link_led_handler, mac);
-}
-
-static void housekeeping_enable(struct zd_mac *mac)
-{
-	dev_dbg_f(zd_mac_dev(mac), "\n");
-	queue_delayed_work(zd_workqueue, &mac->housekeeping.link_led_work,
-			   0);
-}
-
-static void housekeeping_disable(struct zd_mac *mac)
-{
-	dev_dbg_f(zd_mac_dev(mac), "\n");
-	cancel_rearming_delayed_workqueue(zd_workqueue,
-		&mac->housekeeping.link_led_work);
-	zd_chip_control_leds(&mac->chip, LED_OFF);
-}
diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_mac.h 
b/drivers/net/wireless/d80211/zd1211rw/zd_mac.h
index e2ba410..f5817aa 100644
--- a/drivers/net/wireless/d80211/zd1211rw/zd_mac.h
+++ b/drivers/net/wireless/d80211/zd1211rw/zd_mac.h
@@ -19,6 +19,7 @@
 #define _ZD_MAC_H
 
 #include <linux/kernel.h>
+#include <linux/leds.h>
 #include <net/d80211.h>
 
 #include "zd_chip.h"
@@ -118,22 +119,20 @@ enum mac_flags {
 	MAC_FIXED_CHANNEL = 0x01,
 };
 
-struct housekeeping {
-	struct work_struct link_led_work;
-};
-
 #define ZD_MAC_STATS_BUFFER_SIZE 16
 
 struct zd_mac {
 	struct zd_chip chip;
 	spinlock_t lock;
 	struct ieee80211_hw *dev;
-	struct housekeeping housekeeping;
+#ifdef CONFIG_LEDS_CLASS
+	char link_led_name[32];
+	struct led_classdev link_led;
+#endif
 	u8 regdomain;
 	u8 default_regdomain;
 	u8 requested_channel;
 	int mode;
-	int associated;
 	u8 *hwaddr;
 	struct sk_buff_head tx_queue;
 	struct ieee80211_channel channels[14];
@@ -159,6 +158,18 @@ static inline struct zd_mac *zd_usb_to_m
 #define zd_mac_dev(mac) (zd_chip_dev(&(mac)->chip))
 
 struct ieee80211_hw *zd_mac_alloc(struct usb_interface *intf);
+#ifdef CONFIG_LEDS_CLASS
+int zd_mac_register_led(struct ieee80211_hw *dev);
+void zd_mac_unregister_led(struct ieee80211_hw *dev);
+#else
+static inline int zd_mac_register_led(struct ieee80211_hw *dev)
+{
+	return 0;
+}
+static inline void zd_mac_unregister_led(struct ieee80211_hw *dev)
+{
+}
+#endif
 void zd_mac_clear(struct zd_mac *mac);
 
 int zd_mac_init_hw(struct ieee80211_hw *dev, u8 device_type);
diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_usb.c 
b/drivers/net/wireless/d80211/zd1211rw/zd_usb.c
index bacb019..171fb93 100644
--- a/drivers/net/wireless/d80211/zd1211rw/zd_usb.c
+++ b/drivers/net/wireless/d80211/zd1211rw/zd_usb.c
@@ -24,7 +24,6 @@
 #include <linux/errno.h>
 #include <linux/skbuff.h>
 #include <linux/usb.h>
-#include <linux/workqueue.h>
 #include <net/d80211.h>
 
 #include "zd_def.h"
@@ -1058,6 +1057,14 @@ static int probe(struct usb_interface *i
 		goto error;
 	}
 
+	r = zd_mac_register_led(dev);
+	if (r) {
+		dev_dbg_f(&intf->dev,
+			 "couldn't register LED. Error number %d\n", r);
+		ieee80211_unregister_hw(dev);
+		goto error;
+	}
+
 	dev_dbg_f(&intf->dev, "successful\n");
 	dev_info(&intf->dev,"wiphy%d\n", dev->index);
 	return 0;
@@ -1083,6 +1090,7 @@ static void disconnect(struct usb_interf
 
 	dev_dbg_f(zd_usb_dev(usb), "\n");
 
+	zd_mac_unregister_led(dev);
 	ieee80211_unregister_hw(dev);
 
 	/* Just in case something has gone wrong! */
@@ -1108,20 +1116,12 @@ static struct usb_driver driver = {
 	.disconnect	= disconnect,
 };
 
-struct workqueue_struct *zd_workqueue;
-
 static int __init usb_init(void)
 {
 	int r;
 
 	pr_debug("%s usb_init()\n", driver.name);
 
-	zd_workqueue = create_singlethread_workqueue(driver.name);
-	if (zd_workqueue == NULL) {
-		printk(KERN_ERR "%s couldn't create workqueue\n", driver.name);
-		return -ENOMEM;
-	}
-
 	r = usb_register(&driver);
 	if (r) {
 		printk(KERN_ERR "%s usb_register() failed. Error number %d\n",
@@ -1137,7 +1137,6 @@ static void __exit usb_exit(void)
 {
 	pr_debug("%s usb_exit()\n", driver.name);
 	usb_deregister(&driver);
-	destroy_workqueue(zd_workqueue);
 }
 
 module_init(usb_init);
diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_usb.h 
b/drivers/net/wireless/d80211/zd1211rw/zd_usb.h
index 1379e48..cda2259 100644
--- a/drivers/net/wireless/d80211/zd1211rw/zd_usb.h
+++ b/drivers/net/wireless/d80211/zd1211rw/zd_usb.h
@@ -238,6 +238,4 @@ int zd_usb_iowrite16v(struct zd_usb *usb
 
 int zd_usb_rfwrite(struct zd_usb *usb, u32 value, u8 bits);
 
-extern struct workqueue_struct *zd_workqueue;
-
 #endif /* _ZD_USB_H */
-- 
1.4.4.1


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] zd1211rw-d80211: Use LED class

[Ulrich Kunitz]

On 06-12-11 00:44 Michael Wu wrote:

> This makes zd1211rw-d80211 register an LED class to control the link LED
> instead of trying to determine when the LED should be on based on the
> current bssid. No default trigger is set since d80211 doesn't currently
> have a link on/off LED trigger.

I cannot accknowledge this patch. Michael, the LED does not only
show the link status, but also indicate that packets are sent,
which is done by the firmware. However the LED flags have to be
reset every second, because if an odd number of packets have been
transmitted, the LED stays dark. This is done by accessing
registers of the device, which requires process context to
implement synchronous access. From my point of view workqueue
functions are the only way to support this.

I even don't think, that the upper d80211 layers should care for
LED controlling. The LEDs are part of the device and should be
controlled by the device driver. I have a ZD1211 device, which has
two LEDs. Now suddenly the upper layer must know about the two
LEDs. But there could be WLAN devices who would have 6 LEDs for
whatever reason. Should the upper layer know about all those LEDs
and their semantics? The better design would be, that the driver
could register for upper layer events and control it's LEDs
accordingly. 

The zd1211rw driver has it's own workqueue to prevent crashes or
locks in our workqueue functions from taking the kernel down.
However a single workqueue is shared by all zd1211 devices
connected to the system. After the driver has stabilized we could
use the global Linux workqueues.

If you would also have taken the time to look at my out-of-tree
driver, you would have seen the other housekeeping/workqueue
functions for handling strong signals. These changes are still
missing from the kernel, because I have still to resolve issues
with AL2230 RFs. This again can only be implemented in workqueues,
because the register accesses require process context.

Regards,

Uli

> 
> Signed-off-by: Michael Wu <flamingice@sourmilk.net>
> ---
>  drivers/net/wireless/d80211/zd1211rw/zd_chip.c |    2 +-
>  drivers/net/wireless/d80211/zd1211rw/zd_chip.h |    2 +-
>  drivers/net/wireless/d80211/zd1211rw/zd_mac.c  |   90 
> ++++++++++--------------
>  drivers/net/wireless/d80211/zd1211rw/zd_mac.h  |   23 +++++--
>  drivers/net/wireless/d80211/zd1211rw/zd_usb.c  |   19 +++---
>  drivers/net/wireless/d80211/zd1211rw/zd_usb.h  |    2 -
>  6 files changed, 65 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_chip.c 
> b/drivers/net/wireless/d80211/zd1211rw/zd_chip.c
> index daec7b6..97ff136 100644
> --- a/drivers/net/wireless/d80211/zd1211rw/zd_chip.c
> +++ b/drivers/net/wireless/d80211/zd1211rw/zd_chip.c
> @@ -1321,7 +1321,7 @@ int zd_chip_control_leds(struct zd_chip
>  	other_led = chip->link_led == LED1 ? LED2 : LED1;
>  
>  	switch (status) {
> -	case LED_OFF:
> +	case LED_DISABLED:
>  		ioreqs[0].value = FW_LINK_OFF;
>  		ioreqs[1].value = v[1] & ~(LED1|LED2);
>  		break;
> diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_chip.h 
> b/drivers/net/wireless/d80211/zd1211rw/zd_chip.h
> index b9c225d..970b920 100644
> --- a/drivers/net/wireless/d80211/zd1211rw/zd_chip.h
> +++ b/drivers/net/wireless/d80211/zd1211rw/zd_chip.h
> @@ -821,7 +821,7 @@ int zd_chip_lock_phy_regs(struct zd_chip
>  int zd_chip_unlock_phy_regs(struct zd_chip *chip);
>  
>  enum led_status {
> -	LED_OFF = 0,
> +	LED_DISABLED = 0, /* linux/leds.h took LED_OFF.. */
>  	LED_SCANNING = 1,
>  	LED_ASSOCIATED = 2,
>  };
> diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_mac.c 
> b/drivers/net/wireless/d80211/zd1211rw/zd_mac.c
> index 6ee650f..1c76e62 100644
> --- a/drivers/net/wireless/d80211/zd1211rw/zd_mac.c
> +++ b/drivers/net/wireless/d80211/zd1211rw/zd_mac.c
> @@ -29,10 +29,6 @@
>  #include "zd_rf.h"
>  #include "zd_util.h"
>  
> -static void housekeeping_init(struct zd_mac *mac);
> -static void housekeeping_enable(struct zd_mac *mac);
> -static void housekeeping_disable(struct zd_mac *mac);
> -
>  int zd_mac_init_hw(struct ieee80211_hw *dev, u8 device_type)
>  {
>  	int r;
> @@ -137,7 +133,9 @@ static int zd_mac_open(struct ieee80211_
>  	if (r < 0)
>  		goto disable_rx;
>  
> -	housekeeping_enable(mac);
> +#ifdef CONFIG_LEDS_CLASS
> +	led_classdev_resume(&mac->link_led);
> +#endif
>  	return 0;
>  disable_rx:
>  	zd_chip_disable_rx(chip);
> @@ -161,8 +159,10 @@ static int zd_mac_stop(struct ieee80211_
>  	 * frames to be processed by softmac after we have stopped it.
>  	 */
>  
> +#ifdef CONFIG_LEDS_CLASS
> +	led_classdev_suspend(&mac->link_led);
> +#endif
>  	zd_chip_disable_rx(chip);
> -	housekeeping_disable(mac);
>  
>  	zd_chip_disable_hwint(chip);
>  	zd_chip_switch_radio_off(chip);
> @@ -552,10 +552,6 @@ static int zd_mac_config(struct ieee8021
>  static int zd_mac_config_interface(struct ieee80211_hw *dev, int if_id,
>  				   struct ieee80211_if_conf *conf)
>  {
> -	struct zd_mac *mac = zd_dev_mac(dev);
> -
> -	mac->associated = is_valid_ether_addr(conf->bssid);
> -
>  	/* TODO: do hardware bssid filtering */
>  	return 0;
>  }
> @@ -570,6 +566,37 @@ static struct ieee80211_ops zd_ops = {
>  	.config_interface	= zd_mac_config_interface,
>  };
>  
> +#ifdef CONFIG_LEDS_CLASS
> +static void set_link_led(struct led_classdev *led_cdev,
> +			 enum led_brightness brightness)
> +{
> +	struct zd_mac *mac = container_of(led_cdev, struct zd_mac, link_led);
> +	int r;
> +
> +	r = zd_chip_control_leds(&mac->chip,
> +		                 brightness ? LED_ASSOCIATED : LED_DISABLED);
> +	if (r)
> +		dev_err(zd_mac_dev(mac), "zd_chip_control_leds error %d\n", r);
> +}
> +
> +int zd_mac_register_led(struct ieee80211_hw *dev)
> +{
> +	struct zd_mac *mac = zd_dev_mac(dev);
> +	snprintf(mac->link_led_name, sizeof(mac->link_led_name),
> +	         "wiphy%dlink", dev->index);
> +	mac->link_led.name = mac->link_led_name;
> +	mac->link_led.brightness_set = set_link_led;
> +	mac->link_led.flags = LED_SUSPENDED;
> +	return led_classdev_register(dev->dev, &mac->link_led);
> +}
> +
> +void zd_mac_unregister_led(struct ieee80211_hw *dev)
> +{
> +	struct zd_mac *mac = zd_dev_mac(dev);
> +	led_classdev_unregister(&mac->link_led);
> +}
> +#endif
> +
>  struct ieee80211_hw *zd_mac_alloc(struct usb_interface *intf)
>  {
>  	struct zd_mac *mac;
> @@ -616,51 +643,8 @@ struct ieee80211_hw *zd_mac_alloc(struct
>  
>  	skb_queue_head_init(&mac->tx_queue);
>  	zd_chip_init(&mac->chip, dev, intf);
> -	housekeeping_init(mac);
>  
>  	SET_MODULE_OWNER(dev);
>  	dev->dev = &intf->dev;
>  	return dev;
>  }
> -
> -#define LINK_LED_WORK_DELAY HZ
> -
> -static void link_led_handler(void *p)
> -{
> -	struct zd_mac *mac = p;
> -	struct zd_chip *chip = &mac->chip;
> -	int is_associated;
> -	int r;
> -
> -	spin_lock_irq(&mac->lock);
> -	is_associated = mac->associated;
> -	spin_unlock_irq(&mac->lock);
> -
> -	r = zd_chip_control_leds(chip,
> -		                 is_associated ? LED_ASSOCIATED : LED_SCANNING);
> -	if (r)
> -		dev_err(zd_mac_dev(mac), "zd_chip_control_leds error %d\n", r);
> -
> -	queue_delayed_work(zd_workqueue, &mac->housekeeping.link_led_work,
> -		           LINK_LED_WORK_DELAY);
> -}
> -
> -static void housekeeping_init(struct zd_mac *mac)
> -{
> -	INIT_WORK(&mac->housekeeping.link_led_work, link_led_handler, mac);
> -}
> -
> -static void housekeeping_enable(struct zd_mac *mac)
> -{
> -	dev_dbg_f(zd_mac_dev(mac), "\n");
> -	queue_delayed_work(zd_workqueue, &mac->housekeeping.link_led_work,
> -			   0);
> -}
> -
> -static void housekeeping_disable(struct zd_mac *mac)
> -{
> -	dev_dbg_f(zd_mac_dev(mac), "\n");
> -	cancel_rearming_delayed_workqueue(zd_workqueue,
> -		&mac->housekeeping.link_led_work);
> -	zd_chip_control_leds(&mac->chip, LED_OFF);
> -}
> diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_mac.h 
> b/drivers/net/wireless/d80211/zd1211rw/zd_mac.h
> index e2ba410..f5817aa 100644
> --- a/drivers/net/wireless/d80211/zd1211rw/zd_mac.h
> +++ b/drivers/net/wireless/d80211/zd1211rw/zd_mac.h
> @@ -19,6 +19,7 @@
>  #define _ZD_MAC_H
>  
>  #include <linux/kernel.h>
> +#include <linux/leds.h>
>  #include <net/d80211.h>
>  
>  #include "zd_chip.h"
> @@ -118,22 +119,20 @@ enum mac_flags {
>  	MAC_FIXED_CHANNEL = 0x01,
>  };
>  
> -struct housekeeping {
> -	struct work_struct link_led_work;
> -};
> -
>  #define ZD_MAC_STATS_BUFFER_SIZE 16
>  
>  struct zd_mac {
>  	struct zd_chip chip;
>  	spinlock_t lock;
>  	struct ieee80211_hw *dev;
> -	struct housekeeping housekeeping;
> +#ifdef CONFIG_LEDS_CLASS
> +	char link_led_name[32];
> +	struct led_classdev link_led;
> +#endif
>  	u8 regdomain;
>  	u8 default_regdomain;
>  	u8 requested_channel;
>  	int mode;
> -	int associated;
>  	u8 *hwaddr;
>  	struct sk_buff_head tx_queue;
>  	struct ieee80211_channel channels[14];
> @@ -159,6 +158,18 @@ static inline struct zd_mac *zd_usb_to_m
>  #define zd_mac_dev(mac) (zd_chip_dev(&(mac)->chip))
>  
>  struct ieee80211_hw *zd_mac_alloc(struct usb_interface *intf);
> +#ifdef CONFIG_LEDS_CLASS
> +int zd_mac_register_led(struct ieee80211_hw *dev);
> +void zd_mac_unregister_led(struct ieee80211_hw *dev);
> +#else
> +static inline int zd_mac_register_led(struct ieee80211_hw *dev)
> +{
> +	return 0;
> +}
> +static inline void zd_mac_unregister_led(struct ieee80211_hw *dev)
> +{
> +}
> +#endif
>  void zd_mac_clear(struct zd_mac *mac);
>  
>  int zd_mac_init_hw(struct ieee80211_hw *dev, u8 device_type);
> diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_usb.c 
> b/drivers/net/wireless/d80211/zd1211rw/zd_usb.c
> index bacb019..171fb93 100644
> --- a/drivers/net/wireless/d80211/zd1211rw/zd_usb.c
> +++ b/drivers/net/wireless/d80211/zd1211rw/zd_usb.c
> @@ -24,7 +24,6 @@
>  #include <linux/errno.h>
>  #include <linux/skbuff.h>
>  #include <linux/usb.h>
> -#include <linux/workqueue.h>
>  #include <net/d80211.h>
>  
>  #include "zd_def.h"
> @@ -1058,6 +1057,14 @@ static int probe(struct usb_interface *i
>  		goto error;
>  	}
>  
> +	r = zd_mac_register_led(dev);
> +	if (r) {
> +		dev_dbg_f(&intf->dev,
> +			 "couldn't register LED. Error number %d\n", r);
> +		ieee80211_unregister_hw(dev);
> +		goto error;
> +	}
> +
>  	dev_dbg_f(&intf->dev, "successful\n");
>  	dev_info(&intf->dev,"wiphy%d\n", dev->index);
>  	return 0;
> @@ -1083,6 +1090,7 @@ static void disconnect(struct usb_interf
>  
>  	dev_dbg_f(zd_usb_dev(usb), "\n");
>  
> +	zd_mac_unregister_led(dev);
>  	ieee80211_unregister_hw(dev);
>  
>  	/* Just in case something has gone wrong! */
> @@ -1108,20 +1116,12 @@ static struct usb_driver driver = {
>  	.disconnect	= disconnect,
>  };
>  
> -struct workqueue_struct *zd_workqueue;
> -
>  static int __init usb_init(void)
>  {
>  	int r;
>  
>  	pr_debug("%s usb_init()\n", driver.name);
>  
> -	zd_workqueue = create_singlethread_workqueue(driver.name);
> -	if (zd_workqueue == NULL) {
> -		printk(KERN_ERR "%s couldn't create workqueue\n", driver.name);
> -		return -ENOMEM;
> -	}
> -
>  	r = usb_register(&driver);
>  	if (r) {
>  		printk(KERN_ERR "%s usb_register() failed. Error number %d\n",
> @@ -1137,7 +1137,6 @@ static void __exit usb_exit(void)
>  {
>  	pr_debug("%s usb_exit()\n", driver.name);
>  	usb_deregister(&driver);
> -	destroy_workqueue(zd_workqueue);
>  }
>  
>  module_init(usb_init);
> diff --git a/drivers/net/wireless/d80211/zd1211rw/zd_usb.h 
> b/drivers/net/wireless/d80211/zd1211rw/zd_usb.h
> index 1379e48..cda2259 100644
> --- a/drivers/net/wireless/d80211/zd1211rw/zd_usb.h
> +++ b/drivers/net/wireless/d80211/zd1211rw/zd_usb.h
> @@ -238,6 +238,4 @@ int zd_usb_iowrite16v(struct zd_usb *usb
>  
>  int zd_usb_rfwrite(struct zd_usb *usb, u32 value, u8 bits);
>  
> -extern struct workqueue_struct *zd_workqueue;
> -
>  #endif /* _ZD_USB_H */
> -- 
> 1.4.4.1
> 



-- 
Uli Kunitz

Re: [PATCH] zd1211rw-d80211: Use LED class

[Michael Wu]

[-- Attachment #1: Type: text/plain, Size: 2765 bytes --]

On Monday 11 December 2006 17:55, Ulrich Kunitz wrote:
> I cannot accknowledge this patch. Michael, the LED does not only
> show the link status, but also indicate that packets are sent,
> which is done by the firmware. However the LED flags have to be
> reset every second, because if an odd number of packets have been
> transmitted, the LED stays dark.
The d80211 TX trigger has the same behavior. It's probably worse on single LED 
devices since it may look like nothing is on. Even then, I think having it 
reset every second is overkill. Having an "TX LED" watchdog run when we 
haven't TXed anything for the last second or sounds better to me. This would 
force the LEDs to a reasonable state when we know nothing has been TXed 
recently. This could be for STA mode only.. I dunno if the LED blinks when 
sending hardware generated/filled beacons.

> I even don't think, that the upper d80211 layers should care for
> LED controlling. The LEDs are part of the device and should be
> controlled by the device driver. I have a ZD1211 device, which has
> two LEDs. Now suddenly the upper layer must know about the two
> LEDs. But there could be WLAN devices who would have 6 LEDs for
> whatever reason. Should the upper layer know about all those LEDs
> and their semantics? The better design would be, that the driver
> could register for upper layer events and control it's LEDs
> accordingly.
>
The upper layer does not know about the LEDs of individual devices. It merely 
provides a number of LED triggers which device drivers can choose to listen 
to - which sounds like what you're suggesting.

> If you would also have taken the time to look at my out-of-tree
> driver, you would have seen the other housekeeping/workqueue
> functions for handling strong signals. These changes are still
> missing from the kernel, because I have still to resolve issues
> with AL2230 RFs. This again can only be implemented in workqueues,
> because the register accesses require process context.
>
Switching to using LED class makes the housekeeping & workqueue code not do 
anything useful, so I removed it. I am not against the housekeeping & 
workqueue code - but right now this is dead code that will unnecessarily 
cause zd1211rw-d80211 not to compile when wireless-dev gets the workqueue api 
changes. (of course, d80211 doesn't compile either with the workqueue 
changes, but I'm working out a patch for that..)

If you are testing your new RF code with zd1211rw-d80211 also, I can add the 
workqueue & housekeeping code back as soon as things are sorted out in 
wireless-dev. I will also probably port the rts/cts code after wireless-dev 
deals with the workqueue changes.

Sound tolerable to you?

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] zd1211rw-d80211: Use LED class

[Ulrich Kunitz]

On 06-12-11 20:32 Michael Wu wrote:

> On Monday 11 December 2006 17:55, Ulrich Kunitz wrote:
> > I cannot accknowledge this patch. Michael, the LED does not only
> > show the link status, but also indicate that packets are sent,
> > which is done by the firmware. However the LED flags have to be
> > reset every second, because if an odd number of packets have been
> > transmitted, the LED stays dark.
> The d80211 TX trigger has the same behavior. It's probably worse on single LED 
> devices since it may look like nothing is on. Even then, I think having it 
> reset every second is overkill. Having an "TX LED" watchdog run when we 
> haven't TXed anything for the last second or sounds better to me. This would 
> force the LEDs to a reasonable state when we know nothing has been TXed 
> recently. This could be for STA mode only.. I dunno if the LED blinks when 
> sending hardware generated/filled beacons.

Micheal, how resetting an LED every second is overkill is beyond
me. On a USB 2.0 port the available bandwidth is much higher than
on the air. Notify also that the LED is flip-flopped on tx packets
(which might include ACKs), so you simply don't know, when it
becomes dark. If nothing is transmitted the LED stays on, but if
something is transmitted you might have to guess. That's the
reason why I added the reset code. BTW the driver makes the LED
blinks in a certain sequence (1s on, 2s off) while scanning. It is
quite useful for debugging. You cannot do that without workqueues.

> The upper layer does not know about the LEDs of individual devices. It merely 
> provides a number of LED triggers which device drivers can choose to listen 
> to - which sounds like what you're suggesting.

No, this is not true. The upper layer has to support a Link-LED
trigger. I believe that event triggers are a generic concept and
should not be LED-specific. These triggers should inform about
LINK status changes and the driver has to know, whether it has a
LINK LED to switch or a LCD to display some information. There
might be also other uses than for switching LEDs.

> Switching to using LED class makes the housekeeping & workqueue code not do 
> anything useful, so I removed it. I am not against the housekeeping & 
> workqueue code - but right now this is dead code that will unnecessarily 
> cause zd1211rw-d80211 not to compile when wireless-dev gets the workqueue api 
> changes. (of course, d80211 doesn't compile either with the workqueue 
> changes, but I'm working out a patch for that..)

No, it is not dead code as I explained.  Your patch is removing
functionality and if I undestand right, might work in the future.
But it is not tested and doesn't care for the activity
flip-flopping.

Regards,

Uli


-- 
Uli Kunitz

Re: [PATCH] zd1211rw-d80211: Use LED class

[Michael Wu]

[-- Attachment #1: Type: text/plain, Size: 3513 bytes --]

On Tuesday 12 December 2006 17:38, Ulrich Kunitz wrote:
> Micheal, how resetting an LED every second is overkill is beyond
> me.
I mean it is overkill in that is more than needed.

> On a USB 2.0 port the available bandwidth is much higher than 
> on the air. Notify also that the LED is flip-flopped on tx packets
> (which might include ACKs), so you simply don't know, when it
> becomes dark.
Ah, but if we transmit an ACK, we know because we receive a unicast frame. And 
then we can rearm the TX LED reset timer. Any other cases?

> If nothing is transmitted the LED stays on, but if 
> something is transmitted you might have to guess. That's the
> reason why I added the reset code. BTW the driver makes the LED
> blinks in a certain sequence (1s on, 2s off) while scanning. It is
> quite useful for debugging. You cannot do that without workqueues.
>
I don't think being able to flash pretty patterns while scanning is very 
convincing reason to keep this. ;)

> > The upper layer does not know about the LEDs of individual devices. It
> > merely provides a number of LED triggers which device drivers can choose
> > to listen to - which sounds like what you're suggesting.
>
> No, this is not true. The upper layer has to support a Link-LED
> trigger. I believe that event triggers are a generic concept and
> should not be LED-specific. These triggers should inform about
> LINK status changes and the driver has to know, whether it has a
> LINK LED to switch or a LCD to display some information. There
> might be also other uses than for switching LEDs.
>
Yes, userspace can do that by listening for the SIOCGIWAP wireless event. LED 
triggers allow this to be done easily by default with no userspace 
intervention. A link LED trigger would be added in addition to the usual 
wireless event.

> > Switching to using LED class makes the housekeeping & workqueue code not
> > do anything useful, so I removed it. I am not against the housekeeping &
> > workqueue code - but right now this is dead code that will unnecessarily
> > cause zd1211rw-d80211 not to compile when wireless-dev gets the workqueue
> > api changes. (of course, d80211 doesn't compile either with the workqueue
> > changes, but I'm working out a patch for that..)
>
> No, it is not dead code as I explained.  Your patch is removing
> functionality and if I undestand right, might work in the future.
> But it is not tested and doesn't care for the activity
> flip-flopping.
>
It has been tested to the point where I can manually control the state of the 
LED from userspace via sysfs. I had to switch to using LED_OFF (or rather, 
LED_DISABLED now) since the blinking behavior of LED_SCANNING made turning 
off the LED not guaranteed. Turning off the link LED thus forces the activity 
light off also, but that minor and can be easily fixed.

The activity flip flopping can be fixed as I mentioned further above, and I do 
intend to make this all work (eg. link LED trigger) as I need p54 to support 
blinking its LEDs. Do you really mind breaking the LED blinking for now? How 
many users are going to be affected while things are being sorted out?

If you really think the idea of LED classes and LED triggers is that 
incredibly bad, I will drop this patch and just fix the workqueues. However, 
as far as I can tell, it helps move policy out of the kernel while allowing 
the most common use case to continue to work without userspace intervention.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] zd1211rw-d80211: Use LED class

[Ulrich Kunitz]

Michael,

please detach a little bit from LEDs.

A trigger is a generic concept. It doesn't need to have a special
binding to what it triggers. It could be a buzzer, it could be
an LED, it could be text output on an LCD.

So we could have link status change trigger, for which functions
could be registered. There is no reason for the upper layer d80211
code to know, that it triggers an LED. A netlink event generator
could only be one of the users of those generic triggers.

My other opinion is that I find it overkill to handle the LED on a
WLAN adapter as an independent device instantiating a device class
in Linux. I mean on the other hands we break APIs for four or
eight bytes and here we spend a whole device strucuture and sysfs
directory for something the wlan device firmware represents with a
single bit. To call the housekeeping functions overkill under
these circumstances appears not to be very rational.

It should also be mentioned that the LEDs on the ZD1211 devices
are not independent devices, because they are partly controlled
by the firmware. That's exactly where some of the issues with your
patch are.

Let the device drivers do the blinking, but give them a chance to
know about status changes at the MAC layer, without mandating to
implement a LED device. Using the same triggers by a netlink event
generator, would make the code even simpler.

-- 
Uli Kunitz

Re: [PATCH] zd1211rw-d80211: Use LED class

[Michael Wu]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

On Wednesday 13 December 2006 17:54, Ulrich Kunitz wrote:
> I mean on the other hands we break APIs for four or 
> eight bytes and here we spend a whole device strucuture and sysfs
> directory for something the wlan device firmware represents with a
> single bit. To call the housekeeping functions overkill under
> these circumstances appears not to be very rational.
>
I do not consider the housekeeping functions overkill, but the function that 
it performs (eg. not confusing the user by keeping the LED off) can be done 
with less brute force.

> It should also be mentioned that the LEDs on the ZD1211 devices
> are not independent devices, because they are partly controlled
> by the firmware. That's exactly where some of the issues with your
> patch are.
>
The link light can be treated as an independent device as long as the other 
LED bit is not flipped. As I mentioned before, fixing that isn't a problem.

> Let the device drivers do the blinking, but give them a chance to
> know about status changes at the MAC layer, without mandating to
> implement a LED device. 
I guess you don't like the LED trigger code currently in d80211 then and would 
rather see it removed. As far as I can tell, this patch is just a symptom of 
that issue.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]