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.
Attachment:
pgpNHIMMSL7iH.pgp
Description: PGP signature
