lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 2 Jun 2010 02:01:32 +0300 From: Dmytro Milinevskyy <milinevskyy@...il.com> To: Bob Copeland <me@...copeland.com> Cc: ath5k-devel@...ts.ath5k.org, Jiri Slaby <jirislaby@...il.com>, Nick Kossifidis <mickflemm@...il.com>, "Luis R. Rodriguez" <lrodriguez@...eros.com>, "John W. Linville" <linville@...driver.com>, GeunSik Lim <geunsik.lim@...sung.com>, Greg Kroah-Hartman <gregkh@...e.de>, Lukas Turek <8an@...ha12.net>, Mark Hindley <mark@...dley.org.uk>, Johannes Berg <johannes@...solutions.net>, Jiri Kosina <jkosina@...e.cz>, Kalle Valo <kalle.valo@....fi>, Keng-Yu Lin <keng-yu.lin@...onical.com>, Luca Verdesca <magooz@...ug.it>, Shahar Or <shahar@...har-or.co.il>, linux-wireless@...r.kernel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection. 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@...copeland.com> wrote: > On Wed, Apr 7, 2010 at 2:58 PM, Dmytro Milinevskyy > <milinevskyy@...il.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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists