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@xxxxxxx> 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 > -- 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
