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

[Arnaud Patard (Rtp)]

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 ?

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

Thanks,
Arnaud