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@xxxxxxxxxxxxxxx> wrote:
> 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
