[PATCH v4 2/2] drm/msm/dp: rewrite dss_module_power to use bulk clock functions

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-01-31 13:05:13)
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 094b39bfed8c..f16072f33cdb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -10,7 +10,6 @@
>  #include <linux/phy/phy.h>
>  #include <linux/phy/phy-dp.h>
>
> -#include "dp_clk_util.h"
>  #include "msm_drv.h"
>
>  #define DP_LABEL "MDSS DP DISPLAY"
> @@ -106,6 +105,22 @@ struct dp_regulator_cfg {
>         struct dp_reg_entry regs[DP_DEV_REGULATOR_MAX];
>  };
>
> +enum dss_clk_type {
> +       DSS_CLK_AHB, /* no set rate. rate controlled through rpm */
> +       DSS_CLK_PCLK,
> +};
> +
> +struct dss_clk {
> +       enum dss_clk_type type;
> +       unsigned long rate;
> +};
> +
> +struct dss_module_power {
> +       unsigned int num_clk;
> +       struct clk_bulk_data *clocks;
> +       struct dss_clk *clk_config;
> +};
> +
>  /**
>   * struct dp_parser - DP parser's data exposed to clients
>   *
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index b48b45e92bfa..318e1f8629cb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -105,59 +105,40 @@ static int dp_power_clk_init(struct dp_power_private *power)
>         ctrl = &power->parser->mp[DP_CTRL_PM];
>         stream = &power->parser->mp[DP_STREAM_PM];
>
> -       rc = msm_dss_get_clk(dev, core->clk_config, core->num_clk);
> +       rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);

Could we go further and devm_clk_bulk_get_all() and then either have
some new clk API that goes through the bulk list and finds the one named
something and hands over a pointer to it, think "clk_get() on top of
clk_bulk_data", or just get the clk again that you want to set a rate on
and have two pointers but otherwise it's a don't care. Then we wouldn't
need to do much at all here to store the rate settable clk and find it
in a loop.

>         if (rc) {
>                 DRM_ERROR("failed to get %s clk. err=%d\n",
>                         dp_parser_pm_name(DP_CORE_PM), rc);
>                 return rc;
>         }
>
> -       rc = msm_dss_get_clk(dev, ctrl->clk_config, ctrl->num_clk);
> +       rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
>         if (rc) {
>                 DRM_ERROR("failed to get %s clk. err=%d\n",
>                         dp_parser_pm_name(DP_CTRL_PM), rc);
> -               msm_dss_put_clk(core->clk_config, core->num_clk);
>                 return -ENODEV;
>         }
>
> -       rc = msm_dss_get_clk(dev, stream->clk_config, stream->num_clk);
> +       rc = devm_clk_bulk_get(dev, stream->num_clk, stream->clocks);
>         if (rc) {
>                 DRM_ERROR("failed to get %s clk. err=%d\n",
>                         dp_parser_pm_name(DP_CTRL_PM), rc);
> -               msm_dss_put_clk(core->clk_config, core->num_clk);
>                 return -ENODEV;
>         }
>
>         return 0;
>  }
>
> -static int dp_power_clk_deinit(struct dp_power_private *power)
> -{
> -       struct dss_module_power *core, *ctrl, *stream;
> -
> -       core = &power->parser->mp[DP_CORE_PM];
> -       ctrl = &power->parser->mp[DP_CTRL_PM];
> -       stream = &power->parser->mp[DP_STREAM_PM];
> -
> -       if (!core || !ctrl || !stream) {
> -               DRM_ERROR("invalid power_data\n");
> -               return -EINVAL;
> -       }
> -
> -       msm_dss_put_clk(ctrl->clk_config, ctrl->num_clk);
> -       msm_dss_put_clk(core->clk_config, core->num_clk);
> -       msm_dss_put_clk(stream->clk_config, stream->num_clk);
> -       return 0;
> -}
> -
>  static int dp_power_clk_set_link_rate(struct dp_power_private *power,
> -                       struct dss_clk *clk_arry, int num_clk, int enable)
> +                       struct dss_module_power *mp, int enable)
>  {
> +       struct dss_clk *clk_arry = mp->clk_config;
> +       int num_clk = mp->num_clk;
>         u32 rate;
>         int i, rc = 0;
>
>         for (i = 0; i < num_clk; i++) {
> -               if (clk_arry[i].clk) {
> +               if (mp->clocks[i].clk) {
>                         if (clk_arry[i].type == DSS_CLK_PCLK) {
>                                 if (enable)
>                                         rate = clk_arry[i].rate;
> @@ -168,9 +149,49 @@ static int dp_power_clk_set_link_rate(struct dp_power_private *power,
>                                 if (rc)
>                                         break;
>                         }
> +               } else {
> +                       DRM_ERROR("%pS->%s: '%s' is not available\n",
> +                               __builtin_return_address(0), __func__,
> +                               mp->clocks[i].id);
> +                       rc = -EPERM;
> +                       break;
> +               }
> +       }
> +       return rc;
> +}
> +
> +static int dp_clk_set_rate(struct dss_module_power *mp)
> +{
> +       struct dss_clk *clk_arry = mp->clk_config;
> +       int num_clk = mp->num_clk;
> +       int i, rc = 0;
>
> +       for (i = 0; i < num_clk; i++) {
> +               if (mp->clocks[i].clk) {
> +                       if (clk_arry[i].type != DSS_CLK_AHB) {

This loops is gross.

> +                               DRM_DEBUG("%pS->%s: '%s' rate %ld\n",
> +                                       __builtin_return_address(0), __func__,
> +                                       mp->clocks[i].id,
> +                                       clk_arry[i].rate);
> +                               rc = clk_set_rate(mp->clocks[i].clk,
> +                                       clk_arry[i].rate);
> +                               if (rc) {
> +                                       DRM_ERROR("%pS->%s: %s failed. rc=%d\n",
> +                                               __builtin_return_address(0),
> +                                               __func__,
> +                                               mp->clocks[i].id, rc);
> +                                       break;
> +                               }
> +                       }
> +               } else {
> +                       DRM_ERROR("%pS->%s: '%s' is not available\n",
> +                               __builtin_return_address(0), __func__,
> +                               mp->clocks[i].id);
> +                       rc = -EPERM;
> +                       break;
>                 }
>         }
> +
>         return rc;
>  }
>