Re: [PATCH v2 02/14] drm/msm/hdmi: simplify extp clock handling

[Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>]

On 5/22/2024 3:50 AM, Dmitry Baryshkov wrote:
> With the extp being the only "power" clock left, remove the surrounding
> loops and handle the extp clock directly.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>

Reviewed-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 24 ++++--------------------
>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  6 +-----
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 +++++++++++++--------------------
>   3 files changed, 18 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 108c86925780..681265e29aa0 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -235,13 +235,11 @@ static const struct hdmi_platform_config hdmi_tx_8960_config = {
>   };
>   
>   static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
> -static const char *pwr_clk_names_8x74[] = {"extp"};
>   static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
>   static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0, 0};
>   
>   static const struct hdmi_platform_config hdmi_tx_8974_config = {
>   		HDMI_CFG(pwr_reg, 8x74),
> -		HDMI_CFG(pwr_clk, 8x74),
>   		HDMI_CFG(hpd_clk, 8x74),
>   		.hpd_freq      = hpd_clk_freq_8x74,
>   };
> @@ -485,24 +483,10 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   		hdmi->hpd_clks[i] = clk;
>   	}
>   
> -	hdmi->pwr_clks = devm_kcalloc(&pdev->dev,
> -				      config->pwr_clk_cnt,
> -				      sizeof(hdmi->pwr_clks[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->pwr_clks)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < config->pwr_clk_cnt; i++) {
> -		struct clk *clk;
> -
> -		clk = msm_clk_get(pdev, config->pwr_clk_names[i]);
> -		if (IS_ERR(clk))
> -			return dev_err_probe(dev, PTR_ERR(clk),
> -					     "failed to get pwr clk: %s\n",
> -					     config->pwr_clk_names[i]);
> -
> -		hdmi->pwr_clks[i] = clk;
> -	}
> +	hdmi->extp_clk = devm_clk_get_optional(&pdev->dev, "extp");
> +	if (IS_ERR(hdmi->extp_clk))
> +		return dev_err_probe(dev, PTR_ERR(hdmi->extp_clk),
> +				     "failed to get extp clock\n");
>   
>   	hdmi->hpd_gpiod = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
>   	/* This will catch e.g. -EPROBE_DEFER */
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index 4586baf36415..abdbe4779cf9 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -51,7 +51,7 @@ struct hdmi {
>   	struct regulator_bulk_data *hpd_regs;
>   	struct regulator_bulk_data *pwr_regs;
>   	struct clk **hpd_clks;
> -	struct clk **pwr_clks;
> +	struct clk *extp_clk;
>   
>   	struct gpio_desc *hpd_gpiod;
>   
> @@ -98,10 +98,6 @@ struct hdmi_platform_config {
>   	const char **hpd_clk_names;
>   	const long unsigned *hpd_freq;
>   	int hpd_clk_cnt;
> -
> -	/* clks that need to be on for screen pwr (ie pixel clk): */
> -	const char **pwr_clk_names;
> -	int pwr_clk_cnt;
>   };
>   
>   struct hdmi_bridge {
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 4a5b5112227f..9eb4d06bdc0e 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -17,7 +17,7 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
>   	const struct hdmi_platform_config *config = hdmi->config;
> -	int i, ret;
> +	int ret;
>   
>   	pm_runtime_get_sync(&hdmi->pdev->dev);
>   
> @@ -25,21 +25,15 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
>   	if (ret)
>   		DRM_DEV_ERROR(dev->dev, "failed to enable pwr regulator: %d\n", ret);
>   
> -	if (config->pwr_clk_cnt > 0) {
> +	if (hdmi->extp_clk) {
>   		DBG("pixclock: %lu", hdmi->pixclock);
> -		ret = clk_set_rate(hdmi->pwr_clks[0], hdmi->pixclock);
> -		if (ret) {
> -			DRM_DEV_ERROR(dev->dev, "failed to set pixel clk: %s (%d)\n",
> -					config->pwr_clk_names[0], ret);
> -		}
> -	}
> +		ret = clk_set_rate(hdmi->extp_clk, hdmi->pixclock);
> +		if (ret)
> +			DRM_DEV_ERROR(dev->dev, "failed to set extp clk rate: %d\n", ret);
>   
> -	for (i = 0; i < config->pwr_clk_cnt; i++) {
> -		ret = clk_prepare_enable(hdmi->pwr_clks[i]);
> -		if (ret) {
> -			DRM_DEV_ERROR(dev->dev, "failed to enable pwr clk: %s (%d)\n",
> -					config->pwr_clk_names[i], ret);
> -		}
> +		ret = clk_prepare_enable(hdmi->extp_clk);
> +		if (ret)
> +			DRM_DEV_ERROR(dev->dev, "failed to enable extp clk: %d\n", ret);
>   	}
>   }
>   
> @@ -49,15 +43,15 @@ static void power_off(struct drm_bridge *bridge)
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
>   	const struct hdmi_platform_config *config = hdmi->config;
> -	int i, ret;
> +	int ret;
>   
>   	/* TODO do we need to wait for final vblank somewhere before
>   	 * cutting the clocks?
>   	 */
>   	mdelay(16 + 4);
>   
> -	for (i = 0; i < config->pwr_clk_cnt; i++)
> -		clk_disable_unprepare(hdmi->pwr_clks[i]);
> +	if (hdmi->extp_clk)
> +		clk_disable_unprepare(hdmi->extp_clk);
>   
>   	ret = regulator_bulk_disable(config->pwr_reg_cnt, hdmi->pwr_regs);
>   	if (ret)
> @@ -271,7 +265,6 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
>   {
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> -	const struct hdmi_platform_config *config = hdmi->config;
>   	struct msm_drm_private *priv = bridge->dev->dev_private;
>   	struct msm_kms *kms = priv->kms;
>   	long actual, requested;
> @@ -285,8 +278,8 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
>   	if (kms->funcs->round_pixclk)
>   		actual = kms->funcs->round_pixclk(kms,
>   			requested, hdmi_bridge->hdmi->encoder);
> -	else if (config->pwr_clk_cnt > 0)
> -		actual = clk_round_rate(hdmi->pwr_clks[0], requested);
> +	else if (hdmi->extp_clk)
> +		actual = clk_round_rate(hdmi->extp_clk, requested);
>   	else
>   		actual = requested;
>   
>
> --
> 2.39.2