Return-path: Received: from mail-vw0-f46.google.com ([209.85.212.46]:46452 "EHLO mail-vw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753580Ab0FAUec convert rfc822-to-8bit (ORCPT ); Tue, 1 Jun 2010 16:34:32 -0400 MIME-Version: 1.0 In-Reply-To: <4c056b50.0c3fdf0a.500b.44af@mx.google.com> References: <4c056b50.0c3fdf0a.500b.44af@mx.google.com> Date: Tue, 1 Jun 2010 16:34:30 -0400 Message-ID: Subject: Re: [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection. From: Bob Copeland To: Dmytro Milinevskyy Cc: ath5k-devel@lists.ath5k.org, Jiri Slaby , Nick Kossifidis , "Luis R. Rodriguez" , "John W. Linville" , GeunSik Lim , Greg Kroah-Hartman , Lukas Turek <8an@praha12.net>, Mark Hindley , Johannes Berg , Jiri Kosina , Kalle Valo , Keng-Yu Lin , Luca Verdesca , Shahar Or , linux-wireless@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-wireless-owner@vger.kernel.org List-ID: On Wed, Apr 7, 2010 at 2:58 PM, Dmytro Milinevskyy 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