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

login
register
mail settings
Submitter Shawn Guo
Date June 11, 2011, 1:16 p.m.
Message ID <20110611131637.GB29093@S2100-06.ap.freescale.net>
Download mbox | patch
Permalink /patch/100009/
State New
Headers show

Comments

Shawn Guo - June 11, 2011, 1:16 p.m.
On Sat, Jun 11, 2011 at 01:59:54PM +0200, Arnaud Patard wrote:
> Shawn Guo <shawn.guo@freescale.com> writes:
> 
> > On Sat, Jun 11, 2011 at 11:30:42AM +0200, Arnaud Patard wrote:
> >> Shawn Guo <shawn.guo@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.
Arnaud Patard (Rtp) - June 11, 2011, 7:21 p.m.
Shawn Guo <shawn.guo@freescale.com> writes:

> On Sat, Jun 11, 2011 at 01:59:54PM +0200, Arnaud Patard wrote:
>> Shawn Guo <shawn.guo@freescale.com> writes:
>> 
>> > On Sat, Jun 11, 2011 at 11:30:42AM +0200, Arnaud Patard wrote:
>> >> Shawn Guo <shawn.guo@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.

oops, sorry. I mixed with the other patches sent today
(http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052705.html)

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

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));
 }