Messages in this thread | | | Date | Tue, 17 May 2022 15:12:16 +0100 | Subject | Re: [PATCH 2/8] iommu: mtk_iommu: Lookup phandle to retrieve syscon to infracfg | From | 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@collabora.com> > --- > 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;
| |