diff mbox series

[25/49] staging: hikey9xx/gpu: do some code cleanups

Message ID 9fa944021373ec5b82c2c1e118c15d9effe7f964.1597833138.git.mchehab+huawei@kernel.org (mailing list archive)
State New, archived
Headers show
Series DRM driver for Hikey 970 | expand

Commit Message

Mauro Carvalho Chehab Aug. 19, 2020, 11:45 a.m. UTC
- Get rid of a global var meant to store one of its priv
  structs;
- Change the name of the driver, in order to not be confused with
  the kirin6220;
- Remove some unneeded ifdef;
- use drm_of.h helper.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../staging/hikey9xx/gpu/kirin9xx_drm_drv.c   | 81 +++++++------------
 1 file changed, 30 insertions(+), 51 deletions(-)

Comments

John Stultz Aug. 20, 2020, 1:53 a.m. UTC | #1
On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
> @@ -376,7 +355,7 @@ static int kirin_drm_platform_resume(struct platform_device *pdev)
>  }
>
>  static const struct of_device_id kirin_drm_dt_ids[] = {
> -       { .compatible = "hisilicon,hi3660-dpe",
> +       { .compatible = "hisilicon,kirin960-dpe",


One issue, elsewhere in your patch stack you still refer to the
hisilicon,hi3660-dpe compatible string. This should probably be
consistent one way or the other.

thanks
-john
Mauro Carvalho Chehab Aug. 20, 2020, 8:23 a.m. UTC | #2
(added c/c Rob Herring)

Em Wed, 19 Aug 2020 18:53:06 -0700
John Stultz <john.stultz@linaro.org> escreveu:

> On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> > @@ -376,7 +355,7 @@ static int kirin_drm_platform_resume(struct platform_device *pdev)
> >  }
> >
> >  static const struct of_device_id kirin_drm_dt_ids[] = {
> > -       { .compatible = "hisilicon,hi3660-dpe",
> > +       { .compatible = "hisilicon,kirin960-dpe",  
> 
> 
> One issue, elsewhere in your patch stack you still refer to the
> hisilicon,hi3660-dpe compatible string. This should probably be
> consistent one way or the other.

Agreed with regards to consistency.

It sounds to me that calling those as Kirin 9xx (and the previous one
as Kirin 620) is better than using the part number.

Here, googling for "Kirin 970" gave about 6.9 million hits, while "Hi3670"
gave only 75,5K hits.

Kirin 620 has similar results: 6.85 million hits, against 61,9 hits
for "Hi3620".

With "Kirin 960", the numbers are a lot higher: had 21,4 million hits,
against 423K hits for "Hi3660".

So, my preference is to use "Kirin 620, 960 and 970" for future changes.

-

Currently, there are already some inconsistency, as some places
use the part number where others use "Kirin xxx" designation,
when referring to Kirin 620, 960 and 970.

I would love to make this consistent among the Kernel. However,
I'm not sure if changing "compatible" would be acceptable
by DT maintainers.

If something like that would be OK, I can prepare a separate
patchset to be applied at the Kernel.

Thanks,
Mauro
John Stultz Aug. 21, 2020, 8:12 p.m. UTC | #3
On Thu, Aug 20, 2020 at 1:23 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> (added c/c Rob Herring)
>
> Em Wed, 19 Aug 2020 18:53:06 -0700
> John Stultz <john.stultz@linaro.org> escreveu:
>
> > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab
> > <mchehab+huawei@kernel.org> wrote:
> > > @@ -376,7 +355,7 @@ static int kirin_drm_platform_resume(struct platform_device *pdev)
> > >  }
> > >
> > >  static const struct of_device_id kirin_drm_dt_ids[] = {
> > > -       { .compatible = "hisilicon,hi3660-dpe",
> > > +       { .compatible = "hisilicon,kirin960-dpe",
> >
> >
> > One issue, elsewhere in your patch stack you still refer to the
> > hisilicon,hi3660-dpe compatible string. This should probably be
> > consistent one way or the other.
>
> Agreed with regards to consistency.
>
> It sounds to me that calling those as Kirin 9xx (and the previous one
> as Kirin 620) is better than using the part number.
>
> Here, googling for "Kirin 970" gave about 6.9 million hits, while "Hi3670"
> gave only 75,5K hits.
>
> Kirin 620 has similar results: 6.85 million hits, against 61,9 hits
> for "Hi3620".

Hi6620 is kirin 620 I believe.

> With "Kirin 960", the numbers are a lot higher: had 21,4 million hits,
> against 423K hits for "Hi3660".
>
> So, my preference is to use "Kirin 620, 960 and 970" for future changes.

I think traditionally the DTS is developed with the hardware
documentation sometimes before the SoC is announced, so they tend to
stick with whatever those documents call it, rather than (later more
google-able) marketing names.

That said, I don't have a preference, as long as it's consistent, and
we don't change compatible strings that are already upstream.

> I would love to make this consistent among the Kernel. However,
> I'm not sure if changing "compatible" would be acceptable
> by DT maintainers.
>

Existing bindings are already ABI. So we can't change those. New
bindings can be set to what makes the most sense.

thanks
-john
diff mbox series

Patch

diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
index fee686760c78..cede6ccc2dd5 100644
--- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
+++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
@@ -25,20 +25,22 @@ 
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_of.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "kirin9xx_drm_drv.h"
 
-static struct kirin_dc_ops *dc_ops;
-
 static int kirin_drm_kms_cleanup(struct drm_device *dev)
 {
 	struct kirin_drm_private *priv = dev->dev_private;
+	static struct kirin_dc_ops const *dc_ops;
 
 	if (priv->fbdev)
 		priv->fbdev = NULL;
 
+	dc_ops = of_device_get_match_data(dev->dev);
+
 	drm_kms_helper_poll_fini(dev);
 	dc_ops->cleanup(dev);
 	drm_mode_config_cleanup(dev);
@@ -78,6 +80,7 @@  static void kirin_drm_mode_config_init(struct drm_device *dev)
 static int kirin_drm_kms_init(struct drm_device *dev)
 {
 	struct kirin_drm_private *priv = dev->dev_private;
+	static struct kirin_dc_ops const *dc_ops;
 	int ret;
 
 	priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
@@ -92,6 +95,7 @@  static int kirin_drm_kms_init(struct drm_device *dev)
 	kirin_drm_mode_config_init(dev);
 
 	/* display controller init */
+	dc_ops = of_device_get_match_data(dev->dev);
 	ret = dc_ops->init(dev);
 	if (ret)
 		goto err_mode_config_cleanup;
@@ -209,27 +213,17 @@  static struct drm_driver kirin_drm_driver = {
 	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap,
 	.gem_prime_mmap		= drm_gem_cma_prime_mmap,
 
-	.name			= "kirin",
-	.desc			= "Hisilicon Kirin SoCs' DRM Driver",
+	.name			= "kirin9xx",
+	.desc			= "Hisilicon Kirin9xx SoCs' DRM Driver",
 	.date			= "20170309",
 	.major			= 1,
 	.minor			= 0,
 };
 
-#ifdef CONFIG_OF
-/* NOTE: the CONFIG_OF case duplicates the same code as exynos or imx
- * (or probably any other).. so probably some room for some helpers
- */
 static int compare_of(struct device *dev, void *data)
 {
 	return dev->of_node == data;
 }
-#else
-static int compare_dev(struct device *dev, void *data)
-{
-	return dev == data;
-}
-#endif
 
 static int kirin_drm_bind(struct device *dev)
 {
@@ -288,57 +282,30 @@  static const struct component_master_ops kirin_drm_ops = {
 	.unbind = kirin_drm_unbind,
 };
 
-static struct device_node *kirin_get_remote_node(struct device_node *np)
-{
-	struct device_node *endpoint, *remote;
-
-	/* get the first endpoint, in our case only one remote node
-	 * is connected to display controller.
-	 */
-	endpoint = of_graph_get_next_endpoint(np, NULL);
-	if (!endpoint) {
-		DRM_ERROR("no valid endpoint node\n");
-		return ERR_PTR(-ENODEV);
-	}
-	of_node_put(endpoint);
-
-	remote = of_graph_get_remote_port_parent(endpoint);
-	if (!remote) {
-		DRM_ERROR("no valid remote node\n");
-		return ERR_PTR(-ENODEV);
-	}
-	of_node_put(remote);
-
-	if (!of_device_is_available(remote)) {
-		DRM_ERROR("not available for remote node\n");
-		return ERR_PTR(-ENODEV);
-	}
-
-	return remote;
-}
-
 static int kirin_drm_platform_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct component_match *match = NULL;
 	struct device_node *remote;
+	static struct kirin_dc_ops const *dc_ops;
 	int ret;
 
-	dc_ops = (struct kirin_dc_ops *)of_device_get_match_data(dev);
+	dc_ops = of_device_get_match_data(dev);
 	if (!dc_ops) {
 		DRM_ERROR("failed to get dt id data\n");
 		return -EINVAL;
 	}
 
 	DRM_INFO("the device node is %s\n", np->name);
-	remote = kirin_get_remote_node(np);
-	if (IS_ERR(remote))
-		return PTR_ERR(remote);
+	remote = of_graph_get_remote_node(np, 0, 0);
+	if (!remote)
+		return -ENODEV;
 
 	DRM_INFO("the device remote node is %s\n", remote->name);
 
-	component_match_add(dev, &match, compare_of, remote);
+	drm_of_component_match_add(dev, &match, compare_of, remote);
+	of_node_put(remote);
 
 	if (ret)
 		DRM_ERROR("cma device init failed!");
@@ -347,13 +314,20 @@  static int kirin_drm_platform_probe(struct platform_device *pdev)
 
 static int kirin_drm_platform_remove(struct platform_device *pdev)
 {
+	static struct kirin_dc_ops const *dc_ops;
+
+	dc_ops = of_device_get_match_data(&pdev->dev);
 	component_master_del(&pdev->dev, &kirin_drm_ops);
-	dc_ops = NULL;
 	return 0;
 }
 
 static int kirin_drm_platform_suspend(struct platform_device *pdev, pm_message_t state)
 {
+	static struct kirin_dc_ops const *dc_ops;
+	struct device *dev = &pdev->dev;
+
+	dc_ops = of_device_get_match_data(dev);
+
 	DRM_INFO("+. pdev->name is %s, m_message is %d \n", pdev->name, state.event);
 	if (!dc_ops) {
 		DRM_ERROR("dc_ops is NULL\n");
@@ -366,6 +340,11 @@  static int kirin_drm_platform_suspend(struct platform_device *pdev, pm_message_t
 
 static int kirin_drm_platform_resume(struct platform_device *pdev)
 {
+	static struct kirin_dc_ops const *dc_ops;
+	struct device *dev = &pdev->dev;
+
+	dc_ops = of_device_get_match_data(dev);
+
 	if (!dc_ops) {
 		DRM_ERROR("dc_ops is NULL\n");
 		return -EINVAL;
@@ -376,7 +355,7 @@  static int kirin_drm_platform_resume(struct platform_device *pdev)
 }
 
 static const struct of_device_id kirin_drm_dt_ids[] = {
-	{ .compatible = "hisilicon,hi3660-dpe",
+	{ .compatible = "hisilicon,kirin960-dpe",
 	  .data = &dss_dc_ops,
 	},
 	{ .compatible = "hisilicon,kirin970-dpe",
@@ -392,7 +371,7 @@  static struct platform_driver kirin_drm_platform_driver = {
 	.suspend = kirin_drm_platform_suspend,
 	.resume = kirin_drm_platform_resume,
 	.driver = {
-		.name = "kirin-drm",
+		.name = "kirin9xx-drm",
 		.of_match_table = kirin_drm_dt_ids,
 	},
 };