[PATCH 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support

[Shawn Guo]

On Sat, Jun 11, 2011 at 01:59:54PM +0200, Arnaud Patard wrote:
> Shawn Guo <shawn.guo at freescale.com> writes:
> 
> > On Sat, Jun 11, 2011 at 11:30:42AM +0200, Arnaud Patard wrote:
> >> Shawn Guo <shawn.guo at linaro.org> writes:
> >> 
> >> Hi,
> >> 
> >> > The patch extends card_detect and write_protect support to get mx5
> >> > family and more scenarios supported.  The changes include:
> >> >
> >> >  * Turn platform_data from optional to mandatory
> >> >  * Add cd_types and wp_types into platform_data to cover more use
> >> >    cases
> >> >  * Remove the use of flag ESDHC_FLAG_GPIO_FOR_CD
> >> >  * Adjust machine codes to adopt the platform_data changes
> >> 
> >> Before I go and test theses patches, I'd like to get some
> >> clarification. From what I see, you've modified all over the place the
> >> code to provide a platform_data, setting wp/cd type to type "NONE", as
> >> if it was the default you choose. Why this default and not considerer
> >> the "SIGNAL" type being the default ? Is this choice the safest one when
> >> one doesn't know what type to choose or can it have some bad side
> >> effects ?
> >
> > The mx51_babbage is the only board support I'm concerned about in
> > this patch.  For other boards, I chose to translate the NULL pdata
> > into "NONE" for both wp/cd types as the safest one, because I do not
> > have (or care to check) the board schematics telling how wp/cd are
> > routed on those boards.  The patch ensures there is no regression
> > for those boards, and let people who have schematics to set up wp/cd
> > types later.
> 
> ok. Thanks for making things clear. I see some changes for
> loco/imx53qsb. Do you need testers for it too or you've tested it ?
> 
I do not see changes for loco except NULL pdata -> "NONE" types.  But
testing are always welcomed and appreciated.

> >
> >> Also, why didn't you modify the imx*_add_sdhci_esdhc_imx() functions to
> >> provide the default platform_data by themselves that if the 2nd argument
> >> was NULL instead of modifying all theses machines files ?
> >> 
> > As I said above, the wp/cd "NONE" types translated from NULL pdata
> > will be set up properly later by people who have schematics.
> 
> You're not answering my question about moving the NULL-> "NONE" type
> from *all* modified machine file into the imx*add_sdhci_esdhc_imx(). If
> it's the default, why all machines file have to be modified to set it ?
> Moreover, *nothing* AFAICS is preventing to call theses functions will
> NULL. What will happen ? An oops ? To me, a default is the value set
> when nothing is set, and clearly modifying all functions call site due
> to having to provide the default seems imho wrong.
> 
Ah, good point.  Please review changes below.  If it looks good to
you, I will incorporate it in the next version of the patch.

diff --git a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
index 6b2940b..a880f73 100644
--- a/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
+++ b/arch/arm/plat-mxc/devices/platform-sdhci-esdhc-imx.c
@@ -65,6 +65,11 @@ imx53_sdhci_esdhc_imx_data[] __initconst = {
 };
 #endif /* ifdef CONFIG_SOC_IMX53 */

+static const struct esdhc_platform_data default_esdhc_pdata __initconst = {
+       .wp_type = ESDHC_WP_NONE,
+       .cd_type = ESDHC_CD_NONE,
+};
+
 struct platform_device *__init imx_add_sdhci_esdhc_imx(
                const struct imx_sdhci_esdhc_imx_data *data,
                const struct esdhc_platform_data *pdata)
@@ -81,6 +86,13 @@ struct platform_device *__init imx_add_sdhci_esdhc_imx(
                },
        };

+       /*
+        * If machine does not provide a pdata, use the default one
+        * which means none WP/CD support
+        */
+       if (!pdata)
+               pdata = &default_esdhc_pdata;
+
        return imx_add_platform_device("sdhci-esdhc-imx", data->id, res,
                        ARRAY_SIZE(res), pdata, sizeof(*pdata));
 }

-- 
Regards,
Shawn