On Fri, 21 Jan 2022 at 07:30, Stephen Boyd <swboyd@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > */
> >
> > +#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
