Re: [PATCH 3/3] drm/msm/hdmi: move resource allocation to probe function

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 6/16/2022 12:59 AM, Dmitry Baryshkov wrote:
> Rather than having all resource allocation happen in the _bind function
> (resulting in possible EPROBE_DEFER returns and component bind/unbind
> cycles) allocate and check all resources in _probe function. While we
> are at it, use platform_get_irq() to get the IRQ rather than going
> through the irq_of_parse_and_map().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 295 +++++++++++++++-----------------
>   1 file changed, 134 insertions(+), 161 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 8dfe5690366b..429abd622c81 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -75,8 +75,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>   
>   	if (hdmi->i2c)
>   		msm_hdmi_i2c_destroy(hdmi->i2c);
> -
> -	platform_set_drvdata(hdmi->pdev, NULL);
Do we still not need to do this in .remove?
>   }
>   
>   static int msm_hdmi_get_phy(struct hdmi *hdmi)
> @@ -116,138 +114,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>    * we are to EPROBE_DEFER we want to do it here, rather than later
>    * at modeset_init() time
>    */
> -static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
> +static int msm_hdmi_init(struct hdmi *hdmi)
>   {
> -	struct hdmi_platform_config *config = pdev->dev.platform_data;
> -	struct hdmi *hdmi = NULL;
> -	struct resource *res;
> -	int i, ret;
> -
> -	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> -	if (!hdmi) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -
> -	hdmi->pdev = pdev;
> -	hdmi->config = config;
> -	spin_lock_init(&hdmi->reg_lock);
> -
> -	hdmi->mmio = msm_ioremap(pdev, "core_physical");
> -	if (IS_ERR(hdmi->mmio)) {
> -		ret = PTR_ERR(hdmi->mmio);
> -		goto fail;
> -	}
> -
> -	/* HDCP needs physical address of hdmi register */
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> -		"core_physical");
> -	if (!res) {
> -		ret = -EINVAL;
> -		goto fail;
> -	}
> -	hdmi->mmio_phy_addr = res->start;
> -
> -	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
> -	if (IS_ERR(hdmi->qfprom_mmio)) {
> -		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
> -		hdmi->qfprom_mmio = NULL;
> -	}
> -
> -	hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
> -				      config->hpd_reg_cnt,
> -				      sizeof(hdmi->hpd_regs[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->hpd_regs) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	for (i = 0; i < config->hpd_reg_cnt; i++)
> -		hdmi->hpd_regs[i].supply = config->hpd_reg_names[i];
> -
> -	ret = devm_regulator_bulk_get(&pdev->dev, config->hpd_reg_cnt, hdmi->hpd_regs);
> -	if (ret) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %d\n", ret);
> -		goto fail;
> -	}
> -
> -	hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
> -				      config->pwr_reg_cnt,
> -				      sizeof(hdmi->pwr_regs[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->pwr_regs) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -
> -	for (i = 0; i < config->pwr_reg_cnt; i++)
> -		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
> -
> -	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
> -	if (ret) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: %d\n", ret);
> -		goto fail;
> -	}
> -
> -	hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
> -				      config->hpd_clk_cnt,
> -				      sizeof(hdmi->hpd_clks[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->hpd_clks) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	for (i = 0; i < config->hpd_clk_cnt; i++) {
> -		struct clk *clk;
> -
> -		clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
> -		if (IS_ERR(clk)) {
> -			ret = PTR_ERR(clk);
> -			DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s (%d)\n",
> -					config->hpd_clk_names[i], ret);
> -			goto fail;
> -		}
> -
> -		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) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	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)) {
> -			ret = PTR_ERR(clk);
> -			DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s (%d)\n",
> -					config->pwr_clk_names[i], ret);
> -			goto fail;
> -		}
> -
> -		hdmi->pwr_clks[i] = clk;
> -	}
> -
> -	hdmi->hpd_gpiod = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
> -	/* This will catch e.g. -EPROBE_DEFER */
> -	if (IS_ERR(hdmi->hpd_gpiod)) {
> -		ret = PTR_ERR(hdmi->hpd_gpiod);
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd gpio: (%d)\n", ret);
> -		goto fail;
> -	}
> -
> -	if (!hdmi->hpd_gpiod)
> -		DBG("failed to get HPD gpio");
> -
> -	if (hdmi->hpd_gpiod)
> -		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
> -
> -	devm_pm_runtime_enable(&pdev->dev);
> +	struct platform_device *pdev = hdmi->pdev;
> +	int ret;

What about the rest of the msm_hdmi_init() function?

msm_hdmi_i2c_init, msm_hdmi_get_phy and msm_hdmi_hdcp_init have been 
left behind. Any reason for that?


>   
>   	hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
>   
> @@ -271,13 +141,13 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
>   		hdmi->hdcp_ctrl = NULL;
>   	}
>   
> -	return hdmi;
> +	return 0;
>   
>   fail:
>   	if (hdmi)
>   		msm_hdmi_destroy(hdmi);
>   
> -	return ERR_PTR(ret);
> +	return ret;
>   }
>   
>   /* Second part of initialization, the drm/kms level modeset_init,
> @@ -318,13 +188,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   
>   	drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>   
> -	hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> -	if (!hdmi->irq) {
> -		ret = -EINVAL;
> -		DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
> -		goto fail;
> -	}
> -
>   	ret = devm_request_irq(&pdev->dev, hdmi->irq,
>   			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>   			"hdmi_isr", hdmi);
> @@ -344,8 +207,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   
>   	priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>   
> -	platform_set_drvdata(pdev, hdmi);
> -
>   	return 0;
>   
>   fail:
> @@ -373,7 +234,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   static const char *hpd_reg_names_8960[] = {"core-vdda"};
>   static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
>   
> -static struct hdmi_platform_config hdmi_tx_8960_config = {
> +const static struct hdmi_platform_config hdmi_tx_8960_config = {
>   		HDMI_CFG(hpd_reg, 8960),
>   		HDMI_CFG(hpd_clk, 8960),
>   };
> @@ -383,7 +244,7 @@ static const char *pwr_clk_names_8x74[] = {"extp", "alt_iface"};
>   static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core"};
>   static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0};
>   
> -static struct hdmi_platform_config hdmi_tx_8974_config = {
> +const static struct hdmi_platform_config hdmi_tx_8974_config = {
>   		HDMI_CFG(pwr_reg, 8x74),
>   		HDMI_CFG(pwr_clk, 8x74),
>   		HDMI_CFG(hpd_clk, 8x74),
> @@ -498,23 +359,12 @@ static int msm_hdmi_register_audio_driver(struct hdmi *hdmi, struct device *dev)
>   static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
>   {
>   	struct msm_drm_private *priv = dev_get_drvdata(master);
> -	struct hdmi_platform_config *hdmi_cfg;
> -	struct hdmi *hdmi;
> -	struct device_node *of_node = dev->of_node;
> +	struct hdmi *hdmi = dev_get_drvdata(dev);
>   	int err;
>   
> -	hdmi_cfg = (struct hdmi_platform_config *)
> -			of_device_get_match_data(dev);
> -	if (!hdmi_cfg) {
> -		DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node);
> -		return -ENXIO;
> -	}
> -
> -	dev->platform_data = hdmi_cfg;
> -
> -	hdmi = msm_hdmi_init(to_platform_device(dev));
> -	if (IS_ERR(hdmi))
> -		return PTR_ERR(hdmi);
> +	err = msm_hdmi_init(hdmi);
> +	if (err)
> +		return err;
>   	priv->hdmi = hdmi;
>   
>   	err = msm_hdmi_register_audio_driver(hdmi, dev);
> @@ -547,6 +397,129 @@ static const struct component_ops msm_hdmi_ops = {
>   
>   static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   {
> +	const struct hdmi_platform_config *config;
> +	struct device *dev = &pdev->dev;
> +	struct hdmi *hdmi;
> +	struct resource *res;
> +	int i, ret;
> +
> +	config = of_device_get_match_data(dev);
> +	if (!config)
> +		return -EINVAL;
> +
> +	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> +	if (!hdmi)
> +		return -ENOMEM;
> +
> +	hdmi->pdev = pdev;
> +	hdmi->config = config;
> +	spin_lock_init(&hdmi->reg_lock);
> +
> +	hdmi->mmio = msm_ioremap(pdev, "core_physical");
> +	if (IS_ERR(hdmi->mmio))
> +		return PTR_ERR(hdmi->mmio);
> +
> +	/* HDCP needs physical address of hdmi register */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +		"core_physical");
> +	if (!res)
> +		return -EINVAL;
> +	hdmi->mmio_phy_addr = res->start;
> +
> +	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
> +	if (IS_ERR(hdmi->qfprom_mmio)) {
> +		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
> +		hdmi->qfprom_mmio = NULL;
> +	}
> +
> +	hdmi->irq = platform_get_irq(pdev, 0);
> +	if (hdmi->irq < 0)
> +		return hdmi->irq;
> +
> +	hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
> +				      config->hpd_reg_cnt,
> +				      sizeof(hdmi->hpd_regs[0]),
> +				      GFP_KERNEL);
> +	if (!hdmi->hpd_regs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < config->hpd_reg_cnt; i++)
> +		hdmi->hpd_regs[i].supply = config->hpd_reg_names[i];
> +
> +	ret = devm_regulator_bulk_get(&pdev->dev, config->hpd_reg_cnt, hdmi->hpd_regs);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get hpd regulators\n");
> +
> +	hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
> +				      config->pwr_reg_cnt,
> +				      sizeof(hdmi->pwr_regs[0]),
> +				      GFP_KERNEL);
> +	if (!hdmi->pwr_regs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < config->pwr_reg_cnt; i++)
> +		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
> +
> +	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get pwr regulators\n");
> +
> +	hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
> +				      config->hpd_clk_cnt,
> +				      sizeof(hdmi->hpd_clks[0]),
> +				      GFP_KERNEL);
> +	if (!hdmi->hpd_clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < config->hpd_clk_cnt; i++) {
> +		struct clk *clk;
> +
> +		clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
> +		if (IS_ERR(clk))
> +			return dev_err_probe(dev, PTR_ERR(clk),
> +					     "failed to get hpd clk: %s\n",
> +					     config->hpd_clk_names[i]);
> +
> +		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->hpd_gpiod = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
> +	/* This will catch e.g. -EPROBE_DEFER */
> +	if (IS_ERR(hdmi->hpd_gpiod))
> +		return dev_err_probe(dev, PTR_ERR(hdmi->hpd_gpiod),
> +				     "failed to get hpd gpio\n");
> +
> +	if (!hdmi->hpd_gpiod)
> +		DBG("failed to get HPD gpio");
> +
> +	if (hdmi->hpd_gpiod)
> +		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
> +
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, hdmi);
> +
>   	return component_add(&pdev->dev, &msm_hdmi_ops);
>   }
>