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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
