Re: [PATCH 2/8] iommu: mtk_iommu: Lookup phandle to retrieve syscon to infracfg

[Robin Murphy]

On 2022-05-17 14:21, AngeloGioacchino Del Regno wrote:
> This driver will get support for more SoCs and the list of infracfg
> compatibles is expected to grow: in order to prevent getting this
> situation out of control and see a long list of compatible strings,
> add support to retrieve a handle to infracfg's regmap through a
> new "mediatek,infracfg" phandle.
>
> In order to keep retrocompatibility with older devicetrees, the old
> way is kept in place, but also a dev_warn() was added to advertise
> this change in hope that the user will see it and eventually update
> the devicetree if this is possible.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> ---
>   drivers/iommu/mtk_iommu.c | 40 +++++++++++++++++++++++++--------------
>   1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 71b2ace74cd6..cfaaa98d2b50 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1134,22 +1134,34 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>   	data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
>   
>   	if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE)) {
> -		switch (data->plat_data->m4u_plat) {
> -		case M4U_MT2712:
> -			p = "mediatek,mt2712-infracfg";
> -			break;
> -		case M4U_MT8173:
> -			p = "mediatek,mt8173-infracfg";
> -			break;
> -		default:
> -			p = NULL;
> +		infracfg = syscon_regmap_lookup_by_phandle(dev->of_node, "mediatek,infracfg");
> +		if (IS_ERR(infracfg)) {
> +			dev_warn(dev, "Cannot find phandle to mediatek,infracfg:"
> +				      " Please update your devicetree.\n");

Is this really a dev_warn-level problem? There's no functional impact, 
given that we can't stop supporting the original binding any time soon, 
if ever, so I suspect this is more likely to just annoy users and CI 
systems than effect any significant change.

> +			/*
> +			 * Legacy devicetrees will not specify a phandle to
> +			 * mediatek,infracfg: in that case, we use the older
> +			 * way to retrieve a syscon to infra.
> +			 *
> +			 * This is for retrocompatibility purposes only, hence
> +			 * no more compatibles shall be added to this.
> +			 */
> +			switch (data->plat_data->m4u_plat) {
> +			case M4U_MT2712:
> +				p = "mediatek,mt2712-infracfg";
> +				break;
> +			case M4U_MT8173:
> +				p = "mediatek,mt8173-infracfg";
> +				break;
> +			default:
> +				p = NULL;
> +			}
> +
> +			infracfg = syscon_regmap_lookup_by_compatible(p);

Would it not make sense to punt this over to the same mechanism as for 
pericfg, such that it simplifies down to something like:

	if (IS_ERR(infracfg) && plat_data->infracfg) {
		infracfg = syscon_regmap_lookup_by_compatible(plat_data->infracfg);
		...
	}

?

TBH if we're still going to have a load of per-SoC data in the driver 
anyway then I don't see that we really gain much by delegating one 
aspect of it to DT, but meh. I would note that with the phandle 
approach, you still need some *other* flag in the driver to know whether 
a phandle is expected to be present or not, whereas a NULL vs. non-NULL 
string is at least neatly self-describing.

Robin.

> +			if (IS_ERR(infracfg))
> +				return PTR_ERR(infracfg);
>   		}
>   
> -		infracfg = syscon_regmap_lookup_by_compatible(p);
> -
> -		if (IS_ERR(infracfg))
> -			return PTR_ERR(infracfg);
> -
>   		ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
>   		if (ret)
>   			return ret;