| Message ID | 20210111044638.290909-2-joel@jms.id.au |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drm: aspeed: Support more chip families | expand |
Hi Joel, Sounds like a good idea! One comment though: > @@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm) > if (IS_ERR(priv->base)) > return PTR_ERR(priv->base); > > - priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu"); > + priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon"); > if (IS_ERR(priv->scu)) { > - dev_err(&pdev->dev, "failed to find SCU regmap\n"); > - return PTR_ERR(priv->scu); > + priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu"); Is this (more generic) compatible value guaranteed to exist alongside aspeed,ast2500-scu? The scu binding only specifies the model-specific ones: Documentation/devicetree/bindings/mfd/aspeed-scu.txt: Required properties: - compatible: One of: "aspeed,ast2400-scu", "syscon", "simple-mfd" "aspeed,ast2500-scu", "syscon", "simple-mfd" - the only mention of the new compatible value that I can find is this thread. Maybe we should retain the existing one to keep the fallback case working? Cheers, Jeremy
On Tue, 2 Feb 2021 at 04:39, Jeremy Kerr <jk@ozlabs.org> wrote: > > Hi Joel, > > Sounds like a good idea! One comment though: > > > @@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm) > > if (IS_ERR(priv->base)) > > return PTR_ERR(priv->base); > > > > - priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu"); > > + priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon"); > > if (IS_ERR(priv->scu)) { > > - dev_err(&pdev->dev, "failed to find SCU regmap\n"); > > - return PTR_ERR(priv->scu); > > + priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu"); > > Is this (more generic) compatible value guaranteed to exist alongside > aspeed,ast2500-scu? The scu binding only specifies the model-specific > ones: > > Documentation/devicetree/bindings/mfd/aspeed-scu.txt: > > Required properties: > - compatible: One of: > "aspeed,ast2400-scu", "syscon", "simple-mfd" > "aspeed,ast2500-scu", "syscon", "simple-mfd" > > - the only mention of the new compatible value that I can find is this > thread. Maybe we should retain the existing one to keep the fallback > case working? Yes, it was a mistake to change ast2500-scu to aspeed-scu. The only reason to keep the lookup_by_compatible was to decouple this patch from the device tree changes. I will send a v2 with syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu").
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c index 457ec04950f7..8ada7e944147 100644 --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c @@ -103,6 +103,7 @@ static int aspeed_gfx_load(struct drm_device *drm) { struct platform_device *pdev = to_platform_device(drm->dev); struct aspeed_gfx *priv = to_aspeed_gfx(drm); + struct device_node *np = pdev->dev.of_node; struct resource *res; int ret; @@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm) if (IS_ERR(priv->base)) return PTR_ERR(priv->base); - priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu"); + priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon"); if (IS_ERR(priv->scu)) { - dev_err(&pdev->dev, "failed to find SCU regmap\n"); - return PTR_ERR(priv->scu); + priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu"); + if (IS_ERR(priv->scu)) { + dev_err(&pdev->dev, "failed to find SCU regmap\n"); + return PTR_ERR(priv->scu); + } } ret = of_reserved_mem_device_init(drm->dev);
This scales better to multiple families of SoC. The lookup by compatible can be removed in a future change. Signed-off-by: Joel Stanley <joel@jms.id.au> --- drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)