On Mon, May 30, 2011 at 3:11 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Monday 30 May 2011 07:42:04 zhangfei gao wrote:
>> On Fri, May 27, 2011 at 11:46 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> > On Wednesday 25 May 2011, Zhangfei Gao wrote:
>> >> +static unsigned int mmp2_get_ro(struct sdhci_host *host)
>> >> +{
>> >> + /* Micro SD does not support write-protect feature */
>> >> + return 0;
>> >> +}
>> >
>> > You shouldn't need to provide an empty get_ro function, the
>> > default is that there is no write-protect.
>>
>> Thanks Arnd for review.
>> The reason to put get_ro here is some board use micro sd, while some
>> board design is general sd card.
>> The micro sd do not use write-protect, while SDHCI_WRITE_PROTECT will
>> return 1 in our controller, so it shows read only.
>> So add one call back for the board with micro sd card via flag.
>
> Ok, I see.
>
>> >> + pxa = kzalloc(sizeof(struct sdhci_pxa), GFP_KERNEL);
>> >> + if (!pxa) {
>> >> + ret = -ENOMEM;
>> >> + goto out;
>> >> + }
>> >> + pxa->ops = kzalloc(sizeof(struct sdhci_ops), GFP_KERNEL);
>> >> + if (!pxa->ops) {
>> >> + ret = -ENOMEM;
>> >> + goto out;
>> >> + }
>> >
>> > I think you really shouldn't allocate the sdhci_ops dynamically.
>> > In fact, it would be much better if we were able to mark them
>> > as const in all drivers.
>>
>> We found some issues when supporting multi-slot, each slot may have
>> different ops.
>> So use the method of allocating the sdhci_ops dynamically instead of
>> using static ops.
>> For example, emmc has 74_clocks call back, while mmc and sdio slot
>> does not have such ops.
>> If not dynamically allocate sdhci_ops, all slot ops may become same,
>> and 74_clocks may be called for every slot.
>> Also, some board may have get_ro, while other board may not, so
>> transfer the ops via flags.
>>
>> Not sure whether it is worthy to add additional common files to share
>> probe and remove function.
>> Also the init the ops part are different.
>
> IMHO, we should try much harder to keep the ops constant. For the
> two examples you gave, I would probably put a flag into private data for
> the get_ro operation to signify that it's never read-only, and provide
> a second set of operations for emmc, so that in the probe function
> you check the type of device and then set the appropriate one.
>
> You could also do the same for all three cases:
>
> if (emmc)
> host->ops = &sdhci_mmp2_emmc_ops;
> else if (!has_ro_switch)
> host->ops = &sdhci_mmp2_rw_ops;
> else
> host->ops = &sdhci_mmp2_default_ops;
Thanks, will take this method to make the ops constant.
>
>> >> +
>> >> +#ifdef CONFIG_PM
>> >> +static int sdhci_mmp2_suspend(struct platform_device *dev, pm_message_t state)
>> >> +{
>> >> + struct sdhci_host *host = platform_get_drvdata(dev);
>> >> +
>> >> + return sdhci_suspend_host(host, state);
>> >> +}
>> >> +
>> >> +static int sdhci_mmp2_resume(struct platform_device *dev)
>> >> +{
>> >> + struct sdhci_host *host = platform_get_drvdata(dev);
>> >> +
>> >> + return sdhci_resume_host(host);
>> >> +}
>> >> +#else
>> >> +#define sdhci_mmp2_suspend NULL
>> >> +#define sdhci_mmp2_resume NULL
>> >> +#endif
>> >
>> > Similarly, I think it would be good if sdhci-pltfm.c would simply
>> > export these functions so you could refer to them in your driver.
>> > There is no need to have identical copies in each variant.
>> >
>>
>> There are some additional code for suspend and resume, so
>> sdhci_pltfm_suspend may not enough.
>> For example, when enable wifi host sleep feature, additional register
>> have to be configured.
>
> Well, not all drivers would have to use the common functions, but
> right now there are a number of identical copies, so it's certainly
> worth sharing.
Then we use sdhci_pltfm_suspend first, and change to private suspend &
resume when supporting additional feature.
>
> Note that this comment wasn't directed at your patch as much as at
> the overall design of the sdhci sub-drivers.
>
> Arnd
>
--
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
