lkml.org 
[lkml]   [2015]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
    On Sun, Jun 7, 2015 at 10:32 AM,  <ygardi@codeaurora.org> wrote:
    >> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
    >>>> Hi Yaniv,
    >>>>
    >>>> 2015-06-03 18:37 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
    >>>>> @@ -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


    \
     
     \ /
      Last update: 2015-06-08 17:21    [W:2.158 / U:0.448 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site