Re: [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>]

Pavel, thank you for the response here.

I agree with all your comments and will adjust the patch according to them.

I'm new to the submitting patches into the community and I appreciate
telling criticism so that in future I will not cause that much
troubles.

Regards,

-- Dima

On Fri, Jun 4, 2010 at 7:22 PM, Pavel Roskin <proski@gnu.org> wrote:
> On Fri, 2010-06-04 at 16:43 +0300, Dmytro Milinevskyy wrote:
>> Hi!
>>
>> 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.
>> Depency on LEDS_CLASS is kept.
>>
>> Suggestion given by Pavel Roskin and Bob Copeland applied.
>
> It's great that you did it.  The patch is much clearer now.  That makes
> smaller issues visible.  Please don't be discouraged by the criticism,
> you are on the right track.
>
> First of all, your patch doesn't apply cleanly to the current
> wireless-testing because of formatting changes in Makefile.  Please
> update.
>
>> +config ATH5K_LEDS
>> +       tristate "Atheros 5xxx wireless cards LEDs support"
>> +       depends on ATH5K
>> +       select NEW_LEDS
>> +       select LEDS_CLASS
>> +       ---help---
>> +         Atheros 5xxx LED support.
>
> "tristate" is wrong here.  "tristate" would allow users select "m",
> which is wrong, since LED support is not a separate module.  I think you
> want "bool" here.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>  /*
>>   * State for LED triggers
>>   */
>> @@ -95,6 +96,7 @@ struct ath5k_led
>>       struct ath5k_softc *sc;                 /* driver state */
>>       struct led_classdev led_dev;            /* led classdev */
>>  };
>> +#endif
>
> This shouldn't be needed.  I'll rather see a structure that is not used
> in some cases than an extra pair of preprocessor conditionals.
>
>> diff --git a/drivers/net/wireless/ath/ath5k/gpio.c b/drivers/net/wireless/ath/ath5k/gpio.c
>> index 64a27e7..9e757b3 100644
>> --- a/drivers/net/wireless/ath/ath5k/gpio.c
>> +++ b/drivers/net/wireless/ath/ath5k/gpio.c
>> @@ -25,6 +25,7 @@
>>  #include "debug.h"
>>  #include "base.h"
>>
>> +#ifdef CONFIG_ATH5K_LEDS
>>  /*
>>   * Set led state
>>   */
>> @@ -76,6 +77,7 @@ void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state)
>>       else
>>               AR5K_REG_ENABLE_BITS(ah, AR5K_PCICFG, led_5210);
>>  }
>> +#endif
>
> I would just move that function to led.c (and don't forget to include
> reg.h).  The Makefile should take care of the rest.
>
> --
> Regards,
> Pavel Roskin
>