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

[Ohad Ben-Cohen]

On Wed, Aug 4, 2010 at 2:41 PM, Russell King - ARM Linux
<linux at arm.linux.org.uk> wrote:
> On Wed, Aug 04, 2010 at 02:24:39PM +0300, Ohad Ben-Cohen wrote:
>> On Tue, Aug 3, 2010 at 5:17 PM, Vitaly Wool <vitalywool at gmail.com> wrote:
>> > On Mon, Aug 2, 2010 at 11:35 PM, Ohad Ben-Cohen <ohad at wizery.com> wrote:
>> >> 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 ?
>>
>> > 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.
>