Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

[Dmytro Milinevskyy <milinevskyy@gmail.com>]

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@gnu.org> 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
>