Re: [PATCH v3 2/3] drm/msm/dpu: simplify clocks handling

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On Fri, 21 Jan 2022 at 07:30, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Dmitry Baryshkov (2022-01-19 14:16:15)
> > diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
> > index 7b504617833a..5533c87c7158 100644
> > --- a/drivers/gpu/drm/msm/msm_io_utils.c
> > +++ b/drivers/gpu/drm/msm/msm_io_utils.c
> > @@ -5,6 +5,8 @@
> >   * Author: Rob Clark <robdclark@gmail.com>
> >   */
> >
> > +#include <linux/clk/clk-conf.h>
> > +
> >  #include "msm_drv.h"
> >
> >  /*
> > @@ -47,6 +49,54 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
> >         return clk;
> >  }
> >
> > +int msm_parse_clock(struct platform_device *pdev, struct clk_bulk_data **clocks)
> > +{
> > +       u32 i, rc = 0;
> > +       const char *clock_name;
> > +       struct clk_bulk_data *bulk;
> > +       int num_clk = 0;
>
> No need to assign and then reassign before testing. Same goes for 'rc'.

Ack

>
> > +
> > +       if (!pdev)
> > +               return -EINVAL;
> > +
> > +       num_clk = of_property_count_strings(pdev->dev.of_node, "clock-names");
> > +       if (num_clk <= 0) {
> > +               pr_debug("clocks are not defined\n");
> > +               return 0;
> > +       }
> > +
> > +       bulk = devm_kcalloc(&pdev->dev, num_clk, sizeof(struct clk_bulk_data), GFP_KERNEL);
> > +       if (!bulk)
> > +               return -ENOMEM;
> > +
> > +       for (i = 0; i < num_clk; i++) {
> > +               rc = of_property_read_string_index(pdev->dev.of_node,
> > +                                                  "clock-names", i,
> > +                                                  &clock_name);
> > +               if (rc) {
> > +                       DRM_DEV_ERROR(&pdev->dev, "Failed to get clock name for %d\n", i);
> > +                       return rc;
> > +               }
> > +               bulk[i].id = devm_kstrdup(&pdev->dev, clock_name, GFP_KERNEL);
> > +       }
> > +
> > +       rc = devm_clk_bulk_get(&pdev->dev, num_clk, bulk);
>
> Use devm_clk_bulk_get_all()?

Oh, wow. I missed this API. Then this function becomes unnecessary.

>
> > +       if (rc) {
> > +               DRM_DEV_ERROR(&pdev->dev, "Failed to get clock refs %d\n", rc);
> > +               return rc;
> > +       }
> > +
> > +       rc = of_clk_set_defaults(pdev->dev.of_node, false);
>
> Why is this needed?

Both mdss and mdp devices use assigned-clocks properties, while not
being a clock provider (or a child of it).
So I assumed it should call the of_clk_set_defaults(node, false)
Not to mention that this call exists in the msm_dss_parse_clock(),
which is being refactored/replaced.

>
> > +       if (rc) {
> > +               DRM_DEV_ERROR(&pdev->dev, "Failed to set clock defaults %d\n", rc);
> > +               return rc;
> > +       }
> > +
> > +       *clocks = bulk;
> > +
> > +       return num_clk;


-- 
With best wishes
Dmitry