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

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2457 bytes --]

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!

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

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

> 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?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]