On Fri, May 27, 2011 at 11:46 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Wednesday 25 May 2011, Zhangfei Gao wrote:
>> Instead of sharing sdhci-pxa.c, use sdhci-mmp2.c specificlly for mmp2.
>> sdhci-pxa.c is used to share among pxa serious, like pxa910, mmp2, etc.
>> The platfrom difference are put under arch/arm, while is not easy to track.
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxxx>
>> ---
>> arch/arm/plat-pxa/include/plat/sdhci.h | 43 ++++-
>> drivers/mmc/host/Kconfig | 11 +-
>> drivers/mmc/host/Makefile | 2 +-
>> drivers/mmc/host/sdhci-mmp2.c | 303 ++++++++++++++++++++++++++++++++
>> drivers/mmc/host/sdhci-pxa.c | 303 --------------------------------
>> 5 files changed, 349 insertions(+), 313 deletions(-)
>> create mode 100644 drivers/mmc/host/sdhci-mmp2.c
>> delete mode 100644 drivers/mmc/host/sdhci-pxa.c
>
> Yes, looks good for the most part. I was rather confused by the fact that the old
> and new file are both 303 lines, so I assumed they would be identical, when they
> are really completely different.
>
> There is a little room for simplification, I think:
>
>> +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.
>
>> +static void mmp2_set_clock(struct sdhci_host *host, unsigned int clock)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_pxa *pxa = pltfm_host->priv;
>> +
>> + if (clock)
>> + mmp2_soc_set_timing(host, pxa->pdata);
>> +}
>
> The mmp2_soc_set_timing() function is only called here, so you can easily
> merge the two into one, starting with
>
> if (!clock)
> return;
OK, thanks,
>
>> +static int __devinit sdhci_mmp2_probe(struct platform_device *pdev)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host;
>> + struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
>> + struct device *dev = &pdev->dev;
>> + struct sdhci_host *host = NULL;
>> + struct sdhci_pxa *pxa = NULL;
>> + int ret;
>> + struct clk *clk;
>
> The probe and release functions for mmp2 and pxa910 are almost identical.
> I'd suggest you leave them in sdhci-pxa.c as a library, and export them
> using EXPORT_SYMBOL_GPL. Then you can call them from the respective
> probe functions, e.g.
>
> static struct sdhci_mmp2_ops = {
> .set_clock = mmp2_set_clock,
> .set_uhs_signaling = mmp2_set_uhs_signaling,
> .get_ro = mmp2_get_ro,
> };
> static int __devinit sdhci_mmp2_probe(struct platform_device *pdev)
> {
> unsigned int quirks = SDHCI_QUIRK_BROKEN_ADMA
> | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
> | SDHCI_QUIRK_32BIT_DMA_ADDR
> | SDHCI_QUIRK_32BIT_DMA_SIZE
> | SDHCI_QUIRK_32BIT_ADMA_SIZE
> | SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC;
>
> return sdhci_pxa_probe(pdev, quirks, sdhci_mmp2_ops);
> }
>
>> + 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.
>
>> +
>> +#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.
> 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
>
--
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
