lkml.org 
[lkml]   [2017]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
    From
    Date
    Hi Ulf,

    On 2017/1/30 16:41, Ulf Hansson wrote:
    > [...]
    >
    >>>> + */
    >>>> +static int xenon_child_node_of_parse(struct platform_device *pdev)
    >>>> +{
    >>>> + struct device_node *np = pdev->dev.of_node;
    >>>> + struct sdhci_host *host = platform_get_drvdata(pdev);
    >>>> + struct mmc_host *mmc = host->mmc;
    >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
    >>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
    >>>> + struct device_node *child;
    >>>> + int nr_child;
    >>>> +
    >>>> + priv->init_card_type = XENON_CARD_TYPE_UNKNOWN;
    >>>> +
    >>>> + nr_child = of_get_child_count(np);
    >>>> + if (!nr_child)
    >>>> + return 0;
    >>>> +
    >>>> + for_each_child_of_node(np, child) {
    >>>> + if (of_device_is_compatible(child, "mmc-card")) {
    >>>
    >>> To avoid code from being duplicated, I would rather see the DT parsing
    >>> of the child nodes for "mmc-card", to be done by the mmc core.
    >>>
    >>> As a matter of fact it is already being done, but perhaps we need to
    >>> change that a bit as to fit your case.
    >>>
    >>> I suggest you have a look and see how to update this in the core,
    >>> instead of doing it here in the host driver.
    >>>
    >>
    >> I understand your concern.
    >>
    >> It seems that so far "mmc-card" is only used in our Xenon driver.
    >
    > git grep "mmc-card" tells you more about where it's being parsed by
    > the mmc core.
    >
    >> Besides, we set Xenon specific parameters and attributions when
    >> parsing "mmc-card" property.
    >
    > I don't see any Xenon specific properties.
    >
    > Instead I think it would make sense to update the generic
    > interpretation of the binding for mmc-card, as you have done here.
    >

    OK. I will merge it into core layer.

    Our Xenon driver requires to determine whether current SDHC is
    for eMMC before card init. Thus I would like to implement a specific
    function for mmc-card parsing in mmc.c and let mmc_of_parse() in host.c
    to call it.
    But there are some detailed issues then.
    1. mmc_decode_ext_csd() also parses mmc-card to check broken-hpi.
    So mmc-card parse will still be duplicated.
    Shall we merge broken-hpi check into mmc-card parse?
    It might require a new flag to indicate broken-hpi attribution in
    mmc_host structure.

    2. Structure mmc_card is not ready while parsing mmc-card.
    Thus we are not able to indicate card type in mmc_card.
    As a result, our Xenon specific parse function still has to
    parse mmc-card again to check if eMMC card is in used.

    Could you please help suggest any better place in core layer to
    parse mmc-card?

    Thank you.

    Best regards,
    Hu Ziji

    >>
    >> May I keep current implementation?
    >
    > Rather not. Let's try to make the parsing of the child node for
    > mmc-card generic.
    >
    >> In my very own opinion, moving it into core layer should be another
    >> independent patch.
    >
    > Absolutely an independent patch. Whether it can be done as a part of
    > mmc_of_parse() or whether we need yet another new mmc_of* API, we can
    > discuss.
    >
    > My point is that, I don't this to be specific for Xenon (unless there
    > are specific reason, which I don't see here).
    >
    >> And it will also cost some more time. To be honest, it is difficult
    >> for me to bring up a generic core layer implementation to parse
    >> "mmc-card", since I'm not clear about other vendor's requirement.
    >
    > Well, then you need to learn more about how the mmc core works. In
    > this case, it shouldn't be too hard to implement.
    >
    > [...]
    >
    >>
    >>>
    >>>> + MMC_CAP2_NO_SD |
    >>>> + MMC_CAP2_NO_SDIO;
    >>>
    >>> I think we need to update the DT documentation for mmc-card.
    >>> Typically, we should explicitly state what kind of other existing mmc
    >>> DT properties that will be automatically selected, when the mmc-card
    >>> is specified.
    >>>
    >>> Can you please look into updating this DT doc as well.
    >>>
    >>
    >> Similar to above, may I keep it now and bring up another patch later
    >> to update mmc-card DT and parsing?
    >
    > Please, no.
    >
    > [...]
    >
    > Kind regards
    > Uffe
    >

    \
     
     \ /
      Last update: 2017-01-30 16:36    [W:2.343 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site