I got compile errors with the cw1200, which has lead me to take a
closer look. It seems the driver still has a number of issues,
and this addresses some of them and marks others as FIXME:
* The cw1200_sagrad.c file should not be there, hardcoding
hardware configuration in .c files has been obsolete since
Linux-2.4, so we should just remove it. Most of that file
was already commented out since it does not compile.
* Move the parts of cw1200_sagrad.c that actually got used into
cw1200_sdio.c, because it doesn't build otherwise.
* Make the platform_data for the sdio driver private to
cw1200_sdio.c. The platform that was referenced here is
going to use device tree based probing only in the future,
which means the platform data has to go away anyway.
* Move the platform_data for the spi driver to
include/linux/platform_data/net-cw1200.h where all other
drivers have their platform_data.
* Add comments about passing GPIO numbers in platform_data:
You should not use IORESOURCE_IO, which is for legacy ISA
I/O ports on PCs, not for GPIOs.
It may actually be easier to remove the sdio driver entirely,
since it clearly doesn't work and requires a lot of cleanup.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Solomon Peachy <[email protected]>
Cc: John W. Linville <[email protected]>
Cc: Wei Yongjun <[email protected]>
Cc: Dmitry Tarnyagin <[email protected]>
Cc: Ajitpal Singh <[email protected]>
Cc: [email protected]
---
drivers/net/wireless/cw1200/Kconfig | 8 --
drivers/net/wireless/cw1200/Makefile | 2 -
drivers/net/wireless/cw1200/cw1200_sagrad.c | 145 ---------------------
drivers/net/wireless/cw1200/cw1200_sdio.c | 72 ++++++++--
drivers/net/wireless/cw1200/cw1200_spi.c | 2 +-
.../net-cw1200.h} | 20 +--
6 files changed, 62 insertions(+), 187 deletions(-)
delete mode 100644 drivers/net/wireless/cw1200/cw1200_sagrad.c
rename include/linux/{cw1200_platform.h => platform_data/net-cw1200.h} (53%)
diff --git a/drivers/net/wireless/cw1200/Kconfig b/drivers/net/wireless/cw1200/Kconfig
index 13e3611..c3142d4 100644
--- a/drivers/net/wireless/cw1200/Kconfig
+++ b/drivers/net/wireless/cw1200/Kconfig
@@ -20,14 +20,6 @@ config CW1200_WLAN_SPI
help
Enables support for the CW1200 connected via a SPI bus.
-config CW1200_WLAN_SAGRAD
- tristate "Support Sagrad SG901-1091/1098 modules"
- depends on CW1200_WLAN_SDIO
- help
- This provides the platform data glue to support the
- Sagrad SG901-1091/1098 modules in their standard SDIO EVK.
- It also includes example SPI platform data.
-
menu "Driver debug features"
depends on CW1200 && DEBUG_FS
diff --git a/drivers/net/wireless/cw1200/Makefile b/drivers/net/wireless/cw1200/Makefile
index 1aa3682..bc6cbf9 100644
--- a/drivers/net/wireless/cw1200/Makefile
+++ b/drivers/net/wireless/cw1200/Makefile
@@ -16,9 +16,7 @@ cw1200_core-$(CONFIG_PM) += pm.o
cw1200_wlan_sdio-y := cw1200_sdio.o
cw1200_wlan_spi-y := cw1200_spi.o
-cw1200_wlan_sagrad-y := cw1200_sagrad.o
obj-$(CONFIG_CW1200) += cw1200_core.o
obj-$(CONFIG_CW1200_WLAN_SDIO) += cw1200_wlan_sdio.o
obj-$(CONFIG_CW1200_WLAN_SPI) += cw1200_wlan_spi.o
-obj-$(CONFIG_CW1200_WLAN_SAGRAD) += cw1200_wlan_sagrad.o
diff --git a/drivers/net/wireless/cw1200/cw1200_sagrad.c b/drivers/net/wireless/cw1200/cw1200_sagrad.c
deleted file mode 100644
index a5ada0e..0000000
--- a/drivers/net/wireless/cw1200/cw1200_sagrad.c
+++ /dev/null
@@ -1,145 +0,0 @@
-/*
- * Platform glue data for ST-Ericsson CW1200 driver
- *
- * Copyright (c) 2013, Sagrad, Inc
- * Author: Solomon Peachy <[email protected]>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#include <linux/module.h>
-#include <linux/cw1200_platform.h>
-
-MODULE_AUTHOR("Solomon Peachy <[email protected]>");
-MODULE_DESCRIPTION("ST-Ericsson CW1200 Platform glue driver");
-MODULE_LICENSE("GPL");
-
-/* Define just one of these. Feel free to customize as needed */
-#define SAGRAD_1091_1098_EVK_SDIO
-/* #define SAGRAD_1091_1098_EVK_SPI */
-
-#ifdef SAGRAD_1091_1098_EVK_SDIO
-#if 0
-static struct resource cw1200_href_resources[] = {
- {
- .start = 215, /* fix me as appropriate */
- .end = 215, /* ditto */
- .flags = IORESOURCE_IO,
- .name = "cw1200_wlan_reset",
- },
- {
- .start = 216, /* fix me as appropriate */
- .end = 216, /* ditto */
- .flags = IORESOURCE_IO,
- .name = "cw1200_wlan_powerup",
- },
- {
- .start = NOMADIK_GPIO_TO_IRQ(216), /* fix me as appropriate */
- .end = NOMADIK_GPIO_TO_IRQ(216), /* ditto */
- .flags = IORESOURCE_IRQ,
- .name = "cw1200_wlan_irq",
- },
-};
-#endif
-
-static int cw1200_power_ctrl(const struct cw1200_platform_data_sdio *pdata,
- bool enable)
-{
- /* Control 3v3 and 1v8 to hardware as appropriate */
- /* Note this is not needed if it's controlled elsewhere or always on */
-
- /* May require delay for power to stabilize */
- return 0;
-}
-
-static int cw1200_clk_ctrl(const struct cw1200_platform_data_sdio *pdata,
- bool enable)
-{
- /* Turn CLK_32K off and on as appropriate. */
- /* Note this is not needed if it's always on */
-
- /* May require delay for clock to stabilize */
- return 0;
-}
-
-static struct cw1200_platform_data_sdio cw1200_platform_data = {
- .ref_clk = 38400,
- .have_5ghz = false,
-#if 0
- .reset = &cw1200_href_resources[0],
- .powerup = &cw1200_href_resources[1],
- .irq = &cw1200_href_resources[2],
-#endif
- .power_ctrl = cw1200_power_ctrl,
- .clk_ctrl = cw1200_clk_ctrl,
-/* .macaddr = ??? */
- .sdd_file = "sdd_sagrad_1091_1098.bin",
-};
-#endif
-
-#ifdef SAGRAD_1091_1098_EVK_SPI
-/* Note that this is an example of integrating into your board support file */
-static struct resource cw1200_href_resources[] = {
- {
- .start = GPIO_RF_RESET,
- .end = GPIO_RF_RESET,
- .flags = IORESOURCE_IO,
- .name = "cw1200_wlan_reset",
- },
- {
- .start = GPIO_RF_POWERUP,
- .end = GPIO_RF_POWERUP,
- .flags = IORESOURCE_IO,
- .name = "cw1200_wlan_powerup",
- },
-};
-
-static int cw1200_power_ctrl(const struct cw1200_platform_data_spi *pdata,
- bool enable)
-{
- /* Control 3v3 and 1v8 to hardware as appropriate */
- /* Note this is not needed if it's controlled elsewhere or always on */
-
- /* May require delay for power to stabilize */
- return 0;
-}
-static int cw1200_clk_ctrl(const struct cw1200_platform_data_spi *pdata,
- bool enable)
-{
- /* Turn CLK_32K off and on as appropriate. */
- /* Note this is not needed if it's always on */
-
- /* May require delay for clock to stabilize */
- return 0;
-}
-
-static struct cw1200_platform_data_spi cw1200_platform_data = {
- .ref_clk = 38400,
- .spi_bits_per_word = 16,
- .reset = &cw1200_href_resources[0],
- .powerup = &cw1200_href_resources[1],
- .power_ctrl = cw1200_power_ctrl,
- .clk_ctrl = cw1200_clk_ctrl,
-/* .macaddr = ??? */
- .sdd_file = "sdd_sagrad_1091_1098.bin",
-};
-static struct spi_board_info myboard_spi_devices[] __initdata = {
- {
- .modalias = "cw1200_wlan_spi",
- .max_speed_hz = 10000000, /* 52MHz Max */
- .bus_num = 0,
- .irq = WIFI_IRQ,
- .platform_data = &cw1200_platform_data,
- .chip_select = 0,
- },
-};
-#endif
-
-
-const void *cw1200_get_platform_data(void)
-{
- return &cw1200_platform_data;
-}
-EXPORT_SYMBOL_GPL(cw1200_get_platform_data);
diff --git a/drivers/net/wireless/cw1200/cw1200_sdio.c b/drivers/net/wireless/cw1200/cw1200_sdio.c
index 863510d..721e392 100644
--- a/drivers/net/wireless/cw1200/cw1200_sdio.c
+++ b/drivers/net/wireless/cw1200/cw1200_sdio.c
@@ -20,7 +20,6 @@
#include "cw1200.h"
#include "sbus.h"
-#include <linux/cw1200_platform.h>
#include "hwio.h"
MODULE_AUTHOR("Dmitry Tarnyagin <[email protected]>");
@@ -29,6 +28,27 @@ MODULE_LICENSE("GPL");
#define SDIO_BLOCK_SIZE (512)
+/* FIXME: include this into sbus_priv */
+struct cw1200_platform_data_sdio {
+ u16 ref_clk; /* REQUIRED (in KHz) */
+
+ /* All others are optional */
+ /* FIXME: just do gpio_to_irq */
+ const struct resource *irq; /* if using GPIO for IRQ */
+ bool have_5ghz;
+ bool no_nptb; /* SDIO hardware does not support non-power-of-2-blocksizes */
+ /* FIXME: GPIO numbers are not resources */
+ const struct resource *reset; /* GPIO to RSTn signal */
+ const struct resource *powerup; /* GPIO to POWERUP signal */
+ int (*power_ctrl)(const struct cw1200_platform_data_sdio *pdata,
+ bool enable); /* Control 3v3 / 1v8 supply */
+ int (*clk_ctrl)(const struct cw1200_platform_data_sdio *pdata,
+ bool enable); /* Control CLK32K */
+ /* FIXME: us of_get_mac_address */
+ const u8 *macaddr; /* if NULL, use cw1200_mac_template module parameter */
+ const char *sdd_file; /* if NULL, will use default for detected hw type */
+};
+
struct sbus_priv {
struct sdio_func *func;
struct cw1200_common *core;
@@ -265,6 +285,38 @@ static struct sbus_ops cw1200_sdio_sbus_ops = {
.power_mgmt = cw1200_sdio_pm,
};
+static int cw1200_power_ctrl(const struct cw1200_platform_data_sdio *pdata,
+ bool enable)
+{
+ /* Control 3v3 and 1v8 to hardware as appropriate */
+ /* Note this is not needed if it's controlled elsewhere or always on */
+
+ /* May require delay for power to stabilize */
+ return 0;
+}
+
+static int cw1200_clk_ctrl(const struct cw1200_platform_data_sdio *pdata,
+ bool enable)
+{
+ /* Turn CLK_32K off and on as appropriate. */
+ /* Note this is not needed if it's always on */
+
+ /* May require delay for clock to stabilize */
+ return 0;
+}
+
+/*
+ * FIXME: These are just defaults, the driver needs a proper DT binding
+ * to extract the other values and override these if necessary
+ */
+static struct cw1200_platform_data_sdio cw1200_platform_data = {
+ .ref_clk = 38400,
+ .have_5ghz = false,
+ .power_ctrl = cw1200_power_ctrl,
+ .clk_ctrl = cw1200_clk_ctrl,
+ .sdd_file = "sdd_sagrad_1091_1098.bin",
+};
+
/* Probe Function to be called by SDIO stack when device is discovered */
static int cw1200_sdio_probe(struct sdio_func *func,
const struct sdio_device_id *id)
@@ -286,7 +338,8 @@ static int cw1200_sdio_probe(struct sdio_func *func,
func->card->quirks |= MMC_QUIRK_LENIENT_FN0;
- self->pdata = cw1200_get_platform_data();
+ /* FIXME: get this from DT */
+ self->pdata = &cw1200_platform_data;
self->func = func;
sdio_set_drvdata(func, self);
sdio_claim_host(func);
@@ -377,13 +430,11 @@ static struct sdio_driver sdio_driver = {
/* Init Module function -> Called by insmod */
static int __init cw1200_sdio_init(void)
{
- const struct cw1200_platform_data_sdio *pdata;
int ret;
- pdata = cw1200_get_platform_data();
-
- if (cw1200_sdio_on(pdata)) {
- ret = -1;
+ /* FIXME: must not touch hardware until we know the hardware is present */
+ if (cw1200_sdio_on(&cw1200_platform_data)) {
+ ret = -ENODEV;
goto err;
}
@@ -394,19 +445,16 @@ static int __init cw1200_sdio_init(void)
return 0;
err:
- cw1200_sdio_off(pdata);
+ cw1200_sdio_off(&cw1200_platform_data);
return ret;
}
/* Called at Driver Unloading */
static void __exit cw1200_sdio_exit(void)
{
- const struct cw1200_platform_data_sdio *pdata;
- pdata = cw1200_get_platform_data();
sdio_unregister_driver(&sdio_driver);
- cw1200_sdio_off(pdata);
+ cw1200_sdio_off(&cw1200_platform_data);
}
-
module_init(cw1200_sdio_init);
module_exit(cw1200_sdio_exit);
diff --git a/drivers/net/wireless/cw1200/cw1200_spi.c b/drivers/net/wireless/cw1200/cw1200_spi.c
index 75adef0..ef05916 100644
--- a/drivers/net/wireless/cw1200/cw1200_spi.c
+++ b/drivers/net/wireless/cw1200/cw1200_spi.c
@@ -25,7 +25,7 @@
#include "cw1200.h"
#include "sbus.h"
-#include <linux/cw1200_platform.h>
+#include <linux/platform_data/net-cw1200.h>
#include "hwio.h"
MODULE_AUTHOR("Solomon Peachy <[email protected]>");
diff --git a/include/linux/cw1200_platform.h b/include/linux/platform_data/net-cw1200.h
similarity index 53%
rename from include/linux/cw1200_platform.h
rename to include/linux/platform_data/net-cw1200.h
index c168fa5..8f006ac 100644
--- a/include/linux/cw1200_platform.h
+++ b/include/linux/platform_data/net-cw1200.h
@@ -14,6 +14,7 @@ struct cw1200_platform_data_spi {
/* All others are optional */
bool have_5ghz;
+ /* FIXME: use simple numbers rather than resources for GPIO */
const struct resource *reset; /* GPIO to RSTn signal */
const struct resource *powerup; /* GPIO to POWERUP signal */
int (*power_ctrl)(const struct cw1200_platform_data_spi *pdata,
@@ -24,23 +25,4 @@ struct cw1200_platform_data_spi {
const char *sdd_file; /* if NULL, will use default for detected hw type */
};
-struct cw1200_platform_data_sdio {
- u16 ref_clk; /* REQUIRED (in KHz) */
-
- /* All others are optional */
- const struct resource *irq; /* if using GPIO for IRQ */
- bool have_5ghz;
- bool no_nptb; /* SDIO hardware does not support non-power-of-2-blocksizes */
- const struct resource *reset; /* GPIO to RSTn signal */
- const struct resource *powerup; /* GPIO to POWERUP signal */
- int (*power_ctrl)(const struct cw1200_platform_data_sdio *pdata,
- bool enable); /* Control 3v3 / 1v8 supply */
- int (*clk_ctrl)(const struct cw1200_platform_data_sdio *pdata,
- bool enable); /* Control CLK32K */
- const u8 *macaddr; /* if NULL, use cw1200_mac_template module parameter */
- const char *sdd_file; /* if NULL, will use default for detected hw type */
-};
-
-const void *cw1200_get_platform_data(void);
-
#endif /* CW1200_PLAT_H_INCLUDED */
--
1.8.1.2
On Sun, Jun 02, 2013 at 12:37:21AM +0200, Arnd Bergmann wrote:
> I got compile errors with the cw1200, which has lead me to take a
> closer look. It seems the driver still has a number of issues,
> and this addresses some of them and marks others as FIXME:
In short, NACK, at least not as posted.
The concerns are legitimate, but the time to object to such fundamental
stuff was at some point during the past *seven* rounds of patches I
posted over a period of nearly as many months.
Most of the objections you are raising are for deliberate
design/implementation decisions I made to deal with the realities of the
requirements of the cw1200 design and already-in-the-wild hardware that
this driver has to support.
> * The cw1200_sagrad.c file should not be there, hardcoding
> hardware configuration in .c files has been obsolete since
> Linux-2.4, so we should just remove it. Most of that file
> was already commented out since it does not compile.
The problem with the cw1200 is that you need certain information about
the hardware design in order to initialize it; this information has to
come from somewhere.
The Sagrad wifi devices are available in a standard SD form factor
reference design that plugs into a standard SDIO-supporing slot. The
cw1200_sagrad module is there to support these off-the-shelf devices.
Requiring users to recompile their kernel or update their devicetree
data for what amounts to an off-the-shelf hot-pluggable device is simply
not acceptable.
At the same time, if people plop the sagrad device directly on their
board (or independently do a chip-down design) they may need to make
changes to the platform data depending on how closely it tracks the
Sagrad reference design.
Further complicating things, there's no way to alter the SDIO
vendor/device IDs that the device reports in order to distingish between
any of this.
If you have a better suggestion on how to handle this set of conflicting
requirements then I'm all ears, but it's rather pointless to object to
code that's ugly because it has to support ugly hardware.
(That said, the commented-out bits in cw1200_sagrad are mostly there for
documentation purposes, and it could be moved to a proper documentation
file instead.)
> * Move the parts of cw1200_sagrad.c that actually got used into
> cw1200_sdio.c, because it doesn't build otherwise.
The idea was that either you build cw1200_sagrad or provide your own
platform_data. I separated all of the implemenation-specific code out
as cleanly as I could.
> * Make the platform_data for the sdio driver private to
> cw1200_sdio.c. The platform that was referenced here is
> going to use device tree based probing only in the future,
> which means the platform data has to go away anyway.
When you say "the platform" what are you referring to? SDIO? There's
no mention of any specific board (or even arch/subarch) in cw1200_sdio
or cw1200_sagrad.
In the real world, this driver will have to support non-devicetree
operation for as long as Linux does, and indeed longer, thanks to
compat-drivers/backports.
And for what it's worth, none of the platforms I have access to have
devicetree support since I'm stuck using vendor-supplied kernels.
> * Move the platform_data for the spi driver to
> include/linux/platform_data/net-cw1200.h where all other
> drivers have their platform_data.
Not all drivers, but fair enough.
> * Add comments about passing GPIO numbers in platform_data:
> You should not use IORESOURCE_IO, which is for legacy ISA
> I/O ports on PCs, not for GPIOs.
Fair enough. The use of resources was something already in the driver
when I inherited it, but I've seen this pattern a lot elsewhere. Is
there a specific driver I should reference instead?
> It may actually be easier to remove the sdio driver entirely,
> since it clearly doesn't work and requires a lot of cleanup.
Several hundred thousand in-the-field devices already deployed with this
driver clearly disagree with you.
I've personally tested this driver on six different hardware platforms,
using mainline kernels for some and vendor-supplied kernels for others.
With module-down and SD-slot designs. Others have tested other
platforms. Every configurable option and line item in both the SPI and
SDIO platform data is there because it needs to be in order to support
the variety of hardware designs already in the wild.
This driver will also be part of the compat-drivers/backports project,
and as such has to work within that framework as well.
If you have constructive suggestions on how to handle this set of
requirements in a cleaner manner, I'm all ears.
- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Delray Beach, FL ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.
On Sun, Jun 02, 2013 at 08:29:54AM -0400, Solomon Peachy wrote:
> > * Add comments about passing GPIO numbers in platform_data:
> > You should not use IORESOURCE_IO, which is for legacy ISA
> > I/O ports on PCs, not for GPIOs.
>
> Fair enough. The use of resources was something already in the driver
> when I inherited it, but I've seen this pattern a lot elsewhere. Is
> there a specific driver I should reference instead?
Reading linux/ioport.h I don't see a type that seems to be a better fit.
It's not MEM, REG, IRQ, DMA, or BUS. IO seems to be the only type that
fits.
The reason the driver uses struct resources instead of straight-up
numeric GPIO fields is for the 'name' field in the resources.
Given that the use of platform_data pretty much makes it impossible to
have more than one of these devices in a system at a time, there doesn't
seem to be a point to using named resources.
So I'll change these resource lists to using straight-up ints.
- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Delray Beach, FL ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.
On Sunday 02 June 2013 08:29:54 Solomon Peachy wrote:
> On Sun, Jun 02, 2013 at 12:37:21AM +0200, Arnd Bergmann wrote:
> > I got compile errors with the cw1200, which has lead me to take a
> > closer look. It seems the driver still has a number of issues,
> > and this addresses some of them and marks others as FIXME:
>
> In short, NACK, at least not as posted.
>
> The concerns are legitimate, but the time to object to such fundamental
> stuff was at some point during the past *seven* rounds of patches I
> posted over a period of nearly as many months.
The driver doesn't reliably build, and I'm trying to fix it.
>From my perspective, we can just mark it 'depends on !ARM'
to get it off my radar, but other people are doing build
testing on x86 and will run into the same issues.
> > * The cw1200_sagrad.c file should not be there, hardcoding
> > hardware configuration in .c files has been obsolete since
> > Linux-2.4, so we should just remove it. Most of that file
> > was already commented out since it does not compile.
>
> The problem with the cw1200 is that you need certain information about
> the hardware design in order to initialize it; this information has to
> come from somewhere.
If the hardware cannot be identified from register access, the
information should get passed through firmware. On most architectures
that means device tree data, on x86 that would be ACPI tables.
> The Sagrad wifi devices are available in a standard SD form factor
> reference design that plugs into a standard SDIO-supporing slot. The
> cw1200_sagrad module is there to support these off-the-shelf devices.
The code does not look particularly off-the-shelf though:
static struct resource cw1200_href_resources[] = {
{
.start = 215, /* fix me as appropriate */
.end = 215, /* ditto */
.flags = IORESOURCE_IO,
.name = "cw1200_wlan_reset",
},
{
.start = 216, /* fix me as appropriate */
.end = 216, /* ditto */
.flags = IORESOURCE_IO,
.name = "cw1200_wlan_powerup",
},
{
.start = NOMADIK_GPIO_TO_IRQ(216), /* fix me as appropriate */
.end = NOMADIK_GPIO_TO_IRQ(216), /* ditto */
.flags = IORESOURCE_IRQ,
.name = "cw1200_wlan_irq",
},
};
There is nothing generic about that.
> Requiring users to recompile their kernel or update their devicetree
> data for what amounts to an off-the-shelf hot-pluggable device is simply
> not acceptable.
That was the intention of the patch. Right now, you require users
to modify cw1200_sagrad.c.
> At the same time, if people plop the sagrad device directly on their
> board (or independently do a chip-down design) they may need to make
> changes to the platform data depending on how closely it tracks the
> Sagrad reference design.
We can use platform_data if there is convincing evidence that people
are going to be using that in mainline kernels. However, your SDIO driver
doesn't actually do that, it just hardcodes settings in cw1200_sagrad.c
and calls a local function to get that data, rather than getting the data
from the dev_get_platdata() in it's probe callback at a point where
it knows the device is actually there.
> Further complicating things, there's no way to alter the SDIO
> vendor/device IDs that the device reports in order to distingish between
> any of this.
>
> If you have a better suggestion on how to handle this set of conflicting
> requirements then I'm all ears, but it's rather pointless to object to
> code that's ugly because it has to support ugly hardware.
>
> (That said, the commented-out bits in cw1200_sagrad are mostly there for
> documentation purposes, and it could be moved to a proper documentation
> file instead.)
I think a better place for that would be the include/linux/platform_data
header file.
If we only care about two cases a) a default sagrad card that can use
hardcoded data and b) a board that uses the same chip and requires
custom data, then I would suggest including the sagrad data in the
sdio driver as a default (as my patch does) but allow overriding
it with platform data.
> > * Move the parts of cw1200_sagrad.c that actually got used into
> > cw1200_sdio.c, because it doesn't build otherwise.
>
> The idea was that either you build cw1200_sagrad or provide your own
> platform_data. I separated all of the implemenation-specific code out
> as cleanly as I could.
The problem with this is that it is impossible to build a kernel that
supports both, or even two devices of the same type.
> > * Make the platform_data for the sdio driver private to
> > cw1200_sdio.c. The platform that was referenced here is
> > going to use device tree based probing only in the future,
> > which means the platform data has to go away anyway.
>
> When you say "the platform" what are you referring to? SDIO? There's
> no mention of any specific board (or even arch/subarch) in cw1200_sdio
> or cw1200_sagrad.
I mean arch/arm/mach-nomadik, which is the platform that originally
defined NOMADIK_GPIO_TO_IRQ(). Note that in recent kernels, mach-ux500
also uses this macro internally, but it's already gone in nomadik
and will stop working in ux500 in the future.
> In the real world, this driver will have to support non-devicetree
> operation for as long as Linux does, and indeed longer, thanks to
> compat-drivers/backports.
I don't mind the backports supporting that, but we should probably
move on in mainline. The existance of backports is no excuse for
keeping around broken code.
> And for what it's worth, none of the platforms I have access to have
> devicetree support since I'm stuck using vendor-supplied kernels.
Out of tree platforms are also not really an issue, since you already
need to patch the kernel. You can patch it some more to add platform
data back.
> > * Add comments about passing GPIO numbers in platform_data:
> > You should not use IORESOURCE_IO, which is for legacy ISA
> > I/O ports on PCs, not for GPIOs.
>
> Fair enough. The use of resources was something already in the driver
> when I inherited it, but I've seen this pattern a lot elsewhere. Is
> there a specific driver I should reference instead?
just look in include/linux/platform_data/. The most common type for
gpio numbers is 'int'.
> > It may actually be easier to remove the sdio driver entirely,
> > since it clearly doesn't work and requires a lot of cleanup.
>
> Several hundred thousand in-the-field devices already deployed with this
> driver clearly disagree with you.
Sorry for misinterpreting that, the cw1200_sagrad file really looked
like it was written for some out-of-tree board and subsequently
all lines that didn't compile got commented out.
> I've personally tested this driver on six different hardware platforms,
> using mainline kernels for some and vendor-supplied kernels for others.
I was only talking about mainline kernels, obviously the driver has
worked somewhere before, but that is not the point if the working
version is not the one that got merged.
In fact my mobile phone has a cw1200 chip that was working until
recently. Now it just crashes when I do 'ifconfig wlan0 up'
or enable WLAN in the Android settings menu. :(
I'm not blaming you for that ;-)
> With module-down and SD-slot designs. Others have tested other
> platforms. Every configurable option and line item in both the SPI and
> SDIO platform data is there because it needs to be in order to support
> the variety of hardware designs already in the wild.
>
> This driver will also be part of the compat-drivers/backports project,
> and as such has to work within that framework as well.
>
> If you have constructive suggestions on how to handle this set of
> requirements in a cleaner manner, I'm all ears.
I think the most important part is separating the generic sagrad
add-on card code from any platform-specific code and removing the
use of cw1200_get_platform_data() function that breaks the build.
Arnd
On Sun, Jun 02, 2013 at 03:59:12PM +0200, Arnd Bergmann wrote:
> The driver doesn't reliably build, and I'm trying to fix it.
> From my perspective, we can just mark it 'depends on !ARM'
> to get it off my radar, but other people are doing build
> testing on x86 and will run into the same issues.
AFAIK the only remaining build problem is when CONFIG_CW1200_SAGRAD is
not defined so there's no "platform" data provided.
> If the hardware cannot be identified from register access, the
> information should get passed through firmware. On most architectures
> that means device tree data, on x86 that would be ACPI tables.
The cw1200 has specific reset sequencing requirements. Some designs use
a hardware reset circuit (such as the Sagrad EVK/Reference design), but
most others use GPIOs for cost resons.
Without that reset sequence, the device will never attach itself to the
SDIO bus so we will never get to the point where we can probe it via
register accesses (and program the DPLL value) to discern enough
information to load the appropriate firmware.
(The DPLL value is part of the so-called 'SDD' file, but that file is
also tied to the specific implemntation. You have to love those
chicken-and-egg problems...)
> The code does not look particularly off-the-shelf though:
> static struct resource cw1200_href_resources[] = {
That code is #ifdef'd out, and was purely left there as a reference.
It's gone now. :)
> We can use platform_data if there is convincing evidence that people
> are going to be using that in mainline kernels. However, your SDIO driver
> doesn't actually do that, it just hardcodes settings in cw1200_sagrad.c
> and calls a local function to get that data, rather than getting the data
> from the dev_get_platdata() in it's probe callback at a point where
> it knows the device is actually there.
As I mentioned earlier, we don't know the device is there until after we
get enough information from the platform_data to power it up and deal
with the finiky reset sequence.
Basically the Sagrad SD-slot-form-factor EVK is a special case only
meant for evaluation purposes. The "normal" use case for thise driver
is a hard-wired, permanently attached design.
> If we only care about two cases a) a default sagrad card that can use
> hardcoded data and b) a board that uses the same chip and requires
> custom data, then I would suggest including the sagrad data in the
> sdio driver as a default (as my patch does) but allow overriding
> it with platform data.
Yes, those are the only use cases we care about. And that sounds like a
viable approach, so I'll code that up.
I guess adding a function along the lines of
"cw1200_sdio_set_platform_data()" (to be called by the board/platform
setup code) would be the right way to do this?
As an aside, I wish I'd received this feedback a couple of months ago.
> The problem with this is that it is impossible to build a kernel that
> supports both, or even two devices of the same type.
Adding proper devicetree support will solve this problem down the line,
but I don't think there's any way to fix this for non-DT systems thanks
to that chicken-and-egg probing situation.
(I wish I had a DT-capable platform handy to develop against..)
> I mean arch/arm/mach-nomadik, which is the platform that originally
> defined NOMADIK_GPIO_TO_IRQ(). Note that in recent kernels, mach-ux500
> also uses this macro internally, but it's already gone in nomadik
> and will stop working in ux500 in the future.
Ah, okay. FWIW that was a leftover bit from the original driver authors
who'd tied the code rather tightly to the ux500 platform. That
reference is gone now in any case.
> I don't mind the backports supporting that, but we should probably
> move on in mainline. The existance of backports is no excuse for
> keeping around broken code.
I already have one patch queued up for backports to re-enable <2.6.36
support, and I have no problem adding more.
That said, until all SDIO/SPI-supporting targets in the mainline are
converted to a devicetree (or equivalent) model, I imagine the
platform_data crap will have to remain.
> just look in include/linux/platform_data/. The most common type for
> gpio numbers is 'int'.
I've already posted a patch that converts them over.
> Sorry for misinterpreting that, the cw1200_sagrad file really looked
> like it was written for some out-of-tree board and subsequently
> all lines that didn't compile got commented out.
No problem. I just needed to make it clear that this driver did in fact
work -- and it also compiled cleanly on x86_64 as long as the sagrad
symbol was defined.
> In fact my mobile phone has a cw1200 chip that was working until
> recently. Now it just crashes when I do 'ifconfig wlan0 up'
> or enable WLAN in the Android settings menu. :(
>
> I'm not blaming you for that ;-)
What model, out of curiousity?
> I think the most important part is separating the generic sagrad
> add-on card code from any platform-specific code and removing the
> use of cw1200_get_platform_data() function that breaks the build.
I'll convert SDIO driver to using the sagrad data as default and add a
'set_platform_data' sort of function to allow it to be optionally
overridden. It shouldn't take long, and I'll post the patch as soon as
I'm finished.
(I don't know how long it'll take to get merged though.. none of the
already-posted followup cw1200 patches have been merged into
wireless-next yet)
- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Delray Beach, FL ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.
On Sun, Jun 2, 2013 at 4:47 PM, Solomon Peachy <[email protected]> wrote:
>> In fact my mobile phone has a cw1200 chip that was working until
>> recently. Now it just crashes when I do 'ifconfig wlan0 up'
>> or enable WLAN in the Android settings menu. :(
>>
>> I'm not blaming you for that ;-)
>
> What model, out of curiousity?
FYI the CW1200 is used in for example:
http://en.wikipedia.org/wiki/Samsung_Galaxy_S_Advance
http://en.wikipedia.org/wiki/Samsung_Galaxy_S_III_Mini
http://en.wikipedia.org/wiki/Sony_Xperia_P
http://en.wikipedia.org/wiki/Sony_Xperia_U
http://en.wikipedia.org/wiki/Sony_Xperia_sola
(plus some others)
Yours,
Linus Walleij
On Sunday 02 June 2013 10:47:26 Solomon Peachy wrote:
> On Sun, Jun 02, 2013 at 03:59:12PM +0200, Arnd Bergmann wrote:
> > The driver doesn't reliably build, and I'm trying to fix it.
> > From my perspective, we can just mark it 'depends on !ARM'
> > to get it off my radar, but other people are doing build
> > testing on x86 and will run into the same issues.
>
> AFAIK the only remaining build problem is when CONFIG_CW1200_SAGRAD is
> not defined so there's no "platform" data provided.
Yes, that is correct. My first approach was to always 'select CW1200_SAGRAD'
from the CW1200_SDIO driver, but then it becomes impossible to have
a different definition of the data in platform code. Even without doing
it you also have a build error if you try to add a platform_data definition
to more than one board file in the kernel or enable the sagrad driver
at the same time as a platform.
> > If we only care about two cases a) a default sagrad card that can use
> > hardcoded data and b) a board that uses the same chip and requires
> > custom data, then I would suggest including the sagrad data in the
> > sdio driver as a default (as my patch does) but allow overriding
> > it with platform data.
>
> Yes, those are the only use cases we care about. And that sounds like a
> viable approach, so I'll code that up.
>
> I guess adding a function along the lines of
> "cw1200_sdio_set_platform_data()" (to be called by the board/platform
> setup code) would be the right way to do this?
It's much better than what you have today, but not ideal because it
means the driver cannot be a loadable module any more.
> As an aside, I wish I'd received this feedback a couple of months ago.
Yes, I agree it's unfortunate timing. Our review process is certainly
not perfect when you have to wait for stuff to break in linux-next
before you get people to notice the problems.
At least it's not in a released kernel yet.
> > The problem with this is that it is impossible to build a kernel that
> > supports both, or even two devices of the same type.
>
> Adding proper devicetree support will solve this problem down the line,
> but I don't think there's any way to fix this for non-DT systems thanks
> to that chicken-and-egg probing situation.
>
> (I wish I had a DT-capable platform handy to develop against..)
All ST-Ericsson DB8500 (ux500) systems should now work with DT, and will
be DT-only in one of the next few kernel releases.
> > I don't mind the backports supporting that, but we should probably
> > move on in mainline. The existance of backports is no excuse for
> > keeping around broken code.
>
> I already have one patch queued up for backports to re-enable <2.6.36
> support, and I have no problem adding more.
>
> That said, until all SDIO/SPI-supporting targets in the mainline are
> converted to a devicetree (or equivalent) model, I imagine the
> platform_data crap will have to remain.
I'm not sure about this: I would argue that only the boards that
are part of mainline and have a cw1200 wired up are what matters here.
Having an SoC platform with SDIO support and the potential to add
a board file including cw1200 is not relevant as long as that board
file does not actually get added. If someone can patch the kernel
to add a board file, they can also patch the driver in any way
necessary.
In contrast, with devicetree we normally anticipate that people can
have their external dts files which reference DT-enabled drivers
even if the dts file is not itself part of the kernel.
> > just look in include/linux/platform_data/. The most common type for
> > gpio numbers is 'int'.
>
> I've already posted a patch that converts them over.
ok.
> > In fact my mobile phone has a cw1200 chip that was working until
> > recently. Now it just crashes when I do 'ifconfig wlan0 up'
> > or enable WLAN in the Android settings menu. :(
> >
> > I'm not blaming you for that ;-)
>
> What model, out of curiousity?
This is a Sony xperia Sola. Wireless was working for about a year
with the original Android 2.3 image, but it broke a week after
I installed CyanogenMod 9.1.
> > I think the most important part is separating the generic sagrad
> > add-on card code from any platform-specific code and removing the
> > use of cw1200_get_platform_data() function that breaks the build.
>
> I'll convert SDIO driver to using the sagrad data as default and add a
> 'set_platform_data' sort of function to allow it to be optionally
> overridden. It shouldn't take long, and I'll post the patch as soon as
> I'm finished.
Ok, sounds like a good next step.
Regarding a long-term solution, I think what we should do here is to
move the reset logic into the SDIO framework itself: We have similar
requirements even for eMMC and SD cards, where you need to provide
the correct voltage to a device on the MMC port in order for it to
work. Today this is supported to a varying degree on the MMC controller
level, where we hook into various frameworks (clk, reset-controller,
regulator, gpio, pinctrl, power domain) based on platformm data or
device tree information on the controller node.
I think you can do this today with cw1200 as long as you know which
MMC controller you have, which may also solve your problem, but it
would be nice if that could be made more generic for SDIO, since
the problem is neither controller-specific nor device-specific
but can happen on any SDIO device.
Looking at the source code for my phone, there is one clock that
needs to be enabled for the cw1200 through pdata->clk_ctrl. This
should probably just be a turned into an anonymous clock so the
cw1200 driver itself does "clk = clk_get(dev, NULL); clk_enable(clk);"
and the callback can be removed.
There is also IRQ and reset logic in this board-mop500-wlan.c file.
The IRQ logic seems fine, and for the reset, I would suggest using
the gpio-reset driver that is getting proposed for merging in 3.11,
and to add a call to device_reset() into the sdio layer.
Arnd
On Mon, Jun 03, 2013 at 10:40:42AM +0200, Arnd Bergmann wrote:
> It's much better than what you have today, but not ideal because it
> means the driver cannot be a loadable module any more.
At least not when being built with platform data, anyway.
I suppose the next step here is to define some devicetree mappings for
this device.
> At least it's not in a released kernel yet.
Given the boneheaded buffer overflow bug that was just pointed out (and
fixed), I'm glad it's getting a lot more scrutiny.
> Regarding a long-term solution, I think what we should do here is to
> move the reset logic into the SDIO framework itself: We have similar
> requirements even for eMMC and SD cards, where you need to provide
> the correct voltage to a device on the MMC port in order for it to
> work. Today this is supported to a varying degree on the MMC controller
> level, where we hook into various frameworks (clk, reset-controller,
> regulator, gpio, pinctrl, power domain) based on platformm data or
> device tree information on the controller node.
If I understand you correctly, the "traditional" platform data -- power
supply, clock source, and gpio (ie POWERUP, RESET) lines would be
brought up using this mechanism, in order to get the device to attach to
the SDIO bus. One wrinkle here is that the cw1200 has some minimum
timing requirements between when each of those signals are brought up; I
don't know if there's a way to encode that into the devicetree.
But that's only half the equation.
Once the device has identified it to the SDIO bus and the driver's
probe() function is called, the other half of the platform data is
needed to successfully probe the hardware. Namely, the 'ref_clk' field.
This would need to be made available to the probe() call (eg via
func->dev.platform_data) but from what I can tell there's currently no
mechanism to make that connection.
Anyway, it's time for bed.
- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Delray Beach, FL ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.
On Wednesday 05 June 2013, Solomon Peachy wrote:
> > Regarding a long-term solution, I think what we should do here is to
> > move the reset logic into the SDIO framework itself: We have similar
> > requirements even for eMMC and SD cards, where you need to provide
> > the correct voltage to a device on the MMC port in order for it to
> > work. Today this is supported to a varying degree on the MMC controller
> > level, where we hook into various frameworks (clk, reset-controller,
> > regulator, gpio, pinctrl, power domain) based on platformm data or
> > device tree information on the controller node.
>
> If I understand you correctly, the "traditional" platform data -- power
> supply, clock source, and gpio (ie POWERUP, RESET) lines would be
> brought up using this mechanism, in order to get the device to attach to
> the SDIO bus. One wrinkle here is that the cw1200 has some minimum
> timing requirements between when each of those signals are brought up; I
> don't know if there's a way to encode that into the devicetree.
If there is a way to know the timings by the SDIO device ID, we should
not need device tree data. However, if the timings need to be set up
right in order to find out the device ID, I think they should go into
the device tree.
> But that's only half the equation.
>
> Once the device has identified it to the SDIO bus and the driver's
> probe() function is called, the other half of the platform data is
> needed to successfully probe the hardware. Namely, the 'ref_clk' field.
> This would need to be made available to the probe() call (eg via
> func->dev.platform_data) but from what I can tell there's currently no
> mechanism to make that connection.
I may not following right what you say here, but I think we need two
separate platform_data structures, one for the SDIO pins, used by the
SDIO layer to set up the clocks and regulators, and a separate
platform_data structure specific to the actual SDIO device, which is
used by the cw1200 driver and attached to the sdio_func.
I think mmc_attach_sdio would be the right place to pass down the
platform_data from the host to the func, if set by the platform.
I notice that this function already sets up the voltage and the
power domain for the card, so adding any further clock/reset/...
code could also be done there, or you model the reset line through
a power domain.
In case of DT booting, we would do the respective thing and
add the device_node pointer for the children of the mmc host to
the sdio devices. We don't currently do that, but it would be
straightforward to add as well.
Arnd