On 2 September 2022 03:24:17 GMT+03:00, Abhinav Kumar 
<quic_abhin...@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.barysh...@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)

-- 
With best wishes
Dmitry

Reply via email to