Re: [PATCH] cw1200: fix some obvious mistakes

[Solomon Peachy]

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.

Attachment: pgp00000.pgp
Description: PGP signature