Re: [PATCH 2/4] drm/hisilicon: Code cleanup for hibmc_drv_vdac

From: Xinliang Liu <xinliang.liu@linaro.org>
To: Tian Tao <tiantao6@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	tzimmermann@suse.de, kraxel@redhat.com,
	alexander.deucher@amd.com, tglx@linutronix.de,
	dri-devel <dri-devel@lists.freedesktop.org>,
	lkml <linux-kernel@vger.kernel.org>,
	linuxarm@huawei.com
Subject: Re: [PATCH 2/4] drm/hisilicon: Code cleanup for hibmc_drv_vdac
Date: Fri, 3 Apr 2020 10:26:01 +0800	[thread overview]
Message-ID: <CAKoKPbwgiFdvfLHXoD9wPJvJ5yzMMrxNEEJUArSEvNdx46+m9Q@mail.gmail.com> (raw)
In-Reply-To: <1583466184-7060-5-git-send-email-tiantao6@hisilicon.com>

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
>