* [PATCH 1/3] drm: pl111: Fix module autoloading
[not found] <20200409013947.12667-1-robh@kernel.org>
@ 2020-04-09 1:39 ` Rob Herring
2020-04-09 14:12 ` Sam Ravnborg
2020-04-17 20:09 ` Linus Walleij
2020-04-09 1:39 ` [PATCH 2/3] drm: pl111: Simplify vexpress init Rob Herring
2020-04-09 1:39 ` [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init Rob Herring
2 siblings, 2 replies; 12+ messages in thread
From: Rob Herring @ 2020-04-09 1:39 UTC (permalink / raw)
To: Eric Anholt; +Cc: dri-devel, linux-arm-kernel
Add a missing MODULE_DEVICE_TABLE entry to fix module autoloading.
Cc: Eric Anholt <eric@anholt.net>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/gpu/drm/pl111/pl111_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index aa8aa8d9e405..7f405b1374c2 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -444,6 +444,7 @@ static const struct amba_id pl111_id_table[] = {
},
{0, 0},
};
+MODULE_DEVICE_TABLE(amba, pl111_id_table);
static struct amba_driver pl111_amba_driver __maybe_unused = {
.drv = {
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] drm: pl111: Simplify vexpress init
[not found] <20200409013947.12667-1-robh@kernel.org>
2020-04-09 1:39 ` [PATCH 1/3] drm: pl111: Fix module autoloading Rob Herring
@ 2020-04-09 1:39 ` Rob Herring
2020-04-09 14:16 ` Sam Ravnborg
2020-04-17 20:23 ` Linus Walleij
2020-04-09 1:39 ` [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init Rob Herring
2 siblings, 2 replies; 12+ messages in thread
From: Rob Herring @ 2020-04-09 1:39 UTC (permalink / raw)
To: Eric Anholt; +Cc: dri-devel, linux-arm-kernel
The init VExpress variants currently instantiates a 'muxfpga' driver for
the sole purpose of getting a regmap for it. There's no reason to
instantiate a driver and doing so just complicates things. The muxfpga
driver also isn't unregistered properly on module unload. Let's
just simplify all this this by just calling
devm_regmap_init_vexpress_config() directly.
Cc: Eric Anholt <eric@anholt.net>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/gpu/drm/pl111/pl111_versatile.c | 21 +++----------
drivers/gpu/drm/pl111/pl111_vexpress.c | 42 -------------------------
drivers/gpu/drm/pl111/pl111_vexpress.h | 7 -----
3 files changed, 4 insertions(+), 66 deletions(-)
diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
index 09aeaffb7660..8c2551088f26 100644
--- a/drivers/gpu/drm/pl111/pl111_versatile.c
+++ b/drivers/gpu/drm/pl111/pl111_versatile.c
@@ -8,6 +8,7 @@
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/regmap.h>
+#include <linux/vexpress.h>
#include "pl111_versatile.h"
#include "pl111_vexpress.h"
@@ -325,17 +326,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
/* Versatile Express special handling */
- if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
+ if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
struct platform_device *pdev;
-
- /* Registers a driver for the muxfpga */
- ret = vexpress_muxfpga_init();
- if (ret) {
- dev_err(dev, "unable to initialize muxfpga driver\n");
- of_node_put(np);
- return ret;
- }
-
/* Call into deep Vexpress configuration API */
pdev = of_find_device_by_node(np);
if (!pdev) {
@@ -343,13 +335,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
of_node_put(np);
return -EPROBE_DEFER;
}
- map = dev_get_drvdata(&pdev->dev);
- if (!map) {
- dev_err(dev, "sysreg has not yet probed\n");
- platform_device_put(pdev);
- of_node_put(np);
- return -EPROBE_DEFER;
- }
+ map = devm_regmap_init_vexpress_config(&pdev->dev);
+ platform_device_put(pdev);
} else {
map = syscon_node_to_regmap(np);
}
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
index 350570fe06b5..37ed3f1da820 100644
--- a/drivers/gpu/drm/pl111/pl111_vexpress.c
+++ b/drivers/gpu/drm/pl111/pl111_vexpress.c
@@ -94,45 +94,3 @@ int pl111_vexpress_clcd_init(struct device *dev,
return 0;
}
-
-/*
- * This sets up the regmap pointer that will then be retrieved by
- * the detection code in pl111_versatile.c and passed in to the
- * pl111_vexpress_clcd_init() function above.
- */
-static int vexpress_muxfpga_probe(struct platform_device *pdev)
-{
- struct device *dev = &pdev->dev;
- struct regmap *map;
-
- map = devm_regmap_init_vexpress_config(&pdev->dev);
- if (IS_ERR(map))
- return PTR_ERR(map);
- dev_set_drvdata(dev, map);
-
- return 0;
-}
-
-static const struct of_device_id vexpress_muxfpga_match[] = {
- { .compatible = "arm,vexpress-muxfpga", },
- {}
-};
-
-static struct platform_driver vexpress_muxfpga_driver = {
- .driver = {
- .name = "vexpress-muxfpga",
- .of_match_table = of_match_ptr(vexpress_muxfpga_match),
- },
- .probe = vexpress_muxfpga_probe,
-};
-
-int vexpress_muxfpga_init(void)
-{
- int ret;
-
- ret = platform_driver_register(&vexpress_muxfpga_driver);
- /* -EBUSY just means this driver is already registered */
- if (ret == -EBUSY)
- ret = 0;
- return ret;
-}
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
index 5d3681bb4c00..bb54864ca91e 100644
--- a/drivers/gpu/drm/pl111/pl111_vexpress.h
+++ b/drivers/gpu/drm/pl111/pl111_vexpress.h
@@ -10,8 +10,6 @@ int pl111_vexpress_clcd_init(struct device *dev,
struct pl111_drm_dev_private *priv,
struct regmap *map);
-int vexpress_muxfpga_init(void);
-
#else
static inline int pl111_vexpress_clcd_init(struct device *dev,
@@ -21,9 +19,4 @@ static inline int pl111_vexpress_clcd_init(struct device *dev,
return -ENODEV;
}
-static inline int vexpress_muxfpga_init(void)
-{
- return 0;
-}
-
#endif
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init
[not found] <20200409013947.12667-1-robh@kernel.org>
2020-04-09 1:39 ` [PATCH 1/3] drm: pl111: Fix module autoloading Rob Herring
2020-04-09 1:39 ` [PATCH 2/3] drm: pl111: Simplify vexpress init Rob Herring
@ 2020-04-09 1:39 ` Rob Herring
2020-04-09 14:34 ` Sam Ravnborg
2020-04-17 20:27 ` Linus Walleij
2 siblings, 2 replies; 12+ messages in thread
From: Rob Herring @ 2020-04-09 1:39 UTC (permalink / raw)
To: Eric Anholt; +Cc: dri-devel, linux-arm-kernel
Since the VExpress setup in pl111_vexpress.c is now just a single
function call, let's move it into pl111_versatile.c and we can further
simplify pl111_versatile_init() by moving the other pieces for VExpress
into pl111_vexpress_clcd_init().
Cc: Eric Anholt <eric@anholt.net>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/gpu/drm/pl111/Makefile | 1 -
drivers/gpu/drm/pl111/pl111_versatile.c | 122 ++++++++++++++++++++----
drivers/gpu/drm/pl111/pl111_vexpress.c | 96 -------------------
drivers/gpu/drm/pl111/pl111_vexpress.h | 22 -----
4 files changed, 102 insertions(+), 139 deletions(-)
delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.c
delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.h
diff --git a/drivers/gpu/drm/pl111/Makefile b/drivers/gpu/drm/pl111/Makefile
index 0c70f0e91d21..67d430d433e0 100644
--- a/drivers/gpu/drm/pl111/Makefile
+++ b/drivers/gpu/drm/pl111/Makefile
@@ -3,7 +3,6 @@ pl111_drm-y += pl111_display.o \
pl111_versatile.o \
pl111_drv.o
-pl111_drm-$(CONFIG_ARCH_VEXPRESS) += pl111_vexpress.o
pl111_drm-$(CONFIG_ARCH_NOMADIK) += pl111_nomadik.o
pl111_drm-$(CONFIG_DEBUG_FS) += pl111_debugfs.o
diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
index 8c2551088f26..1b55f977e945 100644
--- a/drivers/gpu/drm/pl111/pl111_versatile.c
+++ b/drivers/gpu/drm/pl111/pl111_versatile.c
@@ -11,7 +11,6 @@
#include <linux/vexpress.h>
#include "pl111_versatile.h"
-#include "pl111_vexpress.h"
#include "pl111_drm.h"
static struct regmap *versatile_syscon_map;
@@ -309,13 +308,110 @@ static const struct pl111_variant_data pl111_vexpress = {
.broken_clockdivider = true,
};
+#define VEXPRESS_FPGAMUX_MOTHERBOARD 0x00
+#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1 0x01
+#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2 0x02
+
+static int pl111_vexpress_clcd_init(struct device *dev, struct device_node *np,
+ struct pl111_drm_dev_private *priv)
+{
+ struct platform_device *pdev;
+ struct device_node *root;
+ struct device_node *child;
+ struct device_node *ct_clcd = NULL;
+ struct regmap *map;
+ bool has_coretile_clcd = false;
+ bool has_coretile_hdlcd = false;
+ bool mux_motherboard = true;
+ u32 val;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_VEXPRESS_CONFIG))
+ return -ENODEV;
+
+ /*
+ * Check if we have a CLCD or HDLCD on the core tile by checking if a
+ * CLCD or HDLCD is available in the root of the device tree.
+ */
+ root = of_find_node_by_path("/");
+ if (!root)
+ return -EINVAL;
+
+ for_each_available_child_of_node(root, child) {
+ if (of_device_is_compatible(child, "arm,pl111")) {
+ has_coretile_clcd = true;
+ ct_clcd = child;
+ break;
+ }
+ if (of_device_is_compatible(child, "arm,hdlcd")) {
+ has_coretile_hdlcd = true;
+ of_node_put(child);
+ break;
+ }
+ }
+
+ of_node_put(root);
+
+ /*
+ * If there is a coretile HDLCD and it has a driver,
+ * do not mux the CLCD on the motherboard to the DVI.
+ */
+ if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
+ mux_motherboard = false;
+
+ /*
+ * On the Vexpress CA9 we let the CLCD on the coretile
+ * take precedence, so also in this case do not mux the
+ * motherboard to the DVI.
+ */
+ if (has_coretile_clcd)
+ mux_motherboard = false;
+
+ if (mux_motherboard) {
+ dev_info(dev, "DVI muxed to motherboard CLCD\n");
+ val = VEXPRESS_FPGAMUX_MOTHERBOARD;
+ } else if (ct_clcd == dev->of_node) {
+ dev_info(dev,
+ "DVI muxed to daughterboard 1 (core tile) CLCD\n");
+ val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
+ } else {
+ dev_info(dev, "core tile graphics present\n");
+ dev_info(dev, "this device will be deactivated\n");
+ return -ENODEV;
+ }
+
+ /* Call into deep Vexpress configuration API */
+ pdev = of_find_device_by_node(np);
+ if (!pdev) {
+ dev_err(dev, "can't find the sysreg device, deferring\n");
+ return -EPROBE_DEFER;
+ }
+
+ map = devm_regmap_init_vexpress_config(&pdev->dev);
+ if (IS_ERR(map)) {
+ platform_device_put(pdev);
+ return PTR_ERR(map);
+ }
+
+ ret = regmap_write(map, 0, val);
+ platform_device_put(pdev);
+ if (ret) {
+ dev_err(dev, "error setting DVI muxmode\n");
+ return -ENODEV;
+ }
+
+ priv->variant = &pl111_vexpress;
+ dev_info(dev, "initializing Versatile Express PL111\n");
+
+ return 0;
+}
+
int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
{
const struct of_device_id *clcd_id;
enum versatile_clcd versatile_clcd_type;
struct device_node *np;
struct regmap *map;
- int ret;
np = of_find_matching_node_and_match(NULL, versatile_clcd_of_match,
&clcd_id);
@@ -326,17 +422,10 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
/* Versatile Express special handling */
- if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
- struct platform_device *pdev;
- /* Call into deep Vexpress configuration API */
- pdev = of_find_device_by_node(np);
- if (!pdev) {
- dev_err(dev, "can't find the sysreg device, deferring\n");
- of_node_put(np);
- return -EPROBE_DEFER;
- }
- map = devm_regmap_init_vexpress_config(&pdev->dev);
- platform_device_put(pdev);
+ if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
+ int ret = pl111_vexpress_clcd_init(dev, np, priv);
+ of_node_put(np);
+ return ret;
} else {
map = syscon_node_to_regmap(np);
}
@@ -380,13 +469,6 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
priv->variant_display_disable = pl111_realview_clcd_disable;
dev_info(dev, "set up callbacks for RealView PL111\n");
break;
- case VEXPRESS_CLCD_V2M:
- priv->variant = &pl111_vexpress;
- dev_info(dev, "initializing Versatile Express PL111\n");
- ret = pl111_vexpress_clcd_init(dev, priv, map);
- if (ret)
- return ret;
- break;
default:
dev_info(dev, "unknown Versatile system controller\n");
break;
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
deleted file mode 100644
index 37ed3f1da820..000000000000
--- a/drivers/gpu/drm/pl111/pl111_vexpress.c
+++ /dev/null
@@ -1,96 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Versatile Express PL111 handling
- * Copyright (C) 2018 Linus Walleij
- *
- * This module binds to the "arm,vexpress-muxfpga" device on the
- * Versatile Express configuration bus and sets up which CLCD instance
- * gets muxed out on the DVI bridge.
- */
-#include <linux/device.h>
-#include <linux/module.h>
-#include <linux/regmap.h>
-#include <linux/vexpress.h>
-#include <linux/platform_device.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/of_platform.h>
-#include "pl111_drm.h"
-#include "pl111_vexpress.h"
-
-#define VEXPRESS_FPGAMUX_MOTHERBOARD 0x00
-#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1 0x01
-#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2 0x02
-
-int pl111_vexpress_clcd_init(struct device *dev,
- struct pl111_drm_dev_private *priv,
- struct regmap *map)
-{
- struct device_node *root;
- struct device_node *child;
- struct device_node *ct_clcd = NULL;
- bool has_coretile_clcd = false;
- bool has_coretile_hdlcd = false;
- bool mux_motherboard = true;
- u32 val;
- int ret;
-
- /*
- * Check if we have a CLCD or HDLCD on the core tile by checking if a
- * CLCD or HDLCD is available in the root of the device tree.
- */
- root = of_find_node_by_path("/");
- if (!root)
- return -EINVAL;
-
- for_each_available_child_of_node(root, child) {
- if (of_device_is_compatible(child, "arm,pl111")) {
- has_coretile_clcd = true;
- ct_clcd = child;
- break;
- }
- if (of_device_is_compatible(child, "arm,hdlcd")) {
- has_coretile_hdlcd = true;
- of_node_put(child);
- break;
- }
- }
-
- of_node_put(root);
-
- /*
- * If there is a coretile HDLCD and it has a driver,
- * do not mux the CLCD on the motherboard to the DVI.
- */
- if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
- mux_motherboard = false;
-
- /*
- * On the Vexpress CA9 we let the CLCD on the coretile
- * take precedence, so also in this case do not mux the
- * motherboard to the DVI.
- */
- if (has_coretile_clcd)
- mux_motherboard = false;
-
- if (mux_motherboard) {
- dev_info(dev, "DVI muxed to motherboard CLCD\n");
- val = VEXPRESS_FPGAMUX_MOTHERBOARD;
- } else if (ct_clcd == dev->of_node) {
- dev_info(dev,
- "DVI muxed to daughterboard 1 (core tile) CLCD\n");
- val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
- } else {
- dev_info(dev, "core tile graphics present\n");
- dev_info(dev, "this device will be deactivated\n");
- return -ENODEV;
- }
-
- ret = regmap_write(map, 0, val);
- if (ret) {
- dev_err(dev, "error setting DVI muxmode\n");
- return -ENODEV;
- }
-
- return 0;
-}
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
deleted file mode 100644
index bb54864ca91e..000000000000
--- a/drivers/gpu/drm/pl111/pl111_vexpress.h
+++ /dev/null
@@ -1,22 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-struct device;
-struct pl111_drm_dev_private;
-struct regmap;
-
-#ifdef CONFIG_ARCH_VEXPRESS
-
-int pl111_vexpress_clcd_init(struct device *dev,
- struct pl111_drm_dev_private *priv,
- struct regmap *map);
-
-#else
-
-static inline int pl111_vexpress_clcd_init(struct device *dev,
- struct pl111_drm_dev_private *priv,
- struct regmap *map)
-{
- return -ENODEV;
-}
-
-#endif
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] drm: pl111: Fix module autoloading
2020-04-09 1:39 ` [PATCH 1/3] drm: pl111: Fix module autoloading Rob Herring
@ 2020-04-09 14:12 ` Sam Ravnborg
2020-04-17 20:09 ` Linus Walleij
1 sibling, 0 replies; 12+ messages in thread
From: Sam Ravnborg @ 2020-04-09 14:12 UTC (permalink / raw)
To: Rob Herring; +Cc: linux-arm-kernel, dri-devel
Hi Rob.
On Wed, Apr 08, 2020 at 07:39:45PM -0600, Rob Herring wrote:
> Add a missing MODULE_DEVICE_TABLE entry to fix module autoloading.
>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Change looks straightforward.
Acked-by: Sam Ravnborg <sam@ravnborg.org>
> ---
> drivers/gpu/drm/pl111/pl111_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index aa8aa8d9e405..7f405b1374c2 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -444,6 +444,7 @@ static const struct amba_id pl111_id_table[] = {
> },
> {0, 0},
> };
> +MODULE_DEVICE_TABLE(amba, pl111_id_table);
>
> static struct amba_driver pl111_amba_driver __maybe_unused = {
> .drv = {
> --
> 2.20.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm: pl111: Simplify vexpress init
2020-04-09 1:39 ` [PATCH 2/3] drm: pl111: Simplify vexpress init Rob Herring
@ 2020-04-09 14:16 ` Sam Ravnborg
2020-04-17 17:43 ` Rob Herring
2020-04-17 20:23 ` Linus Walleij
1 sibling, 1 reply; 12+ messages in thread
From: Sam Ravnborg @ 2020-04-09 14:16 UTC (permalink / raw)
To: Rob Herring; +Cc: linux-arm-kernel, dri-devel
Hi Rob.
On Wed, Apr 08, 2020 at 07:39:46PM -0600, Rob Herring wrote:
> The init VExpress variants currently instantiates a 'muxfpga' driver for
> the sole purpose of getting a regmap for it. There's no reason to
> instantiate a driver and doing so just complicates things. The muxfpga
> driver also isn't unregistered properly on module unload. Let's
> just simplify all this this by just calling
> devm_regmap_init_vexpress_config() directly.
>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Procastinating, so I took a look at this.
Nice simplification - on nit below.
Acked-by: Sam Ravnborg <sam@ravnborg.org>
> ---
> drivers/gpu/drm/pl111/pl111_versatile.c | 21 +++----------
> drivers/gpu/drm/pl111/pl111_vexpress.c | 42 -------------------------
> drivers/gpu/drm/pl111/pl111_vexpress.h | 7 -----
> 3 files changed, 4 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> index 09aeaffb7660..8c2551088f26 100644
> --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> @@ -8,6 +8,7 @@
> #include <linux/of.h>
> #include <linux/of_platform.h>
> #include <linux/regmap.h>
> +#include <linux/vexpress.h>
>
> #include "pl111_versatile.h"
> #include "pl111_vexpress.h"
> @@ -325,17 +326,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
>
> /* Versatile Express special handling */
> - if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> + if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> struct platform_device *pdev;
> -
> - /* Registers a driver for the muxfpga */
> - ret = vexpress_muxfpga_init();
> - if (ret) {
> - dev_err(dev, "unable to initialize muxfpga driver\n");
> - of_node_put(np);
> - return ret;
> - }
> -
> /* Call into deep Vexpress configuration API */
> pdev = of_find_device_by_node(np);
> if (!pdev) {
> @@ -343,13 +335,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> of_node_put(np);
> return -EPROBE_DEFER;
> }
> - map = dev_get_drvdata(&pdev->dev);
> - if (!map) {
> - dev_err(dev, "sysreg has not yet probed\n");
> - platform_device_put(pdev);
> - of_node_put(np);
> - return -EPROBE_DEFER;
> - }
> + map = devm_regmap_init_vexpress_config(&pdev->dev);
> + platform_device_put(pdev);
> } else {
> map = syscon_node_to_regmap(np);
> }
On the following line there is:
if (IS_ERR(map)) {
dev_err(dev, "no Versatile syscon regmap\n");
return PTR_ERR(map);
}
The error message no longer tell if this was
devm_regmap_init_vexpress_config() or syscon_node_to_regmap() that
caused the error.
Sam
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
> index 350570fe06b5..37ed3f1da820 100644
> --- a/drivers/gpu/drm/pl111/pl111_vexpress.c
> +++ b/drivers/gpu/drm/pl111/pl111_vexpress.c
> @@ -94,45 +94,3 @@ int pl111_vexpress_clcd_init(struct device *dev,
>
> return 0;
> }
> -
> -/*
> - * This sets up the regmap pointer that will then be retrieved by
> - * the detection code in pl111_versatile.c and passed in to the
> - * pl111_vexpress_clcd_init() function above.
> - */
> -static int vexpress_muxfpga_probe(struct platform_device *pdev)
> -{
> - struct device *dev = &pdev->dev;
> - struct regmap *map;
> -
> - map = devm_regmap_init_vexpress_config(&pdev->dev);
> - if (IS_ERR(map))
> - return PTR_ERR(map);
> - dev_set_drvdata(dev, map);
> -
> - return 0;
> -}
> -
> -static const struct of_device_id vexpress_muxfpga_match[] = {
> - { .compatible = "arm,vexpress-muxfpga", },
> - {}
> -};
> -
> -static struct platform_driver vexpress_muxfpga_driver = {
> - .driver = {
> - .name = "vexpress-muxfpga",
> - .of_match_table = of_match_ptr(vexpress_muxfpga_match),
> - },
> - .probe = vexpress_muxfpga_probe,
> -};
> -
> -int vexpress_muxfpga_init(void)
> -{
> - int ret;
> -
> - ret = platform_driver_register(&vexpress_muxfpga_driver);
> - /* -EBUSY just means this driver is already registered */
> - if (ret == -EBUSY)
> - ret = 0;
> - return ret;
> -}
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
> index 5d3681bb4c00..bb54864ca91e 100644
> --- a/drivers/gpu/drm/pl111/pl111_vexpress.h
> +++ b/drivers/gpu/drm/pl111/pl111_vexpress.h
> @@ -10,8 +10,6 @@ int pl111_vexpress_clcd_init(struct device *dev,
> struct pl111_drm_dev_private *priv,
> struct regmap *map);
>
> -int vexpress_muxfpga_init(void);
> -
> #else
>
> static inline int pl111_vexpress_clcd_init(struct device *dev,
> @@ -21,9 +19,4 @@ static inline int pl111_vexpress_clcd_init(struct device *dev,
> return -ENODEV;
> }
>
> -static inline int vexpress_muxfpga_init(void)
> -{
> - return 0;
> -}
> -
> #endif
> --
> 2.20.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init
2020-04-09 1:39 ` [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init Rob Herring
@ 2020-04-09 14:34 ` Sam Ravnborg
2020-04-17 17:33 ` Rob Herring
2020-04-17 20:27 ` Linus Walleij
1 sibling, 1 reply; 12+ messages in thread
From: Sam Ravnborg @ 2020-04-09 14:34 UTC (permalink / raw)
To: Rob Herring; +Cc: linux-arm-kernel, dri-devel
Hi Rob.
On Wed, Apr 08, 2020 at 07:39:47PM -0600, Rob Herring wrote:
> Since the VExpress setup in pl111_vexpress.c is now just a single
> function call, let's move it into pl111_versatile.c and we can further
> simplify pl111_versatile_init() by moving the other pieces for VExpress
> into pl111_vexpress_clcd_init().
>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Patch looks good - nits below, but anyway:
Acked-by: Sam Ravnborg <sam@ravnborg.org>
> ---
> drivers/gpu/drm/pl111/Makefile | 1 -
> drivers/gpu/drm/pl111/pl111_versatile.c | 122 ++++++++++++++++++++----
> drivers/gpu/drm/pl111/pl111_vexpress.c | 96 -------------------
> drivers/gpu/drm/pl111/pl111_vexpress.h | 22 -----
> 4 files changed, 102 insertions(+), 139 deletions(-)
> delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.c
> delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.h
>
> diff --git a/drivers/gpu/drm/pl111/Makefile b/drivers/gpu/drm/pl111/Makefile
> index 0c70f0e91d21..67d430d433e0 100644
> --- a/drivers/gpu/drm/pl111/Makefile
> +++ b/drivers/gpu/drm/pl111/Makefile
> @@ -3,7 +3,6 @@ pl111_drm-y += pl111_display.o \
> pl111_versatile.o \
> pl111_drv.o
>
> -pl111_drm-$(CONFIG_ARCH_VEXPRESS) += pl111_vexpress.o
> pl111_drm-$(CONFIG_ARCH_NOMADIK) += pl111_nomadik.o
> pl111_drm-$(CONFIG_DEBUG_FS) += pl111_debugfs.o
>
> diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> index 8c2551088f26..1b55f977e945 100644
> --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> @@ -11,7 +11,6 @@
> #include <linux/vexpress.h>
>
> #include "pl111_versatile.h"
> -#include "pl111_vexpress.h"
> #include "pl111_drm.h"
>
> static struct regmap *versatile_syscon_map;
> @@ -309,13 +308,110 @@ static const struct pl111_variant_data pl111_vexpress = {
> .broken_clockdivider = true,
> };
>
> +#define VEXPRESS_FPGAMUX_MOTHERBOARD 0x00
> +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1 0x01
> +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2 0x02
> +
> +static int pl111_vexpress_clcd_init(struct device *dev, struct device_node *np,
> + struct pl111_drm_dev_private *priv)
> +{
> + struct platform_device *pdev;
> + struct device_node *root;
> + struct device_node *child;
> + struct device_node *ct_clcd = NULL;
> + struct regmap *map;
> + bool has_coretile_clcd = false;
> + bool has_coretile_hdlcd = false;
> + bool mux_motherboard = true;
> + u32 val;
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_VEXPRESS_CONFIG))
> + return -ENODEV;
Thats was a simple way to get out of this.
Maybe a WARN() was in place, because should we ever hit this?
> +
> + /*
> + * Check if we have a CLCD or HDLCD on the core tile by checking if a
> + * CLCD or HDLCD is available in the root of the device tree.
> + */
> + root = of_find_node_by_path("/");
> + if (!root)
> + return -EINVAL;
> +
> + for_each_available_child_of_node(root, child) {
> + if (of_device_is_compatible(child, "arm,pl111")) {
> + has_coretile_clcd = true;
> + ct_clcd = child;
> + break;
> + }
> + if (of_device_is_compatible(child, "arm,hdlcd")) {
> + has_coretile_hdlcd = true;
> + of_node_put(child);
> + break;
> + }
> + }
> +
> + of_node_put(root);
> +
> + /*
> + * If there is a coretile HDLCD and it has a driver,
> + * do not mux the CLCD on the motherboard to the DVI.
> + */
> + if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
> + mux_motherboard = false;
> +
> + /*
> + * On the Vexpress CA9 we let the CLCD on the coretile
> + * take precedence, so also in this case do not mux the
> + * motherboard to the DVI.
> + */
> + if (has_coretile_clcd)
> + mux_motherboard = false;
> +
> + if (mux_motherboard) {
> + dev_info(dev, "DVI muxed to motherboard CLCD\n");
> + val = VEXPRESS_FPGAMUX_MOTHERBOARD;
> + } else if (ct_clcd == dev->of_node) {
> + dev_info(dev,
> + "DVI muxed to daughterboard 1 (core tile) CLCD\n");
> + val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
> + } else {
> + dev_info(dev, "core tile graphics present\n");
> + dev_info(dev, "this device will be deactivated\n");
> + return -ENODEV;
> + }
> +
> + /* Call into deep Vexpress configuration API */
> + pdev = of_find_device_by_node(np);
> + if (!pdev) {
> + dev_err(dev, "can't find the sysreg device, deferring\n");
> + return -EPROBE_DEFER;
> + }
> +
> + map = devm_regmap_init_vexpress_config(&pdev->dev);
> + if (IS_ERR(map)) {
> + platform_device_put(pdev);
> + return PTR_ERR(map);
> + }
> +
> + ret = regmap_write(map, 0, val);
> + platform_device_put(pdev);
> + if (ret) {
> + dev_err(dev, "error setting DVI muxmode\n");
> + return -ENODEV;
> + }
> +
> + priv->variant = &pl111_vexpress;
> + dev_info(dev, "initializing Versatile Express PL111\n");
> +
> + return 0;
> +}
> +
> int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> {
> const struct of_device_id *clcd_id;
> enum versatile_clcd versatile_clcd_type;
> struct device_node *np;
> struct regmap *map;
> - int ret;
>
> np = of_find_matching_node_and_match(NULL, versatile_clcd_of_match,
> &clcd_id);
> @@ -326,17 +422,10 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
>
> /* Versatile Express special handling */
> - if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> - struct platform_device *pdev;
> - /* Call into deep Vexpress configuration API */
> - pdev = of_find_device_by_node(np);
> - if (!pdev) {
> - dev_err(dev, "can't find the sysreg device, deferring\n");
> - of_node_put(np);
> - return -EPROBE_DEFER;
> - }
> - map = devm_regmap_init_vexpress_config(&pdev->dev);
> - platform_device_put(pdev);
> + if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> + int ret = pl111_vexpress_clcd_init(dev, np, priv);
> + of_node_put(np);
> + return ret;
It is often in error situations one return early, but in this case it is
a normal case. Maybe a small comment?
> } else {
> map = syscon_node_to_regmap(np);
> }
> @@ -380,13 +469,6 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> priv->variant_display_disable = pl111_realview_clcd_disable;
> dev_info(dev, "set up callbacks for RealView PL111\n");
> break;
> - case VEXPRESS_CLCD_V2M:
> - priv->variant = &pl111_vexpress;
> - dev_info(dev, "initializing Versatile Express PL111\n");
> - ret = pl111_vexpress_clcd_init(dev, priv, map);
> - if (ret)
> - return ret;
> - break;
The switch no longer includes VEXPRESS_CLCD_V2M - because we will never
reach the switch in this case.
I guess some gcc falgs may cause a warning that not all enums are
accounted for. But then again, the default may supress it.
Anyway, I noticed so you got a comment.
I think current code is fine.
Sam
> default:
> dev_info(dev, "unknown Versatile system controller\n");
> break;
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
> deleted file mode 100644
> index 37ed3f1da820..000000000000
> --- a/drivers/gpu/drm/pl111/pl111_vexpress.c
> +++ /dev/null
> @@ -1,96 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Versatile Express PL111 handling
> - * Copyright (C) 2018 Linus Walleij
> - *
> - * This module binds to the "arm,vexpress-muxfpga" device on the
> - * Versatile Express configuration bus and sets up which CLCD instance
> - * gets muxed out on the DVI bridge.
> - */
> -#include <linux/device.h>
> -#include <linux/module.h>
> -#include <linux/regmap.h>
> -#include <linux/vexpress.h>
> -#include <linux/platform_device.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
> -#include <linux/of_platform.h>
> -#include "pl111_drm.h"
> -#include "pl111_vexpress.h"
> -
> -#define VEXPRESS_FPGAMUX_MOTHERBOARD 0x00
> -#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1 0x01
> -#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2 0x02
> -
> -int pl111_vexpress_clcd_init(struct device *dev,
> - struct pl111_drm_dev_private *priv,
> - struct regmap *map)
> -{
> - struct device_node *root;
> - struct device_node *child;
> - struct device_node *ct_clcd = NULL;
> - bool has_coretile_clcd = false;
> - bool has_coretile_hdlcd = false;
> - bool mux_motherboard = true;
> - u32 val;
> - int ret;
> -
> - /*
> - * Check if we have a CLCD or HDLCD on the core tile by checking if a
> - * CLCD or HDLCD is available in the root of the device tree.
> - */
> - root = of_find_node_by_path("/");
> - if (!root)
> - return -EINVAL;
> -
> - for_each_available_child_of_node(root, child) {
> - if (of_device_is_compatible(child, "arm,pl111")) {
> - has_coretile_clcd = true;
> - ct_clcd = child;
> - break;
> - }
> - if (of_device_is_compatible(child, "arm,hdlcd")) {
> - has_coretile_hdlcd = true;
> - of_node_put(child);
> - break;
> - }
> - }
> -
> - of_node_put(root);
> -
> - /*
> - * If there is a coretile HDLCD and it has a driver,
> - * do not mux the CLCD on the motherboard to the DVI.
> - */
> - if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
> - mux_motherboard = false;
> -
> - /*
> - * On the Vexpress CA9 we let the CLCD on the coretile
> - * take precedence, so also in this case do not mux the
> - * motherboard to the DVI.
> - */
> - if (has_coretile_clcd)
> - mux_motherboard = false;
> -
> - if (mux_motherboard) {
> - dev_info(dev, "DVI muxed to motherboard CLCD\n");
> - val = VEXPRESS_FPGAMUX_MOTHERBOARD;
> - } else if (ct_clcd == dev->of_node) {
> - dev_info(dev,
> - "DVI muxed to daughterboard 1 (core tile) CLCD\n");
> - val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
> - } else {
> - dev_info(dev, "core tile graphics present\n");
> - dev_info(dev, "this device will be deactivated\n");
> - return -ENODEV;
> - }
> -
> - ret = regmap_write(map, 0, val);
> - if (ret) {
> - dev_err(dev, "error setting DVI muxmode\n");
> - return -ENODEV;
> - }
> -
> - return 0;
> -}
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
> deleted file mode 100644
> index bb54864ca91e..000000000000
> --- a/drivers/gpu/drm/pl111/pl111_vexpress.h
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -
> -struct device;
> -struct pl111_drm_dev_private;
> -struct regmap;
> -
> -#ifdef CONFIG_ARCH_VEXPRESS
> -
> -int pl111_vexpress_clcd_init(struct device *dev,
> - struct pl111_drm_dev_private *priv,
> - struct regmap *map);
> -
> -#else
> -
> -static inline int pl111_vexpress_clcd_init(struct device *dev,
> - struct pl111_drm_dev_private *priv,
> - struct regmap *map)
> -{
> - return -ENODEV;
> -}
> -
> -#endif
> --
> 2.20.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init
2020-04-09 14:34 ` Sam Ravnborg
@ 2020-04-17 17:33 ` Rob Herring
0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2020-04-17 17:33 UTC (permalink / raw)
To: Sam Ravnborg
Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, dri-devel
On Thu, Apr 9, 2020 at 9:34 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Rob.
>
> On Wed, Apr 08, 2020 at 07:39:47PM -0600, Rob Herring wrote:
> > Since the VExpress setup in pl111_vexpress.c is now just a single
> > function call, let's move it into pl111_versatile.c and we can further
> > simplify pl111_versatile_init() by moving the other pieces for VExpress
> > into pl111_vexpress_clcd_init().
> >
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
> Patch looks good - nits below, but anyway:
>
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
>
>
> > ---
> > drivers/gpu/drm/pl111/Makefile | 1 -
> > drivers/gpu/drm/pl111/pl111_versatile.c | 122 ++++++++++++++++++++----
> > drivers/gpu/drm/pl111/pl111_vexpress.c | 96 -------------------
> > drivers/gpu/drm/pl111/pl111_vexpress.h | 22 -----
> > 4 files changed, 102 insertions(+), 139 deletions(-)
> > delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.c
> > delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.h
> >
> > diff --git a/drivers/gpu/drm/pl111/Makefile b/drivers/gpu/drm/pl111/Makefile
> > index 0c70f0e91d21..67d430d433e0 100644
> > --- a/drivers/gpu/drm/pl111/Makefile
> > +++ b/drivers/gpu/drm/pl111/Makefile
> > @@ -3,7 +3,6 @@ pl111_drm-y += pl111_display.o \
> > pl111_versatile.o \
> > pl111_drv.o
> >
> > -pl111_drm-$(CONFIG_ARCH_VEXPRESS) += pl111_vexpress.o
> > pl111_drm-$(CONFIG_ARCH_NOMADIK) += pl111_nomadik.o
> > pl111_drm-$(CONFIG_DEBUG_FS) += pl111_debugfs.o
> >
> > diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> > index 8c2551088f26..1b55f977e945 100644
> > --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> > +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> > @@ -11,7 +11,6 @@
> > #include <linux/vexpress.h>
> >
> > #include "pl111_versatile.h"
> > -#include "pl111_vexpress.h"
> > #include "pl111_drm.h"
> >
> > static struct regmap *versatile_syscon_map;
> > @@ -309,13 +308,110 @@ static const struct pl111_variant_data pl111_vexpress = {
> > .broken_clockdivider = true,
> > };
> >
> > +#define VEXPRESS_FPGAMUX_MOTHERBOARD 0x00
> > +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1 0x01
> > +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2 0x02
> > +
> > +static int pl111_vexpress_clcd_init(struct device *dev, struct device_node *np,
> > + struct pl111_drm_dev_private *priv)
> > +{
> > + struct platform_device *pdev;
> > + struct device_node *root;
> > + struct device_node *child;
> > + struct device_node *ct_clcd = NULL;
> > + struct regmap *map;
> > + bool has_coretile_clcd = false;
> > + bool has_coretile_hdlcd = false;
> > + bool mux_motherboard = true;
> > + u32 val;
> > + int ret;
> > +
> > + if (!IS_ENABLED(CONFIG_VEXPRESS_CONFIG))
> > + return -ENODEV;
> Thats was a simple way to get out of this.
> Maybe a WARN() was in place, because should we ever hit this?
We didn't warn before if misconfigured. Adding an error print at the
caller should be enough.
> > +
> > + /*
> > + * Check if we have a CLCD or HDLCD on the core tile by checking if a
> > + * CLCD or HDLCD is available in the root of the device tree.
> > + */
> > + root = of_find_node_by_path("/");
> > + if (!root)
> > + return -EINVAL;
> > +
> > + for_each_available_child_of_node(root, child) {
> > + if (of_device_is_compatible(child, "arm,pl111")) {
> > + has_coretile_clcd = true;
> > + ct_clcd = child;
> > + break;
> > + }
> > + if (of_device_is_compatible(child, "arm,hdlcd")) {
> > + has_coretile_hdlcd = true;
> > + of_node_put(child);
> > + break;
> > + }
> > + }
> > +
> > + of_node_put(root);
> > +
> > + /*
> > + * If there is a coretile HDLCD and it has a driver,
> > + * do not mux the CLCD on the motherboard to the DVI.
> > + */
> > + if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
> > + mux_motherboard = false;
> > +
> > + /*
> > + * On the Vexpress CA9 we let the CLCD on the coretile
> > + * take precedence, so also in this case do not mux the
> > + * motherboard to the DVI.
> > + */
> > + if (has_coretile_clcd)
> > + mux_motherboard = false;
> > +
> > + if (mux_motherboard) {
> > + dev_info(dev, "DVI muxed to motherboard CLCD\n");
> > + val = VEXPRESS_FPGAMUX_MOTHERBOARD;
> > + } else if (ct_clcd == dev->of_node) {
> > + dev_info(dev,
> > + "DVI muxed to daughterboard 1 (core tile) CLCD\n");
> > + val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
> > + } else {
> > + dev_info(dev, "core tile graphics present\n");
> > + dev_info(dev, "this device will be deactivated\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* Call into deep Vexpress configuration API */
> > + pdev = of_find_device_by_node(np);
> > + if (!pdev) {
> > + dev_err(dev, "can't find the sysreg device, deferring\n");
> > + return -EPROBE_DEFER;
> > + }
> > +
> > + map = devm_regmap_init_vexpress_config(&pdev->dev);
> > + if (IS_ERR(map)) {
> > + platform_device_put(pdev);
> > + return PTR_ERR(map);
> > + }
> > +
> > + ret = regmap_write(map, 0, val);
> > + platform_device_put(pdev);
> > + if (ret) {
> > + dev_err(dev, "error setting DVI muxmode\n");
> > + return -ENODEV;
> > + }
> > +
> > + priv->variant = &pl111_vexpress;
> > + dev_info(dev, "initializing Versatile Express PL111\n");
> > +
> > + return 0;
> > +}
> > +
> > int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> > {
> > const struct of_device_id *clcd_id;
> > enum versatile_clcd versatile_clcd_type;
> > struct device_node *np;
> > struct regmap *map;
> > - int ret;
> >
> > np = of_find_matching_node_and_match(NULL, versatile_clcd_of_match,
> > &clcd_id);
> > @@ -326,17 +422,10 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> > versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
> >
> > /* Versatile Express special handling */
> > - if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> > - struct platform_device *pdev;
> > - /* Call into deep Vexpress configuration API */
> > - pdev = of_find_device_by_node(np);
> > - if (!pdev) {
> > - dev_err(dev, "can't find the sysreg device, deferring\n");
> > - of_node_put(np);
> > - return -EPROBE_DEFER;
> > - }
> > - map = devm_regmap_init_vexpress_config(&pdev->dev);
> > - platform_device_put(pdev);
> > + if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> > + int ret = pl111_vexpress_clcd_init(dev, np, priv);
> > + of_node_put(np);
> > + return ret;
> It is often in error situations one return early, but in this case it is
> a normal case. Maybe a small comment?
I'm adding a print here to address your other comments and I think
that should be sufficient. I'll also move this up so the Vexpress
handling is before everything else.
>
>
> > } else {
And I don't need this else.
> > map = syscon_node_to_regmap(np);
> > }
> > @@ -380,13 +469,6 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> > priv->variant_display_disable = pl111_realview_clcd_disable;
> > dev_info(dev, "set up callbacks for RealView PL111\n");
> > break;
> > - case VEXPRESS_CLCD_V2M:
> > - priv->variant = &pl111_vexpress;
> > - dev_info(dev, "initializing Versatile Express PL111\n");
> > - ret = pl111_vexpress_clcd_init(dev, priv, map);
> > - if (ret)
> > - return ret;
> > - break;
>
> The switch no longer includes VEXPRESS_CLCD_V2M - because we will never
> reach the switch in this case.
> I guess some gcc falgs may cause a warning that not all enums are
> accounted for. But then again, the default may supress it.
Yes, default suppresses that.
Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm: pl111: Simplify vexpress init
2020-04-09 14:16 ` Sam Ravnborg
@ 2020-04-17 17:43 ` Rob Herring
0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2020-04-17 17:43 UTC (permalink / raw)
To: Sam Ravnborg
Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, dri-devel
On Thu, Apr 9, 2020 at 9:16 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Rob.
>
> On Wed, Apr 08, 2020 at 07:39:46PM -0600, Rob Herring wrote:
> > The init VExpress variants currently instantiates a 'muxfpga' driver for
> > the sole purpose of getting a regmap for it. There's no reason to
> > instantiate a driver and doing so just complicates things. The muxfpga
> > driver also isn't unregistered properly on module unload. Let's
> > just simplify all this this by just calling
> > devm_regmap_init_vexpress_config() directly.
> >
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> Procastinating, so I took a look at this.
> Nice simplification - on nit below.
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > ---
> > drivers/gpu/drm/pl111/pl111_versatile.c | 21 +++----------
> > drivers/gpu/drm/pl111/pl111_vexpress.c | 42 -------------------------
> > drivers/gpu/drm/pl111/pl111_vexpress.h | 7 -----
> > 3 files changed, 4 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> > index 09aeaffb7660..8c2551088f26 100644
> > --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> > +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> > @@ -8,6 +8,7 @@
> > #include <linux/of.h>
> > #include <linux/of_platform.h>
> > #include <linux/regmap.h>
> > +#include <linux/vexpress.h>
> >
> > #include "pl111_versatile.h"
> > #include "pl111_vexpress.h"
> > @@ -325,17 +326,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> > versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
> >
> > /* Versatile Express special handling */
> > - if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> > + if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> > struct platform_device *pdev;
> > -
> > - /* Registers a driver for the muxfpga */
> > - ret = vexpress_muxfpga_init();
> > - if (ret) {
> > - dev_err(dev, "unable to initialize muxfpga driver\n");
> > - of_node_put(np);
> > - return ret;
> > - }
> > -
> > /* Call into deep Vexpress configuration API */
> > pdev = of_find_device_by_node(np);
> > if (!pdev) {
> > @@ -343,13 +335,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> > of_node_put(np);
> > return -EPROBE_DEFER;
> > }
> > - map = dev_get_drvdata(&pdev->dev);
> > - if (!map) {
> > - dev_err(dev, "sysreg has not yet probed\n");
> > - platform_device_put(pdev);
> > - of_node_put(np);
> > - return -EPROBE_DEFER;
> > - }
> > + map = devm_regmap_init_vexpress_config(&pdev->dev);
> > + platform_device_put(pdev);
> > } else {
> > map = syscon_node_to_regmap(np);
> > }
>
> On the following line there is:
> if (IS_ERR(map)) {
> dev_err(dev, "no Versatile syscon regmap\n");
> return PTR_ERR(map);
> }
>
> The error message no longer tell if this was
> devm_regmap_init_vexpress_config() or syscon_node_to_regmap() that
> caused the error.
Hopefully you'd know what platform you are on.
In any case, it's changed after patch 3.
Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] drm: pl111: Fix module autoloading
2020-04-09 1:39 ` [PATCH 1/3] drm: pl111: Fix module autoloading Rob Herring
2020-04-09 14:12 ` Sam Ravnborg
@ 2020-04-17 20:09 ` Linus Walleij
1 sibling, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2020-04-17 20:09 UTC (permalink / raw)
To: Rob Herring; +Cc: open list:DRM PANEL DRIVERS, Linux ARM
On Thu, Apr 9, 2020 at 3:39 AM Rob Herring <robh@kernel.org> wrote:
> Add a missing MODULE_DEVICE_TABLE entry to fix module autoloading.
>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm: pl111: Simplify vexpress init
2020-04-09 1:39 ` [PATCH 2/3] drm: pl111: Simplify vexpress init Rob Herring
2020-04-09 14:16 ` Sam Ravnborg
@ 2020-04-17 20:23 ` Linus Walleij
1 sibling, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2020-04-17 20:23 UTC (permalink / raw)
To: Rob Herring; +Cc: open list:DRM PANEL DRIVERS, Linux ARM
On Thu, Apr 9, 2020 at 3:39 AM Rob Herring <robh@kernel.org> wrote:
> The init VExpress variants currently instantiates a 'muxfpga' driver for
> the sole purpose of getting a regmap for it. There's no reason to
> instantiate a driver and doing so just complicates things. The muxfpga
> driver also isn't unregistered properly on module unload. Let's
> just simplify all this this by just calling
> devm_regmap_init_vexpress_config() directly.
>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>
OK... looking at this.
> + if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> struct platform_device *pdev;
> -
> - /* Registers a driver for the muxfpga */
> - ret = vexpress_muxfpga_init();
> - if (ret) {
> - dev_err(dev, "unable to initialize muxfpga driver\n");
> - of_node_put(np);
> - return ret;
> - }
> -
> /* Call into deep Vexpress configuration API */
> pdev = of_find_device_by_node(np);
So this finds the platform device for compatible "arm,vexpress-muxfpga",
ha!
> + map = devm_regmap_init_vexpress_config(&pdev->dev);
> + platform_device_put(pdev);
So then you can just do it like that.
Clever! Hats off for digging through my (unnecessary complex) code.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init
2020-04-09 1:39 ` [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init Rob Herring
2020-04-09 14:34 ` Sam Ravnborg
@ 2020-04-17 20:27 ` Linus Walleij
2020-04-19 17:12 ` Rob Herring
1 sibling, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2020-04-17 20:27 UTC (permalink / raw)
To: Rob Herring; +Cc: open list:DRM PANEL DRIVERS, Linux ARM
On Thu, Apr 9, 2020 at 3:39 AM Rob Herring <robh@kernel.org> wrote:
> Since the VExpress setup in pl111_vexpress.c is now just a single
> function call, let's move it into pl111_versatile.c and we can further
> simplify pl111_versatile_init() by moving the other pieces for VExpress
> into pl111_vexpress_clcd_init().
>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Yeah that's much nicer, the other boards get a copy of the
Vexpress code but it's so little so it doesn't matter and besides
the Vexpress already had copies of the other boards init code.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init
2020-04-17 20:27 ` Linus Walleij
@ 2020-04-19 17:12 ` Rob Herring
0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2020-04-19 17:12 UTC (permalink / raw)
To: Linus Walleij; +Cc: open list:DRM PANEL DRIVERS, Linux ARM
On Fri, Apr 17, 2020 at 3:27 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Apr 9, 2020 at 3:39 AM Rob Herring <robh@kernel.org> wrote:
>
> > Since the VExpress setup in pl111_vexpress.c is now just a single
> > function call, let's move it into pl111_versatile.c and we can further
> > simplify pl111_versatile_init() by moving the other pieces for VExpress
> > into pl111_vexpress_clcd_init().
> >
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> Yeah that's much nicer, the other boards get a copy of the
> Vexpress code but it's so little so it doesn't matter and besides
> the Vexpress already had copies of the other boards init code.
It shouldn't change. The compiler should be smart enough to drop all
the code when IS_ENABLED(CONFIG_ARCH_VEXPRESS) is false.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Thanks.
Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-04-19 17:12 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200409013947.12667-1-robh@kernel.org>
2020-04-09 1:39 ` [PATCH 1/3] drm: pl111: Fix module autoloading Rob Herring
2020-04-09 14:12 ` Sam Ravnborg
2020-04-17 20:09 ` Linus Walleij
2020-04-09 1:39 ` [PATCH 2/3] drm: pl111: Simplify vexpress init Rob Herring
2020-04-09 14:16 ` Sam Ravnborg
2020-04-17 17:43 ` Rob Herring
2020-04-17 20:23 ` Linus Walleij
2020-04-09 1:39 ` [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init Rob Herring
2020-04-09 14:34 ` Sam Ravnborg
2020-04-17 17:33 ` Rob Herring
2020-04-17 20:27 ` Linus Walleij
2020-04-19 17:12 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).