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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <AANLkTilPVWvsgXkriz68lLl6e8Qslm_wMkHpkhEdWC3_@mail.gmail.com>
Date:	Mon, 7 Jun 2010 10:39:03 +0300
From:	Dmytro Milinevskyy <milinevskyy@...il.com>
To:	Pavel Roskin <proski@....org>
Cc:	ath5k-devel@...ts.ath5k.org, Jiri Slaby <jirislaby@...il.com>,
	Nick Kossifidis <mickflemm@...il.com>,
	"Luis R. Rodriguez" <lrodriguez@...eros.com>,
	Bob Copeland <me@...copeland.com>,
	"John W. Linville" <linville@...driver.com>,
	GeunSik Lim <geunsik.lim@...sung.com>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Lukas Turek <8an@...ha12.net>,
	Mark Hindley <mark@...dley.org.uk>,
	Johannes Berg <johannes@...solutions.net>,
	Jiri Kosina <jkosina@...e.cz>, Kalle Valo <kalle.valo@....fi>,
	Keng-Yu Lin <keng-yu.lin@...onical.com>,
	Luca Verdesca <magooz@...ug.it>,
	Shahar Or <shahar@...har-or.co.il>,
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [ath5k][leds] Ability to disable leds support. If leds 
	support enabled do not force mac802.11 leds layer selection.

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@....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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/