Hello, Pavel.
I will rework the patch considering your suggestions.
> I'm not sure this complexity is needed. Are you going to support LEDs
> if CONFIG_LEDS_CLASS is disabled?
If there's any other place in driver that might want use LEDs w/o
exporting the interface to the userspace.
-- Dima Milinevskyy
On Wed, May 12, 2010 at 6:48 PM, Pavel Roskin <proski@xxxxxxx> wrote:
> On Wed, 2010-04-07 at 21:58 +0300, Dmytro Milinevskyy wrote:
>
>> Here is the patch to disable ath5k leds support on build stage.
>> However if the leds support was enabled do not force selection of 802.11 leds layer.
>
> The idea is good, but the implementation could be improved.
>
> There are too many preprocessor conditionals in your patch.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>> /*
>> * These match net80211 definitions (not used in
>> * mac80211).
>> @@ -939,11 +940,7 @@ enum ath5k_power_mode {
>> #define AR5K_LED_AUTH 2 /*IEEE80211_S_AUTH*/
>> #define AR5K_LED_ASSOC 3 /*IEEE80211_S_ASSOC*/
>> #define AR5K_LED_RUN 4 /*IEEE80211_S_RUN*/
>
> It should be OK to leave the constants defined even if they are not
> used.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>> /* LED functions */
>> extern int ath5k_init_leds(struct ath5k_softc *sc);
>> extern void ath5k_led_enable(struct ath5k_softc *sc);
>> extern void ath5k_led_off(struct ath5k_softc *sc);
>> extern void ath5k_unregister_leds(struct ath5k_softc *sc);
>> +#endif
>
> You could add inline functions for the case when CONFIG_ATH5K_LEDS is
> not defined. That would avoid may conditionals in the code.
>
>> /* GPIO Functions */
>> +#ifdef CONFIG_ATH5K_LEDS
>> extern void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state);
>> +#endif
>
> The same comment applies.
>
> Also, there is nothing wrong with having an external declaration that is
> not used in some particular configuration.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>> /* turn on HW LEDs */
>> ath5k_hw_set_ledstate(ah, AR5K_LED_INIT);
>> +#endif
>
> This is avoidable by having an inline ath5k_hw_set_ledstate() that does
> nothing.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>> struct ieee80211_hw *hw = pci_get_drvdata(to_pci_dev(dev));
>> struct ath5k_softc *sc = hw->priv;
>>
>> ath5k_led_off(sc);
>> +#endif
>
> Even this is avoidable if ath5k_led_off() does nothing. gcc should be
> smart enough to optimize out unneeded function calls.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>> /*
>> * State for LED triggers
>> */
>> struct ath5k_led
>> {
>> +#ifdef CONFIG_LEDS_CLASS
>
> I'm not sure this complexity is needed. Are you going to support LEDs
> if CONFIG_LEDS_CLASS is disabled?
>
>> +#ifdef CONFIG_ATH5K_LEDS
>> unsigned int led_pin, /* GPIO pin for driving LED */
>> led_on; /* pin setting for LED on */
>> +#endif
>>
>> struct tasklet_struct restq; /* reset tasklet */
>>
>> @@ -164,7 +172,9 @@ struct ath5k_softc {
>> spinlock_t rxbuflock;
>> u32 *rxlink; /* link ptr in last RX desc */
>> struct tasklet_struct rxtq; /* rx intr tasklet */
>> +#ifdef CONFIG_ATH5K_LEDS
>> struct ath5k_led rx_led; /* rx led */
>> +#endif
>
> You may want to group those fields together to make the code more
> readable.
>
>> --- a/drivers/net/wireless/ath/ath5k/led.c
>> +++ b/drivers/net/wireless/ath/ath5k/led.c
>
> I wonder if you could omit led.c completely in the Makefile. If there
> are some parts of led.c that are needed without CONFIG_ATH5K_LEDS, maybe
> they belong elsewhere?
>
> --
> Regards,
> Pavel Roskin
>
--
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
