[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:27:30 UTC 2022
On 9/7/2022 12:09 PM, Abhinav Kumar wrote:
>
>
> 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>
The change itself LGTM, so with some change log added, this is
Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>> ---
>>> 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