[lkml]   [2013]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] cw1200: fix some obvious mistakes
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

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.
[unhandled content-type:application/pgp-signature]
 \ /
  Last update: 2013-06-02 15:21    [W:0.087 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site