Re: [PATCH v6 25/35] iommu/mediatek: Migrate to aggregate driver

[Yong Wu <yong.wu@mediatek.com>]

On Thu, 2022-01-27 at 12:01 -0800, Stephen Boyd wrote:
> Use an aggregate driver instead of component ops so that we can get
> proper driver probe ordering of the aggregate device with respect to
> all
> the component devices that make up the aggregate device.
> 
> Cc: Yong Wu <yong.wu@mediatek.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

+ Krzysztof

The memory/mtk-smi.c is expected to get Ack from Krzysztof.

Tested-by: Yong Wu <yong.wu@mediatek.com>

> ---
>  drivers/iommu/mtk_iommu.c    | 14 +++++++++-----
>  drivers/iommu/mtk_iommu.h    |  6 ++++--
>  drivers/iommu/mtk_iommu_v1.c | 14 +++++++++-----
>  drivers/memory/mtk-smi.c     | 10 ++++------
>  4 files changed, 26 insertions(+), 18 deletions(-)

[...]

> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index e201e5976f34..0910fe109f53 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -175,6 +175,8 @@ mtk_smi_larb_bind(struct device *dev, struct
> device *master, void *data)
>  			larb->larbid = i;
>  			larb->mmu = &larb_mmu[i].mmu;
>  			larb->bank = larb_mmu[i].bank;
> +
> +			pm_runtime_enable(dev);
>  			return 0;
>  		}
>  	}
> @@ -450,15 +452,11 @@ static int mtk_smi_larb_probe(struct
> platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	pm_runtime_enable(dev);
>  	platform_set_drvdata(pdev, larb);
>  	ret = component_add(dev, &mtk_smi_larb_component_ops);
> -	if (ret)
> -		goto err_pm_disable;
> -	return 0;
> +	if (!ret)
> +		return 0;
>  
> -err_pm_disable:
> -	pm_runtime_disable(dev);
>  	device_link_remove(dev, larb->smi_common_dev);

Here is right. But at a glance code here, I was confused why it always
call device_link_remove here. If we have v7, Could you help keep the
original format? something like below. This may be helpful when we add
new error flow in future.

        if (ret)
              goto dev_link_remove;
        return ret;

dev_link_remove:
        device_link_remove(dev, larb->smi_common_dev);

Thanks.     

>  	return ret;
>  }