From: Shahar Levi <shahar_levi@ti.com>
To: Luciano Coelho <luciano.coelho@nokia.com>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH ] wl1271: Change wl12xx Files Names
Date: Thu, 28 Oct 2010 18:16:53 +0200 [thread overview]
Message-ID: <4CC9A1F5.4010308@ti.com> (raw)
In-Reply-To: <1288207558.1698.56.camel@powerslave>
On 10/27/2010 09:25 PM, Luciano Coelho wrote:
> On Tue, 2010-10-26 at 13:18 +0200, ext Shahar Levi wrote:
>> As part of adding support to wl1281/3 all files name prefix removed.
>> Also the definition in Kconfig changed respectively.
>>
>> Signed-off-by: Shahar Levi<shahar_levi@ti.com>
>> ---
>
> Thanks, Shahar! I have some comments below.
Hi Luca,
Thanks for you review. comments inline.
> In the commit message, maybe you could mention that the change makes
> sense alto due to wl1273 support (which is already implemented).
Will be fix in v2.
>> diff --git a/drivers/net/wireless/wl12xx/Kconfig b/drivers/net/wireless/wl12xx/Kconfig
>> index 1b3b7bd..1f29284 100644
>> --- a/drivers/net/wireless/wl12xx/Kconfig
>> +++ b/drivers/net/wireless/wl12xx/Kconfig
>> @@ -2,55 +2,55 @@ menuconfig WL12XX
>> tristate "TI wl12xx driver support"
>> depends on MAC80211&& EXPERIMENTAL
>> ---help---
>> - This will enable TI wl12xx driver support. The drivers make
>> - use of the mac80211 stack.
>> + This will enable TI wl12xx driver support: 1271, 1273.
>> + The drivers make use of the mac80211 stack.
>
> Maybe "This will enable TI wl12xx driver support for the following
> chips: wl1271 and wl1273." would look better?
Will be fix in v2.
>> -config WL1271
>> - tristate "TI wl1271 support"
>> +config WL127X
>> + tristate "TI wl127x support"make -j10 modules && (make modules_install ARCH=arm INSTALL_MOD_PATH=/home/nfs/ )
>
> Why not use WL12XX and wl12xx here already? Regarding the wl128x stuff,
> as I mentioned before, I think it's best if we do the check at runtime
> and not in Kconfig.
In case the 128x will check at runtime you right.
Will be fix in v2, if we will agreed otherwise i will revert.
>
>> depends on WL12XX&& GENERIC_HARDIRQS
>> depends on INET
>> select FW_LOADER
>> select CRC7
>> ---help---
>> This module adds support for wireless adapters based on the
>> - TI wl1271 chipset.
>> + TI wl127x chipset.
>
> TI wl127x is not a chipset. Maybe we should say "This module adds
> support for wireless adapters based on TI wl1271 and TI wl1273
> chipsets"?
Will be fix in v2
>
>
>> - If you choose to build a module, it'll be called wl1271. Say N if
>> + If you choose to build a module, it'll be called wl127x. Say N if
>> unsure.
>
> "it will be called wl12xx."
Will be fix in v2
>> -config WL1271_HT
>> - bool "TI wl1271 802.11 HT support (EXPERIMENTAL)"
>> - depends on WL1271&& EXPERIMENTAL
>> +config WL12XX_HT
>> + bool "TI wl12xx 802.11 HT support (EXPERIMENTAL)"
>> + depends on WL127X&& EXPERIMENTAL
>
> WL12XX
Will be fix in v2
>> default n
>> ---help---
>> - This will enable 802.11 HT support for TI wl1271 chipset.
>> + This will enable 802.11 HT support for TI wl12xx chipset.
>
> Maybe, to avoid confusion with wl1251 without repeating "wl1271, wl1273"
> all the time, we could use "enable 802.11 HT support in the wl12xx
> module"?
Will be fix in v2
>> That configuration is temporary due to the code incomplete and
>> still in testing process.
>>
>> -config WL1271_SPI
>> - tristate "TI wl1271 SPI support"
>> - depends on WL1271&& SPI_MASTER
>> +config WL127X_SPI
>> + tristate "TI wl12xx SPI support"
>> + depends on WL127X&& SPI_MASTER
>
> WL12XX_SPI
Will be fix in v2
>> ---help---
>> This module adds support for the SPI interface of adapters using
>> - TI wl1271 chipset. Select this if your platform is using
>> + TI wl12xx chipset. Select this if your platform is using
>
> Maybe also here, something like "TI wl12xx chipsets."
Will be fix in v2
>> the SPI bus.
>>
>> - If you choose to build a module, it'll be called wl1251_spi.
>> + If you choose to build a module, it'll be called spi.
>
> Calling it simply "spi" is very confusing. It should be called
> wl12xx_spi. (wl1251 in the previous code was probably a copy&paste
> mistake ;)
Will be fix in v2
>> Say N if unsure.
>>
>> -config WL1271_SDIO
>> - tristate "TI wl1271 SDIO support"
>> - depends on WL1271&& MMC
>> +config WL12XX_SDIO
>> + tristate "TI wl12xx SDIO support"
>> + depends on WL127X&& MMC
>> ---help---
>> This module adds support for the SDIO interface of adapters using
>> - TI wl1271 chipset. Select this if your platform is using
>> + TI wl12xx chipset. Select this if your platform is using
>> the SDIO bus.
>>
>> - If you choose to build a module, it'll be called
>> - wl1271_sdio. Say N if unsure.
>> + If you choose to build a module, it'll be called sdio.
>> + Say N if unsure.
>
> Same as above for this whole block. "wl12xx chipsets" and wl12xx_sdio.
Will be fix in v2
>
>> config WL12XX_PLATFORM_DATA
>> bool
>> - depends on WL1271_SDIO != n
>> + depends on WL12XX_SDIO != n
>> default y
>> diff --git a/drivers/net/wireless/wl12xx/Makefile b/drivers/net/wireless/wl12xx/Makefile
>> index 3a80744..a19f4f3 100644
>> --- a/drivers/net/wireless/wl12xx/Makefile
>> +++ b/drivers/net/wireless/wl12xx/Makefile
>> @@ -1,12 +1,14 @@
>> -wl1271-objs = wl1271_main.o wl1271_cmd.o wl1271_io.o \
>> - wl1271_event.o wl1271_tx.o wl1271_rx.o \
>> - wl1271_ps.o wl1271_acx.o wl1271_boot.o \
>> - wl1271_init.o wl1271_debugfs.o wl1271_scan.o
>> +wl12xx-objs = main.o cmd.o io.o \
>> + event.o tx.o rx.o \
>> + ps.o acx.o boot.o \
>> + init.o debugfs.o scan.o
>
> You can put these in less lines, now that the names are much shorter.
Will be fix in v2
>
>> +wl12xx_spi-objs = spi.o
>> +wl12xx_sdio-objs = sdio.o
>
> Here...
Will be fix in v2
>
>
>> -wl1271-$(CONFIG_NL80211_TESTMODE) += wl1271_testmode.o
>> -obj-$(CONFIG_WL1271) += wl1271.o
>> -obj-$(CONFIG_WL1271_SPI) += wl1271_spi.o
>> -obj-$(CONFIG_WL1271_SDIO) += wl1271_sdio.o
>> +wl1271-$(CONFIG_NL80211_TESTMODE) += testmode.o
>> +obj-$(CONFIG_WL127X) += wl12xx.o
>> +obj-$(CONFIG_WL12XX_SPI) += wl12xx_spi.o
>> +obj-$(CONFIG_WL12XX_SDIO) += wl12xx_sdio.o
>
> ...and here, you're using wl12xx_spi and wl12xx_sdio correctly, which
> reinforces my comment about calling the modules "spi.ko" and "sdio.ko"
> in the Kconfig section ;)
Will be fix in v2
>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h b/drivers/net/wireless/wl12xx/conf.h
>> similarity index 100%
>> rename from drivers/net/wireless/wl12xx/wl1271_conf.h
>> rename to drivers/net/wireless/wl12xx/conf.h
>
> You forgot to change this:
>
> #ifndef __WL1271_CONF_H__
>
> to:
>
> #ifndef __WL12XX_CONF_H__
>
> And the other appearances of __WL1271_CONF_H__ in that file. This
> applies to all header files.
we agreed that change will be in second stage.
not fix for now.
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_debugfs.h b/drivers/net/wireless/wl12xx/debugfs.h
>> similarity index 98%
>> rename from drivers/net/wireless/wl12xx/wl1271_debugfs.h
>> rename to drivers/net/wireless/wl12xx/debugfs.h
>> index 00a45b2..d12bf93 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_debugfs.h
>> +++ b/drivers/net/wireless/wl12xx/debugfs.h
>> @@ -24,7 +24,7 @@
>> #ifndef WL1271_DEBUGFS_H
>> #define WL1271_DEBUGFS_H
>
> As mentioned above, this #ifndef and #define needs to be changed too.
we agreed that change will be in second stage.
not fix for now.
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/main.c
>> similarity index 99%
>> rename from drivers/net/wireless/wl12xx/wl1271_main.c
>> rename to drivers/net/wireless/wl12xx/main.c
>> index 63036b5..dab10a5 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
>> +++ b/drivers/net/wireless/wl12xx/main.c
>> @@ -31,20 +31,20 @@
>
> [...]
>
>> #define WL1271_BOOT_RETRIES 3
>
> Did we agree not to change this stuff for now? Yes, now I remember, it's
> better to do it in two steps indeed (ie. do the other changes in a
> separate patch). But I'd rather apply all the patches add once.
I believe that patch could stand alone. There isn't any connection
between files names and function+defines names.
next prev parent reply other threads:[~2010-10-28 16:17 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-26 11:18 [PATCH ] wl1271: Change wl12xx Files Names Shahar Levi
2010-10-27 19:25 ` Luciano Coelho
2010-10-28 16:16 ` Shahar Levi [this message]
2010-10-28 17:01 ` Luciano Coelho
2010-10-28 17:01 ` Shahar Levi
2010-10-28 17:53 ` Luciano Coelho
2010-10-28 17:53 ` Shahar Levi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4CC9A1F5.4010308@ti.com \
--to=shahar_levi@ti.com \
--cc=linux-wireless@vger.kernel.org \
--cc=luciano.coelho@nokia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).