[PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

Hi!

Here is the patch to disable ath5k leds support on build stage.
However if the leds support was enabled do not force selection of 802.11 leds layer.
Depency on LEDS_CLASS is kept.

Suggestion given by Pavel Roskin and Bob Copeland applied.

Changes from the previos version of the patch:
 - Use bool directive in Kconfig for switching LEDs support
 - Move ath5k_hw_set_ledstate into led.c and change its semantic to align with other leds routines

Regards,

--Dima

Signed-off-by: Dmytro Milinevskyy <milinevskyy@gmail.com>
---
 drivers/net/wireless/ath/ath5k/Kconfig  |   12 +++++--
 drivers/net/wireless/ath/ath5k/Makefile |    2 +-
 drivers/net/wireless/ath/ath5k/ath5k.h  |   21 +++++++++++-
 drivers/net/wireless/ath/ath5k/attach.c |    2 +-
 drivers/net/wireless/ath/ath5k/base.c   |    6 ++--
 drivers/net/wireless/ath/ath5k/base.h   |   11 ++++--
 drivers/net/wireless/ath/ath5k/gpio.c   |   52 ------------------------------
 drivers/net/wireless/ath/ath5k/led.c    |   53 +++++++++++++++++++++++++++++++
 8 files changed, 94 insertions(+), 65 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/Kconfig b/drivers/net/wireless/ath/ath5k/Kconfig
index eb83b7b..6b2a682 100644
--- a/drivers/net/wireless/ath/ath5k/Kconfig
+++ b/drivers/net/wireless/ath/ath5k/Kconfig
@@ -1,9 +1,6 @@
 config ATH5K
 	tristate "Atheros 5xxx wireless cards support"
 	depends on PCI && MAC80211
-	select MAC80211_LEDS
-	select LEDS_CLASS
-	select NEW_LEDS
 	---help---
 	  This module adds support for wireless adapters based on
 	  Atheros 5xxx chipset.
@@ -18,6 +15,15 @@ config ATH5K
 	  If you choose to build a module, it'll be called ath5k. Say M if
 	  unsure.
 
+
+config ATH5K_LEDS
+	bool "Atheros 5xxx wireless cards LEDs support"
+	depends on ATH5K
+	select NEW_LEDS
+	select LEDS_CLASS
+	---help---
+	  Atheros 5xxx LED support.
+
 config ATH5K_DEBUG
 	bool "Atheros 5xxx debugging"
 	depends on ATH5K
diff --git a/drivers/net/wireless/ath/ath5k/Makefile b/drivers/net/wireless/ath/ath5k/Makefile
index cc09595..6d552dd 100644
--- a/drivers/net/wireless/ath/ath5k/Makefile
+++ b/drivers/net/wireless/ath/ath5k/Makefile
@@ -10,8 +10,8 @@ ath5k-y				+= phy.o
 ath5k-y				+= reset.o
 ath5k-y				+= attach.o
 ath5k-y				+= base.o
-ath5k-y				+= led.o
 ath5k-y				+= rfkill.o
 ath5k-y				+= ani.o
 ath5k-$(CONFIG_ATH5K_DEBUG)	+= debug.o
+ath5k-$(CONFIG_ATH5K_LEDS) += led.o
 obj-$(CONFIG_ATH5K)		+= ath5k.o
diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 2785946..202d143 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1148,11 +1148,31 @@ struct ath5k_hw {
 int ath5k_hw_attach(struct ath5k_softc *sc);
 void ath5k_hw_detach(struct ath5k_hw *ah);
 
+#ifdef CONFIG_ATH5K_LEDS
 /* LED functions */
+void ath5k_hw_set_ledstate(struct ath5k_softc *sc, unsigned int state);
 int ath5k_init_leds(struct ath5k_softc *sc);
 void ath5k_led_enable(struct ath5k_softc *sc);
 void ath5k_led_off(struct ath5k_softc *sc);
 void ath5k_unregister_leds(struct ath5k_softc *sc);
+#else
+static inline void ath5k_hw_set_ledstate(struct ath5k_softc *sc, unsigned int state)
+{
+}
+static inline int ath5k_init_leds(struct ath5k_softc *sc)
+{
+	return 0;
+}
+static inline void ath5k_led_enable(struct ath5k_softc *sc)
+{
+}
+static inline void ath5k_led_off(struct ath5k_softc *sc)
+{
+}
+static inline void ath5k_unregister_leds(struct ath5k_softc *sc)
+{
+}
+#endif
 
 /* Reset Functions */
 int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial);
@@ -1233,7 +1253,6 @@ int ath5k_hw_set_slot_time(struct ath5k_hw *ah, unsigned int slot_time);
 int ath5k_hw_init_desc_functions(struct ath5k_hw *ah);
 
 /* GPIO Functions */
-void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state);
 int ath5k_hw_set_gpio_input(struct ath5k_hw *ah, u32 gpio);
 int ath5k_hw_set_gpio_output(struct ath5k_hw *ah, u32 gpio);
 u32 ath5k_hw_get_gpio(struct ath5k_hw *ah, u32 gpio);
diff --git a/drivers/net/wireless/ath/ath5k/attach.c b/drivers/net/wireless/ath/ath5k/attach.c
index e0c244b..7930a39 100644
--- a/drivers/net/wireless/ath/ath5k/attach.c
+++ b/drivers/net/wireless/ath/ath5k/attach.c
@@ -336,7 +336,7 @@ int ath5k_hw_attach(struct ath5k_softc *sc)
 	ath5k_hw_init_nfcal_hist(ah);
 
 	/* turn on HW LEDs */
-	ath5k_hw_set_ledstate(ah, AR5K_LED_INIT);
+	ath5k_hw_set_ledstate(sc, AR5K_LED_INIT);
 
 	return 0;
 err_free:
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 2978359..64ea83d 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -3411,7 +3411,7 @@ static void ath5k_bss_info_changed(struct ieee80211_hw *hw,
 		sc->assoc = bss_conf->assoc;
 		if (sc->opmode == NL80211_IFTYPE_STATION)
 			set_beacon_filter(hw, sc->assoc);
-		ath5k_hw_set_ledstate(sc->ah, sc->assoc ?
+		ath5k_hw_set_ledstate(sc, sc->assoc ?
 			AR5K_LED_ASSOC : AR5K_LED_INIT);
 		if (bss_conf->assoc) {
 			ATH5K_DBG(sc, ATH5K_DEBUG_ANY,
@@ -3444,13 +3444,13 @@ static void ath5k_sw_scan_start(struct ieee80211_hw *hw)
 {
 	struct ath5k_softc *sc = hw->priv;
 	if (!sc->assoc)
-		ath5k_hw_set_ledstate(sc->ah, AR5K_LED_SCAN);
+		ath5k_hw_set_ledstate(sc, AR5K_LED_SCAN);
 }
 
 static void ath5k_sw_scan_complete(struct ieee80211_hw *hw)
 {
 	struct ath5k_softc *sc = hw->priv;
-	ath5k_hw_set_ledstate(sc->ah, sc->assoc ?
+	ath5k_hw_set_ledstate(sc, sc->assoc ?
 		AR5K_LED_ASSOC : AR5K_LED_INIT);
 }
 
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 56221bc..8d289a3 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -186,9 +186,6 @@ struct ath5k_softc {
 
 	u8			bssidmask[ETH_ALEN];
 
-	unsigned int		led_pin,	/* GPIO pin for driving LED */
-				led_on;		/* pin setting for LED on */
-
 	struct tasklet_struct	restq;		/* reset tasklet */
 
 	unsigned int		rxbufsize;	/* rx size based on mtu */
@@ -196,7 +193,6 @@ struct ath5k_softc {
 	spinlock_t		rxbuflock;
 	u32			*rxlink;	/* link ptr in last RX desc */
 	struct tasklet_struct	rxtq;		/* rx intr tasklet */
-	struct ath5k_led	rx_led;		/* rx led */
 
 	struct list_head	txbuf;		/* transmit buffer */
 	spinlock_t		txbuflock;
@@ -204,7 +200,14 @@ struct ath5k_softc {
 	struct ath5k_txq	txqs[AR5K_NUM_TX_QUEUES];	/* tx queues */
 	struct ath5k_txq	*txq;		/* main tx queue */
 	struct tasklet_struct	txtq;		/* tx intr tasklet */
+
+
+#ifdef CONFIG_ATH5K_LEDS
+	unsigned int		led_pin,	/* GPIO pin for driving LED */
+				led_on;		/* pin setting for LED on */
+	struct ath5k_led	rx_led;		/* rx led */
 	struct ath5k_led	tx_led;		/* tx led */
+#endif
 
 	struct ath5k_rfkill	rf_kill;
 
diff --git a/drivers/net/wireless/ath/ath5k/gpio.c b/drivers/net/wireless/ath/ath5k/gpio.c
index 64a27e7..1e8b145 100644
--- a/drivers/net/wireless/ath/ath5k/gpio.c
+++ b/drivers/net/wireless/ath/ath5k/gpio.c
@@ -26,58 +26,6 @@
 #include "base.h"
 
 /*
- * Set led state
- */
-void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state)
-{
-	u32 led;
-	/*5210 has different led mode handling*/
-	u32 led_5210;
-
-	ATH5K_TRACE(ah->ah_sc);
-
-	/*Reset led status*/
-	if (ah->ah_version != AR5K_AR5210)
-		AR5K_REG_DISABLE_BITS(ah, AR5K_PCICFG,
-			AR5K_PCICFG_LEDMODE |  AR5K_PCICFG_LED);
-	else
-		AR5K_REG_DISABLE_BITS(ah, AR5K_PCICFG, AR5K_PCICFG_LED);
-
-	/*
-	 * Some blinking values, define at your wish
-	 */
-	switch (state) {
-	case AR5K_LED_SCAN:
-	case AR5K_LED_AUTH:
-		led = AR5K_PCICFG_LEDMODE_PROP | AR5K_PCICFG_LED_PEND;
-		led_5210 = AR5K_PCICFG_LED_PEND | AR5K_PCICFG_LED_BCTL;
-		break;
-
-	case AR5K_LED_INIT:
-		led = AR5K_PCICFG_LEDMODE_PROP | AR5K_PCICFG_LED_NONE;
-		led_5210 = AR5K_PCICFG_LED_PEND;
-		break;
-
-	case AR5K_LED_ASSOC:
-	case AR5K_LED_RUN:
-		led = AR5K_PCICFG_LEDMODE_PROP | AR5K_PCICFG_LED_ASSOC;
-		led_5210 = AR5K_PCICFG_LED_ASSOC;
-		break;
-
-	default:
-		led = AR5K_PCICFG_LEDMODE_PROM | AR5K_PCICFG_LED_NONE;
-		led_5210 = AR5K_PCICFG_LED_PEND;
-		break;
-	}
-
-	/*Write new status to the register*/
-	if (ah->ah_version != AR5K_AR5210)
-		AR5K_REG_ENABLE_BITS(ah, AR5K_PCICFG, led);
-	else
-		AR5K_REG_ENABLE_BITS(ah, AR5K_PCICFG, led_5210);
-}
-
-/*
  * Set GPIO inputs
  */
 int ath5k_hw_set_gpio_input(struct ath5k_hw *ah, u32 gpio)
diff --git a/drivers/net/wireless/ath/ath5k/led.c b/drivers/net/wireless/ath/ath5k/led.c
index 67aa52e..9cff1cf 100644
--- a/drivers/net/wireless/ath/ath5k/led.c
+++ b/drivers/net/wireless/ath/ath5k/led.c
@@ -41,6 +41,7 @@
 
 #include <linux/pci.h>
 #include "ath5k.h"
+#include "reg.h"
 #include "base.h"
 
 #define ATH_SDEVICE(subv,subd) \
@@ -86,6 +87,58 @@ static const struct pci_device_id ath5k_led_devices[] = {
 	{ }
 };
 
+/*
+ * Set led state
+ */
+void ath5k_hw_set_ledstate(struct ath5k_softc *sc, unsigned int state)
+{
+	u32 led;
+	/*5210 has different led mode handling*/
+	u32 led_5210;
+
+	ATH5K_TRACE(sc);
+
+	/*Reset led status*/
+	if (sc->ah->ah_version != AR5K_AR5210)
+		AR5K_REG_DISABLE_BITS(sc->ah, AR5K_PCICFG,
+			AR5K_PCICFG_LEDMODE |  AR5K_PCICFG_LED);
+	else
+		AR5K_REG_DISABLE_BITS(sc->ah, AR5K_PCICFG, AR5K_PCICFG_LED);
+
+	/*
+	 * Some blinking values, define at your wish
+	 */
+	switch (state) {
+	case AR5K_LED_SCAN:
+	case AR5K_LED_AUTH:
+		led = AR5K_PCICFG_LEDMODE_PROP | AR5K_PCICFG_LED_PEND;
+		led_5210 = AR5K_PCICFG_LED_PEND | AR5K_PCICFG_LED_BCTL;
+		break;
+
+	case AR5K_LED_INIT:
+		led = AR5K_PCICFG_LEDMODE_PROP | AR5K_PCICFG_LED_NONE;
+		led_5210 = AR5K_PCICFG_LED_PEND;
+		break;
+
+	case AR5K_LED_ASSOC:
+	case AR5K_LED_RUN:
+		led = AR5K_PCICFG_LEDMODE_PROP | AR5K_PCICFG_LED_ASSOC;
+		led_5210 = AR5K_PCICFG_LED_ASSOC;
+		break;
+
+	default:
+		led = AR5K_PCICFG_LEDMODE_PROM | AR5K_PCICFG_LED_NONE;
+		led_5210 = AR5K_PCICFG_LED_PEND;
+		break;
+	}
+
+	/*Write new status to the register*/
+	if (sc->ah->ah_version != AR5K_AR5210)
+		AR5K_REG_ENABLE_BITS(sc->ah, AR5K_PCICFG, led);
+	else
+		AR5K_REG_ENABLE_BITS(sc->ah, AR5K_PCICFG, led_5210);
+}
+
 void ath5k_led_enable(struct ath5k_softc *sc)
 {
 	if (test_bit(ATH_STAT_LEDSOFT, sc->status)) {
-- 
1.7.1

Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Bob Copeland]

On Wed, Jun 9, 2010 at 3:31 AM, Dmytro Milinevskyy
<milinevskyy@gmail.com> wrote:
> However if the leds support was enabled do not force selection of 802.11 leds layer.

I don't understand this part.  What's the point of enabling software LEDs
if nothing triggers them?

Also, we can probably build hardware LEDs (hw_set_ledstate) in regardless.
It's a pure register write and doesn't require the rest of the LED machinery.

I assume you are doing this to reduce the size of the module.  If so, can
you include size(1) output in the commit message?

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

Hi, Bob.

For instance I don't use 802.11 leds layer and trigger leds from
userspace according to my purposes.

-- Dima

On Wed, Jun 9, 2010 at 4:58 PM, Bob Copeland <me@bobcopeland.com> wrote:
> On Wed, Jun 9, 2010 at 3:31 AM, Dmytro Milinevskyy
> <milinevskyy@gmail.com> wrote:
>> However if the leds support was enabled do not force selection of 802.11 leds layer.
>
> I don't understand this part.  What's the point of enabling software LEDs
> if nothing triggers them?
>
> Also, we can probably build hardware LEDs (hw_set_ledstate) in regardless.
> It's a pure register write and doesn't require the rest of the LED machinery.
>
> I assume you are doing this to reduce the size of the module.  If so, can
> you include size(1) output in the commit message?
>
> --
> Bob Copeland %% www.bobcopeland.com
>

Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Bob Copeland]

On Wed, Jun 9, 2010 at 10:43 AM, Dmytro Milinevskyy
<milinevskyy@gmail.com> wrote:
> Hi, Bob.
>
> For instance I don't use 802.11 leds layer and trigger leds from
> userspace according to my purposes.

FWIW you can disable mac80211 triggers from userspace as well.
But, I guess hooking up null triggers will work too.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

I didn't know. Thank you for noting that.
However I think it's better to give a chance to disable 802.11 leds.

-- Dima

On Fri, Jun 11, 2010 at 8:07 PM, Bob Copeland <me@bobcopeland.com> wrote:
> On Wed, Jun 9, 2010 at 10:43 AM, Dmytro Milinevskyy
> <milinevskyy@gmail.com> wrote:
>> Hi, Bob.
>>
>> For instance I don't use 802.11 leds layer and trigger leds from
>> userspace according to my purposes.
>
> FWIW you can disable mac80211 triggers from userspace as well.
> But, I guess hooking up null triggers will work too.
>
> --
> Bob Copeland %% www.bobcopeland.com
>

Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Johannes Berg]

On Wed, 2010-06-09 at 17:43 +0300, Dmytro Milinevskyy wrote:
> Hi, Bob.
> 
> For instance I don't use 802.11 leds layer and trigger leds from
> userspace according to my purposes.

Without the LED stuff in sysfs, how do you do that?

johannes

Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

Generic LED class is not disabled so it's possible to control leds in sysfs.

-- Dima

On Fri, Jun 11, 2010 at 11:09 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2010-06-09 at 17:43 +0300, Dmytro Milinevskyy wrote:
>> Hi, Bob.
>>
>> For instance I don't use 802.11 leds layer and trigger leds from
>> userspace according to my purposes.
>
> Without the LED stuff in sysfs, how do you do that?
>
> johannes
>
>

Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Johannes Berg]

On Fri, 2010-06-11 at 23:26 +0300, Dmytro Milinevskyy wrote:
> Generic LED class is not disabled so it's possible to control leds in sysfs.

But if you turn off ATH5K_LEDS??

johannes

Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

In this case no way to control LEDs. If you turn LEDs support you
can't tune them out, no?

I believe that user should be able to chose whether LEDs support is
needed or not. This is how done in intel wireless drivers. The driver
should be tolerant.

-- Dima

On Fri, Jun 11, 2010 at 11:29 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2010-06-11 at 23:26 +0300, Dmytro Milinevskyy wrote:
>> Generic LED class is not disabled so it's possible to control leds in sysfs.
>
> But if you turn off ATH5K_LEDS??
>
> johannes
>
>

Re: [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

Pavel, thank you for the response here.

I agree with all your comments and will adjust the patch according to them.

I'm new to the submitting patches into the community and I appreciate
telling criticism so that in future I will not cause that much
troubles.

Regards,

-- Dima

On Fri, Jun 4, 2010 at 7:22 PM, Pavel Roskin <proski@gnu.org> wrote:
> On Fri, 2010-06-04 at 16:43 +0300, Dmytro Milinevskyy wrote:
>> Hi!
>>
>> Here is the patch to disable ath5k leds support on build stage.
>> However if the leds support was enabled do not force selection of 802.11 leds layer.
>> Depency on LEDS_CLASS is kept.
>>
>> Suggestion given by Pavel Roskin and Bob Copeland applied.
>
> It's great that you did it.  The patch is much clearer now.  That makes
> smaller issues visible.  Please don't be discouraged by the criticism,
> you are on the right track.
>
> First of all, your patch doesn't apply cleanly to the current
> wireless-testing because of formatting changes in Makefile.  Please
> update.
>
>> +config ATH5K_LEDS
>> +       tristate "Atheros 5xxx wireless cards LEDs support"
>> +       depends on ATH5K
>> +       select NEW_LEDS
>> +       select LEDS_CLASS
>> +       ---help---
>> +         Atheros 5xxx LED support.
>
> "tristate" is wrong here.  "tristate" would allow users select "m",
> which is wrong, since LED support is not a separate module.  I think you
> want "bool" here.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>  /*
>>   * State for LED triggers
>>   */
>> @@ -95,6 +96,7 @@ struct ath5k_led
>>       struct ath5k_softc *sc;                 /* driver state */
>>       struct led_classdev led_dev;            /* led classdev */
>>  };
>> +#endif
>
> This shouldn't be needed.  I'll rather see a structure that is not used
> in some cases than an extra pair of preprocessor conditionals.
>
>> diff --git a/drivers/net/wireless/ath/ath5k/gpio.c b/drivers/net/wireless/ath/ath5k/gpio.c
>> index 64a27e7..9e757b3 100644
>> --- a/drivers/net/wireless/ath/ath5k/gpio.c
>> +++ b/drivers/net/wireless/ath/ath5k/gpio.c
>> @@ -25,6 +25,7 @@
>>  #include "debug.h"
>>  #include "base.h"
>>
>> +#ifdef CONFIG_ATH5K_LEDS
>>  /*
>>   * Set led state
>>   */
>> @@ -76,6 +77,7 @@ void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state)
>>       else
>>               AR5K_REG_ENABLE_BITS(ah, AR5K_PCICFG, led_5210);
>>  }
>> +#endif
>
> I would just move that function to led.c (and don't forget to include
> reg.h).  The Makefile should take care of the rest.
>
> --
> Regards,
> Pavel Roskin
>

Re: [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Pavel Roskin]

On Fri, 2010-06-04 at 16:43 +0300, Dmytro Milinevskyy wrote:
> Hi!
> 
> Here is the patch to disable ath5k leds support on build stage.
> However if the leds support was enabled do not force selection of 802.11 leds layer.
> Depency on LEDS_CLASS is kept.
> 
> Suggestion given by Pavel Roskin and Bob Copeland applied.

It's great that you did it.  The patch is much clearer now.  That makes
smaller issues visible.  Please don't be discouraged by the criticism,
you are on the right track.

First of all, your patch doesn't apply cleanly to the current
wireless-testing because of formatting changes in Makefile.  Please
update.

> +config ATH5K_LEDS
> +       tristate "Atheros 5xxx wireless cards LEDs support"
> +       depends on ATH5K
> +       select NEW_LEDS
> +       select LEDS_CLASS
> +       ---help---
> +         Atheros 5xxx LED support.

"tristate" is wrong here.  "tristate" would allow users select "m",
which is wrong, since LED support is not a separate module.  I think you
want "bool" here.

> +#ifdef CONFIG_ATH5K_LEDS
>  /*
>   * State for LED triggers
>   */
> @@ -95,6 +96,7 @@ struct ath5k_led
>  	struct ath5k_softc *sc;			/* driver state */
>  	struct led_classdev led_dev;		/* led classdev */
>  };
> +#endif

This shouldn't be needed.  I'll rather see a structure that is not used
in some cases than an extra pair of preprocessor conditionals.

> diff --git a/drivers/net/wireless/ath/ath5k/gpio.c b/drivers/net/wireless/ath/ath5k/gpio.c
> index 64a27e7..9e757b3 100644
> --- a/drivers/net/wireless/ath/ath5k/gpio.c
> +++ b/drivers/net/wireless/ath/ath5k/gpio.c
> @@ -25,6 +25,7 @@
>  #include "debug.h"
>  #include "base.h"
>  
> +#ifdef CONFIG_ATH5K_LEDS
>  /*
>   * Set led state
>   */
> @@ -76,6 +77,7 @@ void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state)
>  	else
>  		AR5K_REG_ENABLE_BITS(ah, AR5K_PCICFG, led_5210);
>  }
> +#endif

I would just move that function to led.c (and don't forget to include
reg.h).  The Makefile should take care of the rest.

-- 
Regards,
Pavel Roskin

Re: [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[John W. Linville]

On Thu, Jun 03, 2010 at 10:39:30PM +0300, Dmytro Milinevskyy wrote:
> Hi!
> 
> Here is the patch to disable ath5k leds support on build stage.
> However if the leds support was enabled do not force selection of 802.11 leds layer.
> Depency on LEDS_CLASS is kept.
> 
> Suggestion given by Pavel Roskin and Bob Copeland applied.
> 
> Regards,
> 
> --Dima

Needs at least a Signed-off-by line, and preferrably a changelog that
explains what the patch is doing and (especially) why it is necessary.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

Hi!

Here is the patch to disable ath5k leds support on build stage.
However if the leds support was enabled do not force selection of 802.11 leds layer.
Depency on LEDS_CLASS is kept.

Suggestion given by Pavel Roskin and Bob Copeland applied.

Regards,

--Dima

---
 drivers/net/wireless/ath/ath5k/Kconfig  |   12 +++++++++---
 drivers/net/wireless/ath/ath5k/Makefile |    2 +-
 drivers/net/wireless/ath/ath5k/ath5k.h  |   22 ++++++++++++++++++++++
 drivers/net/wireless/ath/ath5k/base.h   |   13 +++++++++----
 drivers/net/wireless/ath/ath5k/gpio.c   |    2 ++
 5 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/Kconfig b/drivers/net/wireless/ath/ath5k/Kconfig
index eb83b7b..29f4572 100644
--- a/drivers/net/wireless/ath/ath5k/Kconfig
+++ b/drivers/net/wireless/ath/ath5k/Kconfig
@@ -1,9 +1,6 @@
 config ATH5K
 	tristate "Atheros 5xxx wireless cards support"
 	depends on PCI && MAC80211
-	select MAC80211_LEDS
-	select LEDS_CLASS
-	select NEW_LEDS
 	---help---
 	  This module adds support for wireless adapters based on
 	  Atheros 5xxx chipset.
@@ -18,6 +15,15 @@ config ATH5K
 	  If you choose to build a module, it'll be called ath5k. Say M if
 	  unsure.
 
+
+config ATH5K_LEDS
+	tristate "Atheros 5xxx wireless cards LEDs support"
+	depends on ATH5K
+	select NEW_LEDS
+	select LEDS_CLASS
+	---help---
+	  Atheros 5xxx LED support.
+
 config ATH5K_DEBUG
 	bool "Atheros 5xxx debugging"
 	depends on ATH5K
diff --git a/drivers/net/wireless/ath/ath5k/Makefile b/drivers/net/wireless/ath/ath5k/Makefile
index cc09595..6d552dd 100644
--- a/drivers/net/wireless/ath/ath5k/Makefile
+++ b/drivers/net/wireless/ath/ath5k/Makefile
@@ -10,8 +10,8 @@ ath5k-y				+= phy.o
 ath5k-y				+= reset.o
 ath5k-y				+= attach.o
 ath5k-y				+= base.o
-ath5k-y				+= led.o
 ath5k-y				+= rfkill.o
 ath5k-y				+= ani.o
 ath5k-$(CONFIG_ATH5K_DEBUG)	+= debug.o
+ath5k-$(CONFIG_ATH5K_LEDS) += led.o
 obj-$(CONFIG_ATH5K)		+= ath5k.o
diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 2785946..bb7e09a 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1148,11 +1148,27 @@ struct ath5k_hw {
 int ath5k_hw_attach(struct ath5k_softc *sc);
 void ath5k_hw_detach(struct ath5k_hw *ah);
 
+#ifdef CONFIG_ATH5K_LEDS
 /* LED functions */
 int ath5k_init_leds(struct ath5k_softc *sc);
 void ath5k_led_enable(struct ath5k_softc *sc);
 void ath5k_led_off(struct ath5k_softc *sc);
 void ath5k_unregister_leds(struct ath5k_softc *sc);
+#else
+static inline int ath5k_init_leds(struct ath5k_softc *sc)
+{
+	return 0;
+}
+static inline void ath5k_led_enable(struct ath5k_softc *sc)
+{
+}
+static inline void ath5k_led_off(struct ath5k_softc *sc)
+{
+}
+static inline void ath5k_unregister_leds(struct ath5k_softc *sc)
+{
+}
+#endif
 
 /* Reset Functions */
 int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial);
@@ -1233,7 +1249,13 @@ int ath5k_hw_set_slot_time(struct ath5k_hw *ah, unsigned int slot_time);
 int ath5k_hw_init_desc_functions(struct ath5k_hw *ah);
 
 /* GPIO Functions */
+#ifdef CONFIG_ATH5K_LEDS
 void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state);
+#else
+static inline void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state)
+{
+}
+#endif
 int ath5k_hw_set_gpio_input(struct ath5k_hw *ah, u32 gpio);
 int ath5k_hw_set_gpio_output(struct ath5k_hw *ah, u32 gpio);
 u32 ath5k_hw_get_gpio(struct ath5k_hw *ah, u32 gpio);
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 56221bc..97b26c1 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -86,6 +86,7 @@ struct ath5k_txq {
 
 #define ATH5K_LED_MAX_NAME_LEN 31
 
+#ifdef CONFIG_ATH5K_LEDS
 /*
  * State for LED triggers
  */
@@ -95,6 +96,7 @@ struct ath5k_led
 	struct ath5k_softc *sc;			/* driver state */
 	struct led_classdev led_dev;		/* led classdev */
 };
+#endif
 
 /* Rfkill */
 struct ath5k_rfkill {
@@ -186,9 +188,6 @@ struct ath5k_softc {
 
 	u8			bssidmask[ETH_ALEN];
 
-	unsigned int		led_pin,	/* GPIO pin for driving LED */
-				led_on;		/* pin setting for LED on */
-
 	struct tasklet_struct	restq;		/* reset tasklet */
 
 	unsigned int		rxbufsize;	/* rx size based on mtu */
@@ -196,7 +195,6 @@ struct ath5k_softc {
 	spinlock_t		rxbuflock;
 	u32			*rxlink;	/* link ptr in last RX desc */
 	struct tasklet_struct	rxtq;		/* rx intr tasklet */
-	struct ath5k_led	rx_led;		/* rx led */
 
 	struct list_head	txbuf;		/* transmit buffer */
 	spinlock_t		txbuflock;
@@ -204,7 +202,14 @@ struct ath5k_softc {
 	struct ath5k_txq	txqs[AR5K_NUM_TX_QUEUES];	/* tx queues */
 	struct ath5k_txq	*txq;		/* main tx queue */
 	struct tasklet_struct	txtq;		/* tx intr tasklet */
+
+
+#ifdef CONFIG_ATH5K_LEDS
+	unsigned int		led_pin,	/* GPIO pin for driving LED */
+				led_on;		/* pin setting for LED on */
+	struct ath5k_led	rx_led;		/* rx led */
 	struct ath5k_led	tx_led;		/* tx led */
+#endif
 
 	struct ath5k_rfkill	rf_kill;
 
diff --git a/drivers/net/wireless/ath/ath5k/gpio.c b/drivers/net/wireless/ath/ath5k/gpio.c
index 64a27e7..9e757b3 100644
--- a/drivers/net/wireless/ath/ath5k/gpio.c
+++ b/drivers/net/wireless/ath/ath5k/gpio.c
@@ -25,6 +25,7 @@
 #include "debug.h"
 #include "base.h"
 
+#ifdef CONFIG_ATH5K_LEDS
 /*
  * Set led state
  */
@@ -76,6 +77,7 @@ void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state)
 	else
 		AR5K_REG_ENABLE_BITS(ah, AR5K_PCICFG, led_5210);
 }
+#endif
 
 /*
  * Set GPIO inputs
-- 
1.7.1

[PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

Hi!

Here is the patch to disable ath5k leds support on build stage.
However if the leds support was enabled do not force selection of 802.11 leds layer.
Depency on LEDS_CLASS is kept.

Suggestion given by Pavel Roskin and Bob Copeland applied.

Regards,

--Dima

---
 drivers/net/wireless/ath/ath5k/Kconfig  |   12 +++++++++---
 drivers/net/wireless/ath/ath5k/Makefile |    2 +-
 drivers/net/wireless/ath/ath5k/ath5k.h  |   22 ++++++++++++++++++++++
 drivers/net/wireless/ath/ath5k/base.h   |   13 +++++++++----
 drivers/net/wireless/ath/ath5k/gpio.c   |    2 ++
 5 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/Kconfig b/drivers/net/wireless/ath/ath5k/Kconfig
index eb83b7b..29f4572 100644
--- a/drivers/net/wireless/ath/ath5k/Kconfig
+++ b/drivers/net/wireless/ath/ath5k/Kconfig
@@ -1,9 +1,6 @@
 config ATH5K
 	tristate "Atheros 5xxx wireless cards support"
 	depends on PCI && MAC80211
-	select MAC80211_LEDS
-	select LEDS_CLASS
-	select NEW_LEDS
 	---help---
 	  This module adds support for wireless adapters based on
 	  Atheros 5xxx chipset.
@@ -18,6 +15,15 @@ config ATH5K
 	  If you choose to build a module, it'll be called ath5k. Say M if
 	  unsure.
 
+
+config ATH5K_LEDS
+	tristate "Atheros 5xxx wireless cards LEDs support"
+	depends on ATH5K
+	select NEW_LEDS
+	select LEDS_CLASS
+	---help---
+	  Atheros 5xxx LED support.
+
 config ATH5K_DEBUG
 	bool "Atheros 5xxx debugging"
 	depends on ATH5K
diff --git a/drivers/net/wireless/ath/ath5k/Makefile b/drivers/net/wireless/ath/ath5k/Makefile
index cc09595..6d552dd 100644
--- a/drivers/net/wireless/ath/ath5k/Makefile
+++ b/drivers/net/wireless/ath/ath5k/Makefile
@@ -10,8 +10,8 @@ ath5k-y				+= phy.o
 ath5k-y				+= reset.o
 ath5k-y				+= attach.o
 ath5k-y				+= base.o
-ath5k-y				+= led.o
 ath5k-y				+= rfkill.o
 ath5k-y				+= ani.o
 ath5k-$(CONFIG_ATH5K_DEBUG)	+= debug.o
+ath5k-$(CONFIG_ATH5K_LEDS) += led.o
 obj-$(CONFIG_ATH5K)		+= ath5k.o
diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 2785946..bb7e09a 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1148,11 +1148,27 @@ struct ath5k_hw {
 int ath5k_hw_attach(struct ath5k_softc *sc);
 void ath5k_hw_detach(struct ath5k_hw *ah);
 
+#ifdef CONFIG_ATH5K_LEDS
 /* LED functions */
 int ath5k_init_leds(struct ath5k_softc *sc);
 void ath5k_led_enable(struct ath5k_softc *sc);
 void ath5k_led_off(struct ath5k_softc *sc);
 void ath5k_unregister_leds(struct ath5k_softc *sc);
+#else
+static inline int ath5k_init_leds(struct ath5k_softc *sc)
+{
+	return 0;
+}
+static inline void ath5k_led_enable(struct ath5k_softc *sc)
+{
+}
+static inline void ath5k_led_off(struct ath5k_softc *sc)
+{
+}
+static inline void ath5k_unregister_leds(struct ath5k_softc *sc)
+{
+}
+#endif
 
 /* Reset Functions */
 int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial);
@@ -1233,7 +1249,13 @@ int ath5k_hw_set_slot_time(struct ath5k_hw *ah, unsigned int slot_time);
 int ath5k_hw_init_desc_functions(struct ath5k_hw *ah);
 
 /* GPIO Functions */
+#ifdef CONFIG_ATH5K_LEDS
 void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state);
+#else
+static inline void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state)
+{
+}
+#endif
 int ath5k_hw_set_gpio_input(struct ath5k_hw *ah, u32 gpio);
 int ath5k_hw_set_gpio_output(struct ath5k_hw *ah, u32 gpio);
 u32 ath5k_hw_get_gpio(struct ath5k_hw *ah, u32 gpio);
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 56221bc..97b26c1 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -86,6 +86,7 @@ struct ath5k_txq {
 
 #define ATH5K_LED_MAX_NAME_LEN 31
 
+#ifdef CONFIG_ATH5K_LEDS
 /*
  * State for LED triggers
  */
@@ -95,6 +96,7 @@ struct ath5k_led
 	struct ath5k_softc *sc;			/* driver state */
 	struct led_classdev led_dev;		/* led classdev */
 };
+#endif
 
 /* Rfkill */
 struct ath5k_rfkill {
@@ -186,9 +188,6 @@ struct ath5k_softc {
 
 	u8			bssidmask[ETH_ALEN];
 
-	unsigned int		led_pin,	/* GPIO pin for driving LED */
-				led_on;		/* pin setting for LED on */
-
 	struct tasklet_struct	restq;		/* reset tasklet */
 
 	unsigned int		rxbufsize;	/* rx size based on mtu */
@@ -196,7 +195,6 @@ struct ath5k_softc {
 	spinlock_t		rxbuflock;
 	u32			*rxlink;	/* link ptr in last RX desc */
 	struct tasklet_struct	rxtq;		/* rx intr tasklet */
-	struct ath5k_led	rx_led;		/* rx led */
 
 	struct list_head	txbuf;		/* transmit buffer */
 	spinlock_t		txbuflock;
@@ -204,7 +202,14 @@ struct ath5k_softc {
 	struct ath5k_txq	txqs[AR5K_NUM_TX_QUEUES];	/* tx queues */
 	struct ath5k_txq	*txq;		/* main tx queue */
 	struct tasklet_struct	txtq;		/* tx intr tasklet */
+
+
+#ifdef CONFIG_ATH5K_LEDS
+	unsigned int		led_pin,	/* GPIO pin for driving LED */
+				led_on;		/* pin setting for LED on */
+	struct ath5k_led	rx_led;		/* rx led */
 	struct ath5k_led	tx_led;		/* tx led */
+#endif
 
 	struct ath5k_rfkill	rf_kill;
 
diff --git a/drivers/net/wireless/ath/ath5k/gpio.c b/drivers/net/wireless/ath/ath5k/gpio.c
index 64a27e7..9e757b3 100644
--- a/drivers/net/wireless/ath/ath5k/gpio.c
+++ b/drivers/net/wireless/ath/ath5k/gpio.c
@@ -25,6 +25,7 @@
 #include "debug.h"
 #include "base.h"
 
+#ifdef CONFIG_ATH5K_LEDS
 /*
  * Set led state
  */
@@ -76,6 +77,7 @@ void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state)
 	else
 		AR5K_REG_ENABLE_BITS(ah, AR5K_PCICFG, led_5210);
 }
+#endif
 
 /*
  * Set GPIO inputs
-- 
1.7.1

Re: [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

Bob, thanks for the response.

I will rework the patch.

>> -/* GPIO-controlled software LED */
>> -#define AR5K_SOFTLED_PIN       0
>> -#define AR5K_SOFTLED_ON                0
>> -#define AR5K_SOFTLED_OFF       1
>> -
>
> Please drop this hunk, no problem keeping it around.

I suppose this should go away with another patch to keep current
clean. These dfinitions are not related to the subject.

Regards,

-- Dima

On Tue, Jun 1, 2010 at 11:34 PM, Bob Copeland <me@bobcopeland.com> wrote:
> On Wed, Apr 7, 2010 at 2:58 PM, Dmytro Milinevskyy
> <milinevskyy@gmail.com> wrote:
>> Hello!
>
> Thanks, comments inline.
>
>
>> +config ATH5K_LEDS
>> +       tristate "Atheros 5xxx wireless cards LEDs support"
>> +       depends on ATH5K
>> +       ---help---
>> +         Atheros 5xxx LED support.
>> +
>
> This can select the proper LED classes?  Then you can get rid of another
> ifdef check later.
>
>> -/* GPIO-controlled software LED */
>> -#define AR5K_SOFTLED_PIN       0
>> -#define AR5K_SOFTLED_ON                0
>> -#define AR5K_SOFTLED_OFF       1
>> -
>
> Please drop this hunk, no problem keeping it around.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>  /* LED functions */
>>  int ath5k_init_leds(struct ath5k_softc *sc);
>>  void ath5k_led_enable(struct ath5k_softc *sc);
>>  void ath5k_led_off(struct ath5k_softc *sc);
>>  void ath5k_unregister_leds(struct ath5k_softc *sc);
>> +#else
>> +#define ath5k_init_leds(sc) do {} while (0)
>> +#define ath5k_led_enable(sc) do {} while (0)
>> +#define ath5k_led_off(sc) do {} while (0)
>> +#define ath5k_unregister_leds(sc) do {} while (0)
>> +#endif
>
> I prefer:
>
> #ifdef
> ...
> #else
> static inline int ath5k_init_leds(struct ath5k_softc *sc)
> {
>    return 0;
> }
> ...
> #endif
>
> so you get type-checking.  Also this doesn't quite work in your version:
>
>    int foo = ath5k_init_leds(sc);
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>  /*
>>  * State for LED triggers
>>  */
>>  struct ath5k_led
>>  {
>> -       char name[ATH5K_LED_MAX_NAME_LEN + 1];  /* name of the LED in sysfs */
>>        struct ath5k_softc *sc;                 /* driver state */
>> +#ifdef CONFIG_LEDS_CLASS
>> +       char name[ATH5K_LED_MAX_NAME_LEN + 1];  /* name of the LED in sysfs */
>>        struct led_classdev led_dev;            /* led classdev */
>> +#endif
>>  };
>> +#endif
>
> Why move name?
>
>>  /* Rfkill */
>>  struct ath5k_rfkill {
>> @@ -186,9 +190,6 @@ struct ath5k_softc {
>>
>>        u8                      bssidmask[ETH_ALEN];
>>
>> -       unsigned int            led_pin,        /* GPIO pin for driving LED */
>> -                               led_on;         /* pin setting for LED on */
>> -
>>        struct tasklet_struct   restq;          /* reset tasklet */
>>
>>        unsigned int            rxbufsize;      /* rx size based on mtu */
>> @@ -196,7 +197,6 @@ struct ath5k_softc {
>>        spinlock_t              rxbuflock;
>>        u32                     *rxlink;        /* link ptr in last RX desc */
>>        struct tasklet_struct   rxtq;           /* rx intr tasklet */
>> -       struct ath5k_led        rx_led;         /* rx led */
>>
>>        struct list_head        txbuf;          /* transmit buffer */
>>        spinlock_t              txbuflock;
>> @@ -204,7 +204,14 @@ struct ath5k_softc {
>>        struct ath5k_txq        txqs[AR5K_NUM_TX_QUEUES];       /* tx queues */
>>        struct ath5k_txq        *txq;           /* main tx queue */
>>        struct tasklet_struct   txtq;           /* tx intr tasklet */
>> +
>> +
>> +#ifdef CONFIG_ATH5K_LEDS
>> +       unsigned int            led_pin,        /* GPIO pin for driving LED */
>> +                               led_on;         /* pin setting for LED on */
>> +       struct ath5k_led        rx_led;         /* rx led */
>>        struct ath5k_led        tx_led;         /* tx led */
>> +#endif
>
> You might want to do this in two stages: move the led-dependent things
> together in the structure (or into a separate structure) and then only
> have one #ifdef section.
>
> Still too many ifdefs in general.
>
> --
> Bob Copeland %% www.bobcopeland.com
>

Re: [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Bob Copeland]

On Wed, Apr 7, 2010 at 2:58 PM, Dmytro Milinevskyy
<milinevskyy@gmail.com> wrote:
> Hello!

Thanks, comments inline.


> +config ATH5K_LEDS
> +       tristate "Atheros 5xxx wireless cards LEDs support"
> +       depends on ATH5K
> +       ---help---
> +         Atheros 5xxx LED support.
> +

This can select the proper LED classes?  Then you can get rid of another
ifdef check later.

> -/* GPIO-controlled software LED */
> -#define AR5K_SOFTLED_PIN       0
> -#define AR5K_SOFTLED_ON                0
> -#define AR5K_SOFTLED_OFF       1
> -

Please drop this hunk, no problem keeping it around.

> +#ifdef CONFIG_ATH5K_LEDS
>  /* LED functions */
>  int ath5k_init_leds(struct ath5k_softc *sc);
>  void ath5k_led_enable(struct ath5k_softc *sc);
>  void ath5k_led_off(struct ath5k_softc *sc);
>  void ath5k_unregister_leds(struct ath5k_softc *sc);
> +#else
> +#define ath5k_init_leds(sc) do {} while (0)
> +#define ath5k_led_enable(sc) do {} while (0)
> +#define ath5k_led_off(sc) do {} while (0)
> +#define ath5k_unregister_leds(sc) do {} while (0)
> +#endif

I prefer:

#ifdef
...
#else
static inline int ath5k_init_leds(struct ath5k_softc *sc)
{
    return 0;
}
...
#endif

so you get type-checking.  Also this doesn't quite work in your version:

    int foo = ath5k_init_leds(sc);

> +#ifdef CONFIG_ATH5K_LEDS
>  /*
>  * State for LED triggers
>  */
>  struct ath5k_led
>  {
> -       char name[ATH5K_LED_MAX_NAME_LEN + 1];  /* name of the LED in sysfs */
>        struct ath5k_softc *sc;                 /* driver state */
> +#ifdef CONFIG_LEDS_CLASS
> +       char name[ATH5K_LED_MAX_NAME_LEN + 1];  /* name of the LED in sysfs */
>        struct led_classdev led_dev;            /* led classdev */
> +#endif
>  };
> +#endif

Why move name?

>  /* Rfkill */
>  struct ath5k_rfkill {
> @@ -186,9 +190,6 @@ struct ath5k_softc {
>
>        u8                      bssidmask[ETH_ALEN];
>
> -       unsigned int            led_pin,        /* GPIO pin for driving LED */
> -                               led_on;         /* pin setting for LED on */
> -
>        struct tasklet_struct   restq;          /* reset tasklet */
>
>        unsigned int            rxbufsize;      /* rx size based on mtu */
> @@ -196,7 +197,6 @@ struct ath5k_softc {
>        spinlock_t              rxbuflock;
>        u32                     *rxlink;        /* link ptr in last RX desc */
>        struct tasklet_struct   rxtq;           /* rx intr tasklet */
> -       struct ath5k_led        rx_led;         /* rx led */
>
>        struct list_head        txbuf;          /* transmit buffer */
>        spinlock_t              txbuflock;
> @@ -204,7 +204,14 @@ struct ath5k_softc {
>        struct ath5k_txq        txqs[AR5K_NUM_TX_QUEUES];       /* tx queues */
>        struct ath5k_txq        *txq;           /* main tx queue */
>        struct tasklet_struct   txtq;           /* tx intr tasklet */
> +
> +
> +#ifdef CONFIG_ATH5K_LEDS
> +       unsigned int            led_pin,        /* GPIO pin for driving LED */
> +                               led_on;         /* pin setting for LED on */
> +       struct ath5k_led        rx_led;         /* rx led */
>        struct ath5k_led        tx_led;         /* tx led */
> +#endif

You might want to do this in two stages: move the led-dependent things
together in the structure (or into a separate structure) and then only
have one #ifdef section.

Still too many ifdefs in general.

-- 
Bob Copeland %% www.bobcopeland.com

[PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy]

Hello!

Here is the patch to disable ath5k leds support on build stage.
However if the leds support was enabled do not force selection of 802.11 leds layer.

I've applied suggestion given by Pavel Roskin.

Best regards,

--Dima

---
 drivers/net/wireless/ath/ath5k/Kconfig  |   10 +++++++---
 drivers/net/wireless/ath/ath5k/Makefile |    2 +-
 drivers/net/wireless/ath/ath5k/ath5k.h  |   16 +++++++++++-----
 drivers/net/wireless/ath/ath5k/base.h   |   17 ++++++++++++-----
 drivers/net/wireless/ath/ath5k/gpio.c   |    2 ++
 drivers/net/wireless/ath/ath5k/led.c    |    9 +++++++++
 6 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/Kconfig b/drivers/net/wireless/ath/ath5k/Kconfig
index eb83b7b..6b5677e 100644
--- a/drivers/net/wireless/ath/ath5k/Kconfig
+++ b/drivers/net/wireless/ath/ath5k/Kconfig
@@ -1,9 +1,6 @@
 config ATH5K
 	tristate "Atheros 5xxx wireless cards support"
 	depends on PCI && MAC80211
-	select MAC80211_LEDS
-	select LEDS_CLASS
-	select NEW_LEDS
 	---help---
 	  This module adds support for wireless adapters based on
 	  Atheros 5xxx chipset.
@@ -18,6 +15,13 @@ config ATH5K
 	  If you choose to build a module, it'll be called ath5k. Say M if
 	  unsure.
 
+
+config ATH5K_LEDS
+	tristate "Atheros 5xxx wireless cards LEDs support"
+	depends on ATH5K
+	---help---
+	  Atheros 5xxx LED support.
+
 config ATH5K_DEBUG
 	bool "Atheros 5xxx debugging"
 	depends on ATH5K
diff --git a/drivers/net/wireless/ath/ath5k/Makefile b/drivers/net/wireless/ath/ath5k/Makefile
index cc09595..6d552dd 100644
--- a/drivers/net/wireless/ath/ath5k/Makefile
+++ b/drivers/net/wireless/ath/ath5k/Makefile
@@ -10,8 +10,8 @@ ath5k-y				+= phy.o
 ath5k-y				+= reset.o
 ath5k-y				+= attach.o
 ath5k-y				+= base.o
-ath5k-y				+= led.o
 ath5k-y				+= rfkill.o
 ath5k-y				+= ani.o
 ath5k-$(CONFIG_ATH5K_DEBUG)	+= debug.o
+ath5k-$(CONFIG_ATH5K_LEDS) += led.o
 obj-$(CONFIG_ATH5K)		+= ath5k.o
diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 2785946..615b9ca 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -920,11 +920,6 @@ enum ath5k_power_mode {
 #define AR5K_LED_ASSOC	3 /*IEEE80211_S_ASSOC*/
 #define AR5K_LED_RUN	4 /*IEEE80211_S_RUN*/
 
-/* GPIO-controlled software LED */
-#define AR5K_SOFTLED_PIN	0
-#define AR5K_SOFTLED_ON		0
-#define AR5K_SOFTLED_OFF	1
-
 /*
  * Chipset capabilities -see ath5k_hw_get_capability-
  * get_capability function is not yet fully implemented
@@ -1148,11 +1143,18 @@ struct ath5k_hw {
 int ath5k_hw_attach(struct ath5k_softc *sc);
 void ath5k_hw_detach(struct ath5k_hw *ah);
 
+#ifdef CONFIG_ATH5K_LEDS
 /* LED functions */
 int ath5k_init_leds(struct ath5k_softc *sc);
 void ath5k_led_enable(struct ath5k_softc *sc);
 void ath5k_led_off(struct ath5k_softc *sc);
 void ath5k_unregister_leds(struct ath5k_softc *sc);
+#else
+#define ath5k_init_leds(sc) do {} while (0)
+#define ath5k_led_enable(sc) do {} while (0)
+#define ath5k_led_off(sc) do {} while (0)
+#define ath5k_unregister_leds(sc) do {} while (0)
+#endif
 
 /* Reset Functions */
 int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial);
@@ -1233,7 +1235,11 @@ int ath5k_hw_set_slot_time(struct ath5k_hw *ah, unsigned int slot_time);
 int ath5k_hw_init_desc_functions(struct ath5k_hw *ah);
 
 /* GPIO Functions */
+#ifdef CONFIG_ATH5K_LEDS
 void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state);
+#else
+#define ath5k_hw_set_ledstate(ah, state) do {} while (0)
+#endif
 int ath5k_hw_set_gpio_input(struct ath5k_hw *ah, u32 gpio);
 int ath5k_hw_set_gpio_output(struct ath5k_hw *ah, u32 gpio);
 u32 ath5k_hw_get_gpio(struct ath5k_hw *ah, u32 gpio);
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 56221bc..e493d34 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -86,15 +86,19 @@ struct ath5k_txq {
 
 #define ATH5K_LED_MAX_NAME_LEN 31
 
+#ifdef CONFIG_ATH5K_LEDS
 /*
  * State for LED triggers
  */
 struct ath5k_led
 {
-	char name[ATH5K_LED_MAX_NAME_LEN + 1];	/* name of the LED in sysfs */
 	struct ath5k_softc *sc;			/* driver state */
+#ifdef CONFIG_LEDS_CLASS
+	char name[ATH5K_LED_MAX_NAME_LEN + 1];	/* name of the LED in sysfs */
 	struct led_classdev led_dev;		/* led classdev */
+#endif
 };
+#endif
 
 /* Rfkill */
 struct ath5k_rfkill {
@@ -186,9 +190,6 @@ struct ath5k_softc {
 
 	u8			bssidmask[ETH_ALEN];
 
-	unsigned int		led_pin,	/* GPIO pin for driving LED */
-				led_on;		/* pin setting for LED on */
-
 	struct tasklet_struct	restq;		/* reset tasklet */
 
 	unsigned int		rxbufsize;	/* rx size based on mtu */
@@ -196,7 +197,6 @@ struct ath5k_softc {
 	spinlock_t		rxbuflock;
 	u32			*rxlink;	/* link ptr in last RX desc */
 	struct tasklet_struct	rxtq;		/* rx intr tasklet */
-	struct ath5k_led	rx_led;		/* rx led */
 
 	struct list_head	txbuf;		/* transmit buffer */
 	spinlock_t		txbuflock;
@@ -204,7 +204,14 @@ struct ath5k_softc {
 	struct ath5k_txq	txqs[AR5K_NUM_TX_QUEUES];	/* tx queues */
 	struct ath5k_txq	*txq;		/* main tx queue */
 	struct tasklet_struct	txtq;		/* tx intr tasklet */
+
+
+#ifdef CONFIG_ATH5K_LEDS
+	unsigned int		led_pin,	/* GPIO pin for driving LED */
+				led_on;		/* pin setting for LED on */
+	struct ath5k_led	rx_led;		/* rx led */
 	struct ath5k_led	tx_led;		/* tx led */
+#endif
 
 	struct ath5k_rfkill	rf_kill;
 
diff --git a/drivers/net/wireless/ath/ath5k/gpio.c b/drivers/net/wireless/ath/ath5k/gpio.c
index 64a27e7..9e757b3 100644
--- a/drivers/net/wireless/ath/ath5k/gpio.c
+++ b/drivers/net/wireless/ath/ath5k/gpio.c
@@ -25,6 +25,7 @@
 #include "debug.h"
 #include "base.h"
 
+#ifdef CONFIG_ATH5K_LEDS
 /*
  * Set led state
  */
@@ -76,6 +77,7 @@ void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state)
 	else
 		AR5K_REG_ENABLE_BITS(ah, AR5K_PCICFG, led_5210);
 }
+#endif
 
 /*
  * Set GPIO inputs
diff --git a/drivers/net/wireless/ath/ath5k/led.c b/drivers/net/wireless/ath/ath5k/led.c
index 67aa52e..df8e8d2 100644
--- a/drivers/net/wireless/ath/ath5k/led.c
+++ b/drivers/net/wireless/ath/ath5k/led.c
@@ -121,6 +121,7 @@ ath5k_led_brightness_set(struct led_classdev *led_dev,
 		ath5k_led_on(led->sc);
 }
 
+#ifdef CONFIG_LEDS_CLASS
 static int
 ath5k_register_led(struct ath5k_softc *sc, struct ath5k_led *led,
 		   const char *name, char *trigger)
@@ -140,13 +141,16 @@ ath5k_register_led(struct ath5k_softc *sc, struct ath5k_led *led,
 	}
 	return err;
 }
+#endif
 
 static void
 ath5k_unregister_led(struct ath5k_led *led)
 {
 	if (!led->sc)
 		return;
+#ifdef CONFIG_LEDS_CLASS
 	led_classdev_unregister(&led->led_dev);
+#endif
 	ath5k_led_off(led->sc);
 	led->sc = NULL;
 }
@@ -177,6 +181,7 @@ int ath5k_init_leds(struct ath5k_softc *sc)
 
 	ath5k_led_enable(sc);
 
+#ifdef CONFIG_LEDS_CLASS
 	snprintf(name, sizeof(name), "ath5k-%s::rx", wiphy_name(hw->wiphy));
 	ret = ath5k_register_led(sc, &sc->rx_led, name,
 		ieee80211_get_rx_led_name(hw));
@@ -186,6 +191,10 @@ int ath5k_init_leds(struct ath5k_softc *sc)
 	snprintf(name, sizeof(name), "ath5k-%s::tx", wiphy_name(hw->wiphy));
 	ret = ath5k_register_led(sc, &sc->tx_led, name,
 		ieee80211_get_tx_led_name(hw));
+#else
+    sc->rx_led.sc = sc;
+    sc->tx_led.sc = sc;
+#endif
 out:
 	return ret;
 }
-- 
1.7.1