On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@xxxxxxxxxxxxxx> wrote:
>> 2015-06-05 5:53 GMT+09:00 <ygardi@xxxxxxxxxxxxxx>:
>>>> Hi Yaniv,
>>>>
>>>> 2015-06-03 18:37 GMT+09:00 Yaniv Gardi <ygardi@xxxxxxxxxxxxxx>:
>>>>> @@ -321,7 +313,22 @@ static int ufshcd_pltfrm_probe(struct
>>>>> platform_device *pdev)
>>>>> goto out;
>>>>> }
>>>>>
>>>>> - hba->vops = get_variant_ops(&pdev->dev);
>>>>> + err = of_platform_populate(node, NULL, NULL, &pdev->dev);
>>>>> + if (err)
>>>>> + dev_err(&pdev->dev,
>>>>> + "%s: of_platform_populate() failed\n",
>>>>> __func__);
>>>>> +
>>>>> + ufs_variant_node = of_get_next_available_child(node, NULL);
>>>>> +
>>>>> + if (!ufs_variant_node) {
>>>>> + dev_dbg(&pdev->dev, "failed to find ufs_variant_node
>>>>> child\n");
>>>>> + } else {
>>>>> + ufs_variant_pdev =
>>>>> of_find_device_by_node(ufs_variant_node);
>>>>> +
>>>>> + if (ufs_variant_pdev)
>>>>> + hba->vops = (struct ufs_hba_variant_ops *)
>>>>> +
>>>>> dev_get_drvdata(&ufs_variant_pdev->dev);
>>>>> + }
>>>>
>>>> I have no strong objection to 'ufs_variant' sub-node. But why can't we
>>>> simply add an of_device_id to ufs_of_match, like below:
>>>>
>>>> static const struct of_device_id ufs_of_match[] = {
>>>> { .compatible = "jedec,ufs-1.1"},
>>>> #if IS_ENABLED(SCSI_UFS_QCOM)
>>>> { .compatible = "qcom,ufs", .data = &ufs_hba_qcom_vops },
>>>> #neidf
>>>> {},
>>>> };
>>>>
>>>> and get hba->vops by get_variant_ops()?
>>>>
>>>
>>> Hi Mita,
>>> thanks for your comments.
>>>
>>> The whole idea, of having a sub-node which includes all variant specific
>>> attributes is to separate the UFS Platform device component, from the
>>> need
>>> to know "qcom" or any other future variant.
>>> I believe it keeps the code more modular, and clean - meaning - no
>>> #ifdef's and no need to include all variant attributes inside the driver
>>> DT node.
>>> in that case, we simply have a DT node that is compatible to the Jdec
>>> standard, and sub-node to include variant info.
>>>
>>> I hope you agree with this new design, since it provides a good answer
>>> to every future variant that will be added, without the need to change
>>> the
>>> platform file.
>>
>> Thanks for your explanation, I agree with it.
>>
>> I found two problems in the current code, but both can be fixed
>> relatively easily as described below:
>>
>> 1) If ufshcd-pltfrm driver is loaded before ufs-qcom driver,
>> ufshcd_pltfrm_probe() can't find a ufs_variant device.
>>
>> In order to trigger re-probing ufs device when ufs-qcom driver has
>> been loaded, ufshcd_pltfrm_probe() should return -EPROBE_DEFER in
>> case 'ufs_variant' sub-node exists and no hba->vops found.
>>
>> 2) Nothing prevents ufs-qcom module from being unloaded while the
>> variant_ops is referenced by ufshcd-pltfrm.
>>
>> It can be fixed by incrementing module refcount of ufs_variant module
>> by __module_get(ufs_variant_pdev->dev.driver->owener) in
>> ufshcd_pltfrm_probe(), and module_put() in ufshcd_pltfrm_remove()
>> to descrement the refcount.
>>
>
> again, Mita, your comments are very appreciated.
>
> 1)
> If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually happens
> always), then the calling to of_platform_populate() which is added,
> guarantees that ufs-qcom probe will be called and finish, before
> ufshcd_pltfrm probe continues.
> so ufs_variant device is always there, and ready.
> I think it means we are safe - since either way, we make sure ufs-qcom
> probe will be called and finish before dealing with ufs_variant device in
> ufshcd_pltfrm probe.
This is due to the fact that you have 2 platform drivers. You should
only have 1 (and 1 node). If you really think you need 2, then you
should do like many other common *HCIs do and make the base UFS driver
a set of library functions that drivers can use or call. Look at EHCI,
AHCI, SDHCI, etc. for inspiration.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
