Hi Ulf,

On 2017/1/30 16:41, Ulf Hansson wrote:

[...]

quoted

quoted

quoted

+ */
+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.

quoted

    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

quoted

    May I keep current implementation?

Rather not. Let's try to make the parsing of the child node for
mmc-card generic.

quoted

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

quoted

    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.

[...]

quoted

quoted

quoted

+                                     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