Re: [PATCH 7/7] omap hsmmc: fix the hsmmc driver for am3517.

["stanley.miao" <stanley.miao@windriver.com>]

Tony Lindgren wrote:
> Hi,
>
> Some requests to make this more future proof.
>
> * Stanley.Miao <stanley.miao@windriver.com> [100419 23:20]:
>   
>> AM3517 don't have the register OMAP343X_CONTROL_PBIAS_LITE and the regulators
>> like "vmmc", so we set a noop "set_power" function for it.
>>
>> Signed-off-by: Stanley.Miao <stanley.miao@windriver.com>
>> ---
>>  arch/arm/mach-omap2/hsmmc.c |   32 ++++++++++++++++++++++++--------
>>  1 files changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/hsmmc.c b/arch/arm/mach-omap2/hsmmc.c
>> index 9ad2295..5f46797 100644
>> --- a/arch/arm/mach-omap2/hsmmc.c
>> +++ b/arch/arm/mach-omap2/hsmmc.c
>> @@ -139,6 +139,12 @@ static void hsmmc23_before_set_reg(struct device *dev, int slot,
>>  	}
>>  }
>>  
>> +static int am3517_mmc_set_power(struct device *dev, int slot, int power_on,
>> +								int vdd)
>> +{
>> +	return 0;
>> +}
>> +
>>     
>
> Please rename this to nop_mmc_set_power or similar. Other omaps may
> need it too.
>   

Accepted.

>   
>>  static struct omap_mmc_platform_data *hsmmc_data[OMAP34XX_NR_MMC] __initdata;
>>  
>>  void __init omap2_hsmmc_init(struct omap2_hsmmc_info *controllers)
>> @@ -150,7 +156,7 @@ void __init omap2_hsmmc_init(struct omap2_hsmmc_info *controllers)
>>  	if (cpu_is_omap2430()) {
>>  		control_pbias_offset = OMAP243X_CONTROL_PBIAS_LITE;
>>  		control_devconf1_offset = OMAP243X_CONTROL_DEVCONF1;
>> -	} else {
>> +	} else if (!cpu_is_omap3517() && !cpu_is_omap3505()) {
>>  		control_pbias_offset = OMAP343X_CONTROL_PBIAS_LITE;
>>  		control_devconf1_offset = OMAP343X_CONTROL_DEVCONF1;
>>  	}
>>     
>
> Let's get rid of multiple cpu_is_omapxxxx tests please. Could you please
> update this along the lines of something like this:
>
> #define HSMMC_HAS_PBIAS		(1 << 0)
> #define HSMMC_HAS_WHATEVER	(1 << 1)
> ...
>
> 	if (!(cpu_is_omap3517() || cpu_is_omap3505())
> 		mmc->slots[i].features |= HSMMC_HAS_PBIAS;
>   

Accepted.

We can move other features into the variable "features", such as 
"nonremovable", "power_saving".

Stanley.

>   
>> @@ -216,12 +222,25 @@ void __init omap2_hsmmc_init(struct omap2_hsmmc_info *controllers)
>>  		 */
>>  		mmc->slots[0].ocr_mask = c->ocr_mask;
>>  
>> +		if (!cpu_is_omap3517() && !cpu_is_omap3505()) {
>> +			switch (c->mmc) {
>> +			case 1:
>> +				/* on-chip level shifting via PBIAS0/PBIAS1 */
>> +				mmc->slots[0].before_set_reg = hsmmc1_before_set_reg;
>> +				mmc->slots[0].after_set_reg = hsmmc1_after_set_reg;
>> +				break;
>> +			case 2:
>> +			case 3:
>> +				/* off-chip level shifting, or none */
>> +				mmc->slots[0].before_set_reg = hsmmc23_before_set_reg;
>> +				mmc->slots[0].after_set_reg = NULL;
>> +				break;
>> +			}
>> +		} else /* cpu_is_omap3517() || cpu_is_omap3505() */
>> +			mmc->slots[0].set_power = am3517_mmc_set_power;
>> +
>>     
>
> Then you can test for that flag here too. When new omaps get added, it
> just a question of setting the features flags right.
>
> Regards,
>
> Tony
>
>