Prev: Differnce between kthread and worqueue ?
Next: Buildfailure for omap3_defconfig due to patch OMAP: RX51: Add LCD Panel support
From: Dmytro Milinevskyy on 13 May 2010 03:40
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(a)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
>
--
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: Bob Copeland on 9 Jun 2010 10:00
On Wed, Jun 9, 2010 at 3:31 AM, Dmytro Milinevskyy
<milinevskyy(a)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
--
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 9 Jun 2010 10:50
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(a)bobcopeland.com> wrote:
> On Wed, Jun 9, 2010 at 3:31 AM, Dmytro Milinevskyy
> <milinevskyy(a)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
>
--
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: Bob Copeland on 11 Jun 2010 13:10
On Wed, Jun 9, 2010 at 10:43 AM, Dmytro Milinevskyy
<milinevskyy(a)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
--
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 11 Jun 2010 16:00
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(a)bobcopeland.com> wrote:
> On Wed, Jun 9, 2010 at 10:43 AM, Dmytro Milinevskyy
> <milinevskyy(a)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
>
--
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/
 |  Next  |  Last
Pages: 1 2
Prev: Differnce between kthread and worqueue ?
Next: Buildfailure for omap3_defconfig due to patch OMAP: RX51: Add LCD Panel support