Re: [PATCH] mmc: add SDHCI driver for STM platforms (V2)

[Peppe CAVALLARO <peppe.cavallaro@st.com>]

On 09/22/2010 04:12 PM, Wolfram Sang wrote:
> Hi Peppe,
> 
>> 1) I've already a patch to add the suspend/resume in the sdhci_pltfm
>>    driver. Please note this is mandatory for me.
> 
> Great, improvements to the generic pltfm-driver are most welcome!

Good! I'm happy to contribute on it. I'll start soon and post the
patches asap.

> 
>>    Note: I'd like to look at the wake-up on card that should be nice to
>>    have in the future. IIUC, it is missing in the sdhci. Please correct
>>    me if I'm wrong.
> 
> It is not in sdhci yet. Please make sure to send very early RFC-patches
> here, so we can see what you are aiming for.

OK!

> 
>> 2) sdhci_pltfm_data has a "quirk" flag but IMO the quirk macros, that
>>    currently are in linux-2.6/drivers/mmc/host/sdhci.h, should be
>>    moved in a separate file:
>>
>>     include/linux/mmc/sdhci.h or
>>     include/linux/mmc/sdhci-quirk.h or ...
>>
>>    I don't know if it has been already done but I could create a patch
>>    for this too. Let me know the name convention you like, eventually.
>>
>>    Otherwise, in my platforms, where I need to set this flag (e.g. the
>>    sdhci-stm needs: SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC), I should include
>>    drivers/mmc/host/sdhci.h?!? I don't like it :-(
>>    Please, correct me if I've missed something.
> 
> You are correct. So far, all users of the quirk-flags happened to be
> inside the host-directory. If you happen to have a controller which only
> needs quirk-flags and no custom functions (congrats! ;)), then those
> quirk-flags really need to be moved, so your platform code can find it.
> 
> I'd go for sdhci.h as the name. Be sure to catch all users of the quirks
> to include your new file.

OK! I like shdci.h too ;-)
I'll pay attention to add this header file in the other drivers based on
sdhci.

> 
>> 3) In the end, another hook could be added in the sdhci_pltfm_data to
>>    invoke specific own functions for claiming resources etc.
>>    For example, I need an extra callback to invoke the STM pad manager
>>    that's used for managing clocks, PIO lines and syscfg registers.
>>
>>    I'm thinking about something like this:
>>
>>    struct sdhci_pltfm_data {
>>         struct sdhci_ops *ops;
>>         unsigned int quirks;
>>         int (*init)(struct sdhci_host *host);
>>         void (*exit)(struct sdhci_host *host);
>>         int (*claim_resource)(struct platform_device *pdev);
>>                  |
>>                  |_ we can use another name.
>>    };
> 
> And init() is too late?

No it's not late but I need the dev structure from the platform_device.
We could add the "struct device dev;" in the sdhci_host structure
instead of.
In this case the sdhci_host should be moved within the new
include/linux/mmc/sdhci.h file.
Indeed, this could make sense because the drivers/mmc/host/sdhci.h will
only have the HW register configuration. Shared macros (quirks) and
structures among sdhci driver based could be moved in
include/linux/mmc/sdhci.h header.

What do you think?

Regards,
Peppe