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

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

On 09/21/2010 11:44 AM, Wolfram Sang wrote:
> On Tue, Sep 21, 2010 at 11:21:48AM +0200, Peppe CAVALLARO wrote:
>> On 09/20/2010 10:38 AM, Wolfram Sang wrote:
>>> On Mon, Sep 20, 2010 at 10:20:17AM +0200, Giuseppe CAVALLARO wrote:
>>>> This patch adds the Arasan MMC/SD/SDIO driver
>>>> for the STM ST40 platforms. It is based on the
>>>> SDHCI driver.
>>>> It has been tested on the STx7106/STx7108/STx5289
>>>> platforms.
>>>>
>>>> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>>>
>>> Looks to me that you could just use the sdhci-pltfm driver?
>>>
>>
>> Hello Wolfram,
>> some weeks ago I discussed about this driver and I reworked it according
>> to the changes required. This is the meaning of this patch.
>>
>> At any rate, I can look at the sdhci-pltfm driver: at first glance, it
>> could actually help on our platforms. This could take a while.
> 
> Sorry, I didn't spot the first version of your patch. I would have said
> the same then, simply to avoid the code duplication. Oh, and no need to
> hurry from my side :)

Hi Wolfran,
I agree that it's a good idea to reduce duplicated code when possible
(i.e. the probe function that, at first glance, is almost equal for
several sdhci drivers based).

Maybe, I could use on our ST boxes the sdhci_pltfm but I have the
following questions and ideas. Welcome advice and feedback.

1) I've already a patch to add the suspend/resume in the sdhci_pltfm
   driver. Please note this is mandatory for me.

   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.

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.

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.
   };

What do you think?

Regards
Peppe