[PATCH 4/4] mmc: sdhci-esdhc-imx: extend card_detect and write_protect support
From: Arnaud Patard Rtp <hidden>
Date: 2011-06-11 19:21:47
Also in:
linux-mmc
Shawn Guo [off-list ref] writes:
On Sat, Jun 11, 2011 at 01:59:54PM +0200, Arnaud Patard wrote:quoted
Shawn Guo [off-list ref] writes:quoted
On Sat, Jun 11, 2011 at 11:30:42AM +0200, Arnaud Patard wrote:quoted
Shawn Guo [off-list ref] writes: Hi,quoted
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 changesBefore 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.
oops, sorry. I mixed with the other patches sent today (http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052705.html)
quoted hunk
quoted
quoted
quoted
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)); }
Great. I've still not tested it but it's exactly what I was thinking. Thanks, Arnaud