[PATCH v2 5/7] ARM: OMAP2+: Split omap2_hsmmc_init() to properly support I2C GPIO pins

[Rajendra Nayak]

On Friday 24 February 2012 12:17 AM, Tony Lindgren wrote:
> * Igor Grinberg<grinberg at compulab.co.il>  [120223 05:56]:
>>> --- a/arch/arm/mach-omap2/board-cm-t35.c
>>> +++ b/arch/arm/mach-omap2/board-cm-t35.c
>>> @@ -411,9 +411,9 @@ static struct omap2_hsmmc_info mmc[] = {
>>>   	{
>>>   		.mmc		= 1,
>>>   		.caps		= MMC_CAP_4_BIT_DATA,
>>> -		.gpio_cd	= -EINVAL,
>>> +		.gpio_cd	= OMAP_MAX_GPIO_LINES + 0,
>>>   		.gpio_wp	= -EINVAL,
>
> I don't have these changes, in my second revision of the patch.
>
> It's best not to hardcode the values here.

Ok, I just though doing it the other way is more round about
hardcoding where in the board passes a hardcoded value to
twl4030 gpio driver and the driver passes the *same* hardcoded
value back to the board callback, which is then populated in
mmc pdata. The board could as well directly hardcode it in the
mmc pdata. That makes it much more readable than this round about
hardcoding.
But I'll go back to the old way if you think thats how we should
handle this.

>
>>> -
>>> +		.deferred	= true,
>>>   	},
>>>   	{
>>>   		.mmc		= 2,
>>> @@ -422,6 +422,7 @@ static struct omap2_hsmmc_info mmc[] = {
>>>   		.gpio_cd	= -EINVAL,
>>>   		.gpio_wp	= -EINVAL,
>>>   		.ocr_mask	= 0x00100000,	/* 3.3V */
>>> +		.deferred	= true,
>>
>> Why do you defer this one?
>> It does not use external GPIO chip, in fact it does not use CD/WP at all.
>
> Why do you have the following then to set gpio_cd?

There is only one instance of gpio_cd being populated for the
first element in the array mmc[], which is already deferred.

 >>> -	mmc[0].gpio_cd = gpio + 0;

Igor was asking about the second element in the array which never
has gpio_cd populated. Its just initialized to -EINVAL and stays
that way.

>
>>>   	},
>>>   	{}	/* Terminator */
>>>   };
>>> @@ -469,9 +470,7 @@ static int cm_t35_twl_gpio_setup(struct device *dev, unsigned gpio,
>>>   		pr_err("CM-T35: could not obtain gpio for WiFi reset\n");
>>>   	}
>>>
>>> -	/* gpio + 0 is "mmc0_cd" (input/IRQ) */
>>> -	mmc[0].gpio_cd = gpio + 0;
>>> -	omap2_hsmmc_init(mmc);
>>> +	omap_hsmmc_deferred_add(mmc);
>>>
>>>   	return 0;
>>>   }
>
> Hmm I don't have omap_hsmmc_deferred_add() in my second version
> of the patch either.
>
> Rajendra, please do the patches on that, now it's impossible to
> see what else you've changed. That's the version posted here:
>
> http://www.spinics.net/lists/linux-omap/msg64884.html

Sorry for the mess up. I will make sure I have taken all changes/fixes
from your reposts.

regards,
Rajendra
>
>>> @@ -639,6 +638,7 @@ static void __init cm_t3x_common_init(void)
>>>   	omap_serial_init();
>>>   	omap_sdrc_init(mt46h32m32lf6_sdrc_params,
>>>   			     mt46h32m32lf6_sdrc_params);
>>> +	omap_hsmmc_init(mmc);
>>>   	cm_t35_init_i2c();
>>>   	omap_ads7846_init(1, CM_T35_GPIO_PENDOWN, 0, NULL);
>>>   	cm_t35_init_ethernet();
>>
>> Other then the comment above, looks fine.
>> I will probably be able to test this on Sunday.
>
> OK
>
>>> diff --git a/arch/arm/mach-omap2/board-igep0020.c b/arch/arm/mach-omap2/board-igep0020.c
>>> index a59ace0..11a6aa4 100644
>>> --- a/arch/arm/mach-omap2/board-igep0020.c
>>> +++ b/arch/arm/mach-omap2/board-igep0020.c
>>> @@ -293,8 +293,9 @@ static struct omap2_hsmmc_info mmc[] = {
>>>   	{
>>>   		.mmc		= 1,
>>>   		.caps		= MMC_CAP_4_BIT_DATA,
>>> -		.gpio_cd	= -EINVAL,
>>> +		.gpio_cd	= OMAP_MAX_GPIO_LINES + 0,
>>>   		.gpio_wp	= -EINVAL,
>>> +		.deferred	= true,
>>>   	},
>>>   #if defined(CONFIG_LIBERTAS_SDIO) || defined(CONFIG_LIBERTAS_SDIO_MODULE)
>>>   	{
>>> @@ -302,6 +303,7 @@ static struct omap2_hsmmc_info mmc[] = {
>>>   		.caps		= MMC_CAP_4_BIT_DATA,
>>>   		.gpio_cd	= -EINVAL,
>>>   		.gpio_wp	= -EINVAL,
>>> +		.deferred	= true,
>>
>> same here, why defer it?
>
> Because it too sets gpio_cd in the callback.
>
>> ditto
>
> ditto, that too sets gpio_cd..
>
>>>   	},
>>>   #endif
>>>   	{}	/* Terminator */
>>> @@ -360,10 +362,8 @@ static int omap3evm_twl_gpio_setup(struct device *dev,
>>>   {
>>>   	int r, lcd_bl_en;
>>>
>>> -	/* gpio + 0 is "mmc0_cd" (input/IRQ) */
>>>   	omap_mux_init_gpio(63, OMAP_PIN_INPUT);
>>> -	mmc[0].gpio_cd = gpio + 0;
>
> ..here. Same for the others. So maybe check is some are wrong?
>
> Tony