[2/4] drm/hisilicon: Code cleanup for hibmc_drv_vdac
diff mbox series

Message ID 1583466184-7060-5-git-send-email-tiantao6@hisilicon.com
State New, archived
Headers show
Series
  • [1/4] drm/hisilicon: Enforce 128-byte stride alignment to fix the hardware limitation
Related show

Commit Message

Tian Tao March 6, 2020, 3:43 a.m. UTC
code cleanup for hibmc_drv_vdac.c, no actual function changes.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
Signed-off-by: Gong junjie <gongjunjie2@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 49 ++++++++----------------
 1 file changed, 16 insertions(+), 33 deletions(-)

Comments

Xinliang Liu April 3, 2020, 2:26 a.m. UTC | #1
Hi Tao,

On Fri, 6 Mar 2020 at 11:44, Tian Tao <tiantao6@hisilicon.com> wrote:
>
> code cleanup for hibmc_drv_vdac.c, no actual function changes.
>
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> Signed-off-by: Gong junjie <gongjunjie2@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 49 ++++++++----------------
>  1 file changed, 16 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index 678ac2e..f0e6bb8 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -52,32 +52,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
>         .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>
> -static struct drm_connector *
> -hibmc_connector_init(struct hibmc_drm_private *priv)
> -{
> -       struct drm_device *dev = priv->dev;
> -       struct drm_connector *connector;
> -       int ret;
> -
> -       connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
> -       if (!connector) {
> -               DRM_ERROR("failed to alloc memory when init connector\n");
> -               return ERR_PTR(-ENOMEM);
> -       }
> -
> -       ret = drm_connector_init(dev, connector,
> -                                &hibmc_connector_funcs,
> -                                DRM_MODE_CONNECTOR_VGA);
> -       if (ret) {
> -               DRM_ERROR("failed to init connector: %d\n", ret);
> -               return ERR_PTR(ret);
> -       }
> -       drm_connector_helper_add(connector,
> -                                &hibmc_connector_helper_funcs);
> -
> -       return connector;
> -}
> -
>  static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
>                                    struct drm_display_mode *mode,
>                                    struct drm_display_mode *adj_mode)
> @@ -109,13 +83,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>         struct drm_connector *connector;
>         int ret;
>
> -       connector = hibmc_connector_init(priv);
> -       if (IS_ERR(connector)) {
> -               DRM_ERROR("failed to create connector: %ld\n",
> -                         PTR_ERR(connector));
> -               return PTR_ERR(connector);
> -       }
> -
>         encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
>         if (!encoder) {
>                 DRM_ERROR("failed to alloc memory when init encoder\n");
> @@ -131,6 +98,22 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>         }
>
>         drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
> +       connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
> +       if (!connector) {
> +               DRM_ERROR("failed to alloc memory when init connector\n");
> +               return -ENOMEM;
> +       }
> +
> +       ret = drm_connector_init(dev, connector,
> +                                &hibmc_connector_funcs,
> +                                DRM_MODE_CONNECTOR_VGA);
> +       if (ret) {
> +               DRM_ERROR("failed to init connector: %d\n", ret);
> +               return ret;
> +       }
> +
> +       drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
> +       drm_connector_register(connector);

You don't need to register a non-hotplug connector as it will be
registered at drm_dev_register automatically.

See function definition:

 488 /**
 489  * drm_connector_register - register a connector
 490  * @connector: the connector to register
 491  *
 492  * Register userspace interfaces for a connector. Only call this
for connectors
 493  * which can be hotplugged after drm_dev_register() has been
called already,
 494  * e.g. DP MST connectors. All other connectors will be
registered automatically
 495  * when calling drm_dev_register().
 496  *
 497  * Returns:
 498  * Zero on success, error code on failure.
 499  */
 500 int drm_connector_register(struct drm_connector *connector)
 501 {


Besides, I don't think this patch cleans much things.

-Xinliang

>         drm_connector_attach_encoder(connector, encoder);
>
>         return 0;
> --
> 2.7.4
>
Thomas Zimmermann April 3, 2020, 7 a.m. UTC | #2
Hi

Am 03.04.20 um 04:26 schrieb Xinliang Liu:
> Hi Tao,
> 
> On Fri, 6 Mar 2020 at 11:44, Tian Tao <tiantao6@hisilicon.com> wrote:
>>
>> code cleanup for hibmc_drv_vdac.c, no actual function changes.
>>
>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>> Signed-off-by: Gong junjie <gongjunjie2@huawei.com>
>> ---
>>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 49 ++++++++----------------
>>  1 file changed, 16 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> index 678ac2e..f0e6bb8 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> @@ -52,32 +52,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
>>         .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>  };
>>
>> -static struct drm_connector *
>> -hibmc_connector_init(struct hibmc_drm_private *priv)
>> -{
>> -       struct drm_device *dev = priv->dev;
>> -       struct drm_connector *connector;
>> -       int ret;
>> -
>> -       connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
>> -       if (!connector) {
>> -               DRM_ERROR("failed to alloc memory when init connector\n");
>> -               return ERR_PTR(-ENOMEM);
>> -       }
>> -
>> -       ret = drm_connector_init(dev, connector,
>> -                                &hibmc_connector_funcs,
>> -                                DRM_MODE_CONNECTOR_VGA);
>> -       if (ret) {
>> -               DRM_ERROR("failed to init connector: %d\n", ret);
>> -               return ERR_PTR(ret);
>> -       }
>> -       drm_connector_helper_add(connector,
>> -                                &hibmc_connector_helper_funcs);
>> -
>> -       return connector;
>> -}
>> -
>>  static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
>>                                    struct drm_display_mode *mode,
>>                                    struct drm_display_mode *adj_mode)
>> @@ -109,13 +83,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>         struct drm_connector *connector;
>>         int ret;
>>
>> -       connector = hibmc_connector_init(priv);
>> -       if (IS_ERR(connector)) {
>> -               DRM_ERROR("failed to create connector: %ld\n",
>> -                         PTR_ERR(connector));
>> -               return PTR_ERR(connector);
>> -       }
>> -
>>         encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
>>         if (!encoder) {
>>                 DRM_ERROR("failed to alloc memory when init encoder\n");
>> @@ -131,6 +98,22 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>         }
>>
>>         drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
>> +       connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
>> +       if (!connector) {
>> +               DRM_ERROR("failed to alloc memory when init connector\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       ret = drm_connector_init(dev, connector,
>> +                                &hibmc_connector_funcs,
>> +                                DRM_MODE_CONNECTOR_VGA);
>> +       if (ret) {
>> +               DRM_ERROR("failed to init connector: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
>> +       drm_connector_register(connector);
> 
> You don't need to register a non-hotplug connector as it will be
> registered at drm_dev_register automatically.
> 
> See function definition:
> 
>  488 /**
>  489  * drm_connector_register - register a connector
>  490  * @connector: the connector to register
>  491  *
>  492  * Register userspace interfaces for a connector. Only call this
> for connectors
>  493  * which can be hotplugged after drm_dev_register() has been
> called already,
>  494  * e.g. DP MST connectors. All other connectors will be
> registered automatically
>  495  * when calling drm_dev_register().
>  496  *
>  497  * Returns:
>  498  * Zero on success, error code on failure.
>  499  */
>  500 int drm_connector_register(struct drm_connector *connector)
>  501 {
> 
> 
> Besides, I don't think this patch cleans much things.

True. Although the auto-kfree of devm_kzalloc() does not work correctly
with DRM and should be replaced.

I'd like to suggest to either allocate the connector as part of struct
hibmc_drm_private, or implement drmm_connector_create() on top of DRM's
new auto-cleanup functionality.

Best regards
Thomas

> 
> -Xinliang
> 
>>         drm_connector_attach_encoder(connector, encoder);
>>
>>         return 0;
>> --
>> 2.7.4
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index 678ac2e..f0e6bb8 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -52,32 +52,6 @@  static const struct drm_connector_funcs hibmc_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static struct drm_connector *
-hibmc_connector_init(struct hibmc_drm_private *priv)
-{
-	struct drm_device *dev = priv->dev;
-	struct drm_connector *connector;
-	int ret;
-
-	connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
-	if (!connector) {
-		DRM_ERROR("failed to alloc memory when init connector\n");
-		return ERR_PTR(-ENOMEM);
-	}
-
-	ret = drm_connector_init(dev, connector,
-				 &hibmc_connector_funcs,
-				 DRM_MODE_CONNECTOR_VGA);
-	if (ret) {
-		DRM_ERROR("failed to init connector: %d\n", ret);
-		return ERR_PTR(ret);
-	}
-	drm_connector_helper_add(connector,
-				 &hibmc_connector_helper_funcs);
-
-	return connector;
-}
-
 static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
 				   struct drm_display_mode *mode,
 				   struct drm_display_mode *adj_mode)
@@ -109,13 +83,6 @@  int hibmc_vdac_init(struct hibmc_drm_private *priv)
 	struct drm_connector *connector;
 	int ret;
 
-	connector = hibmc_connector_init(priv);
-	if (IS_ERR(connector)) {
-		DRM_ERROR("failed to create connector: %ld\n",
-			  PTR_ERR(connector));
-		return PTR_ERR(connector);
-	}
-
 	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
 	if (!encoder) {
 		DRM_ERROR("failed to alloc memory when init encoder\n");
@@ -131,6 +98,22 @@  int hibmc_vdac_init(struct hibmc_drm_private *priv)
 	}
 
 	drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
+	connector = devm_kzalloc(dev->dev, sizeof(*connector), GFP_KERNEL);
+	if (!connector) {
+		DRM_ERROR("failed to alloc memory when init connector\n");
+		return -ENOMEM;
+	}
+
+	ret = drm_connector_init(dev, connector,
+				 &hibmc_connector_funcs,
+				 DRM_MODE_CONNECTOR_VGA);
+	if (ret) {
+		DRM_ERROR("failed to init connector: %d\n", ret);
+		return ret;
+	}
+
+	drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
+	drm_connector_register(connector);
 	drm_connector_attach_encoder(connector, encoder);
 
 	return 0;