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

Hi Vitaly,

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

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 ?

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):

diff --git a/arch/arm/mach-msm/board-trout-mmc.c b/arch/arm/mach-msm/board-trout
index 13755f5..df32b2f 100644
--- a/arch/arm/mach-msm/board-trout-mmc.c
+++ b/arch/arm/mach-msm/board-trout-mmc.c

@@ -10,6 +10,7 @@
 #include <linux/mmc/sdio_ids.h>
 #include <linux/err.h>
 #include <linux/debugfs.h>
+#include <linux/wl12xx.h>

 #include <asm/gpio.h>
 #include <asm/io.h>

@@ -297,11 +298,16 @@ int trout_wifi_reset(int on)
 EXPORT_SYMBOL(trout_wifi_reset);
 #endif

+struct wl12xx_platform_data trout_wlan_data = {
+       .irq = 62, /* put here your irq number */
+       .board_ref_clock = 1, /* put here your ref clock */
+};
+
 static struct mmc_platform_data trout_wifi_data = {
        .ocr_mask               = MMC_VDD_28_29,
        .status                 = trout_wifi_status,
        .register_status_notify = trout_wifi_status_register,
-       .embedded_sdio          = &trout_wifi_emb_data,
+       .embedded_sdio          = &trout_wlan_data,
 };

 int __init trout_init_mmc(unsigned int sys_rev)

diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
index 1697d42..c40f0d1 100755
--- a/drivers/mmc/host/msm_sdcc.c
+++ b/drivers/mmc/host/msm_sdcc.c

@@ -1261,6 +1261,7 @@ msmsdcc_probe(struct platform_device *pdev)
        mmc->f_min = msmsdcc_fmin;
        mmc->f_max = msmsdcc_fmax;
        mmc->ocr_avail = plat->ocr_mask;
+        mmc_set_embedded_data(mmc, plat->embedded_sdio);

        if (msmsdcc_4bit)
                mmc->caps |= MMC_CAP_4_BIT_DATA;

Is it really that complex ?

Thanks,
Ohad.