From: Luciano Coelho <luciano.coelho@nokia.com>
To: ext Shahar Levi <shahar_levi@ti.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 20:01:03 +0300 [thread overview]
Message-ID: <1288285263.3414.7.camel@powerslave> (raw)
In-Reply-To: <4CC9A1F5.4010308@ti.com>
Hi Shahar,
On Thu, 2010-10-28 at 18:16 +0200, ext Shahar Levi wrote:
> On 10/27/2010 09:25 PM, Luciano Coelho wrote:
> > On Tue, 2010-10-26 at 13:18 +0200, ext Shahar Levi wrote:
> >> -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.
Yes, as I commented to your wl128x patch, I think we should wait until
we can do it at runtime, so no need to differentiate in the
configuration.
> >> +wl12xx_spi-objs = spi.o
> >> +wl12xx_sdio-objs = sdio.o
> >
> > Here...
> Will be fix in v2
There's nothing to fix here. I just pointed out that the modules are
actually called wl12xx_sdio and wl12xx_spi. So it's correct here
already.
> >> -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
Again, nothing to fix here.
> >
> >> 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.
This is a different thing. These macros are always directly related to
the file name. Keeping it as __WL1271_CONF_H__ here would be confusing,
since the file is actually called conf.h. Actually I was wrong when I
said it should be changed to "__WL12XX_CONF_H__" it should be only
"__CONF_H__".
> >> 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.
Same as above. Please fix all the #ifndef and #define in all header
files to correctly match the file names.
> >> 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.
Yes, no need to change these macros or function names in this patch.
--
Cheers,
Luca.
next prev parent reply other threads:[~2010-10-28 16:42 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
2010-10-28 17:01 ` Luciano Coelho [this message]
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=1288285263.3414.7.camel@powerslave \
--to=luciano.coelho@nokia.com \
--cc=linux-wireless@vger.kernel.org \
--cc=shahar_levi@ti.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).