From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Abhinav Kumar <quic_abhinavk@quicinc.com>,
Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>,
linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Stephen Boyd <swboyd@chromium.org>,
freedreno@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm/msm/hdmi: move resource allocation to probe function
Date: Fri, 26 Aug 2022 11:57:36 +0300 [thread overview]
Message-ID: <d2d94589-8513-c261-c5d8-b7ffa3db8616@linaro.org> (raw)
In-Reply-To: <41ca91c6-dc38-af0a-c955-a276f5824cc8@quicinc.com>
On 24/08/2022 02:58, Abhinav Kumar wrote:
>
>
> 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?
msm_hdmi_i2c_init() allocates new adapter, so it should be part of bind().
msm_hdmi_hdcp_init() just allocates a chunk of memory (other actions are
infallible). Also I did not want to move a piece of code that I can not
really test.
As for the msm_hdmi_get_phy(), I don't remember why I didn't move it.
But you are right, it makes sense to move it. I'll check it for v2.
>
>
>> hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
--
With best wishes
Dmitry
prev parent reply other threads:[~2022-08-26 8:57 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-16 7:59 [PATCH 0/3] drm/msm/hdmi: move resource allocation to probe function Dmitry Baryshkov
2022-06-16 7:59 ` [PATCH 1/3] drm/msm/hdmi: use devres helper for runtime PM management Dmitry Baryshkov
2022-08-23 17:39 ` Abhinav Kumar
2022-06-16 7:59 ` [PATCH 2/3] drm/msm/hdmi: drop constant resource names from platform config Dmitry Baryshkov
2022-08-23 17:40 ` Abhinav Kumar
2022-06-16 7:59 ` [PATCH 3/3] drm/msm/hdmi: move resource allocation to probe function Dmitry Baryshkov
2022-08-23 23:58 ` Abhinav Kumar
2022-08-26 8:57 ` Dmitry Baryshkov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d2d94589-8513-c261-c5d8-b7ffa3db8616@linaro.org \
--to=dmitry.baryshkov@linaro.org \
--cc=airlied@linux.ie \
--cc=bjorn.andersson@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=quic_abhinavk@quicinc.com \
--cc=robdclark@gmail.com \
--cc=sean@poorly.run \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).