Re: [PATCH] cw1200: fix some obvious mistakes

[Solomon Peachy]

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