Re: [PATCH ] wl1271: Change wl12xx Files Names

[Luciano Coelho <luciano.coelho@nokia.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.