[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <AANLkTiklAs6MM-LkEzkiZbmLG78WednhDXm4oXRcdgMX@mail.gmail.com>
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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists