On Wed, Apr 7, 2010 at 2:58 PM, Dmytro Milinevskyy
<milinevskyy@xxxxxxxxx> 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 linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
