Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host

On Wed, Aug 4, 2010 at 2:41 PM, Russell King - ARM Linux
[off-list ref] wrote:

On Wed, Aug 04, 2010 at 02:24:39PM +0300, Ohad Ben-Cohen wrote:

quoted

On Tue, Aug 3, 2010 at 5:17 PM, Vitaly Wool [off-list ref] wrote:

quoted

On Mon, Aug 2, 2010 at 11:35 PM, Ohad Ben-Cohen [off-list ref] wrote:

quoted

I'm honestly trying to understand your concerns, but I'm afraid that
just saying "it's a hack" is not too informative. Can you please
explain what do you think is technically wrong with the proposed
solution ? is it not general enough for other use cases ? will it
break something ?

quoted

So if I'd like to set the *same* structure for the *same* WL1271
driver, provided that I'm working with the other platform, I'll need
to do the following:
- add the pointer to the board-specific header;
- add the structure to the board-specific C file and propagate its
pointer to the controller driver;
- add setting the pointer in the core structure into the controller driver.

This is far from being intuitive. This means we need to hack,
generally speaking, all the MMC controller drivers to get it working
on all platforms. This is error prone.

You make it sound really complex.

Let's see what it means to add it to a totally different platform.

As an example, let's take Google's ADP1 which is based on a different
host controller (msm-sdcc), and add the required support (untested of
course, just a quick sketch, patch is based on android's msm git):

What if some other driver gets attached and tries to use this
platform data?  This whole idea sounds extremely silly, cumbersome,
error prone, and is inviting misuse by normal MMC sockets.

Why not arrange for a small piece of code to be built into the kernel
when this driver is selected as a module or built-in, which handles
the passing of platform data to the driver?

It's interesting.

The only drawback I can see is that it has an inherent limitation of
having only a single wl1271 device on board, but there are no such
boards outside development/testing labs.

Vitaly would that work for you too ?

Thanks,
Ohad.

Something like:

wl12xx_platform_data.c:
#include <linux/module.h>
#include <whatever/required/for/wl12xx.h>

static struct wl12xx_platform_data *platform_data;

int __init wl12xx_set_platform_data(const struct wl12xx_platform_data *data)
{
       if (platform_data)
               return -EBUSY;
       platform_data = kmemdup(data, sizeof(*data), GFP_KERNEL);
       if (!platform_data)
               return -ENOMEM;
       return 0;
}

int wl12xx_get_platform_data(struct wl12xx_platform_data *data)
{
       if (platform_data) {
               memcpy(data, platform_data, sizeof(*data));
               return 0;
       }
       return -ENODEV;
}
EXPORT_SYMBOL(wl12xx_get_platform_data);

And in the Kconfig, something like:

config WL12XX_PLATFORM_DATA
       bool
       depends on WL12XX != n
       default y

This means there'll be no confusion over who owns the 'embedded data',
typechecking is preserved, and no possibility for the wrong driver to
pick up the data.