[ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.
in [Kernel]
|
Prev: oom: select_bad_process: PF_EXITING check should take ->mm into account
Next: [PATCH 14/31] vc: Locking clean up
From: Bob Copeland on 1 Jun 2010 16:40 On Wed, Apr 7, 2010 at 2:58 PM, Dmytro Milinevskyy <milinevskyy(a)gmail.com> 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-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dmytro Milinevskyy on 1 Jun 2010 19:10 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(a)bobcopeland.com> wrote: > On Wed, Apr 7, 2010 at 2:58 PM, Dmytro Milinevskyy > <milinevskyy(a)gmail.com> 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-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dmytro Milinevskyy on 7 Jun 2010 03:40
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(a)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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |