On Sat, Jun 11, 2011 at 01:59:54PM +0200, Arnaud Patard wrote:
> Shawn Guo <shawn.guo@xxxxxxxxxxxxx> writes:
>
> > On Sat, Jun 11, 2011 at 11:30:42AM +0200, Arnaud Patard wrote:
> >> Shawn Guo <shawn.guo@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
