[Freedreno] [PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function
Abhinav Kumar
quic_abhinavk at quicinc.com
Wed Sep 7 19:09:15 UTC 2022
On 9/7/2022 11:54 AM, Abhinav Kumar wrote:
>
>
> On 8/26/2022 2:39 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 at linaro.org>
>> ---
>> drivers/gpu/drm/msm/hdmi/hdmi.c | 303 +++++++++++++++-----------------
>> 1 file changed, 138 insertions(+), 165 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c
>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> index 4a364d8f4c0b..c298a36f3b42 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> @@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>> if (hdmi->i2c)
>> msm_hdmi_i2c_destroy(hdmi->i2c);
>> -
>> - platform_set_drvdata(hdmi->pdev, NULL);
>> }
>> static int msm_hdmi_get_phy(struct hdmi *hdmi)
>
> Between v1 and v2, it just seems like a rebase to me on top of the 6.1
> MR. But what about moving msm_hdmi_get_phy() to probe(). I thought you
> were going to check that as well for v2.
Please ignore this part of the comment, I see that moving
msm_hdmi_get_phy() to probe() is in the patch 5 of this series.
Thanks for making that change.
The below comment still holds true.
>
> A change log would have been nice here. Because as part of the rebase
> looks like we even migrate to using panel bridge for hdmi driver.
>
> Usage of drm_of_find_panel_or_bridge was not present in v1 and wasnt
> obvious from the commit text either.
>
>> @@ -117,142 +115,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);
>> -
>> - ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL,
>> &hdmi->next_bridge);
>> - if (ret && ret != -ENODEV)
>> - goto fail;
>> -
>> - 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;
>> hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
>> @@ -276,13 +142,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,
>> @@ -332,13 +198,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);
>> @@ -358,8 +217,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>> priv->bridges[priv->num_bridges++] = hdmi->bridge;
>> - platform_set_drvdata(pdev, hdmi);
>> -
>> return 0;
>> fail:
>> @@ -387,7 +244,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),
>> };
>> @@ -397,7 +254,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),
>> @@ -512,23 +369,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);
>> @@ -561,6 +407,133 @@ 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);
>> +
>> + ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL,
>> &hdmi->next_bridge);
>> + if (ret && ret != -ENODEV)
>> + return ret;
>> +
>> + 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);
>> }
More information about the dri-devel
mailing list