Re: [PATCH v2 4/4] drm/msm: Use exynos's model for handling component driver matching.

From: Rob Clark <robdclark@gmail.com>
To: Eric Anholt <eric@anholt.net>
Cc: "dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	David Airlie <airlied@linux.ie>, Inki Dae <inki.dae@samsung.com>,
	Joonyoung Shim <jy0922.shim@samsung.com>,
	Seung-Woo Kim <sw0312.kim@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH v2 4/4] drm/msm: Use exynos's model for handling component driver matching.
Date: Thu, 17 Sep 2015 15:19:29 -0400	[thread overview]
Message-ID: <CAF6AEGuQ91j9biBoSkrMVgtmLcP87fjgn0Rw_4DRs=JQsigpvw@mail.gmail.com> (raw)
In-Reply-To: <1442501878-9037-4-git-send-email-eric@anholt.net>

On Thu, Sep 17, 2015 at 10:57 AM, Eric Anholt <eric@anholt.net> wrote:
> This eliminates the need for the "connectors" and "gpus" nodes in the
> devicetree, which we were doing nothing connector- or gpu-specific
> with.

btw, looks pretty good.. hopefully I'll have some time to put my
kernel hat on and test this tomorrow, or worst case next week..

jfwiw, one of my TODO list items that I'm not managing to get around
to is figure out how to make component framework deal w/ nested
sub-components, since dsi-phy is already a subcomponent of dsi (which
is a subcomponent of msm), and we kinda want to split things out
further... but at any rate, this would avoid all the
noncomponent_drivers nonsense.  Anyways, not sure if this is something
other drivers need too, if so let me know and I'll CC you on patches
when I eventually get around to that..

BR,
-R

> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 65 +++++++++++++++++--------------------------
>  include/drm/drmP.h            |  2 +-
>  2 files changed, 27 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index a9b0573..4e1263b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1039,34 +1039,7 @@ static const struct dev_pm_ops msm_pm_ops = {
>   * Componentized driver support:
>   */
>
> -#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;
> -}
> -
> -static int add_components(struct device *dev, struct component_match **matchptr,
> -               const char *name)
> -{
> -       struct device_node *np = dev->of_node;
> -       unsigned i;
> -
> -       for (i = 0; ; i++) {
> -               struct device_node *node;
> -
> -               node = of_parse_phandle(np, name, i);
> -               if (!node)
> -                       break;
> -
> -               component_match_add(dev, matchptr, compare_of, node);
> -       }
> -
> -       return 0;
> -}
> -#else
> +#ifndef CONFIG_OF
>  static int compare_dev(struct device *dev, void *data)
>  {
>         return dev == data;
> @@ -1088,6 +1061,20 @@ static const struct component_master_ops msm_drm_ops = {
>         .unbind = msm_drm_unbind,
>  };
>
> +static struct platform_driver *const noncomponent_drivers[] = {
> +#if IS_ENABLED(CONFIG_DRM_MSM_DSI)
> +       &msm_dsi_phy_driver,
> +#endif
> +};
> +
> +static struct platform_driver *const component_drivers[] = {
> +#if IS_ENABLED(CONFIG_DRM_MSM_DSI)
> +       &msm_dsi_driver,
> +#endif
> +       &msm_hdmi_driver,
> +       &adreno_driver,
> +};
> +
>  /*
>   * Platform driver:
>   */
> @@ -1096,8 +1083,9 @@ static int msm_pdev_probe(struct platform_device *pdev)
>  {
>         struct component_match *match = NULL;
>  #ifdef CONFIG_OF
> -       add_components(&pdev->dev, &match, "connectors");
> -       add_components(&pdev->dev, &match, "gpus");
> +       drm_platform_component_match_add_drivers(&pdev->dev, &match,
> +                                                component_drivers,
> +                                                ARRAY_SIZE(component_drivers));
>  #else
>         /* For non-DT case, it kinda sucks.  We don't actually have a way
>          * to know whether or not we are waiting for certain devices (or if
> @@ -1159,20 +1147,17 @@ static struct platform_driver msm_platform_driver = {
>         .id_table   = msm_id,
>  };
>
> -static struct platform_driver *const component_drivers[] = {
> -#if IS_ENABLED(CONFIG_DRM_MSM_DSI)
> -       &msm_dsi_phy_driver,
> -       &msm_dsi_driver,
> -#endif
> -       &msm_hdmi_driver,
> -       &adreno_driver,
> -};
> -
>  static int __init msm_drm_register(void)
>  {
>         int ret;
>
>         DBG("init");
> +
> +       ret = drm_platform_register_drivers(noncomponent_drivers,
> +                                           ARRAY_SIZE(noncomponent_drivers));
> +       if (ret)
> +               return ret;
> +
>         ret = drm_platform_register_drivers(component_drivers,
>                                             ARRAY_SIZE(component_drivers));
>         if (ret)
> @@ -1187,6 +1172,8 @@ static void __exit msm_drm_unregister(void)
>         platform_driver_unregister(&msm_platform_driver);
>         drm_platform_unregister_drivers(component_drivers,
>                                         ARRAY_SIZE(component_drivers));
> +       drm_platform_unregister_drivers(noncomponent_drivers,
> +                                       ARRAY_SIZE(noncomponent_drivers));
>  }
>
>  module_init(msm_drm_register);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index b65d886..0da8599 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1094,7 +1094,7 @@ void drm_platform_unregister_drivers(struct platform_driver *const *drv,
>                                      int count);
>  void drm_platform_component_match_add_drivers(struct device *dev,
>                                               struct component_match **match,
> -                                             struct platform_driver **drivers,
> +                                             struct platform_driver *const *drivers,
>                                               int count);
>
>  /* returns true if currently okay to sleep */
> --
> 2.1.4
>