| Message ID | 20220620213054.1872954-4-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm: move resource allocation to the _probe function | expand |
On 6/20/2022 2:30 PM, Dmitry Baryshkov wrote: > To let the probe function bail early if any of the resources is > unavailable, move resource allocattion from kms_init directly to the > probe callback. While we are at it, replace irq_of_parse_and_map() with > platform_get_irq(). > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 107 +++++++++++------------ > 1 file changed, 51 insertions(+), 56 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > index 41dc60784847..6499713eccf6 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > @@ -139,8 +139,6 @@ static void mdp4_destroy(struct msm_kms *kms) > pm_runtime_disable(dev); > > mdp_kms_destroy(&mdp4_kms->base); > - > - kfree(mdp4_kms); > } > > static const struct mdp_kms_funcs kms_funcs = { > @@ -383,57 +381,27 @@ static int mdp4_kms_init(struct drm_device *dev) > { > struct platform_device *pdev = to_platform_device(dev->dev); > struct msm_drm_private *priv = dev->dev_private; > - struct mdp4_kms *mdp4_kms; > + struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms)); > struct msm_kms *kms = NULL; > struct iommu_domain *iommu; > struct msm_gem_address_space *aspace; > - int irq, ret; > + int ret; > u32 major, minor; > unsigned long max_clk; > > /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */ > max_clk = 266667000; > > - mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL); > - if (!mdp4_kms) { > - DRM_DEV_ERROR(dev->dev, "failed to allocate kms\n"); > - return -ENOMEM; > - } > - > ret = mdp_kms_init(&mdp4_kms->base, &kms_funcs); > if (ret) { > DRM_DEV_ERROR(dev->dev, "failed to init kms\n"); > goto fail; > } > > - priv->kms = &mdp4_kms->base.base; > kms = priv->kms; > > mdp4_kms->dev = dev; > > - mdp4_kms->mmio = msm_ioremap(pdev, NULL); > - if (IS_ERR(mdp4_kms->mmio)) { > - ret = PTR_ERR(mdp4_kms->mmio); > - goto fail; > - } > - > - irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - ret = irq; > - DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret); > - goto fail; > - } > - > - kms->irq = irq; > - > - /* NOTE: driver for this regulator still missing upstream.. use > - * _get_exclusive() and ignore the error if it does not exist > - * (and hope that the bootloader left it on for us) > - */ > - mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); > - if (IS_ERR(mdp4_kms->vdd)) > - mdp4_kms->vdd = NULL; > - > if (mdp4_kms->vdd) { > ret = regulator_enable(mdp4_kms->vdd); > if (ret) { > @@ -442,24 +410,6 @@ static int mdp4_kms_init(struct drm_device *dev) > } > } > > - mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); > - if (IS_ERR(mdp4_kms->clk)) { > - DRM_DEV_ERROR(dev->dev, "failed to get core_clk\n"); > - ret = PTR_ERR(mdp4_kms->clk); > - goto fail; > - } > - > - mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); > - if (IS_ERR(mdp4_kms->pclk)) > - mdp4_kms->pclk = NULL; > - > - mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); > - if (IS_ERR(mdp4_kms->axi_clk)) { > - DRM_DEV_ERROR(dev->dev, "failed to get axi_clk\n"); > - ret = PTR_ERR(mdp4_kms->axi_clk); > - goto fail; > - } > - > clk_set_rate(mdp4_kms->clk, max_clk); > > read_mdp_hw_revision(mdp4_kms, &major, &minor); > @@ -474,10 +424,9 @@ static int mdp4_kms_init(struct drm_device *dev) > mdp4_kms->rev = minor; > > if (mdp4_kms->rev >= 2) { > - mdp4_kms->lut_clk = devm_clk_get(&pdev->dev, "lut_clk"); > - if (IS_ERR(mdp4_kms->lut_clk)) { > + if (!mdp4_kms->lut_clk) { > DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n"); > - ret = PTR_ERR(mdp4_kms->lut_clk); > + ret = -ENODEV; > goto fail; > } > clk_set_rate(mdp4_kms->lut_clk, max_clk); > @@ -560,7 +509,53 @@ static const struct dev_pm_ops mdp4_pm_ops = { > > static int mdp4_probe(struct platform_device *pdev) > { > - return msm_drv_probe(&pdev->dev, mdp4_kms_init, NULL); > + struct device *dev = &pdev->dev; > + struct mdp4_kms *mdp4_kms; > + int irq; > + > + mdp4_kms = devm_kzalloc(dev, sizeof(*mdp4_kms), GFP_KERNEL); > + if (!mdp4_kms) > + return dev_err_probe(dev, -ENOMEM, "failed to allocate kms\n"); > + > + mdp4_kms->mmio = msm_ioremap(pdev, NULL); > + if (IS_ERR(mdp4_kms->mmio)) > + return PTR_ERR(mdp4_kms->mmio); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return dev_err_probe(dev, irq, "failed to get irq\n"); > + > + mdp4_kms->base.base.irq = irq; > + > + /* NOTE: driver for this regulator still missing upstream.. use > + * _get_exclusive() and ignore the error if it does not exist > + * (and hope that the bootloader left it on for us) > + */ > + mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); > + if (IS_ERR(mdp4_kms->vdd)) > + mdp4_kms->vdd = NULL; > + > + mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); > + if (IS_ERR(mdp4_kms->clk)) > + return dev_err_probe(dev, PTR_ERR(mdp4_kms->clk), "failed to get core_clk\n"); > + > + mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); > + if (IS_ERR(mdp4_kms->pclk)) > + mdp4_kms->pclk = NULL; > + > + mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); > + if (IS_ERR(mdp4_kms->axi_clk)) > + return dev_err_probe(dev, PTR_ERR(mdp4_kms->axi_clk), "failed to get axi_clk\n"); > + > + /* > + * This is required for revn >= 2. Handle errors here and let the kms > + * init bail out if the clock is not provided. > + */ > + mdp4_kms->lut_clk = devm_clk_get_optional(&pdev->dev, "lut_clk"); > + if (IS_ERR(mdp4_kms->lut_clk)) > + return dev_err_probe(dev, PTR_ERR(mdp4_kms->lut_clk), "failed to get lut_clk\n"); I can see that you have moved this from init to probe and only rev >=2 needs it. But, your check here will end up returning from probe because you have a return. So I guess you means just having dev_err_probe without the return and let the init fail if the clk is not found because we have the hw_rev only in init. > + > + return msm_drv_probe(&pdev->dev, mdp4_kms_init, &mdp4_kms->base.base); > } > > static int mdp4_remove(struct platform_device *pdev)
On 2 September 2022 03:24:17 GMT+03:00, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > >On 6/20/2022 2:30 PM, Dmitry Baryshkov wrote: >> To let the probe function bail early if any of the resources is >> unavailable, move resource allocattion from kms_init directly to the >> probe callback. While we are at it, replace irq_of_parse_and_map() with >> platform_get_irq(). >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 107 +++++++++++------------ >> 1 file changed, 51 insertions(+), 56 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c >> index 41dc60784847..6499713eccf6 100644 >> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c >> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c >> @@ -139,8 +139,6 @@ static void mdp4_destroy(struct msm_kms *kms) >> pm_runtime_disable(dev); >> mdp_kms_destroy(&mdp4_kms->base); >> - >> - kfree(mdp4_kms); >> } >> static const struct mdp_kms_funcs kms_funcs = { >> @@ -383,57 +381,27 @@ static int mdp4_kms_init(struct drm_device *dev) >> { >> struct platform_device *pdev = to_platform_device(dev->dev); >> struct msm_drm_private *priv = dev->dev_private; >> - struct mdp4_kms *mdp4_kms; >> + struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms)); >> struct msm_kms *kms = NULL; >> struct iommu_domain *iommu; >> struct msm_gem_address_space *aspace; >> - int irq, ret; >> + int ret; >> u32 major, minor; >> unsigned long max_clk; >> /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */ >> max_clk = 266667000; >> - mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL); >> - if (!mdp4_kms) { >> - DRM_DEV_ERROR(dev->dev, "failed to allocate kms\n"); >> - return -ENOMEM; >> - } >> - >> ret = mdp_kms_init(&mdp4_kms->base, &kms_funcs); >> if (ret) { >> DRM_DEV_ERROR(dev->dev, "failed to init kms\n"); >> goto fail; >> } >> - priv->kms = &mdp4_kms->base.base; >> kms = priv->kms; >> mdp4_kms->dev = dev; >> - mdp4_kms->mmio = msm_ioremap(pdev, NULL); >> - if (IS_ERR(mdp4_kms->mmio)) { >> - ret = PTR_ERR(mdp4_kms->mmio); >> - goto fail; >> - } >> - >> - irq = platform_get_irq(pdev, 0); >> - if (irq < 0) { >> - ret = irq; >> - DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret); >> - goto fail; >> - } >> - >> - kms->irq = irq; >> - >> - /* NOTE: driver for this regulator still missing upstream.. use >> - * _get_exclusive() and ignore the error if it does not exist >> - * (and hope that the bootloader left it on for us) >> - */ >> - mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); >> - if (IS_ERR(mdp4_kms->vdd)) >> - mdp4_kms->vdd = NULL; >> - >> if (mdp4_kms->vdd) { >> ret = regulator_enable(mdp4_kms->vdd); >> if (ret) { >> @@ -442,24 +410,6 @@ static int mdp4_kms_init(struct drm_device *dev) >> } >> } >> - mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); >> - if (IS_ERR(mdp4_kms->clk)) { >> - DRM_DEV_ERROR(dev->dev, "failed to get core_clk\n"); >> - ret = PTR_ERR(mdp4_kms->clk); >> - goto fail; >> - } >> - >> - mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); >> - if (IS_ERR(mdp4_kms->pclk)) >> - mdp4_kms->pclk = NULL; >> - >> - mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); >> - if (IS_ERR(mdp4_kms->axi_clk)) { >> - DRM_DEV_ERROR(dev->dev, "failed to get axi_clk\n"); >> - ret = PTR_ERR(mdp4_kms->axi_clk); >> - goto fail; >> - } >> - >> clk_set_rate(mdp4_kms->clk, max_clk); >> read_mdp_hw_revision(mdp4_kms, &major, &minor); >> @@ -474,10 +424,9 @@ static int mdp4_kms_init(struct drm_device *dev) >> mdp4_kms->rev = minor; >> if (mdp4_kms->rev >= 2) { >> - mdp4_kms->lut_clk = devm_clk_get(&pdev->dev, "lut_clk"); >> - if (IS_ERR(mdp4_kms->lut_clk)) { >> + if (!mdp4_kms->lut_clk) { >> DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n"); >> - ret = PTR_ERR(mdp4_kms->lut_clk); >> + ret = -ENODEV; >> goto fail; >> } >> clk_set_rate(mdp4_kms->lut_clk, max_clk); >> @@ -560,7 +509,53 @@ static const struct dev_pm_ops mdp4_pm_ops = { >> static int mdp4_probe(struct platform_device *pdev) >> { >> - return msm_drv_probe(&pdev->dev, mdp4_kms_init, NULL); >> + struct device *dev = &pdev->dev; >> + struct mdp4_kms *mdp4_kms; >> + int irq; >> + >> + mdp4_kms = devm_kzalloc(dev, sizeof(*mdp4_kms), GFP_KERNEL); >> + if (!mdp4_kms) >> + return dev_err_probe(dev, -ENOMEM, "failed to allocate kms\n"); >> + >> + mdp4_kms->mmio = msm_ioremap(pdev, NULL); >> + if (IS_ERR(mdp4_kms->mmio)) >> + return PTR_ERR(mdp4_kms->mmio); >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) >> + return dev_err_probe(dev, irq, "failed to get irq\n"); >> + >> + mdp4_kms->base.base.irq = irq; >> + >> + /* NOTE: driver for this regulator still missing upstream.. use >> + * _get_exclusive() and ignore the error if it does not exist >> + * (and hope that the bootloader left it on for us) >> + */ >> + mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); >> + if (IS_ERR(mdp4_kms->vdd)) >> + mdp4_kms->vdd = NULL; >> + >> + mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); >> + if (IS_ERR(mdp4_kms->clk)) >> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->clk), "failed to get core_clk\n"); >> + >> + mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); >> + if (IS_ERR(mdp4_kms->pclk)) >> + mdp4_kms->pclk = NULL; >> + >> + mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); >> + if (IS_ERR(mdp4_kms->axi_clk)) >> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->axi_clk), "failed to get axi_clk\n"); >> + >> + /* >> + * This is required for revn >= 2. Handle errors here and let the kms >> + * init bail out if the clock is not provided. >> + */ >> + mdp4_kms->lut_clk = devm_clk_get_optional(&pdev->dev, "lut_clk"); >> + if (IS_ERR(mdp4_kms->lut_clk)) >> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->lut_clk), "failed to get lut_clk\n"); > >I can see that you have moved this from init to probe and only rev >=2 needs it. > >But, your check here will end up returning from probe because you have a return. So I guess you means just having dev_err_probe without the return and let the init fail if the clk is not found because we have the hw_rev only in init. No. The function called here is the devm_clk_get_optional(). So the driver will get NULL if the clock is not present in the DT and an error only in an error case (e.g. EINVAL, EPROBE_DEFER). Later on the mdp4_kms_init() will check hw_rev and return -ENODEV if the clock is required, but is set to NULL (not present in DT). > >> + >> + return msm_drv_probe(&pdev->dev, mdp4_kms_init, &mdp4_kms->base.base); >> } >> static int mdp4_remove(struct platform_device *pdev)
On 9/1/2022 11:06 PM, Dmitry Baryshkov wrote: > > > On 2 September 2022 03:24:17 GMT+03:00, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> On 6/20/2022 2:30 PM, Dmitry Baryshkov wrote: >>> To let the probe function bail early if any of the resources is >>> unavailable, move resource allocattion from kms_init directly to the >>> probe callback. While we are at it, replace irq_of_parse_and_map() with >>> platform_get_irq(). >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 107 +++++++++++------------ >>> 1 file changed, 51 insertions(+), 56 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c >>> index 41dc60784847..6499713eccf6 100644 >>> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c >>> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c >>> @@ -139,8 +139,6 @@ static void mdp4_destroy(struct msm_kms *kms) >>> pm_runtime_disable(dev); >>> mdp_kms_destroy(&mdp4_kms->base); >>> - >>> - kfree(mdp4_kms); >>> } >>> static const struct mdp_kms_funcs kms_funcs = { >>> @@ -383,57 +381,27 @@ static int mdp4_kms_init(struct drm_device *dev) >>> { >>> struct platform_device *pdev = to_platform_device(dev->dev); >>> struct msm_drm_private *priv = dev->dev_private; >>> - struct mdp4_kms *mdp4_kms; >>> + struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms)); >>> struct msm_kms *kms = NULL; >>> struct iommu_domain *iommu; >>> struct msm_gem_address_space *aspace; >>> - int irq, ret; >>> + int ret; >>> u32 major, minor; >>> unsigned long max_clk; >>> /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */ >>> max_clk = 266667000; >>> - mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL); >>> - if (!mdp4_kms) { >>> - DRM_DEV_ERROR(dev->dev, "failed to allocate kms\n"); >>> - return -ENOMEM; >>> - } >>> - >>> ret = mdp_kms_init(&mdp4_kms->base, &kms_funcs); >>> if (ret) { >>> DRM_DEV_ERROR(dev->dev, "failed to init kms\n"); >>> goto fail; >>> } >>> - priv->kms = &mdp4_kms->base.base; >>> kms = priv->kms; >>> mdp4_kms->dev = dev; >>> - mdp4_kms->mmio = msm_ioremap(pdev, NULL); >>> - if (IS_ERR(mdp4_kms->mmio)) { >>> - ret = PTR_ERR(mdp4_kms->mmio); >>> - goto fail; >>> - } >>> - >>> - irq = platform_get_irq(pdev, 0); >>> - if (irq < 0) { >>> - ret = irq; >>> - DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret); >>> - goto fail; >>> - } >>> - >>> - kms->irq = irq; >>> - >>> - /* NOTE: driver for this regulator still missing upstream.. use >>> - * _get_exclusive() and ignore the error if it does not exist >>> - * (and hope that the bootloader left it on for us) >>> - */ >>> - mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); >>> - if (IS_ERR(mdp4_kms->vdd)) >>> - mdp4_kms->vdd = NULL; >>> - >>> if (mdp4_kms->vdd) { >>> ret = regulator_enable(mdp4_kms->vdd); >>> if (ret) { >>> @@ -442,24 +410,6 @@ static int mdp4_kms_init(struct drm_device *dev) >>> } >>> } >>> - mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); >>> - if (IS_ERR(mdp4_kms->clk)) { >>> - DRM_DEV_ERROR(dev->dev, "failed to get core_clk\n"); >>> - ret = PTR_ERR(mdp4_kms->clk); >>> - goto fail; >>> - } >>> - >>> - mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); >>> - if (IS_ERR(mdp4_kms->pclk)) >>> - mdp4_kms->pclk = NULL; >>> - >>> - mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); >>> - if (IS_ERR(mdp4_kms->axi_clk)) { >>> - DRM_DEV_ERROR(dev->dev, "failed to get axi_clk\n"); >>> - ret = PTR_ERR(mdp4_kms->axi_clk); >>> - goto fail; >>> - } >>> - >>> clk_set_rate(mdp4_kms->clk, max_clk); >>> read_mdp_hw_revision(mdp4_kms, &major, &minor); >>> @@ -474,10 +424,9 @@ static int mdp4_kms_init(struct drm_device *dev) >>> mdp4_kms->rev = minor; >>> if (mdp4_kms->rev >= 2) { >>> - mdp4_kms->lut_clk = devm_clk_get(&pdev->dev, "lut_clk"); >>> - if (IS_ERR(mdp4_kms->lut_clk)) { >>> + if (!mdp4_kms->lut_clk) { >>> DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n"); >>> - ret = PTR_ERR(mdp4_kms->lut_clk); >>> + ret = -ENODEV; >>> goto fail; >>> } >>> clk_set_rate(mdp4_kms->lut_clk, max_clk); >>> @@ -560,7 +509,53 @@ static const struct dev_pm_ops mdp4_pm_ops = { >>> static int mdp4_probe(struct platform_device *pdev) >>> { >>> - return msm_drv_probe(&pdev->dev, mdp4_kms_init, NULL); >>> + struct device *dev = &pdev->dev; >>> + struct mdp4_kms *mdp4_kms; >>> + int irq; >>> + >>> + mdp4_kms = devm_kzalloc(dev, sizeof(*mdp4_kms), GFP_KERNEL); >>> + if (!mdp4_kms) >>> + return dev_err_probe(dev, -ENOMEM, "failed to allocate kms\n"); >>> + >>> + mdp4_kms->mmio = msm_ioremap(pdev, NULL); >>> + if (IS_ERR(mdp4_kms->mmio)) >>> + return PTR_ERR(mdp4_kms->mmio); >>> + >>> + irq = platform_get_irq(pdev, 0); >>> + if (irq < 0) >>> + return dev_err_probe(dev, irq, "failed to get irq\n"); >>> + >>> + mdp4_kms->base.base.irq = irq; >>> + >>> + /* NOTE: driver for this regulator still missing upstream.. use >>> + * _get_exclusive() and ignore the error if it does not exist >>> + * (and hope that the bootloader left it on for us) >>> + */ >>> + mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); >>> + if (IS_ERR(mdp4_kms->vdd)) >>> + mdp4_kms->vdd = NULL; >>> + >>> + mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); >>> + if (IS_ERR(mdp4_kms->clk)) >>> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->clk), "failed to get core_clk\n"); >>> + >>> + mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); >>> + if (IS_ERR(mdp4_kms->pclk)) >>> + mdp4_kms->pclk = NULL; >>> + >>> + mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); >>> + if (IS_ERR(mdp4_kms->axi_clk)) >>> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->axi_clk), "failed to get axi_clk\n"); >>> + >>> + /* >>> + * This is required for revn >= 2. Handle errors here and let the kms >>> + * init bail out if the clock is not provided. >>> + */ >>> + mdp4_kms->lut_clk = devm_clk_get_optional(&pdev->dev, "lut_clk"); >>> + if (IS_ERR(mdp4_kms->lut_clk)) >>> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->lut_clk), "failed to get lut_clk\n"); >> >> I can see that you have moved this from init to probe and only rev >=2 needs it. >> >> But, your check here will end up returning from probe because you have a return. So I guess you means just having dev_err_probe without the return and let the init fail if the clk is not found because we have the hw_rev only in init. > > No. The function called here is the devm_clk_get_optional(). So the driver will get NULL if the clock is not present in the DT and an error only in an error case (e.g. EINVAL, EPROBE_DEFER). > > Later on the mdp4_kms_init() will check hw_rev and return -ENODEV if the clock is required, but is set to NULL (not present in DT). > Ok, I have understood. But dont you think this is too much convolution just for this check? Why not leave the lut_clk in the init instead of trying to move it to probe and add all this? > >> >>> + >>> + return msm_drv_probe(&pdev->dev, mdp4_kms_init, &mdp4_kms->base.base); >>> } >>> static int mdp4_remove(struct platform_device *pdev) >
On Fri, 2 Sept 2022 at 21:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 9/1/2022 11:06 PM, Dmitry Baryshkov wrote: > > > > > > On 2 September 2022 03:24:17 GMT+03:00, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> On 6/20/2022 2:30 PM, Dmitry Baryshkov wrote: > >>> To let the probe function bail early if any of the resources is > >>> unavailable, move resource allocattion from kms_init directly to the > >>> probe callback. While we are at it, replace irq_of_parse_and_map() with > >>> platform_get_irq(). > >>> > >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>> --- > >>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 107 +++++++++++------------ > >>> 1 file changed, 51 insertions(+), 56 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > >>> index 41dc60784847..6499713eccf6 100644 > >>> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > >>> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > >>> @@ -139,8 +139,6 @@ static void mdp4_destroy(struct msm_kms *kms) > >>> pm_runtime_disable(dev); > >>> mdp_kms_destroy(&mdp4_kms->base); > >>> - > >>> - kfree(mdp4_kms); > >>> } > >>> static const struct mdp_kms_funcs kms_funcs = { > >>> @@ -383,57 +381,27 @@ static int mdp4_kms_init(struct drm_device *dev) > >>> { > >>> struct platform_device *pdev = to_platform_device(dev->dev); > >>> struct msm_drm_private *priv = dev->dev_private; > >>> - struct mdp4_kms *mdp4_kms; > >>> + struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms)); > >>> struct msm_kms *kms = NULL; > >>> struct iommu_domain *iommu; > >>> struct msm_gem_address_space *aspace; > >>> - int irq, ret; > >>> + int ret; > >>> u32 major, minor; > >>> unsigned long max_clk; > >>> /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */ > >>> max_clk = 266667000; > >>> - mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL); > >>> - if (!mdp4_kms) { > >>> - DRM_DEV_ERROR(dev->dev, "failed to allocate kms\n"); > >>> - return -ENOMEM; > >>> - } > >>> - > >>> ret = mdp_kms_init(&mdp4_kms->base, &kms_funcs); > >>> if (ret) { > >>> DRM_DEV_ERROR(dev->dev, "failed to init kms\n"); > >>> goto fail; > >>> } > >>> - priv->kms = &mdp4_kms->base.base; > >>> kms = priv->kms; > >>> mdp4_kms->dev = dev; > >>> - mdp4_kms->mmio = msm_ioremap(pdev, NULL); > >>> - if (IS_ERR(mdp4_kms->mmio)) { > >>> - ret = PTR_ERR(mdp4_kms->mmio); > >>> - goto fail; > >>> - } > >>> - > >>> - irq = platform_get_irq(pdev, 0); > >>> - if (irq < 0) { > >>> - ret = irq; > >>> - DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret); > >>> - goto fail; > >>> - } > >>> - > >>> - kms->irq = irq; > >>> - > >>> - /* NOTE: driver for this regulator still missing upstream.. use > >>> - * _get_exclusive() and ignore the error if it does not exist > >>> - * (and hope that the bootloader left it on for us) > >>> - */ > >>> - mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); > >>> - if (IS_ERR(mdp4_kms->vdd)) > >>> - mdp4_kms->vdd = NULL; > >>> - > >>> if (mdp4_kms->vdd) { > >>> ret = regulator_enable(mdp4_kms->vdd); > >>> if (ret) { > >>> @@ -442,24 +410,6 @@ static int mdp4_kms_init(struct drm_device *dev) > >>> } > >>> } > >>> - mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); > >>> - if (IS_ERR(mdp4_kms->clk)) { > >>> - DRM_DEV_ERROR(dev->dev, "failed to get core_clk\n"); > >>> - ret = PTR_ERR(mdp4_kms->clk); > >>> - goto fail; > >>> - } > >>> - > >>> - mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); > >>> - if (IS_ERR(mdp4_kms->pclk)) > >>> - mdp4_kms->pclk = NULL; > >>> - > >>> - mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); > >>> - if (IS_ERR(mdp4_kms->axi_clk)) { > >>> - DRM_DEV_ERROR(dev->dev, "failed to get axi_clk\n"); > >>> - ret = PTR_ERR(mdp4_kms->axi_clk); > >>> - goto fail; > >>> - } > >>> - > >>> clk_set_rate(mdp4_kms->clk, max_clk); > >>> read_mdp_hw_revision(mdp4_kms, &major, &minor); > >>> @@ -474,10 +424,9 @@ static int mdp4_kms_init(struct drm_device *dev) > >>> mdp4_kms->rev = minor; > >>> if (mdp4_kms->rev >= 2) { > >>> - mdp4_kms->lut_clk = devm_clk_get(&pdev->dev, "lut_clk"); > >>> - if (IS_ERR(mdp4_kms->lut_clk)) { > >>> + if (!mdp4_kms->lut_clk) { > >>> DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n"); > >>> - ret = PTR_ERR(mdp4_kms->lut_clk); > >>> + ret = -ENODEV; > >>> goto fail; > >>> } > >>> clk_set_rate(mdp4_kms->lut_clk, max_clk); > >>> @@ -560,7 +509,53 @@ static const struct dev_pm_ops mdp4_pm_ops = { > >>> static int mdp4_probe(struct platform_device *pdev) > >>> { > >>> - return msm_drv_probe(&pdev->dev, mdp4_kms_init, NULL); > >>> + struct device *dev = &pdev->dev; > >>> + struct mdp4_kms *mdp4_kms; > >>> + int irq; > >>> + > >>> + mdp4_kms = devm_kzalloc(dev, sizeof(*mdp4_kms), GFP_KERNEL); > >>> + if (!mdp4_kms) > >>> + return dev_err_probe(dev, -ENOMEM, "failed to allocate kms\n"); > >>> + > >>> + mdp4_kms->mmio = msm_ioremap(pdev, NULL); > >>> + if (IS_ERR(mdp4_kms->mmio)) > >>> + return PTR_ERR(mdp4_kms->mmio); > >>> + > >>> + irq = platform_get_irq(pdev, 0); > >>> + if (irq < 0) > >>> + return dev_err_probe(dev, irq, "failed to get irq\n"); > >>> + > >>> + mdp4_kms->base.base.irq = irq; > >>> + > >>> + /* NOTE: driver for this regulator still missing upstream.. use > >>> + * _get_exclusive() and ignore the error if it does not exist > >>> + * (and hope that the bootloader left it on for us) > >>> + */ > >>> + mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); > >>> + if (IS_ERR(mdp4_kms->vdd)) > >>> + mdp4_kms->vdd = NULL; > >>> + > >>> + mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); > >>> + if (IS_ERR(mdp4_kms->clk)) > >>> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->clk), "failed to get core_clk\n"); > >>> + > >>> + mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); > >>> + if (IS_ERR(mdp4_kms->pclk)) > >>> + mdp4_kms->pclk = NULL; > >>> + > >>> + mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); > >>> + if (IS_ERR(mdp4_kms->axi_clk)) > >>> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->axi_clk), "failed to get axi_clk\n"); > >>> + > >>> + /* > >>> + * This is required for revn >= 2. Handle errors here and let the kms > >>> + * init bail out if the clock is not provided. > >>> + */ > >>> + mdp4_kms->lut_clk = devm_clk_get_optional(&pdev->dev, "lut_clk"); > >>> + if (IS_ERR(mdp4_kms->lut_clk)) > >>> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->lut_clk), "failed to get lut_clk\n"); > >> > >> I can see that you have moved this from init to probe and only rev >=2 needs it. > >> > >> But, your check here will end up returning from probe because you have a return. So I guess you means just having dev_err_probe without the return and let the init fail if the clk is not found because we have the hw_rev only in init. > > > > No. The function called here is the devm_clk_get_optional(). So the driver will get NULL if the clock is not present in the DT and an error only in an error case (e.g. EINVAL, EPROBE_DEFER). > > > > Later on the mdp4_kms_init() will check hw_rev and return -ENODEV if the clock is required, but is set to NULL (not present in DT). > > > > Ok, I have understood. But dont you think this is too much convolution > just for this check? Why not leave the lut_clk in the init instead of > trying to move it to probe and add all this? No, I don't. I saw no point in leaving the lut_clk getter unconverted. Having everything handled in a similar way is a neat bonus from my point of view.
On 9/2/2022 12:05 PM, Dmitry Baryshkov wrote: > On Fri, 2 Sept 2022 at 21:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 9/1/2022 11:06 PM, Dmitry Baryshkov wrote: >>> >>> >>> On 2 September 2022 03:24:17 GMT+03:00, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> On 6/20/2022 2:30 PM, Dmitry Baryshkov wrote: >>>>> To let the probe function bail early if any of the resources is >>>>> unavailable, move resource allocattion from kms_init directly to the >>>>> probe callback. While we are at it, replace irq_of_parse_and_map() with >>>>> platform_get_irq(). >>>>> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 107 +++++++++++------------ >>>>> 1 file changed, 51 insertions(+), 56 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c >>>>> index 41dc60784847..6499713eccf6 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c >>>>> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c >>>>> @@ -139,8 +139,6 @@ static void mdp4_destroy(struct msm_kms *kms) >>>>> pm_runtime_disable(dev); >>>>> mdp_kms_destroy(&mdp4_kms->base); >>>>> - >>>>> - kfree(mdp4_kms); >>>>> } >>>>> static const struct mdp_kms_funcs kms_funcs = { >>>>> @@ -383,57 +381,27 @@ static int mdp4_kms_init(struct drm_device *dev) >>>>> { >>>>> struct platform_device *pdev = to_platform_device(dev->dev); >>>>> struct msm_drm_private *priv = dev->dev_private; >>>>> - struct mdp4_kms *mdp4_kms; >>>>> + struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms)); >>>>> struct msm_kms *kms = NULL; >>>>> struct iommu_domain *iommu; >>>>> struct msm_gem_address_space *aspace; >>>>> - int irq, ret; >>>>> + int ret; >>>>> u32 major, minor; >>>>> unsigned long max_clk; >>>>> /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */ >>>>> max_clk = 266667000; >>>>> - mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL); >>>>> - if (!mdp4_kms) { >>>>> - DRM_DEV_ERROR(dev->dev, "failed to allocate kms\n"); >>>>> - return -ENOMEM; >>>>> - } >>>>> - >>>>> ret = mdp_kms_init(&mdp4_kms->base, &kms_funcs); >>>>> if (ret) { >>>>> DRM_DEV_ERROR(dev->dev, "failed to init kms\n"); >>>>> goto fail; >>>>> } >>>>> - priv->kms = &mdp4_kms->base.base; >>>>> kms = priv->kms; >>>>> mdp4_kms->dev = dev; >>>>> - mdp4_kms->mmio = msm_ioremap(pdev, NULL); >>>>> - if (IS_ERR(mdp4_kms->mmio)) { >>>>> - ret = PTR_ERR(mdp4_kms->mmio); >>>>> - goto fail; >>>>> - } >>>>> - >>>>> - irq = platform_get_irq(pdev, 0); >>>>> - if (irq < 0) { >>>>> - ret = irq; >>>>> - DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret); >>>>> - goto fail; >>>>> - } >>>>> - >>>>> - kms->irq = irq; >>>>> - >>>>> - /* NOTE: driver for this regulator still missing upstream.. use >>>>> - * _get_exclusive() and ignore the error if it does not exist >>>>> - * (and hope that the bootloader left it on for us) >>>>> - */ >>>>> - mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); >>>>> - if (IS_ERR(mdp4_kms->vdd)) >>>>> - mdp4_kms->vdd = NULL; >>>>> - >>>>> if (mdp4_kms->vdd) { >>>>> ret = regulator_enable(mdp4_kms->vdd); >>>>> if (ret) { >>>>> @@ -442,24 +410,6 @@ static int mdp4_kms_init(struct drm_device *dev) >>>>> } >>>>> } >>>>> - mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); >>>>> - if (IS_ERR(mdp4_kms->clk)) { >>>>> - DRM_DEV_ERROR(dev->dev, "failed to get core_clk\n"); >>>>> - ret = PTR_ERR(mdp4_kms->clk); >>>>> - goto fail; >>>>> - } >>>>> - >>>>> - mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); >>>>> - if (IS_ERR(mdp4_kms->pclk)) >>>>> - mdp4_kms->pclk = NULL; >>>>> - >>>>> - mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); >>>>> - if (IS_ERR(mdp4_kms->axi_clk)) { >>>>> - DRM_DEV_ERROR(dev->dev, "failed to get axi_clk\n"); >>>>> - ret = PTR_ERR(mdp4_kms->axi_clk); >>>>> - goto fail; >>>>> - } >>>>> - >>>>> clk_set_rate(mdp4_kms->clk, max_clk); >>>>> read_mdp_hw_revision(mdp4_kms, &major, &minor); >>>>> @@ -474,10 +424,9 @@ static int mdp4_kms_init(struct drm_device *dev) >>>>> mdp4_kms->rev = minor; >>>>> if (mdp4_kms->rev >= 2) { >>>>> - mdp4_kms->lut_clk = devm_clk_get(&pdev->dev, "lut_clk"); >>>>> - if (IS_ERR(mdp4_kms->lut_clk)) { >>>>> + if (!mdp4_kms->lut_clk) { >>>>> DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n"); >>>>> - ret = PTR_ERR(mdp4_kms->lut_clk); >>>>> + ret = -ENODEV; >>>>> goto fail; >>>>> } >>>>> clk_set_rate(mdp4_kms->lut_clk, max_clk); >>>>> @@ -560,7 +509,53 @@ static const struct dev_pm_ops mdp4_pm_ops = { >>>>> static int mdp4_probe(struct platform_device *pdev) >>>>> { >>>>> - return msm_drv_probe(&pdev->dev, mdp4_kms_init, NULL); >>>>> + struct device *dev = &pdev->dev; >>>>> + struct mdp4_kms *mdp4_kms; >>>>> + int irq; >>>>> + >>>>> + mdp4_kms = devm_kzalloc(dev, sizeof(*mdp4_kms), GFP_KERNEL); >>>>> + if (!mdp4_kms) >>>>> + return dev_err_probe(dev, -ENOMEM, "failed to allocate kms\n"); >>>>> + >>>>> + mdp4_kms->mmio = msm_ioremap(pdev, NULL); >>>>> + if (IS_ERR(mdp4_kms->mmio)) >>>>> + return PTR_ERR(mdp4_kms->mmio); >>>>> + >>>>> + irq = platform_get_irq(pdev, 0); >>>>> + if (irq < 0) >>>>> + return dev_err_probe(dev, irq, "failed to get irq\n"); >>>>> + >>>>> + mdp4_kms->base.base.irq = irq; >>>>> + >>>>> + /* NOTE: driver for this regulator still missing upstream.. use >>>>> + * _get_exclusive() and ignore the error if it does not exist >>>>> + * (and hope that the bootloader left it on for us) >>>>> + */ >>>>> + mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); >>>>> + if (IS_ERR(mdp4_kms->vdd)) >>>>> + mdp4_kms->vdd = NULL; >>>>> + >>>>> + mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); >>>>> + if (IS_ERR(mdp4_kms->clk)) >>>>> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->clk), "failed to get core_clk\n"); >>>>> + >>>>> + mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); >>>>> + if (IS_ERR(mdp4_kms->pclk)) >>>>> + mdp4_kms->pclk = NULL; >>>>> + >>>>> + mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); >>>>> + if (IS_ERR(mdp4_kms->axi_clk)) >>>>> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->axi_clk), "failed to get axi_clk\n"); >>>>> + >>>>> + /* >>>>> + * This is required for revn >= 2. Handle errors here and let the kms >>>>> + * init bail out if the clock is not provided. >>>>> + */ >>>>> + mdp4_kms->lut_clk = devm_clk_get_optional(&pdev->dev, "lut_clk"); >>>>> + if (IS_ERR(mdp4_kms->lut_clk)) >>>>> + return dev_err_probe(dev, PTR_ERR(mdp4_kms->lut_clk), "failed to get lut_clk\n"); >>>> >>>> I can see that you have moved this from init to probe and only rev >=2 needs it. >>>> >>>> But, your check here will end up returning from probe because you have a return. So I guess you means just having dev_err_probe without the return and let the init fail if the clk is not found because we have the hw_rev only in init. >>> >>> No. The function called here is the devm_clk_get_optional(). So the driver will get NULL if the clock is not present in the DT and an error only in an error case (e.g. EINVAL, EPROBE_DEFER). >>> >>> Later on the mdp4_kms_init() will check hw_rev and return -ENODEV if the clock is required, but is set to NULL (not present in DT). >>> >> >> Ok, I have understood. But dont you think this is too much convolution >> just for this check? Why not leave the lut_clk in the init instead of >> trying to move it to probe and add all this? > > No, I don't. I saw no point in leaving the lut_clk getter unconverted. > Having everything handled in a similar way is a neat bonus from my > point of view. > > Alright, its a small enough change and you have left a comment too, hence Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c index 41dc60784847..6499713eccf6 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c @@ -139,8 +139,6 @@ static void mdp4_destroy(struct msm_kms *kms) pm_runtime_disable(dev); mdp_kms_destroy(&mdp4_kms->base); - - kfree(mdp4_kms); } static const struct mdp_kms_funcs kms_funcs = { @@ -383,57 +381,27 @@ static int mdp4_kms_init(struct drm_device *dev) { struct platform_device *pdev = to_platform_device(dev->dev); struct msm_drm_private *priv = dev->dev_private; - struct mdp4_kms *mdp4_kms; + struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms)); struct msm_kms *kms = NULL; struct iommu_domain *iommu; struct msm_gem_address_space *aspace; - int irq, ret; + int ret; u32 major, minor; unsigned long max_clk; /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */ max_clk = 266667000; - mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL); - if (!mdp4_kms) { - DRM_DEV_ERROR(dev->dev, "failed to allocate kms\n"); - return -ENOMEM; - } - ret = mdp_kms_init(&mdp4_kms->base, &kms_funcs); if (ret) { DRM_DEV_ERROR(dev->dev, "failed to init kms\n"); goto fail; } - priv->kms = &mdp4_kms->base.base; kms = priv->kms; mdp4_kms->dev = dev; - mdp4_kms->mmio = msm_ioremap(pdev, NULL); - if (IS_ERR(mdp4_kms->mmio)) { - ret = PTR_ERR(mdp4_kms->mmio); - goto fail; - } - - irq = platform_get_irq(pdev, 0); - if (irq < 0) { - ret = irq; - DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret); - goto fail; - } - - kms->irq = irq; - - /* NOTE: driver for this regulator still missing upstream.. use - * _get_exclusive() and ignore the error if it does not exist - * (and hope that the bootloader left it on for us) - */ - mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); - if (IS_ERR(mdp4_kms->vdd)) - mdp4_kms->vdd = NULL; - if (mdp4_kms->vdd) { ret = regulator_enable(mdp4_kms->vdd); if (ret) { @@ -442,24 +410,6 @@ static int mdp4_kms_init(struct drm_device *dev) } } - mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); - if (IS_ERR(mdp4_kms->clk)) { - DRM_DEV_ERROR(dev->dev, "failed to get core_clk\n"); - ret = PTR_ERR(mdp4_kms->clk); - goto fail; - } - - mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); - if (IS_ERR(mdp4_kms->pclk)) - mdp4_kms->pclk = NULL; - - mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); - if (IS_ERR(mdp4_kms->axi_clk)) { - DRM_DEV_ERROR(dev->dev, "failed to get axi_clk\n"); - ret = PTR_ERR(mdp4_kms->axi_clk); - goto fail; - } - clk_set_rate(mdp4_kms->clk, max_clk); read_mdp_hw_revision(mdp4_kms, &major, &minor); @@ -474,10 +424,9 @@ static int mdp4_kms_init(struct drm_device *dev) mdp4_kms->rev = minor; if (mdp4_kms->rev >= 2) { - mdp4_kms->lut_clk = devm_clk_get(&pdev->dev, "lut_clk"); - if (IS_ERR(mdp4_kms->lut_clk)) { + if (!mdp4_kms->lut_clk) { DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n"); - ret = PTR_ERR(mdp4_kms->lut_clk); + ret = -ENODEV; goto fail; } clk_set_rate(mdp4_kms->lut_clk, max_clk); @@ -560,7 +509,53 @@ static const struct dev_pm_ops mdp4_pm_ops = { static int mdp4_probe(struct platform_device *pdev) { - return msm_drv_probe(&pdev->dev, mdp4_kms_init, NULL); + struct device *dev = &pdev->dev; + struct mdp4_kms *mdp4_kms; + int irq; + + mdp4_kms = devm_kzalloc(dev, sizeof(*mdp4_kms), GFP_KERNEL); + if (!mdp4_kms) + return dev_err_probe(dev, -ENOMEM, "failed to allocate kms\n"); + + mdp4_kms->mmio = msm_ioremap(pdev, NULL); + if (IS_ERR(mdp4_kms->mmio)) + return PTR_ERR(mdp4_kms->mmio); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return dev_err_probe(dev, irq, "failed to get irq\n"); + + mdp4_kms->base.base.irq = irq; + + /* NOTE: driver for this regulator still missing upstream.. use + * _get_exclusive() and ignore the error if it does not exist + * (and hope that the bootloader left it on for us) + */ + mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); + if (IS_ERR(mdp4_kms->vdd)) + mdp4_kms->vdd = NULL; + + mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk"); + if (IS_ERR(mdp4_kms->clk)) + return dev_err_probe(dev, PTR_ERR(mdp4_kms->clk), "failed to get core_clk\n"); + + mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk"); + if (IS_ERR(mdp4_kms->pclk)) + mdp4_kms->pclk = NULL; + + mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk"); + if (IS_ERR(mdp4_kms->axi_clk)) + return dev_err_probe(dev, PTR_ERR(mdp4_kms->axi_clk), "failed to get axi_clk\n"); + + /* + * This is required for revn >= 2. Handle errors here and let the kms + * init bail out if the clock is not provided. + */ + mdp4_kms->lut_clk = devm_clk_get_optional(&pdev->dev, "lut_clk"); + if (IS_ERR(mdp4_kms->lut_clk)) + return dev_err_probe(dev, PTR_ERR(mdp4_kms->lut_clk), "failed to get lut_clk\n"); + + return msm_drv_probe(&pdev->dev, mdp4_kms_init, &mdp4_kms->base.base); } static int mdp4_remove(struct platform_device *pdev)
To let the probe function bail early if any of the resources is unavailable, move resource allocattion from kms_init directly to the probe callback. While we are at it, replace irq_of_parse_and_map() with platform_get_irq(). Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 107 +++++++++++------------ 1 file changed, 51 insertions(+), 56 deletions(-)