Re: [PATCH v2 3/4] drm/msm: split the main platform driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 4 Mar 2022 at 02:00, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Dmitry Baryshkov (2022-01-19 14:40:04)
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > index 06d26c5fb274..6895c056be19 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -451,10 +451,18 @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
> >
> >  #endif
> >
> > +#define KMS_MDP4 4
> > +#define KMS_MDP5 5
> > +#define KMS_DPU  3
> > +
> > +void __init msm_mdp4_register(void);
> > +void __exit msm_mdp4_unregister(void);
> >  void __init msm_mdp_register(void);
> >  void __exit msm_mdp_unregister(void);
> >  void __init msm_dpu_register(void);
> >  void __exit msm_dpu_unregister(void);
> > +void __init msm_mdss_register(void);
> > +void __exit msm_mdss_unregister(void);
>
> Don't need __init or __exit on prototypes.
>
> >
> >  #ifdef CONFIG_DEBUG_FS
> >  void msm_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m);
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index 92562221b517..759076357e0e 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -8,6 +8,8 @@
> >  #include <linux/irqdesc.h>
> >  #include <linux/irqchip/chained_irq.h>
> >
> > +#include <drm/drm_of.h>
>
> What's this include for?
>
> > +
> >  #include "msm_drv.h"
> >  #include "msm_kms.h"
> >
> > @@ -127,7 +129,7 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >         return 0;
> >  }
> >
> > -int msm_mdss_enable(struct msm_mdss *msm_mdss)
> > +static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >  {
> >         int ret;
> >
> > @@ -163,14 +165,14 @@ int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >         return ret;
> >  }
> >
> > -int msm_mdss_disable(struct msm_mdss *msm_mdss)
> > +static int msm_mdss_disable(struct msm_mdss *msm_mdss)
> >  {
> >         clk_bulk_disable_unprepare(msm_mdss->num_clocks, msm_mdss->clocks);
> >
> >         return 0;
> >  }
> >
> > -void msm_mdss_destroy(struct msm_mdss *msm_mdss)
> > +static void msm_mdss_destroy(struct msm_mdss *msm_mdss)
> >  {
> >         struct platform_device *pdev = to_platform_device(msm_mdss->dev);
> >         int irq;
> > @@ -228,7 +230,7 @@ int mdp5_mdss_parse_clock(struct platform_device *pdev, struct clk_bulk_data **c
> >         return num_clocks;
> >  }
> >
> > -struct msm_mdss *msm_mdss_init(struct platform_device *pdev, bool mdp5)
> > +static struct msm_mdss *msm_mdss_init(struct platform_device *pdev, bool mdp5)
> >  {
> >         struct msm_mdss *msm_mdss;
> >         int ret;
> > @@ -269,3 +271,171 @@ struct msm_mdss *msm_mdss_init(struct platform_device *pdev, bool mdp5)
> >
> >         return msm_mdss;
> >  }
> > +
> > +static int __maybe_unused mdss_runtime_suspend(struct device *dev)
> > +{
> > +       struct msm_drm_private *priv = dev_get_drvdata(dev);
> > +
> > +       DBG("");
> > +
> > +       return msm_mdss_disable(priv->mdss);
> > +}
> > +
> > +static int __maybe_unused mdss_runtime_resume(struct device *dev)
> > +{
> > +       struct msm_drm_private *priv = dev_get_drvdata(dev);
> > +
> > +       DBG("");
> > +
> > +       return msm_mdss_enable(priv->mdss);
> > +}
> > +
> > +static int __maybe_unused mdss_pm_suspend(struct device *dev)
> > +{
> > +
> > +       if (pm_runtime_suspended(dev))
> > +               return 0;
> > +
> > +       return mdss_runtime_suspend(dev);
> > +}
> > +
> > +static int __maybe_unused mdss_pm_resume(struct device *dev)
> > +{
> > +       if (pm_runtime_suspended(dev))
> > +               return 0;
> > +
> > +       return mdss_runtime_resume(dev);
> > +}
> > +
> > +static const struct dev_pm_ops mdss_pm_ops = {
> > +       SET_SYSTEM_SLEEP_PM_OPS(mdss_pm_suspend, mdss_pm_resume)
> > +       SET_RUNTIME_PM_OPS(mdss_runtime_suspend, mdss_runtime_resume, NULL)
> > +       .prepare = msm_pm_prepare,
> > +       .complete = msm_pm_complete,
> > +};
> > +
> > +static int get_mdp_ver(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +
> > +       return (int) (unsigned long) of_device_get_match_data(dev);
> > +}
> > +
> > +static int find_mdp_node(struct device *dev, void *data)
> > +{
> > +       return of_match_node(dpu_dt_match, dev->of_node) ||
> > +               of_match_node(mdp5_dt_match, dev->of_node);
> > +}
> > +
> > +static int mdss_probe(struct platform_device *pdev)
> > +{
> > +       struct msm_mdss *mdss;
> > +       struct msm_drm_private *priv;
> > +       int mdp_ver = get_mdp_ver(pdev);
> > +       struct device *mdp_dev;
> > +       struct device *dev = &pdev->dev;
> > +       int ret;
> > +
> > +       if (mdp_ver != KMS_MDP5 && mdp_ver != KMS_DPU)
> > +               return -EINVAL;
>
> Is it possible anymore? Now that the driver is split it seems like no.

Yes, I'll drop this.

>
> > +
> > +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       platform_set_drvdata(pdev, priv);
> > +
> > +       mdss = msm_mdss_init(pdev, mdp_ver == KMS_MDP5);
> > +       if (IS_ERR(mdss)) {
> > +               ret = PTR_ERR(mdss);
> > +               platform_set_drvdata(pdev, NULL);
> > +
> > +               return ret;
> > +       }
> > +
> > +       priv->mdss = mdss;
> > +       pm_runtime_enable(&pdev->dev);
> > +
> > +       /*
> > +        * MDP5/DPU based devices don't have a flat hierarchy. There is a top
> > +        * level parent: MDSS, and children: MDP5/DPU, DSI, HDMI, eDP etc.
> > +        * Populate the children devices, find the MDP5/DPU node, and then add
> > +        * the interfaces to our components list.
> > +        */
> > +       ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> > +       if (ret) {
> > +               DRM_DEV_ERROR(dev, "failed to populate children devices\n");
> > +               goto fail;
> > +       }
> > +
> > +       mdp_dev = device_find_child(dev, NULL, find_mdp_node);
> > +       if (!mdp_dev) {
> > +               DRM_DEV_ERROR(dev, "failed to find MDSS MDP node\n");
> > +               of_platform_depopulate(dev);
> > +               ret = -ENODEV;
> > +               goto fail;
> > +       }
> > +
> > +       /*
> > +        * on MDP5 based platforms, the MDSS platform device is the component
> > +        * master that adds MDP5 and other display interface components to
>
> s/master//
>
> > +        * itself.
> > +        */
> > +       ret = msm_drv_probe(dev, mdp_dev);
> > +       put_device(mdp_dev);
> > +       if (ret)
> > +               goto fail;
> > +
> > +       return 0;
> > +
> > +fail:
> > +       of_platform_depopulate(dev);
> > +       msm_mdss_destroy(priv->mdss);
> > +
> > +       return ret;
> > +}
> > +
> > +static int mdss_remove(struct platform_device *pdev)
> > +{
> > +       struct msm_drm_private *priv = platform_get_drvdata(pdev);
> > +       struct msm_mdss *mdss = priv->mdss;
> > +
> > +       component_master_del(&pdev->dev, &msm_drm_ops);
> > +       of_platform_depopulate(&pdev->dev);
> > +
> > +       msm_mdss_destroy(mdss);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id mdss_dt_match[] = {
> > +       { .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
> > +       { .compatible = "qcom,sdm845-mdss", .data = (void *)KMS_DPU },
> > +       { .compatible = "qcom,sc7180-mdss", .data = (void *)KMS_DPU },
> > +       { .compatible = "qcom,sc7280-mdss", .data = (void *)KMS_DPU },
> > +       { .compatible = "qcom,sm8150-mdss", .data = (void *)KMS_DPU },
> > +       { .compatible = "qcom,sm8250-mdss", .data = (void *)KMS_DPU },
> > +       {}
> > +};
> > +MODULE_DEVICE_TABLE(of, dt_match);
> > +
> > +static struct platform_driver mdss_platform_driver = {
> > +       .probe      = mdss_probe,
> > +       .remove     = mdss_remove,
> > +       .shutdown   = msm_drv_shutdown,
> > +       .driver     = {
> > +               .name   = "msm-mdss",
> > +               .of_match_table = mdss_dt_match,
> > +               .pm     = &mdss_pm_ops,
> > +       },
> > +};
> > +
> > +void __init msm_mdss_register(void)
> > +{
> > +       platform_driver_register(&mdss_platform_driver);
>
> I'd like to go a step further and not even compile drivers in that we
> don't use. Can we get some Kconfigs for these new drivers so that the
> number of drivers registered with the system is reduced and memory is
> conserved?

Sure. I'll add them, but in the separate commit, if you don't mind.



-- 
With best wishes
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux