2010/9/17 Levi, Shahar <shahar_levi@xxxxxx>: >> -----Original Message----- >> From: Julian Calaby [mailto:julian.calaby@xxxxxxxxx] >> Sent: Thursday, September 16, 2010 2:39 AM >> To: Luciano Coelho >> Cc: Levi, Shahar; linux-wireless@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH 04/04] wl1271: 11n Support, 11n Kconfig Configurable >> >> On Thu, Sep 16, 2010 at 05:21, Luciano Coelho <luciano.coelho@xxxxxxxxx> >> wrote: >> > On Wed, 2010-09-15 at 20:31 +0200, ext Shahar Levi wrote: >> >> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c >> b/drivers/net/wireless/wl12xx/wl1271_main.c >> >> index e89e574..8f2cea9 100644 >> >> --- a/drivers/net/wireless/wl12xx/wl1271_main.c >> >> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c >> >> @@ -2087,7 +2087,9 @@ static struct ieee80211_supported_band >> wl1271_band_2ghz = { >> >> .n_channels = ARRAY_SIZE(wl1271_channels), >> >> .bitrates = wl1271_rates, >> >> .n_bitrates = ARRAY_SIZE(wl1271_rates), >> >> +#ifdef CONFIG_WL1271_HT >> >> .ht_cap = WL12xx_HT_CAP, >> >> +#endif >> > >> > Here you can use #ifdef CONFIG_WL1271_80211_HT directly, no need to >> > duplicate it into a new flag. >> >> Another thing you might want to do could be to adjust the order of the >> patches so you introduce this config option *before* you introduce the >> actual code that is protected by it. At the moment, this functionality >> is enabled unconditionally if this patch isn't applied, which could >> potentially occur during bisection, given that John doesn't squash the >> patches into one when he applies them. >> >> Thanks, >> >> -- >> >> Julian Calaby >> >> Email: julian.calaby@xxxxxxxxx >> Profile: http://www.google.com/profiles/julian.calaby/ >> .Plan: http://sites.google.com/site/juliancalaby/ > > Hi Julian, > It is a good point. ".ht_cap = WL12xx_HT_CAP" in wl1271_main.c adds only on patch 03/04. I believe that adding "#ifdef CONFIG_WL1271_HT" in wl1271_main.c with early patch on code that not exist is less clear. Squashing the patches 03 and 04 to one in v2 could prevent that but it will not emphasize the configuration option. Is there are cases that the some patches from series not applies? Bisection (which seems to be my watch word, at the moment) could jump into the middle of random patch sets like this. Personally, I'd squash 3 and 4 into one patch: I.e. you're saying "here's the functionality and it's protected by a Kconfig variable." The fact that it's touching the Kconfig file is probably enough emphasis in and of itself. > Thanks for your review, No problem, -- Julian Calaby Email: julian.calaby@xxxxxxxxx Profile: http://www.google.com/profiles/julian.calaby/ .Plan: http://sites.google.com/site/juliancalaby/ -- 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
