Return-path: Received: from mail-wy0-f174.google.com ([74.125.82.174]:57026 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751364Ab0FAXBy convert rfc822-to-8bit (ORCPT ); Tue, 1 Jun 2010 19:01:54 -0400 MIME-Version: 1.0 In-Reply-To: References: <4c056b50.0c3fdf0a.500b.44af@mx.google.com> From: Dmytro Milinevskyy Date: Wed, 2 Jun 2010 02:01:32 +0300 Message-ID: Subject: Re: [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection. To: Bob Copeland 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: 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 wrote: > 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 >