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

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

[Pavel Roskin]

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

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]

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
>

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

[Bob Copeland]

On Wed, Jun 9, 2010 at 3:31 AM, Dmytro Milinevskyy
<milinevskyy@gmail.com> wrote:
> However if the leds support was enabled do not force selection of 802.11 leds layer.

I don't understand this part.  What's the point of enabling software LEDs
if nothing triggers them?

Also, we can probably build hardware LEDs (hw_set_ledstate) in regardless.
It's a pure register write and doesn't require the rest of the LED machinery.

I assume you are doing this to reduce the size of the module.  If so, can
you include size(1) output in the commit message?

-- 
Bob Copeland %% www.bobcopeland.com

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]

Hi, Bob.

For instance I don't use 802.11 leds layer and trigger leds from
userspace according to my purposes.

-- Dima

On Wed, Jun 9, 2010 at 4:58 PM, Bob Copeland <me@bobcopeland.com> wrote:
> On Wed, Jun 9, 2010 at 3:31 AM, Dmytro Milinevskyy
> <milinevskyy@gmail.com> wrote:
>> However if the leds support was enabled do not force selection of 802.11 leds layer.
>
> I don't understand this part.  What's the point of enabling software LEDs
> if nothing triggers them?
>
> Also, we can probably build hardware LEDs (hw_set_ledstate) in regardless.
> It's a pure register write and doesn't require the rest of the LED machinery.
>
> I assume you are doing this to reduce the size of the module.  If so, can
> you include size(1) output in the commit message?
>
> --
> Bob Copeland %% www.bobcopeland.com
>

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

[Bob Copeland]

On Wed, Jun 9, 2010 at 10:43 AM, Dmytro Milinevskyy
<milinevskyy@gmail.com> wrote:
> Hi, Bob.
>
> For instance I don't use 802.11 leds layer and trigger leds from
> userspace according to my purposes.

FWIW you can disable mac80211 triggers from userspace as well.
But, I guess hooking up null triggers will work too.

-- 
Bob Copeland %% www.bobcopeland.com

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]

I didn't know. Thank you for noting that.
However I think it's better to give a chance to disable 802.11 leds.

-- Dima

On Fri, Jun 11, 2010 at 8:07 PM, Bob Copeland <me@bobcopeland.com> wrote:
> On Wed, Jun 9, 2010 at 10:43 AM, Dmytro Milinevskyy
> <milinevskyy@gmail.com> wrote:
>> Hi, Bob.
>>
>> For instance I don't use 802.11 leds layer and trigger leds from
>> userspace according to my purposes.
>
> FWIW you can disable mac80211 triggers from userspace as well.
> But, I guess hooking up null triggers will work too.
>
> --
> Bob Copeland %% www.bobcopeland.com
>

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

[Johannes Berg]

On Wed, 2010-06-09 at 17:43 +0300, Dmytro Milinevskyy wrote:
> Hi, Bob.
> 
> For instance I don't use 802.11 leds layer and trigger leds from
> userspace according to my purposes.

Without the LED stuff in sysfs, how do you do that?

johannes

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]

Generic LED class is not disabled so it's possible to control leds in sysfs.

-- Dima

On Fri, Jun 11, 2010 at 11:09 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2010-06-09 at 17:43 +0300, Dmytro Milinevskyy wrote:
>> Hi, Bob.
>>
>> For instance I don't use 802.11 leds layer and trigger leds from
>> userspace according to my purposes.
>
> Without the LED stuff in sysfs, how do you do that?
>
> johannes
>
>

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

[Johannes Berg]

On Fri, 2010-06-11 at 23:26 +0300, Dmytro Milinevskyy wrote:
> Generic LED class is not disabled so it's possible to control leds in sysfs.

But if you turn off ATH5K_LEDS??

johannes

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]

In this case no way to control LEDs. If you turn LEDs support you
can't tune them out, no?

I believe that user should be able to chose whether LEDs support is
needed or not. This is how done in intel wireless drivers. The driver
should be tolerant.

-- Dima

On Fri, Jun 11, 2010 at 11:29 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2010-06-11 at 23:26 +0300, Dmytro Milinevskyy wrote:
>> Generic LED class is not disabled so it's possible to control leds in sysfs.
>
> But if you turn off ATH5K_LEDS??
>
> johannes
>
>